[PATCH v4 0/3] VIRTIO-IOMMU: Set default granule to host page size

[PATCH v4 0/3] VIRTIO-IOMMU: Set default granule to host page size

[Eric Auger]

We used to set the default granule to 4KB but with VFIO assignment
it makes more sense to use the actual host page size.

Indeed when hotplugging a VFIO device protected by a virtio-iommu
on a 64kB/64kB host/guest config, we currently get a qemu crash:

"vfio: DMA mapping failed, unable to continue"

This is due to the hot-attached VFIO device calling
memory_region_iommu_set_page_size_mask() with 64kB granule
whereas the virtio-iommu granule was already frozen to 4KB on
machine init done.

Introduce a new granule property and set this latter to "host"
and introduce a new compat.

Note that the new default will prevent 4kB guest on 64kB host
because the granule will be set to 64kB which would be larger
than the guest page size. In that situation, the virtio-iommu
driver fails on viommu_domain_finalise() with
"granule 0x10000 larger than system page size 0x1000".

In that case 4K granule should be used.

To summarize, before the series, the support matrix (credit
to Jean-Philippe Brucker) was:

 Host | Guest | virtio-net | IGB passthrough
  4k  | 4k    | Y          | Y
  64k | 64k   | Y          | N
  64k | 4k    | Y          | N
  4k  | 64k   | Y          | Y

After the series:

 Host | Guest | virtio-net | IGB passthrough
  4k  | 4k    | Y          | Y
  64k | 64k   | Y          | Y
  64k | 4k    | 4K         | N
  4k  | 64k   | Y          | Y

The current limitation of global granule in the virtio-iommu
should be removed and turned into per domain granule. But
until we get this upgraded, this new default is probably
better because I don't think anyone is currently interested in
running a 4KB page size guest with virtio-iommu on a 64KB host.
However supporting 64kB guest on 64kB host with virtio-iommu and
VFIO looks a more important feature.

This series can be found at:
https://github.com/eauger/qemu/tree/granule-v2

Applied on top of
[PATCH v5 0/4] VIRTIO-IOMMU: Introduce an aw-bits option
https://lore.kernel.org/all/20240215084315.863897-1-eric.auger@redhat.com/

History:
v3 -> v4:
- Add 8K granule (Richard)

v2 -> v3
- introduce a dedicated granule option to handle the compat

Eric Auger (3):
  qdev: Add a granule_mode property
  virtio-iommu: Add a granule property
  virtio-iommu: Change the default granule to the host page size

 include/hw/qdev-properties-system.h |  3 +++
 include/hw/virtio/virtio-iommu.h    | 12 +++++++++
 hw/core/machine.c                   |  1 +
 hw/core/qdev-properties-system.c    | 15 ++++++++++++
 hw/virtio/virtio-iommu.c            | 38 ++++++++++++++++++++++++++---
 qemu-options.hx                     |  3 +++
 6 files changed, 69 insertions(+), 3 deletions(-)

-- 
2.41.0

[PATCH v4 1/3] qdev: Add a granule_mode property

[Eric Auger]

Introduce a new enum type property allowing to set an
IOMMU granule. Values are 4K, 8K, 16K, 64K and host.
This latter indicates the vIOMMU granule will match
the host page size.

A subsequent patch will add such a property to the
virtio-iommu device.

Signed-off-by: Eric Auger <eric.auger@redhat.com>

---

v3 -> v4:
- Add 8K
---
 include/hw/qdev-properties-system.h |  3 +++
 include/hw/virtio/virtio-iommu.h    | 11 +++++++++++
 hw/core/qdev-properties-system.c    | 15 +++++++++++++++
 hw/virtio/virtio-iommu.c            | 11 +++++++++++
 4 files changed, 40 insertions(+)

diff --git a/include/hw/qdev-properties-system.h b/include/hw/qdev-properties-system.h
index 06c359c190..626be87dd3 100644
--- a/include/hw/qdev-properties-system.h
+++ b/include/hw/qdev-properties-system.h
@@ -8,6 +8,7 @@ extern const PropertyInfo qdev_prop_macaddr;
 extern const PropertyInfo qdev_prop_reserved_region;
 extern const PropertyInfo qdev_prop_multifd_compression;
 extern const PropertyInfo qdev_prop_mig_mode;
+extern const PropertyInfo qdev_prop_granule_mode;
 extern const PropertyInfo qdev_prop_losttickpolicy;
 extern const PropertyInfo qdev_prop_blockdev_on_error;
 extern const PropertyInfo qdev_prop_bios_chs_trans;
@@ -47,6 +48,8 @@ extern const PropertyInfo qdev_prop_iothread_vq_mapping_list;
 #define DEFINE_PROP_MIG_MODE(_n, _s, _f, _d) \
     DEFINE_PROP_SIGNED(_n, _s, _f, _d, qdev_prop_mig_mode, \
                        MigMode)
+#define DEFINE_PROP_GRANULE_MODE(_n, _s, _f, _d) \
+    DEFINE_PROP_SIGNED(_n, _s, _f, _d, qdev_prop_granule_mode, GranuleMode)
 #define DEFINE_PROP_LOSTTICKPOLICY(_n, _s, _f, _d) \
     DEFINE_PROP_SIGNED(_n, _s, _f, _d, qdev_prop_losttickpolicy, \
                         LostTickPolicy)
