hw/block/nvme: Align I/O BAR to 4 KiB

[PATCH] hw/block/nvme: Align I/O BAR to 4 KiB

Posted by Philippe Mathieu-Daudé 3 years, 10 months ago

Simplify the NVMe emulated device by aligning the I/O BAR to 4 KiB.

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 include/block/nvme.h | 3 +++
 hw/block/nvme.c      | 5 ++---
 2 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/include/block/nvme.h b/include/block/nvme.h
index 1720ee1d51..6d87c9c146 100644
--- a/include/block/nvme.h
+++ b/include/block/nvme.h
@@ -22,6 +22,8 @@ typedef struct NvmeBar {
     uint32_t    pmrebs;
     uint32_t    pmrswtp;
     uint32_t    pmrmsc;
+    uint32_t    reserved[58];
+    uint8_t     cmd_set_specfic[0x100];
 } NvmeBar;
 
 enum NvmeCapShift {
@@ -879,6 +881,7 @@ enum NvmeIdNsDps {
 
 static inline void _nvme_check_size(void)
 {
+    QEMU_BUILD_BUG_ON(sizeof(NvmeBar) != 4096);
     QEMU_BUILD_BUG_ON(sizeof(NvmeAerResult) != 4);
     QEMU_BUILD_BUG_ON(sizeof(NvmeCqe) != 16);
     QEMU_BUILD_BUG_ON(sizeof(NvmeDsmRange) != 16);
diff --git a/hw/block/nvme.c b/hw/block/nvme.c
index 1aee042d4c..1938891e50 100644
--- a/hw/block/nvme.c
+++ b/hw/block/nvme.c
@@ -55,7 +55,6 @@
 #include "nvme.h"
 
 #define NVME_MAX_IOQPAIRS 0xffff
-#define NVME_REG_SIZE 0x1000
 #define NVME_DB_SIZE  4
 #define NVME_CMB_BIR 2
 #define NVME_PMR_BIR 2
@@ -1322,7 +1321,7 @@ static void nvme_mmio_write(void *opaque, hwaddr addr, uint64_t data,
     NvmeCtrl *n = (NvmeCtrl *)opaque;
     if (addr < sizeof(n->bar)) {
         nvme_write_bar(n, addr, data, size);
-    } else if (addr >= 0x1000) {
+    } else {
         nvme_process_db(n, addr, data);
     }
 }
@@ -1416,7 +1415,7 @@ static void nvme_init_state(NvmeCtrl *n)
 {
     n->num_namespaces = 1;
     /* add one to max_ioqpairs to account for the admin queue pair */
-    n->reg_size = pow2ceil(NVME_REG_SIZE +
+    n->reg_size = pow2ceil(sizeof(NvmeBar) +
                            2 * (n->params.max_ioqpairs + 1) * NVME_DB_SIZE);
     n->namespaces = g_new0(NvmeNamespace, n->num_namespaces);
     n->sq = g_new0(NvmeSQueue *, n->params.max_ioqpairs + 1);
-- 
2.21.3

Re: [PATCH] hw/block/nvme: Align I/O BAR to 4 KiB

Posted by Klaus Jensen 3 years, 10 months ago

On Jun 25 17:48, Philippe Mathieu-Daudé wrote:
> Simplify the NVMe emulated device by aligning the I/O BAR to 4 KiB.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  include/block/nvme.h | 3 +++
>  hw/block/nvme.c      | 5 ++---
>  2 files changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/include/block/nvme.h b/include/block/nvme.h
> index 1720ee1d51..6d87c9c146 100644
> --- a/include/block/nvme.h
> +++ b/include/block/nvme.h
> @@ -22,6 +22,8 @@ typedef struct NvmeBar {
>      uint32_t    pmrebs;
>      uint32_t    pmrswtp;
>      uint32_t    pmrmsc;
> +    uint32_t    reserved[58];
> +    uint8_t     cmd_set_specfic[0x100];
>  } NvmeBar;

This ends up as a freak mix of v1.3 and v1.4 specs. Since we already
have the PMR stuff in there, I think it makes more sense to align with
v1.4 and remove the reserved bytes.

Otherwise, LGTM.

Reviewed-by: Klaus Jensen <k.jensen@samsung.com>

Re: [PATCH] hw/block/nvme: Align I/O BAR to 4 KiB

Posted by Philippe Mathieu-Daudé 3 years, 9 months ago

Hi Klaus,

On 6/25/20 8:23 PM, Klaus Jensen wrote:
> On Jun 25 17:48, Philippe Mathieu-Daudé wrote:
>> Simplify the NVMe emulated device by aligning the I/O BAR to 4 KiB.
>>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>> ---
>>  include/block/nvme.h | 3 +++
>>  hw/block/nvme.c      | 5 ++---
>>  2 files changed, 5 insertions(+), 3 deletions(-)
>>
>> diff --git a/include/block/nvme.h b/include/block/nvme.h
>> index 1720ee1d51..6d87c9c146 100644
>> --- a/include/block/nvme.h
>> +++ b/include/block/nvme.h
>> @@ -22,6 +22,8 @@ typedef struct NvmeBar {
>>      uint32_t    pmrebs;
>>      uint32_t    pmrswtp;
>>      uint32_t    pmrmsc;
>> +    uint32_t    reserved[58];
>> +    uint8_t     cmd_set_specfic[0x100];
>>  } NvmeBar;
> 
> This ends up as a freak mix of v1.3 and v1.4 specs. Since we already
> have the PMR stuff in there, I think it makes more sense to align with
> v1.4 and remove the reserved bytes.

I'm sorry but I don't understand what you'd prefer, removing the
cmd_set_specfic[] for v1.3 and instead use this?

      uint32_t    pmrmsc;
 +    uint32_t    reserved[122];
  } NvmeBar;

Or this?

      uint32_t    pmrmsc;
 +    uint8_t     reserved[488];
  } NvmeBar;

> 
> Otherwise, LGTM.
> 
> Reviewed-by: Klaus Jensen <k.jensen@samsung.com>
>

Re: [PATCH] hw/block/nvme: Align I/O BAR to 4 KiB

Posted by Klaus Jensen 3 years, 9 months ago

On Jun 30 10:35, Philippe Mathieu-Daudé wrote:
> Hi Klaus,
> 
> On 6/25/20 8:23 PM, Klaus Jensen wrote:
> > On Jun 25 17:48, Philippe Mathieu-Daudé wrote:
> >> Simplify the NVMe emulated device by aligning the I/O BAR to 4 KiB.
> >>
> >> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> >> ---
> >>  include/block/nvme.h | 3 +++
> >>  hw/block/nvme.c      | 5 ++---
> >>  2 files changed, 5 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/include/block/nvme.h b/include/block/nvme.h
> >> index 1720ee1d51..6d87c9c146 100644
> >> --- a/include/block/nvme.h
> >> +++ b/include/block/nvme.h
> >> @@ -22,6 +22,8 @@ typedef struct NvmeBar {
> >>      uint32_t    pmrebs;
> >>      uint32_t    pmrswtp;
> >>      uint32_t    pmrmsc;
> >> +    uint32_t    reserved[58];
> >> +    uint8_t     cmd_set_specfic[0x100];
> >>  } NvmeBar;
> > 
> > This ends up as a freak mix of v1.3 and v1.4 specs. Since we already
> > have the PMR stuff in there, I think it makes more sense to align with
> > v1.4 and remove the reserved bytes.
> 
> I'm sorry but I don't understand what you'd prefer, removing the
> cmd_set_specfic[] for v1.3 and instead use this?
> 
>       uint32_t    pmrmsc;
>  +    uint32_t    reserved[122];
>   } NvmeBar;
> 
> Or this?
> 
>       uint32_t    pmrmsc;
>  +    uint8_t     reserved[488];
>   } NvmeBar;
> 

Yes, the second one.

But it should be 484 bytes reserved and the bug is in the pmrmsc field
that should be uint64_t. Can you fix that as well?  :)

