RE: [PATCH v2 3/3] hw: Set virtio-iommu aw-bits default value on pc_q35_9.0 and arm virt

[Duan, Zhenzhong]

>-----Original Message-----
>From: Eric Auger <eric.auger@redhat.com>
>Subject: Re: [PATCH v2 3/3] hw: Set virtio-iommu aw-bits default value on
>pc_q35_9.0 and arm virt
>
>Hi Zhenzhong,
>
>On 2/2/24 07:51, Duan, Zhenzhong wrote:
>> Hi Eric,
>>
>>> -----Original Message-----
>>> From: Eric Auger <eric.auger@redhat.com>
>>> pc_q35_9.0 and arm virt
>>>
>>> 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>
>>>
>>> ---
>>>
>>> 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 +-
>>> 4 files changed, 17 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
>>> index e6ead2c5c8..56539f2fc5 100644
>>> --- a/hw/arm/virt.c
>>> +++ b/hw/arm/virt.c
>>> @@ -2718,10 +2718,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", NULL);
>>>         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 803244e5cc..0e2bcb4840 100644
>>> --- a/hw/i386/pc.c
>>> +++ b/hw/i386/pc.c
>>> @@ -1458,6 +1458,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", NULL);
>>>         /* 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);
>>> @@ -1466,6 +1468,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", 39);
>>> +        }
>>> +
>>>         g_free(resv_prop_str);
>>>     }
>>>
>>> diff --git a/hw/virtio/virtio-iommu.c b/hw/virtio/virtio-iommu.c
>>> index 7870bdbeee..c468e9b13b 100644
>>> --- a/hw/virtio/virtio-iommu.c
>>> +++ b/hw/virtio/virtio-iommu.c
>>> @@ -1529,7 +1529,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),
>> Not clear if virtio-iommu support other archs besides x86 and arm.
>> It looks on those archs, aw_bits is default 0 on machine 9.0 above
>> and will fails the check in realize?
>
>At the moment the virtio-iommu only is supported along with q35 and
>arm-virt.
>Only those machines set the reserved-regions prop and the aw-bits, which
>are both requested for a correct behavior.

Clear, thanks Eric.

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

BRs.
Zhenzhong

>
>Thank you for the review!
>
>Eric
>
>>
>> Thanks
>> Zhenzhong
>>
>>>     DEFINE_PROP_END_OF_LIST(),
>>> };
>>>
>>> --
>>> 2.41.0