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

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

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