| Message ID | 20240215084315.863897-4-eric.auger@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | VIRTIO-IOMMU: Introduce an aw-bits option | expand |
Hello Eric, On 2/15/24 09:42, Eric Auger wrote: > 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. On ARM we set 48b as a default (matching > SMMUv3 SMMU_IDR5.VAX == 0). > > 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> > Reviewed-by: Zhenzhong Duan <zhenzhong.duan@intel.com> > Tested-by: Yanghang Liu<yanghliu@redhat.com> We need a property fixup for pseries also: $ build/ppc64-softmmu/qemu-system-ppc64 -M pseries -device virtio-iommu-pci,addr=04.0 qemu-system-ppc64: -device virtio-iommu-pci,addr=04.0: aw-bits must be within [32,64] Thanks, C. > --- > > 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 > --- > hw/arm/virt.c | 6 ++++++ > hw/core/machine.c | 5 ++++- > hw/i386/pc.c | 6 ++++++ > hw/virtio/virtio-iommu.c | 2 +- > tests/qtest/virtio-iommu-test.c | 2 +- > 5 files changed, 18 insertions(+), 3 deletions(-) > > diff --git a/hw/arm/virt.c b/hw/arm/virt.c > index 368c2a415a..0994f2a560 100644 > --- a/hw/arm/virt.c > +++ b/hw/arm/virt.c > @@ -2716,10 +2716,16 @@ static void virt_machine_device_pre_plug_cb(HotplugHandler *hotplug_dev, > } else if (object_dynamic_cast(OBJECT(dev), TYPE_VIRTIO_MD_PCI)) { > virtio_md_pci_pre_plug(VIRTIO_MD_PCI(dev), MACHINE(hotplug_dev), errp); > } else if (object_dynamic_cast(OBJECT(dev), TYPE_VIRTIO_IOMMU_PCI)) { > + uint8_t aw_bits = object_property_get_uint(OBJECT(dev), > + "aw-bits", &error_abort); > hwaddr db_start = 0, db_end = 0; > QList *reserved_regions; > char *resv_prop_str; > > + if (!aw_bits) { > + qdev_prop_set_uint8(dev, "aw-bits", 48); > + } > + > if (vms->iommu != VIRT_IOMMU_NONE) { > error_setg(errp, "virt machine does not support multiple IOMMUs"); > return; > diff --git a/hw/core/machine.c b/hw/core/machine.c > index fb5afdcae4..70ac96954c 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, "aw-bits", "64" }, > +}; > const size_t hw_compat_8_2_len = G_N_ELEMENTS(hw_compat_8_2); > > GlobalProperty hw_compat_8_1[] = { > diff --git a/hw/i386/pc.c b/hw/i386/pc.c > index 196827531a..ee2d379c90 100644 > --- a/hw/i386/pc.c > +++ b/hw/i386/pc.c > @@ -1456,6 +1456,8 @@ static void pc_machine_device_pre_plug_cb(HotplugHandler *hotplug_dev, > } else if (object_dynamic_cast(OBJECT(dev), TYPE_VIRTIO_MD_PCI)) { > virtio_md_pci_pre_plug(VIRTIO_MD_PCI(dev), MACHINE(hotplug_dev), errp); > } else if (object_dynamic_cast(OBJECT(dev), TYPE_VIRTIO_IOMMU_PCI)) { > + uint8_t aw_bits = object_property_get_uint(OBJECT(dev), > + "aw-bits", &error_abort); > /* Declare the APIC range as the reserved MSI region */ > char *resv_prop_str = g_strdup_printf("0xfee00000:0xfeefffff:%d", > VIRTIO_IOMMU_RESV_MEM_T_MSI); > @@ -1464,6 +1466,10 @@ static void pc_machine_device_pre_plug_cb(HotplugHandler *hotplug_dev, > qlist_append_str(reserved_regions, resv_prop_str); > qdev_prop_set_array(dev, "reserved-regions", reserved_regions); > > + if (!aw_bits) { > + qdev_prop_set_uint8(dev, "aw-bits", VTD_HOST_AW_39BIT); > + } > + > g_free(resv_prop_str); > } > > diff --git a/hw/virtio/virtio-iommu.c b/hw/virtio/virtio-iommu.c > index 8b541de850..2ec5ef3cd1 100644 > --- a/hw/virtio/virtio-iommu.c > +++ b/hw/virtio/virtio-iommu.c > @@ -1526,7 +1526,7 @@ 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_UINT8("aw-bits", VirtIOIOMMU, aw_bits, 64), > + DEFINE_PROP_UINT8("aw-bits", VirtIOIOMMU, aw_bits, 0), > DEFINE_PROP_END_OF_LIST(), > }; > > 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);
Hi Cédric, On 2/27/24 18:17, Cédric Le Goater wrote: > Hello Eric, > > On 2/15/24 09:42, Eric Auger wrote: >> 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. On ARM we set 48b as a default (matching >> SMMUv3 SMMU_IDR5.VAX == 0). >> >> 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> >> Reviewed-by: Zhenzhong Duan <zhenzhong.duan@intel.com> >> Tested-by: Yanghang Liu<yanghliu@redhat.com> > > We need a property fixup for pseries also: > > $ build/ppc64-softmmu/qemu-system-ppc64 -M pseries -device > virtio-iommu-pci,addr=04.0 > qemu-system-ppc64: -device virtio-iommu-pci,addr=04.0: aw-bits must be > within [32,64] as stated in the doc (now! see [PATCH v5 4/4] qemu-options.hx: Add an entry for virtio-iommu-pci and document aw-bits) , the virtio-iommu-pci device only is supported in q35 and arm virt. If you want to support virtio-iommu on other machines, you need to take care of this aw-bits settings and also of the exposed reserved regions. Thanks Eric > > > Thanks, > > C. > > > >> --- >> >> 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 >> --- >> hw/arm/virt.c | 6 ++++++ >> hw/core/machine.c | 5 ++++- >> hw/i386/pc.c | 6 ++++++ >> hw/virtio/virtio-iommu.c | 2 +- >> tests/qtest/virtio-iommu-test.c | 2 +- >> 5 files changed, 18 insertions(+), 3 deletions(-) >> >> diff --git a/hw/arm/virt.c b/hw/arm/virt.c >> index 368c2a415a..0994f2a560 100644 >> --- a/hw/arm/virt.c >> +++ b/hw/arm/virt.c >> @@ -2716,10 +2716,16 @@ static void >> virt_machine_device_pre_plug_cb(HotplugHandler *hotplug_dev, >> } else if (object_dynamic_cast(OBJECT(dev), TYPE_VIRTIO_MD_PCI)) { >> virtio_md_pci_pre_plug(VIRTIO_MD_PCI(dev), >> MACHINE(hotplug_dev), errp); >> } else if (object_dynamic_cast(OBJECT(dev), >> TYPE_VIRTIO_IOMMU_PCI)) { >> + uint8_t aw_bits = object_property_get_uint(OBJECT(dev), >> + "aw-bits", >> &error_abort); >> hwaddr db_start = 0, db_end = 0; >> QList *reserved_regions; >> char *resv_prop_str; >> + if (!aw_bits) { >> + qdev_prop_set_uint8(dev, "aw-bits", 48); >> + } >> + >> if (vms->iommu != VIRT_IOMMU_NONE) { >> error_setg(errp, "virt machine does not support >> multiple IOMMUs"); >> return; >> diff --git a/hw/core/machine.c b/hw/core/machine.c >> index fb5afdcae4..70ac96954c 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, "aw-bits", "64" }, >> +}; >> const size_t hw_compat_8_2_len = G_N_ELEMENTS(hw_compat_8_2); >> GlobalProperty hw_compat_8_1[] = { >> diff --git a/hw/i386/pc.c b/hw/i386/pc.c >> index 196827531a..ee2d379c90 100644 >> --- a/hw/i386/pc.c >> +++ b/hw/i386/pc.c >> @@ -1456,6 +1456,8 @@ static void >> pc_machine_device_pre_plug_cb(HotplugHandler *hotplug_dev, >> } else if (object_dynamic_cast(OBJECT(dev), TYPE_VIRTIO_MD_PCI)) { >> virtio_md_pci_pre_plug(VIRTIO_MD_PCI(dev), >> MACHINE(hotplug_dev), errp); >> } else if (object_dynamic_cast(OBJECT(dev), >> TYPE_VIRTIO_IOMMU_PCI)) { >> + uint8_t aw_bits = object_property_get_uint(OBJECT(dev), >> + "aw-bits", >> &error_abort); >> /* Declare the APIC range as the reserved MSI region */ >> char *resv_prop_str = >> g_strdup_printf("0xfee00000:0xfeefffff:%d", >> >> VIRTIO_IOMMU_RESV_MEM_T_MSI); >> @@ -1464,6 +1466,10 @@ static void >> pc_machine_device_pre_plug_cb(HotplugHandler *hotplug_dev, >> qlist_append_str(reserved_regions, resv_prop_str); >> qdev_prop_set_array(dev, "reserved-regions", >> reserved_regions); >> + if (!aw_bits) { >> + qdev_prop_set_uint8(dev, "aw-bits", VTD_HOST_AW_39BIT); >> + } >> + >> g_free(resv_prop_str); >> } >> diff --git a/hw/virtio/virtio-iommu.c b/hw/virtio/virtio-iommu.c >> index 8b541de850..2ec5ef3cd1 100644 >> --- a/hw/virtio/virtio-iommu.c >> +++ b/hw/virtio/virtio-iommu.c >> @@ -1526,7 +1526,7 @@ 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_UINT8("aw-bits", VirtIOIOMMU, aw_bits, 64), >> + DEFINE_PROP_UINT8("aw-bits", VirtIOIOMMU, aw_bits, 0), >> DEFINE_PROP_END_OF_LIST(), >> }; >> 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); >
Hi, On 2/27/24 18:17, Cédric Le Goater wrote: > Hello Eric, > > On 2/15/24 09:42, Eric Auger wrote: >> 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. On ARM we set 48b as a default (matching >> SMMUv3 SMMU_IDR5.VAX == 0). >> >> 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> >> Reviewed-by: Zhenzhong Duan <zhenzhong.duan@intel.com> >> Tested-by: Yanghang Liu<yanghliu@redhat.com> > > We need a property fixup for pseries also: > > $ build/ppc64-softmmu/qemu-system-ppc64 -M pseries -device > virtio-iommu-pci,addr=04.0 > qemu-system-ppc64: -device virtio-iommu-pci,addr=04.0: aw-bits must be > within [32,64] after offline discssion with Cédric, despite the virtio-iommu-pci device is not supported on ppc64 the tests/qtest/qos-test attempts to execute the tests/qtest/virtio-iommu-test.c because the machine has a pci bus. Since by default the aw-bits is 0 the test fails. I need to rework that. Thanks! Eric > > > Thanks, > > C. > > > >> --- >> >> 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 >> --- >> hw/arm/virt.c | 6 ++++++ >> hw/core/machine.c | 5 ++++- >> hw/i386/pc.c | 6 ++++++ >> hw/virtio/virtio-iommu.c | 2 +- >> tests/qtest/virtio-iommu-test.c | 2 +- >> 5 files changed, 18 insertions(+), 3 deletions(-) >> >> diff --git a/hw/arm/virt.c b/hw/arm/virt.c >> index 368c2a415a..0994f2a560 100644 >> --- a/hw/arm/virt.c >> +++ b/hw/arm/virt.c >> @@ -2716,10 +2716,16 @@ static void >> virt_machine_device_pre_plug_cb(HotplugHandler *hotplug_dev, >> } else if (object_dynamic_cast(OBJECT(dev), TYPE_VIRTIO_MD_PCI)) { >> virtio_md_pci_pre_plug(VIRTIO_MD_PCI(dev), >> MACHINE(hotplug_dev), errp); >> } else if (object_dynamic_cast(OBJECT(dev), >> TYPE_VIRTIO_IOMMU_PCI)) { >> + uint8_t aw_bits = object_property_get_uint(OBJECT(dev), >> + "aw-bits", >> &error_abort); >> hwaddr db_start = 0, db_end = 0; >> QList *reserved_regions; >> char *resv_prop_str; >> + if (!aw_bits) { >> + qdev_prop_set_uint8(dev, "aw-bits", 48); >> + } >> + >> if (vms->iommu != VIRT_IOMMU_NONE) { >> error_setg(errp, "virt machine does not support >> multiple IOMMUs"); >> return; >> diff --git a/hw/core/machine.c b/hw/core/machine.c >> index fb5afdcae4..70ac96954c 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, "aw-bits", "64" }, >> +}; >> const size_t hw_compat_8_2_len = G_N_ELEMENTS(hw_compat_8_2); >> GlobalProperty hw_compat_8_1[] = { >> diff --git a/hw/i386/pc.c b/hw/i386/pc.c >> index 196827531a..ee2d379c90 100644 >> --- a/hw/i386/pc.c >> +++ b/hw/i386/pc.c >> @@ -1456,6 +1456,8 @@ static void >> pc_machine_device_pre_plug_cb(HotplugHandler *hotplug_dev, >> } else if (object_dynamic_cast(OBJECT(dev), TYPE_VIRTIO_MD_PCI)) { >> virtio_md_pci_pre_plug(VIRTIO_MD_PCI(dev), >> MACHINE(hotplug_dev), errp); >> } else if (object_dynamic_cast(OBJECT(dev), >> TYPE_VIRTIO_IOMMU_PCI)) { >> + uint8_t aw_bits = object_property_get_uint(OBJECT(dev), >> + "aw-bits", >> &error_abort); >> /* Declare the APIC range as the reserved MSI region */ >> char *resv_prop_str = >> g_strdup_printf("0xfee00000:0xfeefffff:%d", >> >> VIRTIO_IOMMU_RESV_MEM_T_MSI); >> @@ -1464,6 +1466,10 @@ static void >> pc_machine_device_pre_plug_cb(HotplugHandler *hotplug_dev, >> qlist_append_str(reserved_regions, resv_prop_str); >> qdev_prop_set_array(dev, "reserved-regions", >> reserved_regions); >> + if (!aw_bits) { >> + qdev_prop_set_uint8(dev, "aw-bits", VTD_HOST_AW_39BIT); >> + } >> + >> g_free(resv_prop_str); >> } >> diff --git a/hw/virtio/virtio-iommu.c b/hw/virtio/virtio-iommu.c >> index 8b541de850..2ec5ef3cd1 100644 >> --- a/hw/virtio/virtio-iommu.c >> +++ b/hw/virtio/virtio-iommu.c >> @@ -1526,7 +1526,7 @@ 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_UINT8("aw-bits", VirtIOIOMMU, aw_bits, 64), >> + DEFINE_PROP_UINT8("aw-bits", VirtIOIOMMU, aw_bits, 0), >> DEFINE_PROP_END_OF_LIST(), >> }; >> 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); >
On Thu, 15 Feb 2024 09:42:13 +0100 Eric Auger <eric.auger@redhat.com> wrote: > 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. On ARM we set 48b as a default (matching > SMMUv3 SMMU_IDR5.VAX == 0). > > 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. so we have different defaults per target/machine while open codding fixup in _pre_plug_ works it's a bit unexpected place to manage defaults and avoid adding 0 magic. How about using compat machinery instead to set machine dependent defaults: For example: pc_i440fx_machine_options(MachineClass *m) { ... + compat_props_add(m->compat_props, pc_compat_defaults,pc_compat_defaults_len); } > Of course if aw-bits is set from the command line, the default > is overriden. > > Signed-off-by: Eric Auger <eric.auger@redhat.com> > Reviewed-by: Zhenzhong Duan <zhenzhong.duan@intel.com> > Tested-by: Yanghang Liu<yanghliu@redhat.com> > > --- > > 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 > --- > hw/arm/virt.c | 6 ++++++ > hw/core/machine.c | 5 ++++- > hw/i386/pc.c | 6 ++++++ > hw/virtio/virtio-iommu.c | 2 +- > tests/qtest/virtio-iommu-test.c | 2 +- > 5 files changed, 18 insertions(+), 3 deletions(-) > > diff --git a/hw/arm/virt.c b/hw/arm/virt.c > index 368c2a415a..0994f2a560 100644 > --- a/hw/arm/virt.c > +++ b/hw/arm/virt.c > @@ -2716,10 +2716,16 @@ static void virt_machine_device_pre_plug_cb(HotplugHandler *hotplug_dev, > } else if (object_dynamic_cast(OBJECT(dev), TYPE_VIRTIO_MD_PCI)) { > virtio_md_pci_pre_plug(VIRTIO_MD_PCI(dev), MACHINE(hotplug_dev), errp); > } else if (object_dynamic_cast(OBJECT(dev), TYPE_VIRTIO_IOMMU_PCI)) { > + uint8_t aw_bits = object_property_get_uint(OBJECT(dev), > + "aw-bits", &error_abort); > hwaddr db_start = 0, db_end = 0; > QList *reserved_regions; > char *resv_prop_str; > > + if (!aw_bits) { > + qdev_prop_set_uint8(dev, "aw-bits", 48); s/48/macro name/? > + } > > > if (vms->iommu != VIRT_IOMMU_NONE) { > error_setg(errp, "virt machine does not support multiple IOMMUs"); > return; > diff --git a/hw/core/machine.c b/hw/core/machine.c > index fb5afdcae4..70ac96954c 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, "aw-bits", "64" }, > +}; > const size_t hw_compat_8_2_len = G_N_ELEMENTS(hw_compat_8_2); > > GlobalProperty hw_compat_8_1[] = { > diff --git a/hw/i386/pc.c b/hw/i386/pc.c > index 196827531a..ee2d379c90 100644 > --- a/hw/i386/pc.c > +++ b/hw/i386/pc.c > @@ -1456,6 +1456,8 @@ static void pc_machine_device_pre_plug_cb(HotplugHandler *hotplug_dev, > } else if (object_dynamic_cast(OBJECT(dev), TYPE_VIRTIO_MD_PCI)) { > virtio_md_pci_pre_plug(VIRTIO_MD_PCI(dev), MACHINE(hotplug_dev), errp); > } else if (object_dynamic_cast(OBJECT(dev), TYPE_VIRTIO_IOMMU_PCI)) { > + uint8_t aw_bits = object_property_get_uint(OBJECT(dev), > + "aw-bits", &error_abort); > /* Declare the APIC range as the reserved MSI region */ > char *resv_prop_str = g_strdup_printf("0xfee00000:0xfeefffff:%d", > VIRTIO_IOMMU_RESV_MEM_T_MSI); > @@ -1464,6 +1466,10 @@ static void pc_machine_device_pre_plug_cb(HotplugHandler *hotplug_dev, > qlist_append_str(reserved_regions, resv_prop_str); > qdev_prop_set_array(dev, "reserved-regions", reserved_regions); > > + if (!aw_bits) { > + qdev_prop_set_uint8(dev, "aw-bits", VTD_HOST_AW_39BIT); > + } > + > g_free(resv_prop_str); > } > > diff --git a/hw/virtio/virtio-iommu.c b/hw/virtio/virtio-iommu.c > index 8b541de850..2ec5ef3cd1 100644 > --- a/hw/virtio/virtio-iommu.c > +++ b/hw/virtio/virtio-iommu.c > @@ -1526,7 +1526,7 @@ 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_UINT8("aw-bits", VirtIOIOMMU, aw_bits, 64), > + DEFINE_PROP_UINT8("aw-bits", VirtIOIOMMU, aw_bits, 0), I'd set some valid value here (obviously not universal) and skip default property setting for that machine/target For example pick x86 one. > DEFINE_PROP_END_OF_LIST(), > }; > > 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);
Hi Igor, On 2/29/24 10:20, Igor Mammedov wrote: > On Thu, 15 Feb 2024 09:42:13 +0100 > Eric Auger <eric.auger@redhat.com> wrote: > >> 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. On ARM we set 48b as a default (matching >> SMMUv3 SMMU_IDR5.VAX == 0). >> >> 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. > so we have different defaults per target/machine > while open codding fixup in _pre_plug_ works it's > a bit unexpected place to manage defaults and > avoid adding 0 magic. > > How about using compat machinery instead to set > machine dependent defaults: > For example: > > pc_i440fx_machine_options(MachineClass *m) > { > ... > + compat_props_add(m->compat_props, pc_compat_defaults,pc_compat_defaults_len); > } Yes I can go this way. > >> Of course if aw-bits is set from the command line, the default >> is overriden. >> >> Signed-off-by: Eric Auger <eric.auger@redhat.com> >> Reviewed-by: Zhenzhong Duan <zhenzhong.duan@intel.com> >> Tested-by: Yanghang Liu<yanghliu@redhat.com> >> >> --- >> >> 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 >> --- >> hw/arm/virt.c | 6 ++++++ >> hw/core/machine.c | 5 ++++- >> hw/i386/pc.c | 6 ++++++ >> hw/virtio/virtio-iommu.c | 2 +- >> tests/qtest/virtio-iommu-test.c | 2 +- >> 5 files changed, 18 insertions(+), 3 deletions(-) >> >> diff --git a/hw/arm/virt.c b/hw/arm/virt.c >> index 368c2a415a..0994f2a560 100644 >> --- a/hw/arm/virt.c >> +++ b/hw/arm/virt.c >> @@ -2716,10 +2716,16 @@ static void virt_machine_device_pre_plug_cb(HotplugHandler *hotplug_dev, >> } else if (object_dynamic_cast(OBJECT(dev), TYPE_VIRTIO_MD_PCI)) { >> virtio_md_pci_pre_plug(VIRTIO_MD_PCI(dev), MACHINE(hotplug_dev), errp); >> } else if (object_dynamic_cast(OBJECT(dev), TYPE_VIRTIO_IOMMU_PCI)) { >> + uint8_t aw_bits = object_property_get_uint(OBJECT(dev), >> + "aw-bits", &error_abort); >> hwaddr db_start = 0, db_end = 0; >> QList *reserved_regions; >> char *resv_prop_str; >> >> + if (!aw_bits) { >> + qdev_prop_set_uint8(dev, "aw-bits", 48); > s/48/macro name/? with compats it becomes a string. So I cannot directly reuse X86_64 VTD or ARM SMMU macros. I would introduce a dummy VIRTIO_IOMMU string which does not sound really helpful. > >> + } >> >> >> if (vms->iommu != VIRT_IOMMU_NONE) { >> error_setg(errp, "virt machine does not support multiple IOMMUs"); >> return; >> diff --git a/hw/core/machine.c b/hw/core/machine.c >> index fb5afdcae4..70ac96954c 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, "aw-bits", "64" }, >> +}; >> const size_t hw_compat_8_2_len = G_N_ELEMENTS(hw_compat_8_2); >> >> GlobalProperty hw_compat_8_1[] = { >> diff --git a/hw/i386/pc.c b/hw/i386/pc.c >> index 196827531a..ee2d379c90 100644 >> --- a/hw/i386/pc.c >> +++ b/hw/i386/pc.c >> @@ -1456,6 +1456,8 @@ static void pc_machine_device_pre_plug_cb(HotplugHandler *hotplug_dev, >> } else if (object_dynamic_cast(OBJECT(dev), TYPE_VIRTIO_MD_PCI)) { >> virtio_md_pci_pre_plug(VIRTIO_MD_PCI(dev), MACHINE(hotplug_dev), errp); >> } else if (object_dynamic_cast(OBJECT(dev), TYPE_VIRTIO_IOMMU_PCI)) { >> + uint8_t aw_bits = object_property_get_uint(OBJECT(dev), >> + "aw-bits", &error_abort); >> /* Declare the APIC range as the reserved MSI region */ >> char *resv_prop_str = g_strdup_printf("0xfee00000:0xfeefffff:%d", >> VIRTIO_IOMMU_RESV_MEM_T_MSI); >> @@ -1464,6 +1466,10 @@ static void pc_machine_device_pre_plug_cb(HotplugHandler *hotplug_dev, >> qlist_append_str(reserved_regions, resv_prop_str); >> qdev_prop_set_array(dev, "reserved-regions", reserved_regions); >> >> + if (!aw_bits) { >> + qdev_prop_set_uint8(dev, "aw-bits", VTD_HOST_AW_39BIT); >> + } >> + >> g_free(resv_prop_str); >> } >> >> diff --git a/hw/virtio/virtio-iommu.c b/hw/virtio/virtio-iommu.c >> index 8b541de850..2ec5ef3cd1 100644 >> --- a/hw/virtio/virtio-iommu.c >> +++ b/hw/virtio/virtio-iommu.c >> @@ -1526,7 +1526,7 @@ 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_UINT8("aw-bits", VirtIOIOMMU, aw_bits, 64), >> + DEFINE_PROP_UINT8("aw-bits", VirtIOIOMMU, aw_bits, 0), > I'd set some valid value here (obviously not universal) and skip > default property setting for that machine/target > For example pick x86 one. Yes the purpose is to have a non null aw_bits value which makes the virtio-iommu unusable without proper machine integration and puts the mess on the libqos tests. > >> DEFINE_PROP_END_OF_LIST(), >> }; >> >> 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); Thank you for the suggestions Eric
diff --git a/hw/arm/virt.c b/hw/arm/virt.c index 368c2a415a..0994f2a560 100644 --- a/hw/arm/virt.c +++ b/hw/arm/virt.c @@ -2716,10 +2716,16 @@ static void virt_machine_device_pre_plug_cb(HotplugHandler *hotplug_dev, } else if (object_dynamic_cast(OBJECT(dev), TYPE_VIRTIO_MD_PCI)) { virtio_md_pci_pre_plug(VIRTIO_MD_PCI(dev), MACHINE(hotplug_dev), errp); } else if (object_dynamic_cast(OBJECT(dev), TYPE_VIRTIO_IOMMU_PCI)) { + uint8_t aw_bits = object_property_get_uint(OBJECT(dev), + "aw-bits", &error_abort); hwaddr db_start = 0, db_end = 0; QList *reserved_regions; char *resv_prop_str; + if (!aw_bits) { + qdev_prop_set_uint8(dev, "aw-bits", 48); + } + if (vms->iommu != VIRT_IOMMU_NONE) { error_setg(errp, "virt machine does not support multiple IOMMUs"); return; diff --git a/hw/core/machine.c b/hw/core/machine.c index fb5afdcae4..70ac96954c 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, "aw-bits", "64" }, +}; const size_t hw_compat_8_2_len = G_N_ELEMENTS(hw_compat_8_2); GlobalProperty hw_compat_8_1[] = { diff --git a/hw/i386/pc.c b/hw/i386/pc.c index 196827531a..ee2d379c90 100644 --- a/hw/i386/pc.c +++ b/hw/i386/pc.c @@ -1456,6 +1456,8 @@ static void pc_machine_device_pre_plug_cb(HotplugHandler *hotplug_dev, } else if (object_dynamic_cast(OBJECT(dev), TYPE_VIRTIO_MD_PCI)) { virtio_md_pci_pre_plug(VIRTIO_MD_PCI(dev), MACHINE(hotplug_dev), errp); } else if (object_dynamic_cast(OBJECT(dev), TYPE_VIRTIO_IOMMU_PCI)) { + uint8_t aw_bits = object_property_get_uint(OBJECT(dev), + "aw-bits", &error_abort); /* Declare the APIC range as the reserved MSI region */ char *resv_prop_str = g_strdup_printf("0xfee00000:0xfeefffff:%d", VIRTIO_IOMMU_RESV_MEM_T_MSI); @@ -1464,6 +1466,10 @@ static void pc_machine_device_pre_plug_cb(HotplugHandler *hotplug_dev, qlist_append_str(reserved_regions, resv_prop_str); qdev_prop_set_array(dev, "reserved-regions", reserved_regions); + if (!aw_bits) { + qdev_prop_set_uint8(dev, "aw-bits", VTD_HOST_AW_39BIT); + } + g_free(resv_prop_str); } diff --git a/hw/virtio/virtio-iommu.c b/hw/virtio/virtio-iommu.c index 8b541de850..2ec5ef3cd1 100644 --- a/hw/virtio/virtio-iommu.c +++ b/hw/virtio/virtio-iommu.c @@ -1526,7 +1526,7 @@ 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_UINT8("aw-bits", VirtIOIOMMU, aw_bits, 64), + DEFINE_PROP_UINT8("aw-bits", VirtIOIOMMU, aw_bits, 0), DEFINE_PROP_END_OF_LIST(), }; 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);