diff --git a/include/hw/virtio/virtio-iommu.h b/include/hw/virtio/virtio-iommu.h
index 5fbe4677c2..e22327548f 100644
--- a/include/hw/virtio/virtio-iommu.h
+++ b/include/hw/virtio/virtio-iommu.h
@@ -31,6 +31,17 @@ OBJECT_DECLARE_SIMPLE_TYPE(VirtIOIOMMU, VIRTIO_IOMMU)
 
 #define TYPE_VIRTIO_IOMMU_MEMORY_REGION "virtio-iommu-memory-region"
 
+typedef enum GranuleMode {
+    GRANULE_MODE_4K,
+    GRANULE_MODE_8K,
+    GRANULE_MODE_16K,
+    GRANULE_MODE_64K,
+    GRANULE_MODE_HOST,
+    GRANULE_MODE__MAX,
+} GranuleMode;
+
+extern const QEnumLookup GranuleMode_lookup;
+
 typedef struct IOMMUDevice {
     void         *viommu;
     PCIBus       *bus;
diff --git a/hw/core/qdev-properties-system.c b/hw/core/qdev-properties-system.c
index 1a396521d5..578eac6b14 100644
--- a/hw/core/qdev-properties-system.c
+++ b/hw/core/qdev-properties-system.c
@@ -34,6 +34,7 @@
 #include "net/net.h"
 #include "hw/pci/pci.h"
 #include "hw/pci/pcie.h"
+#include "hw/virtio/virtio-iommu.h"
 #include "hw/i386/x86.h"
 #include "util/block-helpers.h"
 
@@ -679,6 +680,20 @@ const PropertyInfo qdev_prop_mig_mode = {
     .set_default_value = qdev_propinfo_set_default_value_enum,
 };
 
+/* --- GranuleMode --- */
+
+QEMU_BUILD_BUG_ON(sizeof(GranuleMode) != sizeof(int));
+
+const PropertyInfo qdev_prop_granule_mode = {
+    .name = "GranuleMode",
+    .description = "granule_mode values, "
+                   "4K, 8K, 16K, 64K, host",
+    .enum_table = &GranuleMode_lookup,
+    .get = qdev_propinfo_get_enum,
+    .set = qdev_propinfo_set_enum,
+    .set_default_value = qdev_propinfo_set_default_value_enum,
+};
+
 /* --- Reserved Region --- */
 
 /*
diff --git a/hw/virtio/virtio-iommu.c b/hw/virtio/virtio-iommu.c
index 2ec5ef3cd1..a9bdc03d12 100644
--- a/hw/virtio/virtio-iommu.c
+++ b/hw/virtio/virtio-iommu.c
@@ -44,6 +44,17 @@
 #define VIOMMU_DEFAULT_QUEUE_SIZE 256
 #define VIOMMU_PROBE_SIZE 512
 
+const QEnumLookup GranuleMode_lookup = {
+    .array = (const char *const[]) {
+        [GRANULE_MODE_4K]   = "4K",
+        [GRANULE_MODE_8K]   = "8K",
+        [GRANULE_MODE_16K]  = "16K",
+        [GRANULE_MODE_64K]  = "64K",
+        [GRANULE_MODE_HOST] = "host",
+    },
+    .size = GRANULE_MODE__MAX
+};
+
 typedef struct VirtIOIOMMUDomain {
     uint32_t id;
     bool bypass;
-- 
2.41.0

[PATCH v4 2/3] virtio-iommu: Add a granule property

[Eric Auger]

This allows to choose which granule will be used by
default by the virtio-iommu. Current page size mask
default is qemu_target_page_mask so this translates
into a 4K granule.

Signed-off-by: Eric Auger <eric.auger@redhat.com>

---

v3 -> v4:
- granule_mode introduction moved to that patch
---
 include/hw/virtio/virtio-iommu.h |  1 +
 hw/virtio/virtio-iommu.c         | 27 ++++++++++++++++++++++++---
 qemu-options.hx                  |  3 +++
 3 files changed, 28 insertions(+), 3 deletions(-)

diff --git a/include/hw/virtio/virtio-iommu.h b/include/hw/virtio/virtio-iommu.h
index e22327548f..a3e5b35b1e 100644
--- a/include/hw/virtio/virtio-iommu.h
+++ b/include/hw/virtio/virtio-iommu.h
@@ -78,6 +78,7 @@ struct VirtIOIOMMU {
     Notifier machine_done;
     bool granule_frozen;
     uint8_t aw_bits;
+    GranuleMode granule_mode;
 };
 
 #endif
diff --git a/hw/virtio/virtio-iommu.c b/hw/virtio/virtio-iommu.c
index a9bdc03d12..0461b87ef2 100644
--- a/hw/virtio/virtio-iommu.c
+++ b/hw/virtio/virtio-iommu.c
@@ -1126,8 +1126,8 @@ static int virtio_iommu_notify_flag_changed(IOMMUMemoryRegion *iommu_mr,
 }
 
 /*
- * The default mask (TARGET_PAGE_MASK) is the smallest supported guest granule,
- * for example 0xfffffffffffff000. When an assigned device has page size
+ * The default mask depends on the "granule" property. For example, with
+ * 4K granule, it is ~0xFFF. When an assigned device has page size
  * restrictions due to the hardware IOMMU configuration, apply this restriction
  * to the mask.
  */
@@ -1324,7 +1324,26 @@ static void virtio_iommu_device_realize(DeviceState *dev, Error **errp)
      * in vfio realize
      */
     s->config.bypass = s->boot_bypass;
-    s->config.page_size_mask = qemu_target_page_mask();
+
+    switch (s->granule_mode) {
+    case GRANULE_MODE_4K:
+        s->config.page_size_mask = ~0xFFF;
+        break;
+    case GRANULE_MODE_8K:
+        s->config.page_size_mask = ~0x1FFF;
+        break;
+    case GRANULE_MODE_16K:
+        s->config.page_size_mask = ~0x3FFF;
+        break;
+    case GRANULE_MODE_64K:
+        s->config.page_size_mask = ~0xFFFF;
+        break;
+    case GRANULE_MODE_HOST:
+        s->config.page_size_mask = qemu_real_host_page_mask();
+        break;
+    default:
+        error_setg(errp, "Unsupported granule mode");
+    }
     if (s->aw_bits < 32 || s->aw_bits > 64) {
         error_setg(errp, "aw-bits must be within [32,64]");
     }
@@ -1538,6 +1557,8 @@ static Property virtio_iommu_properties[] = {
                      TYPE_PCI_BUS, PCIBus *),
     DEFINE_PROP_BOOL("boot-bypass", VirtIOIOMMU, boot_bypass, true),
     DEFINE_PROP_UINT8("aw-bits", VirtIOIOMMU, aw_bits, 0),
+    DEFINE_PROP_GRANULE_MODE("granule", VirtIOIOMMU, granule_mode,
+                             GRANULE_MODE_4K),
     DEFINE_PROP_END_OF_LIST(),
 };
 
