Re: [PATCH v3 3/4] hw/block/nvme: Fix pmrmsc register size

["Philippe Mathieu-Daudé" <philmd@redhat.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>
>