[PATCH v6 0/9] VIRTIO-IOMMU: Introduce aw-bits and granule options

[PATCH v6 0/9] VIRTIO-IOMMU: Introduce aw-bits and granule options

[Eric Auger]

This is a respin of
[1] [PATCH v5 0/4] VIRTIO-IOMMU: Introduce an aw-bits option
(https://lore.kernel.org/all/20240215084315.863897-1-eric.auger@redhat.com/)

which now also integrates

[PATCH v6 0/3] VIRTIO-IOMMU: Set default granule to host page size
(https://lore.kernel.org/all/20240227165730.14099-1-eric.auger@redhat.com/)

The introduction of those 2 new options and their new default values
fix bugs when assigning VFIO devices protected by a virtio-iommu.

patches 1 - 4: intro of the granule property, collected reviews
- we used to set the default granule to 4k. This causes failures
  when hotplugging a VFIO device on a 64kB/64kB host/guest config:
  "vfio: DMA mapping failed, unable to continue". When the device
  is hotplugged the granule is already frozen to 4k wheras 64k is
  needed. This series introduces a new granule option which is set
  by default to the host page size.

patches 5 - 9: intro of the aw-bits property, needs further review
- we used to set the input address width to 64b. This causes
  failures with some assigned devices where the guest driver
  tries to use the full 64b input range whereas the physical IOMMU
  supports less bits (39/48 gaw for instance on VTD). New default
  usually match the host HW capability.

For more details please see the cover letter of [1] and [2].

This series can be found at:
https://github.com/eauger/qemu/tree/granule_aw_bits_v4


Eric Auger (9):
  qdev: Add a granule_mode property
  virtio-iommu: Add a granule property
  virtio-iommu: Change the default granule to the host page size
  qemu-options.hx: Document the virtio-iommu-pci granule option
  virtio-iommu: Trace domain range limits as unsigned int
  virtio-iommu: Add an option to define the input range width
  hw/i386/q35: Set virtio-iommu aw-bits default value to 39
  hw/arm/virt: Set virtio-iommu aw-bits default value to 48
  qemu-options.hx: Document the virtio-iommu-pci aw-bits option

 qapi/virtio.json                    | 18 +++++++++++++++
 include/hw/i386/pc.h                |  3 +++
 include/hw/qdev-properties-system.h |  3 +++
 include/hw/virtio/virtio-iommu.h    |  3 +++
 hw/arm/virt.c                       | 17 ++++++++++++++
 hw/core/machine.c                   |  6 ++++-
 hw/core/qdev-properties-system.c    | 14 ++++++++++++
 hw/i386/pc.c                        |  6 +++++
 hw/i386/pc_q35.c                    |  2 ++
 hw/virtio/virtio-iommu.c            | 35 +++++++++++++++++++++++++----
 tests/qtest/virtio-iommu-test.c     |  2 +-
 hw/virtio/trace-events              |  2 +-
 qemu-options.hx                     | 11 +++++++++
 13 files changed, 115 insertions(+), 7 deletions(-)

-- 
2.41.0

[PATCH v6 1/9] 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>
Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Reviewed-by: Zhenzhong Duan <zhenzhong.duan@intel.com>

---
v5 -> v6
- remove #include "hw/virtio/virtio-iommu.h" (Zhenzhong)

v4 -> v5
- remove code that can be automatically generated
  and add the new enum in qapi/virtio.json (Philippe).
  Added Phild's SOB. low case needs to be used due to
  the Jason generation.

v3 -> v4:
- Add 8K
---
 qapi/virtio.json                    | 18 ++++++++++++++++++
 include/hw/qdev-properties-system.h |  3 +++
 hw/core/qdev-properties-system.c    | 14 ++++++++++++++
 3 files changed, 35 insertions(+)

diff --git a/qapi/virtio.json b/qapi/virtio.json
index a79013fe89..95745fdfd7 100644
--- a/qapi/virtio.json
+++ b/qapi/virtio.json
@@ -957,3 +957,21 @@
 
 { 'struct': 'DummyVirtioForceArrays',
   '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' ] }
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/hw/core/qdev-properties-system.c b/hw/core/qdev-properties-system.c
index 1a396521d5..b45e90edb2 100644
--- a/hw/core/qdev-properties-system.c
+++ b/hw/core/qdev-properties-system.c
@@ -679,6 +679,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 --- */
 
 /*
-- 
2.41.0

[PATCH v6 2/9] 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 on ARM and x86_64 where virtio-iommu
is supported.

Signed-off-by: Eric Auger <eric.auger@redhat.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Reviewed-by: Zhenzhong Duan <zhenzhong.duan@intel.com>

---
v4 -> v5:
- use -(n * KiB) (Phild)

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

diff --git a/include/hw/virtio/virtio-iommu.h b/include/hw/virtio/virtio-iommu.h
index 781ebaea8f..67ea5022af 100644
--- a/include/hw/virtio/virtio-iommu.h
+++ b/include/hw/virtio/virtio-iommu.h
@@ -24,6 +24,7 @@
 #include "hw/virtio/virtio.h"
 #include "hw/pci/pci.h"
 #include "qom/object.h"
+#include "qapi/qapi-types-virtio.h"
 
 #define TYPE_VIRTIO_IOMMU "virtio-iommu-device"
 #define TYPE_VIRTIO_IOMMU_PCI "virtio-iommu-pci"
@@ -66,6 +67,7 @@ struct VirtIOIOMMU {
     bool boot_bypass;
     Notifier machine_done;
     bool granule_frozen;
+    GranuleMode granule_mode;
 };
 
 #endif
diff --git a/hw/virtio/virtio-iommu.c b/hw/virtio/virtio-iommu.c
index 86623d55a5..84d6819d3b 100644
--- a/hw/virtio/virtio-iommu.c
+++ b/hw/virtio/virtio-iommu.c
@@ -29,6 +29,7 @@
 #include "sysemu/reset.h"
 #include "sysemu/sysemu.h"
 #include "qemu/reserved-region.h"
+#include "qemu/units.h"
 #include "qapi/error.h"
 #include "qemu/error-report.h"
 #include "trace.h"
@@ -1115,8 +1116,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 -(4 * KiB). When an assigned device has page size
  * restrictions due to the hardware IOMMU configuration, apply this restriction
  * to the mask.
  */
@@ -1313,8 +1314,27 @@ 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();
     s->config.input_range.end = UINT64_MAX;
+
+    switch (s->granule_mode) {
+    case GRANULE_MODE_4K:
+        s->config.page_size_mask = -(4 * KiB);
+        break;
+    case GRANULE_MODE_8K:
+        s->config.page_size_mask = -(8 * KiB);
+        break;
+    case GRANULE_MODE_16K:
+        s->config.page_size_mask = -(16 * KiB);
+        break;
+    case GRANULE_MODE_64K:
+        s->config.page_size_mask = -(64 * KiB);
+        break;
+    case GRANULE_MODE_HOST:
+        s->config.page_size_mask = qemu_real_host_page_mask();
+        break;
+    default:
+        error_setg(errp, "Unsupported granule mode");
+    }
     s->config.domain_range.end = UINT32_MAX;
     s->config.probe_size = VIOMMU_PROBE_SIZE;
 
@@ -1522,6 +1542,8 @@ static Property virtio_iommu_properties[] = {
     DEFINE_PROP_LINK("primary-bus", VirtIOIOMMU, primary_bus,
                      TYPE_PCI_BUS, PCIBus *),
     DEFINE_PROP_BOOL("boot-bypass", VirtIOIOMMU, boot_bypass, true),
+    DEFINE_PROP_GRANULE_MODE("granule", VirtIOIOMMU, granule_mode,
+                             GRANULE_MODE_4K),
     DEFINE_PROP_END_OF_LIST(),
 };
 
-- 
2.41.0

[PATCH v6 3/9] 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.
The page size mask used before 9.0 was qemu_target_page_mask().
Since the virtio-iommu currently only supports x86_64 and aarch64,
this matched a 4KB granule.

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>
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Reviewed-by: Zhenzhong Duan <zhenzhong.duan@intel.com>

---

v4 -> v5
- use low case, mandated by the jason qapi
---
 hw/core/machine.c        | 5 ++++-
 hw/virtio/virtio-iommu.c | 2 +-
 2 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/hw/core/machine.c b/hw/core/machine.c
index 9ac5d5389a..6bd09d4592 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -30,9 +30,12 @@
 #include "exec/confidential-guest-support.h"
 #include "hw/virtio/virtio-pci.h"
 #include "hw/virtio/virtio-net.h"
+#include "hw/virtio/virtio-iommu.h"
 #include "audio/audio.h"
 
-GlobalProperty hw_compat_8_2[] = {};
+GlobalProperty hw_compat_8_2[] = {
+    { TYPE_VIRTIO_IOMMU_PCI, "granule", "4k" },
+};
 const size_t hw_compat_8_2_len = G_N_ELEMENTS(hw_compat_8_2);
 
 GlobalProperty hw_compat_8_1[] = {
diff --git a/hw/virtio/virtio-iommu.c b/hw/virtio/virtio-iommu.c
index 84d6819d3b..aab97e1527 100644
--- a/hw/virtio/virtio-iommu.c
+++ b/hw/virtio/virtio-iommu.c
@@ -1543,7 +1543,7 @@ static Property virtio_iommu_properties[] = {
                      TYPE_PCI_BUS, PCIBus *),
     DEFINE_PROP_BOOL("boot-bypass", VirtIOIOMMU, boot_bypass, true),
     DEFINE_PROP_GRANULE_MODE("granule", VirtIOIOMMU, granule_mode,
-                             GRANULE_MODE_4K),
+                             GRANULE_MODE_HOST),
     DEFINE_PROP_END_OF_LIST(),
 };
 