Re: [PATCH] hw/block/nvme: Align I/O BAR to 4 KiB

Posted by Philippe Mathieu-Daudé 3 years, 9 months ago

On 6/30/20 10:46 AM, Klaus Jensen wrote:
> On Jun 30 10:35, Philippe Mathieu-Daudé wrote:
>> Hi Klaus,
>>
>> On 6/25/20 8:23 PM, Klaus Jensen wrote:
>>> On Jun 25 17:48, Philippe Mathieu-Daudé wrote:
>>>> Simplify the NVMe emulated device by aligning the I/O BAR to 4 KiB.
>>>>
>>>> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>>>> ---
>>>>  include/block/nvme.h | 3 +++
>>>>  hw/block/nvme.c      | 5 ++---
>>>>  2 files changed, 5 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/include/block/nvme.h b/include/block/nvme.h
>>>> index 1720ee1d51..6d87c9c146 100644
>>>> --- a/include/block/nvme.h
>>>> +++ b/include/block/nvme.h
>>>> @@ -22,6 +22,8 @@ typedef struct NvmeBar {
>>>>      uint32_t    pmrebs;
>>>>      uint32_t    pmrswtp;
>>>>      uint32_t    pmrmsc;
>>>> +    uint32_t    reserved[58];
>>>> +    uint8_t     cmd_set_specfic[0x100];
>>>>  } NvmeBar;
>>>
>>> This ends up as a freak mix of v1.3 and v1.4 specs. Since we already
>>> have the PMR stuff in there, I think it makes more sense to align with
>>> v1.4 and remove the reserved bytes.
>>
>> I'm sorry but I don't understand what you'd prefer, removing the
>> cmd_set_specfic[] for v1.3 and instead use this?
>>
>>       uint32_t    pmrmsc;
>>  +    uint32_t    reserved[122];
>>   } NvmeBar;
>>
>> Or this?
>>
>>       uint32_t    pmrmsc;
>>  +    uint8_t     reserved[488];
>>   } NvmeBar;
>>
> 
> Yes, the second one.
> 
> But it should be 484 bytes reserved and the bug is in the pmrmsc field
> that should be uint64_t. Can you fix that as well?  :)
> 

Ah this is what you did in "hw/block/nvme: add NVMe 1.4 specific fields"
https://www.mail-archive.com/qemu-devel@nongnu.org/msg717891.html