Re: [PATCH v3 3/4] hw/block/nvme: Fix pmrmsc register size
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
> Reviewed-by: Klaus Jensen
> Signed-off-by: Philippe Mathieu-Daudé
> ---
> Cc: Andrzej Jakowski
> ---
> 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_tpmrsts;
> uint32_tpmrebs;
> uint32_tpmrswtp;
> -uint32_tpmrmsc;
> +uint64_tpmrmsc;
> } NvmeBar;
>
> enum NvmeCapShift {
> --
> 2.21.3
>
>
Reviewed-by: Dmitry Fomichev
Re: [PATCH v3 3/4] hw/block/nvme: Fix pmrmsc register size
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
Reviewed-by: Klaus Jensen
Signed-off-by: Philippe Mathieu-Daudé
---
Cc: Andrzej Jakowski
---
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_tpmrsts;
uint32_tpmrebs;
uint32_tpmrswtp;
-uint32_tpmrmsc;
+uint64_tpmrmsc;
} 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_tpmrmsc;
>> +uint32_tpmrmsc_upper32; /* the spec define this, but *
>> + * only low 32-bit are used */
>>
>> Or eventually an unnamed struct:
>>
>> -uint32_tpmrmsc;
>> +struct {
>> +uint32_t pmrmsc;
>> +uint32_t pmrmsc_upper32; /* the spec define this, but *
>> + * only low 32-bit are used */
>> +};
>>
>>>
>>> Reviewed-by Andrzej Jakowski
>>>
>>
>
> I'm also not sure what you mean Andrzej. The odd alignment is exactly
> what the spec (v1.4) says?
>
Re: [PATCH v3 3/4] hw/block/nvme: Fix pmrmsc register size
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
> >> Reviewed-by: Klaus Jensen
> >> Signed-off-by: Philippe Mathieu-Daudé
> >> ---
> >> Cc: Andrzej Jakowski
> >> ---
> >> 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_tpmrsts;
> >> uint32_tpmrebs;
> >> uint32_tpmrswtp;
> >> -uint32_tpmrmsc;
> >> +uint64_tpmrmsc;
> >> } 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_tpmrmsc;
> +uint32_tpmrmsc_upper32; /* the spec define this, but *
> + * only low 32-bit are used */
>
> Or eventually an unnamed struct:
>
> -uint32_tpmrmsc;
> +struct {
> +uint32_t pmrmsc;
> +uint32_t pmrmsc_upper32; /* the spec define this, but *
> + * only low 32-bit are used */
> +};
>
> >
> > Reviewed-by Andrzej Jakowski
> >
>
I'm also not sure what you mean Andrzej. The odd alignment is exactly
what the spec (v1.4) says?
Re: [PATCH v3 3/4] hw/block/nvme: Fix pmrmsc register size
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
>> Reviewed-by: Klaus Jensen
>> Signed-off-by: Philippe Mathieu-Daudé
>> ---
>> Cc: Andrzej Jakowski
>> ---
>> 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_tpmrsts;
>> uint32_tpmrebs;
>> uint32_tpmrswtp;
>> -uint32_tpmrmsc;
>> +uint64_tpmrmsc;
>> } 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_tpmrmsc;
+uint32_tpmrmsc_upper32; /* the spec define this, but *
+ * only low 32-bit are used */
Or eventually an unnamed struct:
-uint32_tpmrmsc;
+struct {
+uint32_t pmrmsc;
+uint32_t pmrmsc_upper32; /* the spec define this, but *
+ * only low 32-bit are used */
+};
>
> Reviewed-by Andrzej Jakowski
>
Re: [PATCH v3 3/4] hw/block/nvme: Fix pmrmsc register size
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
> Reviewed-by: Klaus Jensen
> Signed-off-by: Philippe Mathieu-Daudé
> ---
> Cc: Andrzej Jakowski
> ---
> 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_tpmrsts;
> uint32_tpmrebs;
> uint32_tpmrswtp;
> -uint32_tpmrmsc;
> +uint64_tpmrmsc;
> } 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
[PATCH v3 3/4] hw/block/nvme: Fix pmrmsc register size
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
Reviewed-by: Klaus Jensen
Signed-off-by: Philippe Mathieu-Daudé
---
Cc: Andrzej Jakowski
---
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_tpmrsts;
uint32_tpmrebs;
uint32_tpmrswtp;
-uint32_tpmrmsc;
+uint64_tpmrmsc;
} NvmeBar;
enum NvmeCapShift {
--
2.21.3
