diff mbox series

[v2,03/19] block/nvme: Introduce device/iommu 'page_size_min' variables

Message ID 20201026105504.4023620-4-philmd@redhat.com
State New, archived
Headers show
Series util/vfio-helpers: Allow using multiple MSIX IRQs | expand

Commit Message

Philippe Mathieu-Daudé Oct. 26, 2020, 10:54 a.m. UTC
Introduce device/iommu 'page_size_min' variables to make
the code clearer.

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 block/nvme.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

Comments

Auger Eric Oct. 26, 2020, 5:38 p.m. UTC | #1
Hi Philippe,

On 10/26/20 11:54 AM, Philippe Mathieu-Daudé wrote:
> Introduce device/iommu 'page_size_min' variables to make
> the code clearer.

I am unclear how much the device and the iommu page size must equal. For
instance, in [RFC 0/5] NVMe passthrough: Support 64kB page host, I have
a 64kB host page and an MPS set to 4kB.

Thanks

Eric
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  block/nvme.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/block/nvme.c b/block/nvme.c
> index aa290996679..5abd7257cac 100644
> --- a/block/nvme.c
> +++ b/block/nvme.c
> @@ -690,6 +690,8 @@ static int nvme_init(BlockDriverState *bs, const char *device, int namespace,
>      uint64_t deadline, now;
>      Error *local_err = NULL;
>      volatile NvmeBar *regs = NULL;
> +    size_t device_page_size_min;
> +    size_t iommu_page_size_min = 4096;
>  
>      qemu_co_mutex_init(&s->dma_map_lock);
>      qemu_co_queue_init(&s->dma_flush_queue);
> @@ -724,7 +726,8 @@ static int nvme_init(BlockDriverState *bs, const char *device, int namespace,
>          goto out;
>      }
>  
> -    s->page_size = MAX(4096, 1u << (12 + NVME_CAP_MPSMIN(cap)));
> +    device_page_size_min = 1u << (12 + NVME_CAP_MPSMIN(cap));
> +    s->page_size = MAX(iommu_page_size_min, device_page_size_min);
>      s->doorbell_scale = (4 << NVME_CAP_DSTRD(cap)) / sizeof(uint32_t);
>      bs->bl.opt_mem_alignment = s->page_size;
>      bs->bl.request_alignment = s->page_size;
>
Stefan Hajnoczi Oct. 27, 2020, 9:32 a.m. UTC | #2
On Mon, Oct 26, 2020 at 11:54:48AM +0100, Philippe Mathieu-Daudé wrote:
> Introduce device/iommu 'page_size_min' variables to make
> the code clearer.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  block/nvme.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/block/nvme.c b/block/nvme.c
> index aa290996679..5abd7257cac 100644
> --- a/block/nvme.c
> +++ b/block/nvme.c
> @@ -690,6 +690,8 @@ static int nvme_init(BlockDriverState *bs, const char *device, int namespace,
>      uint64_t deadline, now;
>      Error *local_err = NULL;
>      volatile NvmeBar *regs = NULL;
> +    size_t device_page_size_min;
> +    size_t iommu_page_size_min = 4096;
>  
>      qemu_co_mutex_init(&s->dma_map_lock);
>      qemu_co_queue_init(&s->dma_flush_queue);
> @@ -724,7 +726,8 @@ static int nvme_init(BlockDriverState *bs, const char *device, int namespace,
>          goto out;
>      }
>  
> -    s->page_size = MAX(4096, 1u << (12 + NVME_CAP_MPSMIN(cap)));
> +    device_page_size_min = 1u << (12 + NVME_CAP_MPSMIN(cap));
> +    s->page_size = MAX(iommu_page_size_min, device_page_size_min);

It's not clear to me that the 4096 value is related to the IOMMU page
size here. The MAX(4096) expression seems like a sanity-check to me. An
MPS value of 0 is a 4KB page size, so it's never possible to express a
smaller page size. I guess MAX() was used in case a device incorrectly
reports MPSMIN.

I think introducing the concept of IOMMU page size is premature and
maybe not the intention of the existing code, but the concept will be
needed soon, so this patch is okay:

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
diff mbox series

Patch

diff --git a/block/nvme.c b/block/nvme.c
index aa290996679..5abd7257cac 100644
--- a/block/nvme.c
+++ b/block/nvme.c
@@ -690,6 +690,8 @@  static int nvme_init(BlockDriverState *bs, const char *device, int namespace,
     uint64_t deadline, now;
     Error *local_err = NULL;
     volatile NvmeBar *regs = NULL;
+    size_t device_page_size_min;
+    size_t iommu_page_size_min = 4096;
 
     qemu_co_mutex_init(&s->dma_map_lock);
     qemu_co_queue_init(&s->dma_flush_queue);
@@ -724,7 +726,8 @@  static int nvme_init(BlockDriverState *bs, const char *device, int namespace,
         goto out;
     }
 
-    s->page_size = MAX(4096, 1u << (12 + NVME_CAP_MPSMIN(cap)));
+    device_page_size_min = 1u << (12 + NVME_CAP_MPSMIN(cap));
+    s->page_size = MAX(iommu_page_size_min, device_page_size_min);
     s->doorbell_scale = (4 << NVME_CAP_DSTRD(cap)) / sizeof(uint32_t);
     bs->bl.opt_mem_alignment = s->page_size;
     bs->bl.request_alignment = s->page_size;