diff --git a/qemu-options.hx b/qemu-options.hx
index a98bc7bd60..8bc1e9e4aa 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -1179,6 +1179,9 @@ SRST
     ``aw-bits=val`` (val between 32 and 64, default depends on machine)
         This decides the address width of IOVA address space. It defaults
         to 39 bits on q35 machines and 48 bits on ARM virt machines.
+    ``granule=val`` (possible values are 4K, 8K, 16K, 64K and host)
+        This decides the default granule to be be exposed by the
+        virtio-iommu. If host, the granule matches the host page size.
 
 ERST
 
-- 
2.41.0

[PATCH v4 3/3] virtio-iommu: Change the default granule to the host page size

[Eric Auger]

We used to set the default granule to 4KB but with VFIO assignment
it makes more sense to use the actual host page size.

Indeed when hotplugging a VFIO device protected by a virtio-iommu
on a 64kB/64kB host/guest config, we current get a qemu crash:

"vfio: DMA mapping failed, unable to continue"

This is due to the hot-attached VFIO device calling
memory_region_iommu_set_page_size_mask() with 64kB granule
whereas the virtio-iommu granule was already frozen to 4KB on
machine init done.

Set the granule property to "host" and introduce a new compat.

Note that the new default will prevent 4kB guest on 64kB host
because the granule will be set to 64kB which would be larger
than the guest page size. In that situation, the virtio-iommu
driver fails on viommu_domain_finalise() with
"granule 0x10000 larger than system page size 0x1000".

In that case the workaround is to request 4K granule.

The current limitation of global granule in the virtio-iommu
should be removed and turned into per domain granule. But
until we get this upgraded, this new default is probably
better because I don't think anyone is currently interested in
running a 4KB page size guest with virtio-iommu on a 64KB host.
However supporting 64kB guest on 64kB host with virtio-iommu and
VFIO looks a more important feature.

Signed-off-by: Eric Auger <eric.auger@redhat.com>
---
 hw/core/machine.c        | 1 +
 hw/virtio/virtio-iommu.c | 2 +-
 2 files changed, 2 insertions(+), 1 deletion(-)

diff --git a/hw/core/machine.c b/hw/core/machine.c
index 70ac96954c..38851df4b8 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -35,6 +35,7 @@
 
 GlobalProperty hw_compat_8_2[] = {
     { TYPE_VIRTIO_IOMMU_PCI, "aw-bits", "64" },
+    { TYPE_VIRTIO_IOMMU_PCI, "granule", "4K" },
 };
 const size_t hw_compat_8_2_len = G_N_ELEMENTS(hw_compat_8_2);
 
diff --git a/hw/virtio/virtio-iommu.c b/hw/virtio/virtio-iommu.c
index 0461b87ef2..e9e44a8ad8 100644
--- a/hw/virtio/virtio-iommu.c
+++ b/hw/virtio/virtio-iommu.c
@@ -1558,7 +1558,7 @@ static Property virtio_iommu_properties[] = {
     DEFINE_PROP_BOOL("boot-bypass", VirtIOIOMMU, boot_bypass, true),
     DEFINE_PROP_UINT8("aw-bits", VirtIOIOMMU, aw_bits, 0),
     DEFINE_PROP_GRANULE_MODE("granule", VirtIOIOMMU, granule_mode,
-                             GRANULE_MODE_4K),
+                             GRANULE_MODE_HOST),
     DEFINE_PROP_END_OF_LIST(),
 };
 
