1. Patchew
  2. QEMU
  3. [PATCH v6 0/9] VIRTIO-IOMMU: Introduce aw-bits and granule options
  4. View patch

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

Eric Auger posted 9 patches 8 months, 3 weeks ago
Maintainers: Peter Maydell <peter.maydell@linaro.org>, Eduardo Habkost <eduardo@habkost.net>, Marcel Apfelbaum <marcel.apfelbaum@gmail.com>, "Philippe Mathieu-Daudé" <philmd@linaro.org>, Yanan Wang <wangyanan55@huawei.com>, Paolo Bonzini <pbonzini@redhat.com>, "Daniel P. Berrangé" <berrange@redhat.com>, "Michael S. Tsirkin" <mst@redhat.com>, Richard Henderson <richard.henderson@linaro.org>, Eric Auger <eric.auger@redhat.com>, Eric Blake <eblake@redhat.com>, Markus Armbruster <armbru@redhat.com>, Thomas Huth <thuth@redhat.com>, Laurent Vivier <lvivier@redhat.com>
There is a newer version of this series
[PATCH v6 7/9] hw/i386/q35: Set virtio-iommu aw-bits default value to 39
Posted by Eric Auger 8 months, 3 weeks ago 
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
Posted by Duan, Zhenzhong 8 months, 3 weeks ago 
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
Posted by Eric Auger 8 months, 3 weeks ago 
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

Patchew 2.1-devel-b67e4c2

© 2016 - 2024 Red Hat, Inc.