-- 
2.41.0

[PATCH v6 4/9] qemu-options.hx: Document the virtio-iommu-pci granule option

[Eric Auger]

We are missing an entry for the virtio-iommu-pci device. Add the
information on which machine it is currently supported and document
the new granule option.

Signed-off-by: Eric Auger <eric.auger@redhat.com>
---
 qemu-options.hx | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/qemu-options.hx b/qemu-options.hx
index 9a47385c15..757df3eac0 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -1172,6 +1172,14 @@ SRST
     Please also refer to the wiki page for general scenarios of VT-d
     emulation in QEMU: https://wiki.qemu.org/Features/VT-d.
 
+``-device virtio-iommu-pci[,option=...]``
+    This is only supported by ``-machine q35`` and ``-machine virt``.
+    It supports below options:
+
+    ``granule=val`` (possible values are 4k, 8k, 16k, 64k and host; default: host)
+        This decides the default granule to be be exposed by the
+        virtio-iommu. If host, the granule matches the host page size.
+
 ERST
 
 DEF("name", HAS_ARG, QEMU_OPTION_name,
-- 
2.41.0

[PATCH v6 5/9] virtio-iommu: Trace domain range limits as unsigned int

[Eric Auger]

Use %u format to trace domain_range limits.

Signed-off-by: Eric Auger <eric.auger@redhat.com>
Reviewed-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
Reviewed-by: Cédric Le Goater <clg@redhat.com>
---
 hw/virtio/trace-events | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/virtio/trace-events b/hw/virtio/trace-events
index 77905d1994..2350849fbd 100644
--- a/hw/virtio/trace-events
+++ b/hw/virtio/trace-events
@@ -111,7 +111,7 @@ virtio_iommu_device_reset(void) "reset!"
 virtio_iommu_system_reset(void) "system reset!"
 virtio_iommu_get_features(uint64_t features) "device supports features=0x%"PRIx64
 virtio_iommu_device_status(uint8_t status) "driver status = %d"
-virtio_iommu_get_config(uint64_t page_size_mask, uint64_t start, uint64_t end, uint32_t domain_start, uint32_t domain_end, uint32_t probe_size, uint8_t bypass) "page_size_mask=0x%"PRIx64" input range start=0x%"PRIx64" input range end=0x%"PRIx64" domain range start=%d domain range end=%d probe_size=0x%x bypass=0x%x"
+virtio_iommu_get_config(uint64_t page_size_mask, uint64_t start, uint64_t end, uint32_t domain_start, uint32_t domain_end, uint32_t probe_size, uint8_t bypass) "page_size_mask=0x%"PRIx64" input range start=0x%"PRIx64" input range end=0x%"PRIx64" domain range start=%u domain range end=%u probe_size=0x%x bypass=0x%x"
 virtio_iommu_set_config(uint8_t bypass) "bypass=0x%x"
 virtio_iommu_attach(uint32_t domain_id, uint32_t ep_id) "domain=%d endpoint=%d"
 virtio_iommu_detach(uint32_t domain_id, uint32_t ep_id) "domain=%d endpoint=%d"
-- 
2.41.0

[PATCH v6 6/9] virtio-iommu: Add an option to define the input range width

[Eric Auger]

aw-bits is a new option that allows to set the bit width of
the input address range. This value will be used as a default for
the device config input_range.end. By default it is set to 64 bits
which is the current value.

Signed-off-by: Eric Auger <eric.auger@redhat.com>
Reviewed-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
Reviewed-by: Cédric Le Goater <clg@redhat.com>

---

v1 -> v2:
- Check the aw-bits value is within [32,64]
---
 include/hw/virtio/virtio-iommu.h | 1 +
 hw/virtio/virtio-iommu.c         | 7 ++++++-
 2 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/include/hw/virtio/virtio-iommu.h b/include/hw/virtio/virtio-iommu.h
index 67ea5022af..83a52cc446 100644
--- a/include/hw/virtio/virtio-iommu.h
+++ b/include/hw/virtio/virtio-iommu.h
@@ -68,6 +68,7 @@ struct VirtIOIOMMU {
     Notifier machine_done;
     bool granule_frozen;
     GranuleMode granule_mode;
+    uint8_t aw_bits;
 };
 
 #endif
diff --git a/hw/virtio/virtio-iommu.c b/hw/virtio/virtio-iommu.c
index aab97e1527..9b2813188b 100644
--- a/hw/virtio/virtio-iommu.c
+++ b/hw/virtio/virtio-iommu.c
@@ -1314,7 +1314,11 @@ static void virtio_iommu_device_realize(DeviceState *dev, Error **errp)
      * in vfio realize
      */
     s->config.bypass = s->boot_bypass;
-    s->config.input_range.end = UINT64_MAX;
+    if (s->aw_bits < 32 || s->aw_bits > 64) {
+        error_setg(errp, "aw-bits must be within [32,64]");
+    }
+    s->config.input_range.end =
+        s->aw_bits == 64 ? UINT64_MAX : BIT_ULL(s->aw_bits) - 1;
 
     switch (s->granule_mode) {
     case GRANULE_MODE_4K:
@@ -1544,6 +1548,7 @@ static Property virtio_iommu_properties[] = {
     DEFINE_PROP_BOOL("boot-bypass", VirtIOIOMMU, boot_bypass, true),
     DEFINE_PROP_GRANULE_MODE("granule", VirtIOIOMMU, granule_mode,
                              GRANULE_MODE_HOST),
+    DEFINE_PROP_UINT8("aw-bits", VirtIOIOMMU, aw_bits, 64),
     DEFINE_PROP_END_OF_LIST(),
 };
 
-- 
2.41.0

[PATCH v6 7/9] hw/i386/q35: Set virtio-iommu aw-bits default value to 39

[Eric Auger]

Currently the default input range can extend to 64 bits. On x86,
when the virtio-iommu protects vfio devices, the physical iommu
may support only 39 bits. Let's set the default to 39, as done
for the intel-iommu.

We use hw_compat_8_2 to handle the compatibility for machines
before 9.0 which used to have a virtio-iommu default input range
of 64 bits.

Of course if aw-bits is set from the command line, the default
is overriden.

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

---

v5 -> v6:
- split pc/arm settings

v3 -> v4:
- update the qos test to relax the check on the max input IOVA

v2 -> v3:
- collected Zhenzhong's R-b
- use &error_abort instead of NULL error handle
  on object_property_get_uint() call (Cédric)
- use VTD_HOST_AW_39BIT (Cédric)

v1 -> v2:
- set aw-bits to 48b on ARM
- use hw_compat_8_2 to handle the compat for older machines
  which used 64b as a default
---
 include/hw/i386/pc.h            | 3 +++
 hw/core/machine.c               | 1 +
 hw/i386/pc.c                    | 6 ++++++
 hw/i386/pc_q35.c                | 2 ++
 tests/qtest/virtio-iommu-test.c | 2 +-
 5 files changed, 13 insertions(+), 1 deletion(-)

diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
index 5065590281..b3229f98de 100644
--- a/include/hw/i386/pc.h
+++ b/include/hw/i386/pc.h
@@ -198,6 +198,9 @@ void pc_system_parse_ovmf_flash(uint8_t *flash_ptr, size_t flash_size);
 /* sgx.c */
 void pc_machine_init_sgx_epc(PCMachineState *pcms);
 
+extern GlobalProperty pc_compat_defaults[];
+extern const size_t pc_compat_defaults_len;
+
 extern GlobalProperty pc_compat_8_2[];
 extern const size_t pc_compat_8_2_len;
 
diff --git a/hw/core/machine.c b/hw/core/machine.c
index 6bd09d4592..4b89172d1c 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -35,6 +35,7 @@
 
 GlobalProperty hw_compat_8_2[] = {
     { TYPE_VIRTIO_IOMMU_PCI, "granule", "4k" },
+    { TYPE_VIRTIO_IOMMU_PCI, "aw-bits", "64" },
 };
 const size_t hw_compat_8_2_len = G_N_ELEMENTS(hw_compat_8_2);
 
diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index f5ff970acf..9024483356 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -59,6 +59,7 @@
 #include "hw/i386/kvm/xen_evtchn.h"
 #include "hw/i386/kvm/xen_gnttab.h"
 #include "hw/i386/kvm/xen_xenstore.h"
+#include "hw/i386/intel_iommu.h"
 #include "hw/mem/memory-device.h"
 #include "e820_memory_layout.h"
 #include "trace.h"
@@ -78,6 +79,11 @@
     { "qemu64-" TYPE_X86_CPU, "model-id", "QEMU Virtual CPU version " v, },\
     { "athlon-" TYPE_X86_CPU, "model-id", "QEMU Virtual CPU version " v, },
 
+GlobalProperty pc_compat_defaults[] =  {
+    { TYPE_VIRTIO_IOMMU_PCI, "aw-bits", "39" },
+};
+const size_t pc_compat_defaults_len = G_N_ELEMENTS(pc_compat_defaults);
+
 GlobalProperty pc_compat_8_2[] = {};
 const size_t pc_compat_8_2_len = G_N_ELEMENTS(pc_compat_8_2);
 
diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
index 45a4102e75..32421a0a5f 100644
--- a/hw/i386/pc_q35.c
+++ b/hw/i386/pc_q35.c
@@ -356,6 +356,8 @@ static void pc_q35_machine_options(MachineClass *m)
     machine_class_allow_dynamic_sysbus_dev(m, TYPE_INTEL_IOMMU_DEVICE);
     machine_class_allow_dynamic_sysbus_dev(m, TYPE_RAMFB_DEVICE);
     machine_class_allow_dynamic_sysbus_dev(m, TYPE_VMBUS_BRIDGE);
+    compat_props_add(m->compat_props,
+                     pc_compat_defaults, pc_compat_defaults_len);
 }
 
 static void pc_q35_9_0_machine_options(MachineClass *m)
diff --git a/tests/qtest/virtio-iommu-test.c b/tests/qtest/virtio-iommu-test.c
index 068e7a9e6c..0f36381acb 100644
--- a/tests/qtest/virtio-iommu-test.c
+++ b/tests/qtest/virtio-iommu-test.c
@@ -34,7 +34,7 @@ static void pci_config(void *obj, void *data, QGuestAllocator *t_alloc)
     uint8_t bypass = qvirtio_config_readb(dev, 36);
 
     g_assert_cmpint(input_range_start, ==, 0);
-    g_assert_cmphex(input_range_end, ==, UINT64_MAX);
+    g_assert_cmphex(input_range_end, >=, 32);
     g_assert_cmpint(domain_range_start, ==, 0);
     g_assert_cmpint(domain_range_end, ==, UINT32_MAX);
     g_assert_cmpint(bypass, ==, 1);
-- 
2.41.0

RE: [PATCH v6 7/9] hw/i386/q35: Set virtio-iommu aw-bits default value to 39

[Duan, Zhenzhong]

Hi Eric,

>-----Original Message-----
>From: Eric Auger <eric.auger@redhat.com>
>Subject: [PATCH v6 7/9] hw/i386/q35: Set virtio-iommu aw-bits default
>value to 39
>
>Currently the default input range can extend to 64 bits. On x86,
>when the virtio-iommu protects vfio devices, the physical iommu
>may support only 39 bits. Let's set the default to 39, as done
>for the intel-iommu.
>
>We use hw_compat_8_2 to handle the compatibility for machines
>before 9.0 which used to have a virtio-iommu default input range
>of 64 bits.
>
>Of course if aw-bits is set from the command line, the default
>is overriden.
>
>Signed-off-by: Eric Auger <eric.auger@redhat.com>
>
>---
>
>v5 -> v6:
>- split pc/arm settings
>
>v3 -> v4:
>- update the qos test to relax the check on the max input IOVA
>
>v2 -> v3:
>- collected Zhenzhong's R-b
>- use &error_abort instead of NULL error handle
>  on object_property_get_uint() call (Cédric)
>- use VTD_HOST_AW_39BIT (Cédric)
>
>v1 -> v2:
>- set aw-bits to 48b on ARM
>- use hw_compat_8_2 to handle the compat for older machines
>  which used 64b as a default
>---
> include/hw/i386/pc.h            | 3 +++
> hw/core/machine.c               | 1 +
> hw/i386/pc.c                    | 6 ++++++
> hw/i386/pc_q35.c                | 2 ++
> tests/qtest/virtio-iommu-test.c | 2 +-
> 5 files changed, 13 insertions(+), 1 deletion(-)
>
>diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
>index 5065590281..b3229f98de 100644
>--- a/include/hw/i386/pc.h
>+++ b/include/hw/i386/pc.h
>@@ -198,6 +198,9 @@ void pc_system_parse_ovmf_flash(uint8_t
>*flash_ptr, size_t flash_size);
> /* sgx.c */
> void pc_machine_init_sgx_epc(PCMachineState *pcms);
>
>+extern GlobalProperty pc_compat_defaults[];
>+extern const size_t pc_compat_defaults_len;

If we only want to support q35 and not i440fx, better to add _q35 suffix and move into pc_q35.c and made static?

>+
> extern GlobalProperty pc_compat_8_2[];
> extern const size_t pc_compat_8_2_len;
>
>diff --git a/hw/core/machine.c b/hw/core/machine.c
>index 6bd09d4592..4b89172d1c 100644
>--- a/hw/core/machine.c
>+++ b/hw/core/machine.c
>@@ -35,6 +35,7 @@
>
> GlobalProperty hw_compat_8_2[] = {
>     { TYPE_VIRTIO_IOMMU_PCI, "granule", "4k" },
>+    { TYPE_VIRTIO_IOMMU_PCI, "aw-bits", "64" },
> };
> const size_t hw_compat_8_2_len = G_N_ELEMENTS(hw_compat_8_2);
>
>diff --git a/hw/i386/pc.c b/hw/i386/pc.c
>index f5ff970acf..9024483356 100644
>--- a/hw/i386/pc.c
>+++ b/hw/i386/pc.c
>@@ -59,6 +59,7 @@
> #include "hw/i386/kvm/xen_evtchn.h"
> #include "hw/i386/kvm/xen_gnttab.h"
> #include "hw/i386/kvm/xen_xenstore.h"
>+#include "hw/i386/intel_iommu.h"

This can be removed?

> #include "hw/mem/memory-device.h"
> #include "e820_memory_layout.h"
> #include "trace.h"
>@@ -78,6 +79,11 @@
>     { "qemu64-" TYPE_X86_CPU, "model-id", "QEMU Virtual CPU version "
>v, },\
>     { "athlon-" TYPE_X86_CPU, "model-id", "QEMU Virtual CPU version " v, },
>
>+GlobalProperty pc_compat_defaults[] =  {
>+    { TYPE_VIRTIO_IOMMU_PCI, "aw-bits", "39" },
>+};
>+const size_t pc_compat_defaults_len =
>G_N_ELEMENTS(pc_compat_defaults);
>+
> GlobalProperty pc_compat_8_2[] = {};
> const size_t pc_compat_8_2_len = G_N_ELEMENTS(pc_compat_8_2);
>
>diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
>index 45a4102e75..32421a0a5f 100644
>--- a/hw/i386/pc_q35.c
>+++ b/hw/i386/pc_q35.c
>@@ -356,6 +356,8 @@ static void pc_q35_machine_options(MachineClass
>*m)
>     machine_class_allow_dynamic_sysbus_dev(m,
>TYPE_INTEL_IOMMU_DEVICE);
>     machine_class_allow_dynamic_sysbus_dev(m, TYPE_RAMFB_DEVICE);
>     machine_class_allow_dynamic_sysbus_dev(m, TYPE_VMBUS_BRIDGE);
>+    compat_props_add(m->compat_props,
>+                     pc_compat_defaults, pc_compat_defaults_len);
> }
>
> static void pc_q35_9_0_machine_options(MachineClass *m)
>diff --git a/tests/qtest/virtio-iommu-test.c b/tests/qtest/virtio-iommu-test.c
>index 068e7a9e6c..0f36381acb 100644
>--- a/tests/qtest/virtio-iommu-test.c
>+++ b/tests/qtest/virtio-iommu-test.c
>@@ -34,7 +34,7 @@ static void pci_config(void *obj, void *data,
>QGuestAllocator *t_alloc)
>     uint8_t bypass = qvirtio_config_readb(dev, 36);
>
>     g_assert_cmpint(input_range_start, ==, 0);
>-    g_assert_cmphex(input_range_end, ==, UINT64_MAX);
>+    g_assert_cmphex(input_range_end, >=, 32);

