nvme: fix LBA format data structure

[SeaBIOS] [PATCH] nvme: fix LBA format data structure

Posted by Florian Larysch 2 years, 3 months ago

The LBA Format Data structure is dword-sized, but struct nvme_lba_format
erroneously contains an additional member, misaligning all LBAF
descriptors after the first and causing them to be misinterpreted.
Remove it.

Signed-off-by: Florian Larysch <fl@n621.de>
---
 src/hw/nvme-int.h | 1 -
 1 file changed, 1 deletion(-)

diff --git a/src/hw/nvme-int.h b/src/hw/nvme-int.h
index a4c1555..5779203 100644
--- a/src/hw/nvme-int.h
+++ b/src/hw/nvme-int.h
@@ -156,7 +156,6 @@ struct nvme_lba_format {
     u16 ms;
     u8  lbads;
     u8  rp;
-    u8  res;
 };
 
 struct nvme_identify_ns {
-- 
2.34.1

_______________________________________________
SeaBIOS mailing list -- seabios@seabios.org
To unsubscribe send an email to seabios-leave@seabios.org

[SeaBIOS] Re: [PATCH] nvme: fix LBA format data structure

Posted by Kevin O'Connor 2 years, 2 months ago

On Sun, Jan 23, 2022 at 05:43:57PM +0100, Florian Larysch wrote:
> The LBA Format Data structure is dword-sized, but struct nvme_lba_format
> erroneously contains an additional member, misaligning all LBAF
> descriptors after the first and causing them to be misinterpreted.
> Remove it.

Thanks.  I don't know enough about NVMe to review this patch though.
Maybe Julian or Alex could comment?

-Kevin


> 
> Signed-off-by: Florian Larysch <fl@n621.de>
> ---
>  src/hw/nvme-int.h | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/src/hw/nvme-int.h b/src/hw/nvme-int.h
> index a4c1555..5779203 100644
> --- a/src/hw/nvme-int.h
> +++ b/src/hw/nvme-int.h
> @@ -156,7 +156,6 @@ struct nvme_lba_format {
>      u16 ms;
>      u8  lbads;
>      u8  rp;
> -    u8  res;
>  };
>  
>  struct nvme_identify_ns {
> -- 
> 2.34.1
> 
> _______________________________________________
> SeaBIOS mailing list -- seabios@seabios.org
> To unsubscribe send an email to seabios-leave@seabios.org
_______________________________________________
SeaBIOS mailing list -- seabios@seabios.org
To unsubscribe send an email to seabios-leave@seabios.org

[SeaBIOS] Re: [PATCH] nvme: fix LBA format data structure

Posted by Florian Larysch 2 years, 2 months ago

On Thu, Jan 27, 2022 at 11:37:52AM -0500, Kevin O'Connor wrote:
> Thanks.  I don't know enough about NVMe to review this patch though.
> Maybe Julian or Alex could comment?

Happy to hear their comments on this. However, I can also try to explain
the reasoning in a bit more detail.

This follows from the NVMe spec[1]: The Identify command returns a data
structure that contains packed LBAF records (Figure 249, starting at
offset 131). This is represented by struct nvme_identify_ns in the
SeaBIOS code.

Figure 250 gives the structure of these records and this is where the
aforementioned discrepancy lies.

I think what happened was that somebody mistook the reserved 6 uppermost
bits as a whole byte and put it into struct nvme_lba_format as such. To
correctly parse the RP field, you'd still need to mask off these bits,
but the code only uses the LBADS/MS fields anyway.

I encountered this on an NVMe device that had FLBAS[3:0] != 0, which
caused nonsensical outputs of the various dprintf()s and also throws off
everything that depends on knowing the block size. Empirically, the
patch fixes these problems.

[1] https://nvmexpress.org/wp-content/uploads/NVM-Express-1_4c-2021.06.28-Ratified.pdf
_______________________________________________
SeaBIOS mailing list -- seabios@seabios.org
To unsubscribe send an email to seabios-leave@seabios.org

[SeaBIOS] Re: [PATCH] nvme: fix LBA format data structure

Posted by Kevin O'Connor 2 years, 2 months ago

On Tue, Feb 01, 2022 at 08:39:10PM +0100, Florian Larysch wrote:
> On Thu, Jan 27, 2022 at 11:37:52AM -0500, Kevin O'Connor wrote:
> > Thanks.  I don't know enough about NVMe to review this patch though.
> > Maybe Julian or Alex could comment?
> 
> Happy to hear their comments on this. However, I can also try to explain
> the reasoning in a bit more detail.
> 
> This follows from the NVMe spec[1]: The Identify command returns a data
> structure that contains packed LBAF records (Figure 249, starting at
> offset 131). This is represented by struct nvme_identify_ns in the
> SeaBIOS code.
> 
> Figure 250 gives the structure of these records and this is where the
> aforementioned discrepancy lies.

Thanks.  I agree that this should be fixed in SeaBIOS.

However, your patch removes the "reserved" field, which doesn't look
correct.  It sounds like the "res" struct member should remain, but
the "lbads" and "rp" members should be turned into a new "lbads_rp"
member.  Also, the nvme.c code should be updated so that the read of
lbads performs the proper bit masking.

-Kevin


> 
> I think what happened was that somebody mistook the reserved 6 uppermost
> bits as a whole byte and put it into struct nvme_lba_format as such. To
> correctly parse the RP field, you'd still need to mask off these bits,
> but the code only uses the LBADS/MS fields anyway.
> 
> I encountered this on an NVMe device that had FLBAS[3:0] != 0, which
> caused nonsensical outputs of the various dprintf()s and also throws off
> everything that depends on knowing the block size. Empirically, the
> patch fixes these problems.
> 
> [1] https://nvmexpress.org/wp-content/uploads/NVM-Express-1_4c-2021.06.28-Ratified.pdf
_______________________________________________
SeaBIOS mailing list -- seabios@seabios.org
To unsubscribe send an email to seabios-leave@seabios.org

[SeaBIOS] Re: [PATCH] nvme: fix LBA format data structure

Posted by Alexander Graf via SeaBIOS 2 years, 2 months ago


On 02.02.22 02:52, Kevin O'Connor wrote:
> On Tue, Feb 01, 2022 at 08:39:10PM +0100, Florian Larysch wrote:
>> On Thu, Jan 27, 2022 at 11:37:52AM -0500, Kevin O'Connor wrote:
>>> Thanks.  I don't know enough about NVMe to review this patch though.
>>> Maybe Julian or Alex could comment?
>> Happy to hear their comments on this. However, I can also try to explain
>> the reasoning in a bit more detail.
>>
>> This follows from the NVMe spec[1]: The Identify command returns a data
>> structure that contains packed LBAF records (Figure 249, starting at
>> offset 131). This is represented by struct nvme_identify_ns in the
>> SeaBIOS code.
>>
>> Figure 250 gives the structure of these records and this is where the
>> aforementioned discrepancy lies.
> Thanks.  I agree that this should be fixed in SeaBIOS.
>
> However, your patch removes the "reserved" field, which doesn't look
> correct.  It sounds like the "res" struct member should remain, but
> the "lbads" and "rp" members should be turned into a new "lbads_rp"
> member.  Also, the nvme.c code should be updated so that the read of
> lbads performs the proper bit masking.


Looking at the spec, lbads is a full well aligned 8 bits. The collision 
here is between rp and res, with rp taking only 2 bits and res the 
remaining 6. Is using bitfields acceptable in SeaBIOS? If so, the patch 
below should automatically give us masking as well.

Alex

diff --git a/src/hw/nvme-int.h b/src/hw/nvme-int.h
index f9c807e..9b3015b 100644
--- a/src/hw/nvme-int.h
+++ b/src/hw/nvme-int.h
@@ -145,8 +145,8 @@ struct nvme_identify_ns_list {
  struct nvme_lba_format {
      u16 ms;
      u8  lbads;
-    u8  rp;
-    u8  res;
+    u8  rp : 2;
+    u8  res : 6;
  };

  struct nvme_identify_ns {




Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrung: Christian Schlaeger, Jonathan Weiss
Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B
Sitz: Berlin
Ust-ID: DE 289 237 879


_______________________________________________
SeaBIOS mailing list -- seabios@seabios.org
To unsubscribe send an email to seabios-leave@seabios.org

[SeaBIOS] Re: [PATCH] nvme: fix LBA format data structure

Posted by Kevin O'Connor 2 years, 2 months ago

On Wed, Feb 02, 2022 at 11:50:50AM +0100, Alexander Graf wrote:
> 
> On 02.02.22 02:52, Kevin O'Connor wrote:
> > On Tue, Feb 01, 2022 at 08:39:10PM +0100, Florian Larysch wrote:
> > > On Thu, Jan 27, 2022 at 11:37:52AM -0500, Kevin O'Connor wrote:
> > > > Thanks.  I don't know enough about NVMe to review this patch though.
> > > > Maybe Julian or Alex could comment?
> > > Happy to hear their comments on this. However, I can also try to explain
> > > the reasoning in a bit more detail.
> > > 
> > > This follows from the NVMe spec[1]: The Identify command returns a data
> > > structure that contains packed LBAF records (Figure 249, starting at
> > > offset 131). This is represented by struct nvme_identify_ns in the
> > > SeaBIOS code.
> > > 
> > > Figure 250 gives the structure of these records and this is where the
> > > aforementioned discrepancy lies.
> > Thanks.  I agree that this should be fixed in SeaBIOS.
> > 
> > However, your patch removes the "reserved" field, which doesn't look
> > correct.  It sounds like the "res" struct member should remain, but
> > the "lbads" and "rp" members should be turned into a new "lbads_rp"
> > member.  Also, the nvme.c code should be updated so that the read of
> > lbads performs the proper bit masking.
> 
> 
> Looking at the spec, lbads is a full well aligned 8 bits. The collision here
> is between rp and res, with rp taking only 2 bits and res the remaining 6.

Ah, I misread the spec.  So, the original patch removing the "res"
struct member should be fine.

> Is using bitfields acceptable in SeaBIOS? If so, the patch below should
> automatically give us masking as well.

I'd recommend against using C bitfields to access hardware registers.

-Kevin
_______________________________________________
SeaBIOS mailing list -- seabios@seabios.org
To unsubscribe send an email to seabios-leave@seabios.org

[SeaBIOS] Re: [PATCH] nvme: fix LBA format data structure

Posted by Alexander Graf via SeaBIOS 2 years, 2 months ago

On 02.02.22 16:24, Kevin O'Connor wrote:
> On Wed, Feb 02, 2022 at 11:50:50AM +0100, Alexander Graf wrote:
>> On 02.02.22 02:52, Kevin O'Connor wrote:
>>> On Tue, Feb 01, 2022 at 08:39:10PM +0100, Florian Larysch wrote:
>>>> On Thu, Jan 27, 2022 at 11:37:52AM -0500, Kevin O'Connor wrote:
>>>>> Thanks.  I don't know enough about NVMe to review this patch though.
>>>>> Maybe Julian or Alex could comment?
>>>> Happy to hear their comments on this. However, I can also try to explain
>>>> the reasoning in a bit more detail.
>>>>
>>>> This follows from the NVMe spec[1]: The Identify command returns a data
>>>> structure that contains packed LBAF records (Figure 249, starting at
>>>> offset 131). This is represented by struct nvme_identify_ns in the
>>>> SeaBIOS code.
>>>>
>>>> Figure 250 gives the structure of these records and this is where the
>>>> aforementioned discrepancy lies.
>>> Thanks.  I agree that this should be fixed in SeaBIOS.
>>>
>>> However, your patch removes the "reserved" field, which doesn't look
>>> correct.  It sounds like the "res" struct member should remain, but
>>> the "lbads" and "rp" members should be turned into a new "lbads_rp"
>>> member.  Also, the nvme.c code should be updated so that the read of
>>> lbads performs the proper bit masking.
>>
>> Looking at the spec, lbads is a full well aligned 8 bits. The collision here
>> is between rp and res, with rp taking only 2 bits and res the remaining 6.
> Ah, I misread the spec.  So, the original patch removing the "res"
> struct member should be fine.


Yes, the original patch works too. There is currently no user of the rp 
field, so nothing to mask against.


>
>> Is using bitfields acceptable in SeaBIOS? If so, the patch below should
>> automatically give us masking as well.
> I'd recommend against using C bitfields to access hardware registers.


This is an in-RAM data structure. But in any case, let's just go with 
the original patch.


Reviewed-by: Alexander Graf <graf@amazon.com>

Alex





Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrung: Christian Schlaeger, Jonathan Weiss
Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B
Sitz: Berlin
Ust-ID: DE 289 237 879


_______________________________________________
SeaBIOS mailing list -- seabios@seabios.org
To unsubscribe send an email to seabios-leave@seabios.org