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.jen...@samsung.com>
> >> Reviewed-by: Klaus Jensen <k.jen...@samsung.com>
> >> Signed-off-by: Philippe Mathieu-Daudé <phi...@redhat.com>
> >> ---
> >> Cc: Andrzej Jakowski <andrzej.jakow...@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.jakow...@linux.intel.com>
> >
>
I'm also not sure what you mean Andrzej. The odd alignment is exactly
what the spec (v1.4) says?
