[PATCH 0/3] hw/nvme: bug fixes and doc update

[PATCH 0/3] hw/nvme: bug fixes and doc update

[Ankit Kumar]

This series fixes two bugs
1. CRC64 generation when metadata buffer is used.
2. Protection information disable check for Type 3 protection.

This series also updates the documentaion for pi (protection information),
and adds missing pif (protection information format) entry.

Ankit Kumar (3):
  hw/nvme: fix CRC64 for guard tag
  hw/nvme: fix disable pi checks for Type 3 protection
  docs: update hw/nvme documentation for protection information

 docs/system/devices/nvme.rst | 10 +++++++---
 hw/nvme/dif.c                |  9 +++++----
 2 files changed, 12 insertions(+), 7 deletions(-)

-- 
2.25.1

[PATCH 1/3] hw/nvme: fix CRC64 for guard tag

[Ankit Kumar]

The nvme CRC64 generator expects the caller to pass inverted seed value.
Pass inverted crc value for metadata buffer.

Signed-off-by: Ankit Kumar <ankit.kumar@samsung.com>
---
 hw/nvme/dif.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/nvme/dif.c b/hw/nvme/dif.c
index 63c44c86ab..01b19c3373 100644
--- a/hw/nvme/dif.c
+++ b/hw/nvme/dif.c
@@ -115,7 +115,7 @@ static void nvme_dif_pract_generate_dif_crc64(NvmeNamespace *ns, uint8_t *buf,
         uint64_t crc = crc64_nvme(~0ULL, buf, ns->lbasz);
 
         if (pil) {
-            crc = crc64_nvme(crc, mbuf, pil);
+            crc = crc64_nvme(~crc, mbuf, pil);
         }
 
         dif->g64.guard = cpu_to_be64(crc);
@@ -246,7 +246,7 @@ static uint16_t nvme_dif_prchk_crc64(NvmeNamespace *ns, NvmeDifTuple *dif,
         uint64_t crc = crc64_nvme(~0ULL, buf, ns->lbasz);
 
         if (pil) {
-            crc = crc64_nvme(crc, mbuf, pil);
+            crc = crc64_nvme(~crc, mbuf, pil);
         }
 
         trace_pci_nvme_dif_prchk_guard_crc64(be64_to_cpu(dif->g64.guard), crc);
-- 
2.25.1

Re: [PATCH 1/3] hw/nvme: fix CRC64 for guard tag

[Klaus Jensen]

[-- Attachment #1: Type: text/plain, Size: 1289 bytes --]

On Aug  8 02:57, Ankit Kumar wrote:
> The nvme CRC64 generator expects the caller to pass inverted seed value.
> Pass inverted crc value for metadata buffer.
> 
> Signed-off-by: Ankit Kumar <ankit.kumar@samsung.com>
> ---
>  hw/nvme/dif.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/nvme/dif.c b/hw/nvme/dif.c
> index 63c44c86ab..01b19c3373 100644
> --- a/hw/nvme/dif.c
> +++ b/hw/nvme/dif.c
> @@ -115,7 +115,7 @@ static void nvme_dif_pract_generate_dif_crc64(NvmeNamespace *ns, uint8_t *buf,
>          uint64_t crc = crc64_nvme(~0ULL, buf, ns->lbasz);
>  
>          if (pil) {
> -            crc = crc64_nvme(crc, mbuf, pil);
> +            crc = crc64_nvme(~crc, mbuf, pil);
>          }
>  
>          dif->g64.guard = cpu_to_be64(crc);
> @@ -246,7 +246,7 @@ static uint16_t nvme_dif_prchk_crc64(NvmeNamespace *ns, NvmeDifTuple *dif,
>          uint64_t crc = crc64_nvme(~0ULL, buf, ns->lbasz);
>  
>          if (pil) {
> -            crc = crc64_nvme(crc, mbuf, pil);
> +            crc = crc64_nvme(~crc, mbuf, pil);
>          }
>  
>          trace_pci_nvme_dif_prchk_guard_crc64(be64_to_cpu(dif->g64.guard), crc);
> -- 
> 2.25.1
> 

Good catch, thanks!

Reviewed-by: Klaus Jensen <k.jensen@samsung.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[PATCH 2/3] hw/nvme: fix disable pi checks for Type 3 protection

[Ankit Kumar]

As per the NVM command set specification, the protection information
checks for Type 3 protection are disabled, only when both application
and reference tag have all bits set to 1.

Signed-off-by: Ankit Kumar <ankit.kumar@samsung.com>
---
 hw/nvme/dif.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/hw/nvme/dif.c b/hw/nvme/dif.c
index 01b19c3373..f9bd29a2a6 100644
--- a/hw/nvme/dif.c
+++ b/hw/nvme/dif.c
@@ -157,7 +157,8 @@ static uint16_t nvme_dif_prchk_crc16(NvmeNamespace *ns, NvmeDifTuple *dif,
 {
     switch (NVME_ID_NS_DPS_TYPE(ns->id_ns.dps)) {
     case NVME_ID_NS_DPS_TYPE_3:
-        if (be32_to_cpu(dif->g16.reftag) != 0xffffffff) {
+        if ((be32_to_cpu(dif->g16.reftag) != 0xffffffff) ||
+            (be16_to_cpu(dif->g16.apptag) != 0xffff)) {
             break;
         }
 
@@ -225,7 +226,7 @@ static uint16_t nvme_dif_prchk_crc64(NvmeNamespace *ns, NvmeDifTuple *dif,
 
     switch (NVME_ID_NS_DPS_TYPE(ns->id_ns.dps)) {
     case NVME_ID_NS_DPS_TYPE_3:
-        if (r != 0xffffffffffff) {
+        if (r != 0xffffffffffff || (be16_to_cpu(dif->g64.apptag) != 0xffff)) {
             break;
         }
 
-- 
2.25.1

Re: [PATCH 2/3] hw/nvme: fix disable pi checks for Type 3 protection

[Klaus Jensen]

[-- Attachment #1: Type: text/plain, Size: 1488 bytes --]

On Aug  8 02:57, Ankit Kumar wrote:
> As per the NVM command set specification, the protection information
> checks for Type 3 protection are disabled, only when both application
> and reference tag have all bits set to 1.
> 
> Signed-off-by: Ankit Kumar <ankit.kumar@samsung.com>
> ---
>  hw/nvme/dif.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/nvme/dif.c b/hw/nvme/dif.c
> index 01b19c3373..f9bd29a2a6 100644
> --- a/hw/nvme/dif.c
> +++ b/hw/nvme/dif.c
> @@ -157,7 +157,8 @@ static uint16_t nvme_dif_prchk_crc16(NvmeNamespace *ns, NvmeDifTuple *dif,
>  {
>      switch (NVME_ID_NS_DPS_TYPE(ns->id_ns.dps)) {
>      case NVME_ID_NS_DPS_TYPE_3:
> -        if (be32_to_cpu(dif->g16.reftag) != 0xffffffff) {
> +        if ((be32_to_cpu(dif->g16.reftag) != 0xffffffff) ||
> +            (be16_to_cpu(dif->g16.apptag) != 0xffff)) {
>              break;
>          }

For type 3, if reftag is 0xffffffff the NVME_ID_NS_DPS_TYPE_3 case will
fallthrough to the next cases (_TYPE_1 and _TYPE_2), checking if apptag
is 0xffff, and disable checking if so. 

>  
> @@ -225,7 +226,7 @@ static uint16_t nvme_dif_prchk_crc64(NvmeNamespace *ns, NvmeDifTuple *dif,
>  
>      switch (NVME_ID_NS_DPS_TYPE(ns->id_ns.dps)) {
>      case NVME_ID_NS_DPS_TYPE_3:
> -        if (r != 0xffffffffffff) {
> +        if (r != 0xffffffffffff || (be16_to_cpu(dif->g64.apptag) != 0xffff)) {
>              break;
>          }

Same here.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[PATCH 3/3] docs: update hw/nvme documentation for protection information

[Ankit Kumar]

Add missing entry for pif ("protection information format").
Protection information size can be 8 or 16 bytes, Update the pil entry
as per the NVM command set specification.

Signed-off-by: Ankit Kumar <ankit.kumar@samsung.com>
---
 docs/system/devices/nvme.rst | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/docs/system/devices/nvme.rst b/docs/system/devices/nvme.rst
index 2a3af268f7..30d46d9338 100644
--- a/docs/system/devices/nvme.rst
+++ b/docs/system/devices/nvme.rst
@@ -271,9 +271,13 @@ The virtual namespace device supports DIF- and DIX-based protection information
 
 ``pil=UINT8`` (default: ``0``)
   Controls the location of the protection information within the metadata. Set
-  to ``1`` to transfer protection information as the first eight bytes of
-  metadata. Otherwise, the protection information is transferred as the last
-  eight bytes.
+  to ``1`` to transfer protection information as the first bytes of metadata.
+  Otherwise, the protection information is transferred as the last bytes of
+  metadata.
+
+``pif=UINT8`` (default: ``0``)
+  By default, the namespace device uses 16 bit guard protection information
+  format. Set to ``2`` to enable 64 bit guard protection information format.
 
 Virtualization Enhancements and SR-IOV (Experimental Support)
 -------------------------------------------------------------
-- 
2.25.1

Re: [PATCH 3/3] docs: update hw/nvme documentation for protection information

[Klaus Jensen]

[-- Attachment #1: Type: text/plain, Size: 1501 bytes --]

On Aug  8 02:57, Ankit Kumar wrote:
> Add missing entry for pif ("protection information format").
> Protection information size can be 8 or 16 bytes, Update the pil entry
> as per the NVM command set specification.
> 
> Signed-off-by: Ankit Kumar <ankit.kumar@samsung.com>
> ---
>  docs/system/devices/nvme.rst | 10 +++++++---
>  1 file changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/docs/system/devices/nvme.rst b/docs/system/devices/nvme.rst
> index 2a3af268f7..30d46d9338 100644
> --- a/docs/system/devices/nvme.rst
> +++ b/docs/system/devices/nvme.rst
> @@ -271,9 +271,13 @@ The virtual namespace device supports DIF- and DIX-based protection information
>  
>  ``pil=UINT8`` (default: ``0``)
>    Controls the location of the protection information within the metadata. Set
> -  to ``1`` to transfer protection information as the first eight bytes of
> -  metadata. Otherwise, the protection information is transferred as the last
> -  eight bytes.
> +  to ``1`` to transfer protection information as the first bytes of metadata.
> +  Otherwise, the protection information is transferred as the last bytes of
> +  metadata.
> +
> +``pif=UINT8`` (default: ``0``)
> +  By default, the namespace device uses 16 bit guard protection information
> +  format. Set to ``2`` to enable 64 bit guard protection information format.
>  

I'll add a small note that pif=1 (32b guard) is not supported.

Thanks,

Reviewed-by: Klaus Jensen <k.jensen@samsung.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 0/3] hw/nvme: bug fixes and doc update

[Michael Tokarev]

08.08.2023 00:27, Ankit Kumar wrote:
> This series fixes two bugs
> 1. CRC64 generation when metadata buffer is used.
> 2. Protection information disable check for Type 3 protection.
> 
> This series also updates the documentaion for pi (protection information),
> and adds missing pif (protection information format) entry.
> 
> Ankit Kumar (3):
>    hw/nvme: fix CRC64 for guard tag
>    hw/nvme: fix disable pi checks for Type 3 protection
>    docs: update hw/nvme documentation for protection information

At least the CRC64 change smells like a -stable material, - the bug
is present in, for example, qemu-7.2 too. But I don't know how important
it is to keep nvme updated in 8.0 or before, and what the outcome of this
bug is, to begin with. Somehow I think nvme was in preliminary shape
before 8.0.

Are the other changes also relevant for -stable?

Please keep Cc: qemu-stable@nongnu.org for anything you think is worth
to have in previous/stable releases.

Thanks,

/mjt

Re: [PATCH 0/3] hw/nvme: bug fixes and doc update

[Klaus Jensen]

[-- Attachment #1: Type: text/plain, Size: 1225 bytes --]

On Aug  8 09:04, Michael Tokarev wrote:
> 08.08.2023 00:27, Ankit Kumar wrote:
> > This series fixes two bugs
> > 1. CRC64 generation when metadata buffer is used.
> > 2. Protection information disable check for Type 3 protection.
> > 
> > This series also updates the documentaion for pi (protection information),
> > and adds missing pif (protection information format) entry.
> > 
> > Ankit Kumar (3):
> >    hw/nvme: fix CRC64 for guard tag
> >    hw/nvme: fix disable pi checks for Type 3 protection
> >    docs: update hw/nvme documentation for protection information
> 
> At least the CRC64 change smells like a -stable material, - the bug
> is present in, for example, qemu-7.2 too. But I don't know how important
> it is to keep nvme updated in 8.0 or before, and what the outcome of this
> bug is, to begin with. Somehow I think nvme was in preliminary shape
> before 8.0.
> 
> Are the other changes also relevant for -stable?
> 
> Please keep Cc: qemu-stable@nongnu.org for anything you think is worth
> to have in previous/stable releases.
> 

Hi Michael,

Yes, this is stable worthy.

I'll add Cc: stable and fixes tags on relevant patches on the pull.

Thanks for the heads up!

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]