-- 
2.41.0

Re: [PATCH v4 1/3] qdev: Add a granule_mode property

[Philippe Mathieu-Daudé]

Hi Eric,

On 23/2/24 08:27, Eric Auger wrote:
> Introduce a new enum type property allowing to set an
> IOMMU granule. Values are 4K, 8K, 16K, 64K and host.
> This latter indicates the vIOMMU granule will match
> the host page size.
> 
> A subsequent patch will add such a property to the
> virtio-iommu device.
> 
> Signed-off-by: Eric Auger <eric.auger@redhat.com>
> 
> ---
> 
> v3 -> v4:
> - Add 8K
> ---
>   include/hw/qdev-properties-system.h |  3 +++
>   include/hw/virtio/virtio-iommu.h    | 11 +++++++++++
>   hw/core/qdev-properties-system.c    | 15 +++++++++++++++
>   hw/virtio/virtio-iommu.c            | 11 +++++++++++
>   4 files changed, 40 insertions(+)


> diff --git a/include/hw/virtio/virtio-iommu.h b/include/hw/virtio/virtio-iommu.h
> index 5fbe4677c2..e22327548f 100644
> --- a/include/hw/virtio/virtio-iommu.h
> +++ b/include/hw/virtio/virtio-iommu.h
> @@ -31,6 +31,17 @@ OBJECT_DECLARE_SIMPLE_TYPE(VirtIOIOMMU, VIRTIO_IOMMU)
>   
>   #define TYPE_VIRTIO_IOMMU_MEMORY_REGION "virtio-iommu-memory-region"
>   
> +typedef enum GranuleMode {
> +    GRANULE_MODE_4K,
> +    GRANULE_MODE_8K,
> +    GRANULE_MODE_16K,
> +    GRANULE_MODE_64K,
> +    GRANULE_MODE_HOST,
> +    GRANULE_MODE__MAX,
> +} GranuleMode;
> +
> +extern const QEnumLookup GranuleMode_lookup;

Aren't this, ...

> diff --git a/hw/virtio/virtio-iommu.c b/hw/virtio/virtio-iommu.c
> index 2ec5ef3cd1..a9bdc03d12 100644
> --- a/hw/virtio/virtio-iommu.c
> +++ b/hw/virtio/virtio-iommu.c
> @@ -44,6 +44,17 @@
>   #define VIOMMU_DEFAULT_QUEUE_SIZE 256
>   #define VIOMMU_PROBE_SIZE 512
>   
> +const QEnumLookup GranuleMode_lookup = {
> +    .array = (const char *const[]) {
> +        [GRANULE_MODE_4K]   = "4K",
> +        [GRANULE_MODE_8K]   = "8K",
> +        [GRANULE_MODE_16K]  = "16K",
> +        [GRANULE_MODE_64K]  = "64K",
> +        [GRANULE_MODE_HOST] = "host",
> +    },
> +    .size = GRANULE_MODE__MAX
> +};
... and this supposed to be QAPI generated?

Re: [PATCH v4 2/3] virtio-iommu: Add a granule property

[Philippe Mathieu-Daudé]

On 23/2/24 08:27, Eric Auger wrote:
> This allows to choose which granule will be used by
> default by the virtio-iommu. Current page size mask
> default is qemu_target_page_mask so this translates
> into a 4K granule.
> 
> Signed-off-by: Eric Auger <eric.auger@redhat.com>
> 
> ---
> 
> v3 -> v4:
> - granule_mode introduction moved to that patch
> ---
>   include/hw/virtio/virtio-iommu.h |  1 +
>   hw/virtio/virtio-iommu.c         | 27 ++++++++++++++++++++++++---
>   qemu-options.hx                  |  3 +++
>   3 files changed, 28 insertions(+), 3 deletions(-)


> @@ -1324,7 +1324,26 @@ static void virtio_iommu_device_realize(DeviceState *dev, Error **errp)
>        * in vfio realize
>        */
>       s->config.bypass = s->boot_bypass;
> -    s->config.page_size_mask = qemu_target_page_mask();
> +
> +    switch (s->granule_mode) {
> +    case GRANULE_MODE_4K:
> +        s->config.page_size_mask = ~0xFFF;

Alternatively:

           s->config.page_size_mask = -(4 * KiB);

> +        break;
> +    case GRANULE_MODE_8K:
> +        s->config.page_size_mask = ~0x1FFF;

           s->config.page_size_mask = -(8 * KiB);

> +        break;
> +    case GRANULE_MODE_16K:
> +        s->config.page_size_mask = ~0x3FFF;

...

> +        break;
> +    case GRANULE_MODE_64K:
> +        s->config.page_size_mask = ~0xFFFF;
> +        break;
> +    case GRANULE_MODE_HOST:
> +        s->config.page_size_mask = qemu_real_host_page_mask();
> +        break;
> +    default:
> +        error_setg(errp, "Unsupported granule mode");
> +    }

Re: [PATCH v4 3/3] virtio-iommu: Change the default granule to the host page size

[Philippe Mathieu-Daudé]

Hi Eric,

