| Message ID | 20200630110429.19972-4-philmd@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | hw/block/nvme: Fix I/O BAR structure | expand |
On 6/30/20 4:04 AM, Philippe Mathieu-Daudé wrote: > The Persistent Memory Region Controller Memory Space Control > register is 64-bit wide. See 'Figure 68: Register Definition' > of the 'NVM Express Base Specification Revision 1.4'. > > Fixes: 6cf9413229 ("introduce PMR support from NVMe 1.4 spec") > Reported-by: Klaus Jensen <k.jensen@samsung.com> > Reviewed-by: Klaus Jensen <k.jensen@samsung.com> > Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> > --- > Cc: Andrzej Jakowski <andrzej.jakowski@linux.intel.com> > --- > include/block/nvme.h | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/include/block/nvme.h b/include/block/nvme.h > index 71c5681912..82c384614a 100644 > --- a/include/block/nvme.h > +++ b/include/block/nvme.h > @@ -21,7 +21,7 @@ typedef struct QEMU_PACKED NvmeBar { > uint32_t pmrsts; > uint32_t pmrebs; > uint32_t pmrswtp; > - uint32_t pmrmsc; > + uint64_t pmrmsc; > } NvmeBar; > > enum NvmeCapShift { > -- 2.21.3 This is good catch, though I wanted to highlight that this will still need to change as this register is not aligned properly and thus not in compliance with spec. Reviewed-by Andrzej Jakowski <andrzej.jakowski@linux.intel.com>
On 6/30/20 5:10 PM, Andrzej Jakowski wrote: > On 6/30/20 4:04 AM, Philippe Mathieu-Daudé wrote: >> The Persistent Memory Region Controller Memory Space Control >> register is 64-bit wide. See 'Figure 68: Register Definition' >> of the 'NVM Express Base Specification Revision 1.4'. >> >> Fixes: 6cf9413229 ("introduce PMR support from NVMe 1.4 spec") >> Reported-by: Klaus Jensen <k.jensen@samsung.com> >> Reviewed-by: Klaus Jensen <k.jensen@samsung.com> >> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> >> --- >> Cc: Andrzej Jakowski <andrzej.jakowski@linux.intel.com> >> --- >> include/block/nvme.h | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/include/block/nvme.h b/include/block/nvme.h >> index 71c5681912..82c384614a 100644 >> --- a/include/block/nvme.h >> +++ b/include/block/nvme.h >> @@ -21,7 +21,7 @@ typedef struct QEMU_PACKED NvmeBar { >> uint32_t pmrsts; >> uint32_t pmrebs; >> uint32_t pmrswtp; >> - uint32_t pmrmsc; >> + uint64_t pmrmsc; >> } NvmeBar; >> >> enum NvmeCapShift { >> -- 2.21.3 > > This is good catch, though I wanted to highlight that this will still > need to change as this register is not aligned properly and thus not in > compliance with spec. I was wondering the odd alignment too. So you are saying at some time in the future the spec will be updated to correct the alignment? Should we use this instead? uint32_t pmrmsc; + uint32_t pmrmsc_upper32; /* the spec define this, but * + * only low 32-bit are used */ Or eventually an unnamed struct: - uint32_t pmrmsc; + struct { + uint32_t pmrmsc; + uint32_t pmrmsc_upper32; /* the spec define this, but * + * only low 32-bit are used */ + }; > > Reviewed-by Andrzej Jakowski <andrzej.jakowski@linux.intel.com> >
On Jun 30 17:16, Philippe Mathieu-Daudé wrote: > On 6/30/20 5:10 PM, Andrzej Jakowski wrote: > > On 6/30/20 4:04 AM, Philippe Mathieu-Daudé wrote: > >> The Persistent Memory Region Controller Memory Space Control > >> register is 64-bit wide. See 'Figure 68: Register Definition' > >> of the 'NVM Express Base Specification Revision 1.4'. > >> > >> Fixes: 6cf9413229 ("introduce PMR support from NVMe 1.4 spec") > >> Reported-by: Klaus Jensen <k.jensen@samsung.com> > >> Reviewed-by: Klaus Jensen <k.jensen@samsung.com> > >> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> > >> --- > >> Cc: Andrzej Jakowski <andrzej.jakowski@linux.intel.com> > >> --- > >> include/block/nvme.h | 2 +- > >> 1 file changed, 1 insertion(+), 1 deletion(-) > >> > >> diff --git a/include/block/nvme.h b/include/block/nvme.h > >> index 71c5681912..82c384614a 100644 > >> --- a/include/block/nvme.h > >> +++ b/include/block/nvme.h > >> @@ -21,7 +21,7 @@ typedef struct QEMU_PACKED NvmeBar { > >> uint32_t pmrsts; > >> uint32_t pmrebs; > >> uint32_t pmrswtp; > >> - uint32_t pmrmsc; > >> + uint64_t pmrmsc; > >> } NvmeBar; > >> > >> enum NvmeCapShift { > >> -- 2.21.3 > > > > This is good catch, though I wanted to highlight that this will still > > need to change as this register is not aligned properly and thus not in > > compliance with spec. > > I was wondering the odd alignment too. So you are saying at some time > in the future the spec will be updated to correct the alignment? > > Should we use this instead? > > uint32_t pmrmsc; > + uint32_t pmrmsc_upper32; /* the spec define this, but * > + * only low 32-bit are used */ > > Or eventually an unnamed struct: > > - uint32_t pmrmsc; > + struct { > + uint32_t pmrmsc; > + uint32_t pmrmsc_upper32; /* the spec define this, but * > + * only low 32-bit are used */ > + }; > > > > > Reviewed-by Andrzej Jakowski <andrzej.jakowski@linux.intel.com> > > > I'm also not sure what you mean Andrzej. The odd alignment is exactly what the spec (v1.4) says?
On 6/30/20 9:45 AM, Klaus Jensen wrote: > On Jun 30 17:16, Philippe Mathieu-Daudé wrote: >> On 6/30/20 5:10 PM, Andrzej Jakowski wrote: >>> On 6/30/20 4:04 AM, Philippe Mathieu-Daudé wrote: >>>> The Persistent Memory Region Controller Memory Space Control >>>> register is 64-bit wide. See 'Figure 68: Register Definition' >>>> of the 'NVM Express Base Specification Revision 1.4'. >>>> >>>> Fixes: 6cf9413229 ("introduce PMR support from NVMe 1.4 spec") >>>> Reported-by: Klaus Jensen <k.jensen@samsung.com> >>>> Reviewed-by: Klaus Jensen <k.jensen@samsung.com> >>>> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> >>>> --- >>>> Cc: Andrzej Jakowski <andrzej.jakowski@linux.intel.com> >>>> --- >>>> include/block/nvme.h | 2 +- >>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>> >>>> diff --git a/include/block/nvme.h b/include/block/nvme.h >>>> index 71c5681912..82c384614a 100644 >>>> --- a/include/block/nvme.h >>>> +++ b/include/block/nvme.h >>>> @@ -21,7 +21,7 @@ typedef struct QEMU_PACKED NvmeBar { >>>> uint32_t pmrsts; >>>> uint32_t pmrebs; >>>> uint32_t pmrswtp; >>>> - uint32_t pmrmsc; >>>> + uint64_t pmrmsc; >>>> } NvmeBar; >>>> >>>> enum NvmeCapShift { >>>> -- 2.21.3 >>> >>> This is good catch, though I wanted to highlight that this will still >>> need to change as this register is not aligned properly and thus not in >>> compliance with spec. >> >> I was wondering the odd alignment too. So you are saying at some time >> in the future the spec will be updated to correct the alignment? Yep I think so. So PMRMSC currently is 64-bit register that is defined at E14h offset. It is in conflict with spec because spec requires 64-bit registers to be 64-bit aligned. I anticipate that changes will be needed. >> >> Should we use this instead? >> >> uint32_t pmrmsc; >> + uint32_t pmrmsc_upper32; /* the spec define this, but * >> + * only low 32-bit are used */ >> >> Or eventually an unnamed struct: >> >> - uint32_t pmrmsc; >> + struct { >> + uint32_t pmrmsc; >> + uint32_t pmrmsc_upper32; /* the spec define this, but * >> + * only low 32-bit are used */ >> + }; >> >>> >>> Reviewed-by Andrzej Jakowski <andrzej.jakowski@linux.intel.com> >>> >> > > I'm also not sure what you mean Andrzej. The odd alignment is exactly > what the spec (v1.4) says? >
On Tue, 2020-06-30 at 13:04 +0200, Philippe Mathieu-Daudé wrote: > The Persistent Memory Region Controller Memory Space Control > register is 64-bit wide. See 'Figure 68: Register Definition' > of the 'NVM Express Base Specification Revision 1.4'. > > Fixes: 6cf9413229 ("introduce PMR support from NVMe 1.4 spec") > Reported-by: Klaus Jensen <k.jensen@samsung.com> > Reviewed-by: Klaus Jensen <k.jensen@samsung.com> > Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> > --- > Cc: Andrzej Jakowski <andrzej.jakowski@linux.intel.com> > --- > include/block/nvme.h | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/include/block/nvme.h b/include/block/nvme.h > index 71c5681912..82c384614a 100644 > --- a/include/block/nvme.h > +++ b/include/block/nvme.h > @@ -21,7 +21,7 @@ typedef struct QEMU_PACKED NvmeBar { > uint32_t pmrsts; > uint32_t pmrebs; > uint32_t pmrswtp; > - uint32_t pmrmsc; > + uint64_t pmrmsc; > } NvmeBar; > > enum NvmeCapShift { > -- > 2.21.3 > > Reviewed-by: Dmitry Fomichev <dmitry.fomichev@wdc.com>
diff --git a/include/block/nvme.h b/include/block/nvme.h index 71c5681912..82c384614a 100644 --- a/include/block/nvme.h +++ b/include/block/nvme.h @@ -21,7 +21,7 @@ typedef struct QEMU_PACKED NvmeBar { uint32_t pmrsts; uint32_t pmrebs; uint32_t pmrswtp; - uint32_t pmrmsc; + uint64_t pmrmsc; } NvmeBar; enum NvmeCapShift {
The Persistent Memory Region Controller Memory Space Control register is 64-bit wide. See 'Figure 68: Register Definition' of the 'NVM Express Base Specification Revision 1.4'. Fixes: 6cf9413229 ("introduce PMR support from NVMe 1.4 spec") Reported-by: Klaus Jensen <k.jensen@samsung.com> Reviewed-by: Klaus Jensen <k.jensen@samsung.com> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> --- Cc: Andrzej Jakowski <andrzej.jakowski@linux.intel.com> --- include/block/nvme.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)