UINT32_MAX?

Thanks
Zhenzhong

>     g_assert_cmpint(domain_range_start, ==, 0);
>     g_assert_cmpint(domain_range_end, ==, UINT32_MAX);
>     g_assert_cmpint(bypass, ==, 1);
>--
>2.41.0

Re: [PATCH v6 7/9] hw/i386/q35: Set virtio-iommu aw-bits default value to 39

[Eric Auger]

Hi Zhenzhong,

On 3/6/24 04:15, Duan, Zhenzhong wrote:
> Hi Eric,
>
>> -----Original Message-----
>> From: Eric Auger <eric.auger@redhat.com>
>> Subject: [PATCH v6 7/9] hw/i386/q35: Set virtio-iommu aw-bits default
>> value to 39
>>
>> Currently the default input range can extend to 64 bits. On x86,
>> when the virtio-iommu protects vfio devices, the physical iommu
>> may support only 39 bits. Let's set the default to 39, as done
>> for the intel-iommu.
>>
>> We use hw_compat_8_2 to handle the compatibility for machines
>> before 9.0 which used to have a virtio-iommu default input range
>> of 64 bits.
>>
>> Of course if aw-bits is set from the command line, the default
>> is overriden.
>>
>> Signed-off-by: Eric Auger <eric.auger@redhat.com>
>>
>> ---
>>
>> v5 -> v6:
>> - split pc/arm settings
>>
>> v3 -> v4:
>> - update the qos test to relax the check on the max input IOVA
>>
>> v2 -> v3:
>> - collected Zhenzhong's R-b
>> - use &error_abort instead of NULL error handle
>>  on object_property_get_uint() call (Cédric)
>> - use VTD_HOST_AW_39BIT (Cédric)
>>
>> v1 -> v2:
>> - set aw-bits to 48b on ARM
>> - use hw_compat_8_2 to handle the compat for older machines
>>  which used 64b as a default
>> ---
>> include/hw/i386/pc.h            | 3 +++
>> hw/core/machine.c               | 1 +
>> hw/i386/pc.c                    | 6 ++++++
>> hw/i386/pc_q35.c                | 2 ++
>> tests/qtest/virtio-iommu-test.c | 2 +-
>> 5 files changed, 13 insertions(+), 1 deletion(-)
>>
>> diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
>> index 5065590281..b3229f98de 100644
>> --- a/include/hw/i386/pc.h
>> +++ b/include/hw/i386/pc.h
>> @@ -198,6 +198,9 @@ void pc_system_parse_ovmf_flash(uint8_t
>> *flash_ptr, size_t flash_size);
>> /* sgx.c */
>> void pc_machine_init_sgx_epc(PCMachineState *pcms);
>>
>> +extern GlobalProperty pc_compat_defaults[];
>> +extern const size_t pc_compat_defaults_len;
> If we only want to support q35 and not i440fx, better to add _q35 suffix and move into pc_q35.c and made static?
Sure. I was initially following Igor's suggestion but it makes sense to
restrict to q35 and turn in static I think.
>
>> +
>> extern GlobalProperty pc_compat_8_2[];
>> extern const size_t pc_compat_8_2_len;
>>
>> diff --git a/hw/core/machine.c b/hw/core/machine.c
>> index 6bd09d4592..4b89172d1c 100644
>> --- a/hw/core/machine.c
>> +++ b/hw/core/machine.c
>> @@ -35,6 +35,7 @@
>>
>> GlobalProperty hw_compat_8_2[] = {
>>     { TYPE_VIRTIO_IOMMU_PCI, "granule", "4k" },
>> +    { TYPE_VIRTIO_IOMMU_PCI, "aw-bits", "64" },
>> };
>> const size_t hw_compat_8_2_len = G_N_ELEMENTS(hw_compat_8_2);
>>
>> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
>> index f5ff970acf..9024483356 100644
>> --- a/hw/i386/pc.c
>> +++ b/hw/i386/pc.c
>> @@ -59,6 +59,7 @@
>> #include "hw/i386/kvm/xen_evtchn.h"
>> #include "hw/i386/kvm/xen_gnttab.h"
>> #include "hw/i386/kvm/xen_xenstore.h"
>> +#include "hw/i386/intel_iommu.h"
> This can be removed?
yes
>
>> #include "hw/mem/memory-device.h"
>> #include "e820_memory_layout.h"
>> #include "trace.h"
>> @@ -78,6 +79,11 @@
>>     { "qemu64-" TYPE_X86_CPU, "model-id", "QEMU Virtual CPU version "
>> v, },\
>>     { "athlon-" TYPE_X86_CPU, "model-id", "QEMU Virtual CPU version " v, },
>>
>> +GlobalProperty pc_compat_defaults[] =  {
>> +    { TYPE_VIRTIO_IOMMU_PCI, "aw-bits", "39" },
>> +};
>> +const size_t pc_compat_defaults_len =
>> G_N_ELEMENTS(pc_compat_defaults);
>> +
>> GlobalProperty pc_compat_8_2[] = {};
>> const size_t pc_compat_8_2_len = G_N_ELEMENTS(pc_compat_8_2);
>>
>> diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
>> index 45a4102e75..32421a0a5f 100644
>> --- a/hw/i386/pc_q35.c
>> +++ b/hw/i386/pc_q35.c
>> @@ -356,6 +356,8 @@ static void pc_q35_machine_options(MachineClass
>> *m)
>>     machine_class_allow_dynamic_sysbus_dev(m,
>> TYPE_INTEL_IOMMU_DEVICE);
>>     machine_class_allow_dynamic_sysbus_dev(m, TYPE_RAMFB_DEVICE);
>>     machine_class_allow_dynamic_sysbus_dev(m, TYPE_VMBUS_BRIDGE);
>> +    compat_props_add(m->compat_props,
>> +                     pc_compat_defaults, pc_compat_defaults_len);
>> }
>>
>> static void pc_q35_9_0_machine_options(MachineClass *m)
>> diff --git a/tests/qtest/virtio-iommu-test.c b/tests/qtest/virtio-iommu-test.c
>> index 068e7a9e6c..0f36381acb 100644
>> --- a/tests/qtest/virtio-iommu-test.c
>> +++ b/tests/qtest/virtio-iommu-test.c
>> @@ -34,7 +34,7 @@ static void pci_config(void *obj, void *data,
>> QGuestAllocator *t_alloc)
>>     uint8_t bypass = qvirtio_config_readb(dev, 36);
>>
>>     g_assert_cmpint(input_range_start, ==, 0);
>> -    g_assert_cmphex(input_range_end, ==, UINT64_MAX);
>> +    g_assert_cmphex(input_range_end, >=, 32);
> UINT32_MAX?
sure!