On 23/2/24 08:27, Eric Auger wrote:
> We used to set the default granule to 4KB but with VFIO assignment
> it makes more sense to use the actual host page size.
> 
> Indeed when hotplugging a VFIO device protected by a virtio-iommu
> on a 64kB/64kB host/guest config, we current get a qemu crash:
> 
> "vfio: DMA mapping failed, unable to continue"
> 
> This is due to the hot-attached VFIO device calling
> memory_region_iommu_set_page_size_mask() with 64kB granule
> whereas the virtio-iommu granule was already frozen to 4KB on
> machine init done.
> 
> Set the granule property to "host" and introduce a new compat.
> 
> Note that the new default will prevent 4kB guest on 64kB host
> because the granule will be set to 64kB which would be larger
> than the guest page size. In that situation, the virtio-iommu
> driver fails on viommu_domain_finalise() with
> "granule 0x10000 larger than system page size 0x1000".
> 
> In that case the workaround is to request 4K granule.
> 
> The current limitation of global granule in the virtio-iommu
> should be removed and turned into per domain granule. But
> until we get this upgraded, this new default is probably
> better because I don't think anyone is currently interested in
> running a 4KB page size guest with virtio-iommu on a 64KB host.
> However supporting 64kB guest on 64kB host with virtio-iommu and
> VFIO looks a more important feature.
> 
> Signed-off-by: Eric Auger <eric.auger@redhat.com>
> ---
>   hw/core/machine.c        | 1 +
>   hw/virtio/virtio-iommu.c | 2 +-
>   2 files changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/core/machine.c b/hw/core/machine.c
> index 70ac96954c..38851df4b8 100644
> --- a/hw/core/machine.c
> +++ b/hw/core/machine.c
> @@ -35,6 +35,7 @@
>   
>   GlobalProperty hw_compat_8_2[] = {
>       { TYPE_VIRTIO_IOMMU_PCI, "aw-bits", "64" },
> +    { TYPE_VIRTIO_IOMMU_PCI, "granule", "4K" },

IIUC the current value is qemu_target_page_mask(), not 4KiB.
Could this be an issue for arm / mips / mips / sparc64 guests?

>   };
>   const size_t hw_compat_8_2_len = G_N_ELEMENTS(hw_compat_8_2);
>   
> diff --git a/hw/virtio/virtio-iommu.c b/hw/virtio/virtio-iommu.c
> index 0461b87ef2..e9e44a8ad8 100644
> --- a/hw/virtio/virtio-iommu.c
> +++ b/hw/virtio/virtio-iommu.c
> @@ -1558,7 +1558,7 @@ static Property virtio_iommu_properties[] = {
>       DEFINE_PROP_BOOL("boot-bypass", VirtIOIOMMU, boot_bypass, true),
>       DEFINE_PROP_UINT8("aw-bits", VirtIOIOMMU, aw_bits, 0),
>       DEFINE_PROP_GRANULE_MODE("granule", VirtIOIOMMU, granule_mode,
> -                             GRANULE_MODE_4K),
> +                             GRANULE_MODE_HOST),
>       DEFINE_PROP_END_OF_LIST(),
>   };
>   

Re: [PATCH v4 1/3] qdev: Add a granule_mode property

[Eric Auger]

Hi Philippe,

On 2/23/24 08:52, Philippe Mathieu-Daudé wrote:
> Hi Eric,
>
> On 23/2/24 08:27, Eric Auger wrote:
>> Introduce a new enum type property allowing to set an
>> IOMMU granule. Values are 4K, 8K, 16K, 64K and host.
>> This latter indicates the vIOMMU granule will match
>> the host page size.
>>
>> A subsequent patch will add such a property to the
>> virtio-iommu device.
>>
>> Signed-off-by: Eric Auger <eric.auger@redhat.com>
>>
>> ---
>>
>> v3 -> v4:
>> - Add 8K
>> ---
>>   include/hw/qdev-properties-system.h |  3 +++
>>   include/hw/virtio/virtio-iommu.h    | 11 +++++++++++
>>   hw/core/qdev-properties-system.c    | 15 +++++++++++++++
>>   hw/virtio/virtio-iommu.c            | 11 +++++++++++
>>   4 files changed, 40 insertions(+)
>
>
>> diff --git a/include/hw/virtio/virtio-iommu.h
>> b/include/hw/virtio/virtio-iommu.h
>> index 5fbe4677c2..e22327548f 100644
>> --- a/include/hw/virtio/virtio-iommu.h
>> +++ b/include/hw/virtio/virtio-iommu.h
>> @@ -31,6 +31,17 @@ OBJECT_DECLARE_SIMPLE_TYPE(VirtIOIOMMU, VIRTIO_IOMMU)
>>     #define TYPE_VIRTIO_IOMMU_MEMORY_REGION "virtio-iommu-memory-region"
>>   +typedef enum GranuleMode {
>> +    GRANULE_MODE_4K,
>> +    GRANULE_MODE_8K,
>> +    GRANULE_MODE_16K,
>> +    GRANULE_MODE_64K,
>> +    GRANULE_MODE_HOST,
>> +    GRANULE_MODE__MAX,
>> +} GranuleMode;
>> +
>> +extern const QEnumLookup GranuleMode_lookup;
>
> Aren't this, ...
>
>> diff --git a/hw/virtio/virtio-iommu.c b/hw/virtio/virtio-iommu.c
>> index 2ec5ef3cd1..a9bdc03d12 100644
>> --- a/hw/virtio/virtio-iommu.c
>> +++ b/hw/virtio/virtio-iommu.c
>> @@ -44,6 +44,17 @@
>>   #define VIOMMU_DEFAULT_QUEUE_SIZE 256
>>   #define VIOMMU_PROBE_SIZE 512
>>   +const QEnumLookup GranuleMode_lookup = {
>> +    .array = (const char *const[]) {
>> +        [GRANULE_MODE_4K]   = "4K",
>> +        [GRANULE_MODE_8K]   = "8K",
>> +        [GRANULE_MODE_16K]  = "16K",
>> +        [GRANULE_MODE_64K]  = "64K",
>> +        [GRANULE_MODE_HOST] = "host",
>> +    },
>> +    .size = GRANULE_MODE__MAX
>> +};
> ... and this supposed to be QAPI generated?
If I remove both, it does not compile. How do you make it generated?

