Re: [edk2-devel] [PATCH v1] MdeModulePkg: Add a check for metadata size in NvmExpress Driver

["Wu, Hao A" <hao.a.wu@intel.com>]

Thanks for the patch, a couple of inline comments below:


> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Ma, Hua
> Sent: Thursday, March 3, 2022 10:06 AM
> To: devel@edk2.groups.io
> Cc: Wang, Jian J <jian.j.wang@intel.com>; Gao, Liming
> <gaoliming@byosoft.com.cn>; Wu, Hao A <hao.a.wu@intel.com>; Ni, Ray
> <ray.ni@intel.com>; Ma, Hua <hua.ma@intel.com>
> Subject: [edk2-devel] [PATCH v1] MdeModulePkg: Add a check for metadata
> size in NvmExpress Driver
> 
> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=3856
> 
> Currently this NvmeExpress Driver do not support metadata handling.
> According to the NVME specs, metadata may be transferred to the host after
> the logical block data. It can overrun the input buffer which may only be the
> size of logical block data.
> 
> Add a check to return not support for the namespaces formatted with
> metadata.
> 
> Cc: Jian J Wang <jian.j.wang@intel.com>
> Cc: Liming Gao <gaoliming@byosoft.com.cn>
> Cc: Hao A Wu <hao.a.wu@intel.com>
> Cc: Ray Ni <ray.ni@intel.com>
> 
> Signed-off-by: Hua Ma <hua.ma@intel.com>
> ---
>  MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpress.c   | 15
> +++++++++++++++
>  .../Bus/Pci/NvmExpressPei/NvmExpressPei.c         | 15 +++++++++++++++
>  2 files changed, 30 insertions(+)
> 
> diff --git a/MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpress.c
> b/MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpress.c
> index 5a1eda8e8d..46b7dcba20 100644
> --- a/MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpress.c
> +++ b/MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpress.c
> @@ -139,6 +139,21 @@ EnumerateNvmeDevNamespace (
> 
>      Flbas                   = NamespaceData->Flbas;
>      LbaFmtIdx               = Flbas & 0xF;
> +
> +    //
> +    // Currently this NVME driver only suport Metadata Size == 0
> +    //
> +    if (NamespaceData->LbaFormat[LbaFmtIdx].Ms) {


1. Please help to update to:
if (NamespaceData->LbaFormat[LbaFmtIdx].Ms != 0) {
(Similar comment applies to NvmExpressPei)


> +      DEBUG ((
> +        DEBUG_INFO,


2. My preference is to use DEBUG_ERROR level here. Could you help to update debug output level?
(Similar comment applies to NvmExpressPei)

Best Regards,
Hao Wu


> +        "NVME IDENTIFY NAMESPACE [%d] Ms(%d) is not supported.\n",
> +        NamespaceId,
> +        NamespaceData->LbaFormat[LbaFmtIdx].Ms
> +        ));
> +      Status = EFI_UNSUPPORTED;
> +      goto Exit;
> +    }
> +
>      Lbads                   = NamespaceData->LbaFormat[LbaFmtIdx].Lbads;
>      Device->Media.BlockSize = (UINT32)1 << Lbads;
> 
> diff --git a/MdeModulePkg/Bus/Pci/NvmExpressPei/NvmExpressPei.c
> b/MdeModulePkg/Bus/Pci/NvmExpressPei/NvmExpressPei.c
> index f73053fc3f..6e27950648 100644
> --- a/MdeModulePkg/Bus/Pci/NvmExpressPei/NvmExpressPei.c
> +++ b/MdeModulePkg/Bus/Pci/NvmExpressPei/NvmExpressPei.c
> @@ -104,6 +104,21 @@ EnumerateNvmeDevNamespace (
>    //
>    Flbas     = NamespaceData->Flbas;
>    LbaFmtIdx = Flbas & 0xF;
> +
> +  //
> +  // Currently this NVME driver only suport Metadata Size == 0  //  if
> + (NamespaceData->LbaFormat[LbaFmtIdx].Ms) {
> +    DEBUG ((
> +      DEBUG_INFO,
> +      "NVME IDENTIFY NAMESPACE [%d] Ms(%d) is not supported.\n",
> +      NamespaceId,
> +      NamespaceData->LbaFormat[LbaFmtIdx].Ms
> +      ));
> +    Status = EFI_UNSUPPORTED;
> +    goto Exit;
> +  }
> +
>    Lbads     = NamespaceData->LbaFormat[LbaFmtIdx].Lbads;
> 
>    NamespaceInfo->Media.InterfaceType  = MSG_NVME_NAMESPACE_DP;
> --
> 2.32.0.windows.2
> 
> 
> 
> 
>