[v3,3/4] hw/block/nvme: Fix pmrmsc register size
diff mbox series

Message ID 20200630110429.19972-4-philmd@redhat.com
State New
Headers show
Series
  • hw/block/nvme: Fix I/O BAR structure
Related show

Commit Message

Philippe Mathieu-Daudé June 30, 2020, 11:04 a.m. UTC
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(-)

Comments

Andrzej Jakowski June 30, 2020, 3:10 p.m. UTC | #1
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>
Philippe Mathieu-Daudé June 30, 2020, 3:16 p.m. UTC | #2
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>
>
Klaus Jensen June 30, 2020, 4:45 p.m. UTC | #3
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?
Andrzej Jakowski July 1, 2020, 11:08 p.m. UTC | #4
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?
>
Dmitry Fomichev July 13, 2020, 1:07 a.m. UTC | #5
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>

Patch
diff mbox series

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 {