Thanks

Eric

Re: [PATCH v4 2/3] virtio-iommu: Add a granule property

[Eric Auger]

On 2/23/24 08:57, Philippe Mathieu-Daudé wrote:
> On 23/2/24 08:27, Eric Auger wrote:
>> This allows to choose which granule will be used by
>> default by the virtio-iommu. Current page size mask
>> default is qemu_target_page_mask so this translates
>> into a 4K granule.
>>
>> Signed-off-by: Eric Auger <eric.auger@redhat.com>
>>
>> ---
>>
>> v3 -> v4:
>> - granule_mode introduction moved to that patch
>> ---
>>   include/hw/virtio/virtio-iommu.h |  1 +
>>   hw/virtio/virtio-iommu.c         | 27 ++++++++++++++++++++++++---
>>   qemu-options.hx                  |  3 +++
>>   3 files changed, 28 insertions(+), 3 deletions(-)
>
>
>> @@ -1324,7 +1324,26 @@ static void
>> virtio_iommu_device_realize(DeviceState *dev, Error **errp)
>>        * in vfio realize
>>        */
>>       s->config.bypass = s->boot_bypass;
>> -    s->config.page_size_mask = qemu_target_page_mask();
>> +
>> +    switch (s->granule_mode) {
>> +    case GRANULE_MODE_4K:
>> +        s->config.page_size_mask = ~0xFFF;
>
> Alternatively:
>
>           s->config.page_size_mask = -(4 * KiB);
Yep that's more readable.

Thanks

Eric
>
>> +        break;
>> +    case GRANULE_MODE_8K:
>> +        s->config.page_size_mask = ~0x1FFF;
>
>           s->config.page_size_mask = -(8 * KiB);
>
>> +        break;
>> +    case GRANULE_MODE_16K:
>> +        s->config.page_size_mask = ~0x3FFF;
>
> ...
>
>> +        break;
>> +    case GRANULE_MODE_64K:
>> +        s->config.page_size_mask = ~0xFFFF;
>> +        break;
>> +    case GRANULE_MODE_HOST:
>> +        s->config.page_size_mask = qemu_real_host_page_mask();
>> +        break;
>> +    default:
>> +        error_setg(errp, "Unsupported granule mode");
>> +    }
>

Re: [PATCH v4 3/3] virtio-iommu: Change the default granule to the host page size

[Eric Auger]

On 2/23/24 09:08, Philippe Mathieu-Daudé wrote:
> Hi Eric,
>
> On 23/2/24 08:27, Eric Auger wrote:
>> We used to set the default granule to 4KB but with VFIO assignment
>> it makes more sense to use the actual host page size.
>>
>> Indeed when hotplugging a VFIO device protected by a virtio-iommu
>> on a 64kB/64kB host/guest config, we current get a qemu crash:
>>
>> "vfio: DMA mapping failed, unable to continue"
>>
>> This is due to the hot-attached VFIO device calling
>> memory_region_iommu_set_page_size_mask() with 64kB granule
>> whereas the virtio-iommu granule was already frozen to 4KB on
>> machine init done.
>>
>> Set the granule property to "host" and introduce a new compat.
>>
>> Note that the new default will prevent 4kB guest on 64kB host
>> because the granule will be set to 64kB which would be larger
>> than the guest page size. In that situation, the virtio-iommu
>> driver fails on viommu_domain_finalise() with
>> "granule 0x10000 larger than system page size 0x1000".
>>
>> In that case the workaround is to request 4K granule.
>>
>> The current limitation of global granule in the virtio-iommu
>> should be removed and turned into per domain granule. But
>> until we get this upgraded, this new default is probably
>> better because I don't think anyone is currently interested in
>> running a 4KB page size guest with virtio-iommu on a 64KB host.
>> However supporting 64kB guest on 64kB host with virtio-iommu and
>> VFIO looks a more important feature.
>>
>> Signed-off-by: Eric Auger <eric.auger@redhat.com>
>> ---
>>   hw/core/machine.c        | 1 +
>>   hw/virtio/virtio-iommu.c | 2 +-
>>   2 files changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/core/machine.c b/hw/core/machine.c
>> index 70ac96954c..38851df4b8 100644
>> --- a/hw/core/machine.c
>> +++ b/hw/core/machine.c
>> @@ -35,6 +35,7 @@
>>     GlobalProperty hw_compat_8_2[] = {
>>       { TYPE_VIRTIO_IOMMU_PCI, "aw-bits", "64" },
>> +    { TYPE_VIRTIO_IOMMU_PCI, "granule", "4K" },
>
> IIUC the current value is qemu_target_page_mask(), not 4KiB.
> Could this be an issue for arm / mips / mips / sparc64 guests?
The virtio-iommu currently is only supported on ARM and x86 where
qemu_target_page_mask() corresponds to a 4K granule. So I don't think it
makes any issue.

Thanks

Eric
>
>>   };
>>   const size_t hw_compat_8_2_len = G_N_ELEMENTS(hw_compat_8_2);
>>   diff --git a/hw/virtio/virtio-iommu.c b/hw/virtio/virtio-iommu.c
>> index 0461b87ef2..e9e44a8ad8 100644
>> --- a/hw/virtio/virtio-iommu.c
>> +++ b/hw/virtio/virtio-iommu.c
>> @@ -1558,7 +1558,7 @@ static Property virtio_iommu_properties[] = {
>>       DEFINE_PROP_BOOL("boot-bypass", VirtIOIOMMU, boot_bypass, true),
>>       DEFINE_PROP_UINT8("aw-bits", VirtIOIOMMU, aw_bits, 0),
>>       DEFINE_PROP_GRANULE_MODE("granule", VirtIOIOMMU, granule_mode,
>> -                             GRANULE_MODE_4K),
>> +                             GRANULE_MODE_HOST),
>>       DEFINE_PROP_END_OF_LIST(),
>>   };
>>   
>

