Notify NVME HW when system reset happens

[edk2] [PATCH 0/2] Notify NVME HW when system reset happens

Posted by Ruiyu Ni 7 years, 4 months ago

Per NVM Express Spec, software should notify NVME HW when shutdown
occurs.

The host should set the Shutdown Notification (CC.SHN) field to 01b
to indicate a normal shutdown operation. The controller indicates
when shutdown processing is completed by updating the Shutdown Status
(CSTS.SHST) field to 10b.

Ruiyu Ni (2):
  MdePkg/Nvme: Add NVME shutdown notification related macros
  MdeModulePkg/NvmExpressDxe: Notify NVME HW when system reset happens

 MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpress.c    |   7 +-
 MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpress.h    |  23 ++-
 .../Bus/Pci/NvmExpressDxe/NvmExpressDxe.inf        |   3 +-
 MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpressHci.c | 181 ++++++++++++++++++++-
 MdePkg/Include/IndustryStandard/Nvme.h             |   6 +-
 5 files changed, 215 insertions(+), 5 deletions(-)

-- 
2.12.2.windows.2

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH 0/2] Notify NVME HW when system reset happens

Posted by Wu, Hao A 7 years, 4 months ago

Hi Ray,

Just for curiosity, I checked the NVME spec for the purpose of a shutdown
notification to the controller when there is a power-off for the device. And
found the following:

1. Will increase the field 'Unsafe Shutdowns' in the SMART/Health Information
data (Section 5.10.1.2).

2. Subsequent reads for locations written to the volatile write cache that were
not written to non-volatile storage may return older data. (Section 6.4.2.1)

For 2, the implementation of the NVME driver always sets the Force Unit 
Access (FUA) bit for write operations. Hence, the read operation will reflect
the latest data on device.

So missing the shutdown notification will increase the number of 'Unsafe
Shutdowns' tracked by the controller. Is there any other issue being introduced
by missing the notification?

Besides, the change is good for me.


Best Regards,
Hao Wu

> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Ruiyu
> Ni
> Sent: Monday, August 07, 2017 11:32 AM
> To: edk2-devel@lists.01.org
> Subject: [edk2] [PATCH 0/2] Notify NVME HW when system reset happens
> 
> Per NVM Express Spec, software should notify NVME HW when shutdown
> occurs.
> 
> The host should set the Shutdown Notification (CC.SHN) field to 01b
> to indicate a normal shutdown operation. The controller indicates
> when shutdown processing is completed by updating the Shutdown Status
> (CSTS.SHST) field to 10b.
> 
> Ruiyu Ni (2):
>   MdePkg/Nvme: Add NVME shutdown notification related macros
>   MdeModulePkg/NvmExpressDxe: Notify NVME HW when system reset
> happens
> 
>  MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpress.c    |   7 +-
>  MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpress.h    |  23 ++-
>  .../Bus/Pci/NvmExpressDxe/NvmExpressDxe.inf        |   3 +-
>  MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpressHci.c | 181
> ++++++++++++++++++++-
>  MdePkg/Include/IndustryStandard/Nvme.h             |   6 +-
>  5 files changed, 215 insertions(+), 5 deletions(-)
> 
> --
> 2.12.2.windows.2
> 
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH 0/2] Notify NVME HW when system reset happens

Posted by Ni, Ruiyu 7 years, 4 months ago

The NVME controller requires notification for shutdown as part of its management of internal structures.  Even with FUA, failing to notify the NVME controller to shutdown power off causes the NVME controller to take quite some time to organize its tables on the next power on.  This time exceeds the normal timeout, so we would fail to boot the NVME disk. 

I will put above explanation into the commit message as well.

Thanks/Ray

> -----Original Message-----
> From: Wu, Hao A
> Sent: Monday, August 7, 2017 3:14 PM
> To: Ni, Ruiyu <ruiyu.ni@intel.com>; edk2-devel@lists.01.org
> Subject: RE: [edk2] [PATCH 0/2] Notify NVME HW when system reset
> happens
> 
> Hi Ray,
> 
> Just for curiosity, I checked the NVME spec for the purpose of a shutdown
> notification to the controller when there is a power-off for the device. And
> found the following:
> 
> 1. Will increase the field 'Unsafe Shutdowns' in the SMART/Health
> Information data (Section 5.10.1.2).
> 
> 2. Subsequent reads for locations written to the volatile write cache that
> were not written to non-volatile storage may return older data. (Section
> 6.4.2.1)
> 
> For 2, the implementation of the NVME driver always sets the Force Unit
> Access (FUA) bit for write operations. Hence, the read operation will reflect
> the latest data on device.
> 
> So missing the shutdown notification will increase the number of 'Unsafe
> Shutdowns' tracked by the controller. Is there any other issue being
> introduced by missing the notification?
> 
> Besides, the change is good for me.
> 
> 
> Best Regards,
> Hao Wu
> 
> > -----Original Message-----
> > From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of
> > Ruiyu Ni
> > Sent: Monday, August 07, 2017 11:32 AM
> > To: edk2-devel@lists.01.org
> > Subject: [edk2] [PATCH 0/2] Notify NVME HW when system reset happens
> >
> > Per NVM Express Spec, software should notify NVME HW when shutdown
> > occurs.
> >
> > The host should set the Shutdown Notification (CC.SHN) field to 01b to
> > indicate a normal shutdown operation. The controller indicates when
> > shutdown processing is completed by updating the Shutdown Status
> > (CSTS.SHST) field to 10b.
> >
> > Ruiyu Ni (2):
> >   MdePkg/Nvme: Add NVME shutdown notification related macros
> >   MdeModulePkg/NvmExpressDxe: Notify NVME HW when system reset
> happens
> >
> >  MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpress.c    |   7 +-
> >  MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpress.h    |  23 ++-
> >  .../Bus/Pci/NvmExpressDxe/NvmExpressDxe.inf        |   3 +-
> >  MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpressHci.c | 181
> > ++++++++++++++++++++-
> >  MdePkg/Include/IndustryStandard/Nvme.h             |   6 +-
> >  5 files changed, 215 insertions(+), 5 deletions(-)
> >
> > --
> > 2.12.2.windows.2
> >
> > _______________________________________________
> > edk2-devel mailing list
> > edk2-devel@lists.01.org
> > https://lists.01.org/mailman/listinfo/edk2-devel
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH 0/2] Notify NVME HW when system reset happens

