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>
---
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);
--
2.41.0
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
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,
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);
>
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);
>
© 2016 - 2024 Red Hat, Inc.