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
© 2016 - 2023 Red Hat, Inc.