Thank you for the review!

Eric
>
> Thanks
> Zhenzhong
>
>>     g_assert_cmpint(domain_range_start, ==, 0);
>>     g_assert_cmpint(domain_range_end, ==, UINT32_MAX);
>>     g_assert_cmpint(bypass, ==, 1);
>> --
>> 2.41.0

[PATCH v6 8/9] hw/arm/virt: Set virtio-iommu aw-bits default value to 48

[Eric Auger]

On ARM we set 48b as a default (matching SMMUv3 SMMU_IDR5.VAX == 0).

hw_compat_8_2 is used to handle the compatibility for machine types
before 9.0 (default was 64 bits).

Signed-off-by: Eric Auger <eric.auger@redhat.com>
---
 hw/arm/virt.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index 0af1943697..dcfb25369b 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -85,11 +85,28 @@
 #include "hw/char/pl011.h"
 #include "qemu/guest-random.h"
 
+GlobalProperty arm_virt_compat[] = {
+    { TYPE_VIRTIO_IOMMU_PCI, "aw-bits", "48" },
+};
+const size_t arm_virt_compat_len = G_N_ELEMENTS(arm_virt_compat);
+
+/*
+ * This cannot be called from the virt_machine_class_init() because
+ * TYPE_VIRT_MACHINE is abstract and mc->compat_props g_ptr_array_new()
+ * only is called on virt non abstract class init.
+ */
+static void arm_virt_compat_set(MachineClass *mc)
+{
+    compat_props_add(mc->compat_props, arm_virt_compat,
+                     arm_virt_compat_len);
+}
+
 #define DEFINE_VIRT_MACHINE_LATEST(major, minor, latest) \
     static void virt_##major##_##minor##_class_init(ObjectClass *oc, \
                                                     void *data) \
     { \
         MachineClass *mc = MACHINE_CLASS(oc); \
+        arm_virt_compat_set(mc); \
         virt_machine_##major##_##minor##_options(mc); \
         mc->desc = "QEMU " # major "." # minor " ARM Virtual Machine"; \
         if (latest) { \
-- 
2.41.0

RE: [PATCH v6 8/9] hw/arm/virt: Set virtio-iommu aw-bits default value to 48

[Duan, Zhenzhong]

>-----Original Message-----
>From: Eric Auger <eric.auger@redhat.com>
>Subject: [PATCH v6 8/9] hw/arm/virt: Set virtio-iommu aw-bits default value
>to 48
>
>On ARM we set 48b as a default (matching SMMUv3 SMMU_IDR5.VAX == 0).
>
>hw_compat_8_2 is used to handle the compatibility for machine types
>before 9.0 (default was 64 bits).
>
>Signed-off-by: Eric Auger <eric.auger@redhat.com>
>---
> hw/arm/virt.c | 17 +++++++++++++++++
> 1 file changed, 17 insertions(+)
>
>diff --git a/hw/arm/virt.c b/hw/arm/virt.c
>index 0af1943697..dcfb25369b 100644
>--- a/hw/arm/virt.c
>+++ b/hw/arm/virt.c
>@@ -85,11 +85,28 @@
> #include "hw/char/pl011.h"
> #include "qemu/guest-random.h"
>
>+GlobalProperty arm_virt_compat[] = {
>+    { TYPE_VIRTIO_IOMMU_PCI, "aw-bits", "48" },
>+};
>+const size_t arm_virt_compat_len = G_N_ELEMENTS(arm_virt_compat);

This can be static, otherwise,

Reviewed-by: Zhenzhong Duan <Zhenzhong.duan@intel.com>

Thanks
Zhenzhong

>+
>+/*
>+ * This cannot be called from the virt_machine_class_init() because
>+ * TYPE_VIRT_MACHINE is abstract and mc->compat_props
>g_ptr_array_new()
>+ * only is called on virt non abstract class init.
>+ */
>+static void arm_virt_compat_set(MachineClass *mc)
>+{
>+    compat_props_add(mc->compat_props, arm_virt_compat,
>+                     arm_virt_compat_len);
>+}
>+
> #define DEFINE_VIRT_MACHINE_LATEST(major, minor, latest) \
>     static void virt_##major##_##minor##_class_init(ObjectClass *oc, \
>                                                     void *data) \
>     { \
>         MachineClass *mc = MACHINE_CLASS(oc); \
>+        arm_virt_compat_set(mc); \
>         virt_machine_##major##_##minor##_options(mc); \
>         mc->desc = "QEMU " # major "." # minor " ARM Virtual Machine"; \
>         if (latest) { \
>--
>2.41.0

Re: [PATCH v6 8/9] hw/arm/virt: Set virtio-iommu aw-bits default value to 48

[Eric Auger]

On 3/6/24 04:25, Duan, Zhenzhong wrote:
>
>> -----Original Message-----
>> From: Eric Auger <eric.auger@redhat.com>
>> Subject: [PATCH v6 8/9] hw/arm/virt: Set virtio-iommu aw-bits default value
>> to 48
>>
>> On ARM we set 48b as a default (matching SMMUv3 SMMU_IDR5.VAX == 0).
>>
>> hw_compat_8_2 is used to handle the compatibility for machine types
>> before 9.0 (default was 64 bits).
>>
>> Signed-off-by: Eric Auger <eric.auger@redhat.com>
>> ---
>> hw/arm/virt.c | 17 +++++++++++++++++
>> 1 file changed, 17 insertions(+)
>>
>> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
>> index 0af1943697..dcfb25369b 100644
>> --- a/hw/arm/virt.c
>> +++ b/hw/arm/virt.c
>> @@ -85,11 +85,28 @@
>> #include "hw/char/pl011.h"
>> #include "qemu/guest-random.h"
>>
>> +GlobalProperty arm_virt_compat[] = {
>> +    { TYPE_VIRTIO_IOMMU_PCI, "aw-bits", "48" },
>> +};
>> +const size_t arm_virt_compat_len = G_N_ELEMENTS(arm_virt_compat);
> This can be static, otherwise,
sure
>
> Reviewed-by: Zhenzhong Duan <Zhenzhong.duan@intel.com>
thanks!

Eric
>
> Thanks
> Zhenzhong
>
>> +
>> +/*
>> + * This cannot be called from the virt_machine_class_init() because
>> + * TYPE_VIRT_MACHINE is abstract and mc->compat_props
>> g_ptr_array_new()
>> + * only is called on virt non abstract class init.
>> + */
>> +static void arm_virt_compat_set(MachineClass *mc)
>> +{
>> +    compat_props_add(mc->compat_props, arm_virt_compat,
>> +                     arm_virt_compat_len);
>> +}
>> +
>> #define DEFINE_VIRT_MACHINE_LATEST(major, minor, latest) \
>>     static void virt_##major##_##minor##_class_init(ObjectClass *oc, \
>>                                                     void *data) \
>>     { \
>>         MachineClass *mc = MACHINE_CLASS(oc); \
>> +        arm_virt_compat_set(mc); \
>>         virt_machine_##major##_##minor##_options(mc); \
>>         mc->desc = "QEMU " # major "." # minor " ARM Virtual Machine"; \
>>         if (latest) { \
>> --
>> 2.41.0

[PATCH v6 9/9] qemu-options.hx: Document the virtio-iommu-pci aw-bits option

[Eric Auger]

Document the new aw-bits option.

Signed-off-by: Eric Auger <eric.auger@redhat.com>
Reviewed-by: Cédric Le Goater <clg@redhat.com>

---

v4 -> v5
- tweek the aw-bits option description according to Cédric's
  suggestion
---
 qemu-options.hx | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/qemu-options.hx b/qemu-options.hx
index 757df3eac0..87959ede08 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -1180,6 +1180,9 @@ SRST
         This decides the default granule to be be exposed by the
         virtio-iommu. If host, the granule matches the host page size.
 
+    ``aw-bits=val`` (val between 32 and 64, default depends on machine)
+        This decides the address width of IOVA address space. It defaults
+
 ERST
 
 DEF("name", HAS_ARG, QEMU_OPTION_name,
-- 
2.41.0