Re: [PATCH v4 3/3] virtio-iommu: Change the default granule to the host page size

[Philippe Mathieu-Daudé]

On 23/2/24 09:38, Eric Auger wrote:
> 
> 
> On 2/23/24 09:08, Philippe Mathieu-Daudé wrote:
>> Hi Eric,
>>
>> On 23/2/24 08:27, Eric Auger wrote:
>>> We used to set the default granule to 4KB but with VFIO assignment
>>> it makes more sense to use the actual host page size.
>>>
>>> Indeed when hotplugging a VFIO device protected by a virtio-iommu
>>> on a 64kB/64kB host/guest config, we current get a qemu crash:
>>>
>>> "vfio: DMA mapping failed, unable to continue"
>>>
>>> This is due to the hot-attached VFIO device calling
>>> memory_region_iommu_set_page_size_mask() with 64kB granule
>>> whereas the virtio-iommu granule was already frozen to 4KB on
>>> machine init done.
>>>
>>> Set the granule property to "host" and introduce a new compat.
>>>
>>> Note that the new default will prevent 4kB guest on 64kB host
>>> because the granule will be set to 64kB which would be larger
>>> than the guest page size. In that situation, the virtio-iommu
>>> driver fails on viommu_domain_finalise() with
>>> "granule 0x10000 larger than system page size 0x1000".
>>>
>>> In that case the workaround is to request 4K granule.
>>>
>>> The current limitation of global granule in the virtio-iommu
>>> should be removed and turned into per domain granule. But
>>> until we get this upgraded, this new default is probably
>>> better because I don't think anyone is currently interested in
>>> running a 4KB page size guest with virtio-iommu on a 64KB host.
>>> However supporting 64kB guest on 64kB host with virtio-iommu and
>>> VFIO looks a more important feature.
>>>
>>> Signed-off-by: Eric Auger <eric.auger@redhat.com>
>>> ---
>>>    hw/core/machine.c        | 1 +
>>>    hw/virtio/virtio-iommu.c | 2 +-
>>>    2 files changed, 2 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/hw/core/machine.c b/hw/core/machine.c
>>> index 70ac96954c..38851df4b8 100644
>>> --- a/hw/core/machine.c
>>> +++ b/hw/core/machine.c
>>> @@ -35,6 +35,7 @@
>>>      GlobalProperty hw_compat_8_2[] = {
>>>        { TYPE_VIRTIO_IOMMU_PCI, "aw-bits", "64" },
>>> +    { TYPE_VIRTIO_IOMMU_PCI, "granule", "4K" },
>>
>> IIUC the current value is qemu_target_page_mask(), not 4KiB.
>> Could this be an issue for arm / mips / mips / sparc64 guests?
> The virtio-iommu currently is only supported on ARM and x86 where
> qemu_target_page_mask() corresponds to a 4K granule. So I don't think it
> makes any issue.

Thanks, please explicit that in the commit description.

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Regards,

Phil.

>>>    };
>>>    const size_t hw_compat_8_2_len = G_N_ELEMENTS(hw_compat_8_2);
>>>    diff --git a/hw/virtio/virtio-iommu.c b/hw/virtio/virtio-iommu.c
>>> index 0461b87ef2..e9e44a8ad8 100644
>>> --- a/hw/virtio/virtio-iommu.c
>>> +++ b/hw/virtio/virtio-iommu.c
>>> @@ -1558,7 +1558,7 @@ static Property virtio_iommu_properties[] = {
>>>        DEFINE_PROP_BOOL("boot-bypass", VirtIOIOMMU, boot_bypass, true),
>>>        DEFINE_PROP_UINT8("aw-bits", VirtIOIOMMU, aw_bits, 0),
>>>        DEFINE_PROP_GRANULE_MODE("granule", VirtIOIOMMU, granule_mode,
>>> -                             GRANULE_MODE_4K),
>>> +                             GRANULE_MODE_HOST),
>>>        DEFINE_PROP_END_OF_LIST(),
>>>    };
>>>    
>>
> 

Re: [PATCH v4 1/3] qdev: Add a granule_mode property

[Philippe Mathieu-Daudé]

On 23/2/24 09:30, Eric Auger wrote:
> Hi Philippe,
> 
> On 2/23/24 08:52, Philippe Mathieu-Daudé wrote:
>> Hi Eric,
>>
>> On 23/2/24 08:27, Eric Auger wrote:
>>> Introduce a new enum type property allowing to set an
>>> IOMMU granule. Values are 4K, 8K, 16K, 64K and host.
>>> This latter indicates the vIOMMU granule will match
>>> the host page size.
>>>
>>> A subsequent patch will add such a property to the
>>> virtio-iommu device.
>>>
>>> Signed-off-by: Eric Auger <eric.auger@redhat.com>
>>>
>>> ---
>>>
>>> v3 -> v4:
>>> - Add 8K
>>> ---
>>>    include/hw/qdev-properties-system.h |  3 +++
>>>    include/hw/virtio/virtio-iommu.h    | 11 +++++++++++
>>>    hw/core/qdev-properties-system.c    | 15 +++++++++++++++
>>>    hw/virtio/virtio-iommu.c            | 11 +++++++++++
>>>    4 files changed, 40 insertions(+)
>>
>>
>>> diff --git a/include/hw/virtio/virtio-iommu.h
>>> b/include/hw/virtio/virtio-iommu.h
>>> index 5fbe4677c2..e22327548f 100644
>>> --- a/include/hw/virtio/virtio-iommu.h
>>> +++ b/include/hw/virtio/virtio-iommu.h
>>> @@ -31,6 +31,17 @@ OBJECT_DECLARE_SIMPLE_TYPE(VirtIOIOMMU, VIRTIO_IOMMU)
>>>      #define TYPE_VIRTIO_IOMMU_MEMORY_REGION "virtio-iommu-memory-region"
>>>    +typedef enum GranuleMode {
>>> +    GRANULE_MODE_4K,
>>> +    GRANULE_MODE_8K,
>>> +    GRANULE_MODE_16K,
>>> +    GRANULE_MODE_64K,
>>> +    GRANULE_MODE_HOST,
>>> +    GRANULE_MODE__MAX,
>>> +} GranuleMode;
>>> +
>>> +extern const QEnumLookup GranuleMode_lookup;
>>
>> Aren't this, ...
>>
>>> diff --git a/hw/virtio/virtio-iommu.c b/hw/virtio/virtio-iommu.c
>>> index 2ec5ef3cd1..a9bdc03d12 100644
>>> --- a/hw/virtio/virtio-iommu.c
>>> +++ b/hw/virtio/virtio-iommu.c
>>> @@ -44,6 +44,17 @@
>>>    #define VIOMMU_DEFAULT_QUEUE_SIZE 256
>>>    #define VIOMMU_PROBE_SIZE 512
>>>    +const QEnumLookup GranuleMode_lookup = {
>>> +    .array = (const char *const[]) {
>>> +        [GRANULE_MODE_4K]   = "4K",
>>> +        [GRANULE_MODE_8K]   = "8K",
>>> +        [GRANULE_MODE_16K]  = "16K",
>>> +        [GRANULE_MODE_64K]  = "64K",
>>> +        [GRANULE_MODE_HOST] = "host",
>>> +    },
>>> +    .size = GRANULE_MODE__MAX
>>> +};
>> ... and this supposed to be QAPI generated?
> If I remove both, it does not compile. How do you make it generated?

The following:

-- >8 --
diff --git a/qapi/virtio.json b/qapi/virtio.json
index 19c7c36e36..c8d38d23e2 100644
--- a/qapi/virtio.json
+++ b/qapi/virtio.json
@@ -959 +959,19 @@
    'data': { 'unused-iothread-vq-mapping': ['IOThreadVirtQueueMapping'] } }
+
+##
+# @GranuleMode:
+#
+# @4k: granule page size of 4KiB
+#
+# @8k: granule page size of 8KiB
+#
+# @16k: granule page size of 16KiB
+#
+# @64k: granule page size of 64KiB
+#
+# @host: granule matches the host page size
+#
+# Since: 9.0
+##
+{ 'enum': 'GranuleMode',
+  'data': [ '4k', '8k', '16k', '64k', 'host' ] }
---

generates in "qapi/qapi-types-virtio.h":

---
typedef enum GranuleMode {
     GRANULE_MODE_4K,
     GRANULE_MODE_8K,
     GRANULE_MODE_16K,
     GRANULE_MODE_64K,
     GRANULE_MODE_HOST,
     GRANULE_MODE__MAX,
} GranuleMode;

#define GranuleMode_str(val) \
     qapi_enum_lookup(&GranuleMode_lookup, (val))

extern const QEnumLookup GranuleMode_lookup;
---