Posted by Wu, Hao A 7 years, 4 months ago

> -----Original Message-----
> From: Ni, Ruiyu
> Sent: Wednesday, August 09, 2017 3:57 PM
> To: Wu, Hao A; edk2-devel@lists.01.org
> Subject: RE: [edk2] [PATCH 0/2] Notify NVME HW when system reset happens
> 
> The NVME controller requires notification for shutdown as part of its
> management of internal structures.  Even with FUA, failing to notify the NVME
> controller to shutdown power off causes the NVME controller to take quite
> some time to organize its tables on the next power on.  This time exceeds the
> normal timeout, so we would fail to boot the NVME disk.

Got it, thanks for the clarification.

> 
> I will put above explanation into the commit message as well.

The series is good to me.
Reviewed-by: Hao Wu <hao.a.wu@intel.com>


Best Regards,
Hao Wu

> 
> Thanks/Ray
> 
> > -----Original Message-----
> > From: Wu, Hao A
> > Sent: Monday, August 7, 2017 3:14 PM
> > To: Ni, Ruiyu <ruiyu.ni@intel.com>; edk2-devel@lists.01.org
> > Subject: RE: [edk2] [PATCH 0/2] Notify NVME HW when system reset
> > happens
> >
> > Hi Ray,
> >
> > Just for curiosity, I checked the NVME spec for the purpose of a shutdown
> > notification to the controller when there is a power-off for the device. And
> > found the following:
> >
> > 1. Will increase the field 'Unsafe Shutdowns' in the SMART/Health
> > Information data (Section 5.10.1.2).
> >
> > 2. Subsequent reads for locations written to the volatile write cache that
> > were not written to non-volatile storage may return older data. (Section
> > 6.4.2.1)
> >
> > For 2, the implementation of the NVME driver always sets the Force Unit
> > Access (FUA) bit for write operations. Hence, the read operation will reflect
> > the latest data on device.
> >
> > So missing the shutdown notification will increase the number of 'Unsafe
> > Shutdowns' tracked by the controller. Is there any other issue being
> > introduced by missing the notification?
> >
> > Besides, the change is good for me.
> >
> >
> > Best Regards,
> > Hao Wu
> >
> > > -----Original Message-----
> > > From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of
> > > Ruiyu Ni
> > > Sent: Monday, August 07, 2017 11:32 AM
> > > To: edk2-devel@lists.01.org
> > > Subject: [edk2] [PATCH 0/2] Notify NVME HW when system reset happens
> > >
> > > Per NVM Express Spec, software should notify NVME HW when shutdown
> > > occurs.
> > >
> > > The host should set the Shutdown Notification (CC.SHN) field to 01b to
> > > indicate a normal shutdown operation. The controller indicates when
> > > shutdown processing is completed by updating the Shutdown Status
> > > (CSTS.SHST) field to 10b.
> > >
> > > Ruiyu Ni (2):
> > >   MdePkg/Nvme: Add NVME shutdown notification related macros
> > >   MdeModulePkg/NvmExpressDxe: Notify NVME HW when system reset
> > happens
> > >
> > >  MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpress.c    |   7 +-
> > >  MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpress.h    |  23 ++-
> > >  .../Bus/Pci/NvmExpressDxe/NvmExpressDxe.inf        |   3 +-
> > >  MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpressHci.c | 181
> > > ++++++++++++++++++++-
> > >  MdePkg/Include/IndustryStandard/Nvme.h             |   6 +-
> > >  5 files changed, 215 insertions(+), 5 deletions(-)
> > >
> > > --
> > > 2.12.2.windows.2
> > >
> > > _______________________________________________
> > > edk2-devel mailing list
> > > edk2-devel@lists.01.org
> > > https://lists.01.org/mailman/listinfo/edk2-devel
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel