Re: [PULL 08/34] hw/arm/virt: Let the virtio-iommu bypass MSIs

["Philippe Mathieu-Daudé" <philmd@linaro.org>]

On 2/2/23 11:47, Philippe Mathieu-Daudé wrote:
> Hi Eric,
> 
> On 3/7/20 17:53, Peter Maydell wrote:
>> From: Eric Auger <eric.auger@redhat.com>
>>
>> At the moment the virtio-iommu translates MSI transactions.
>> This behavior is inherited from ARM SMMU. The virt machine
>> code knows where the guest MSI doorbells are so we can easily
>> declare those regions as VIRTIO_IOMMU_RESV_MEM_T_MSI. With that
>> setting the guest will not map MSIs through the IOMMU and those
>> transactions will be simply bypassed.
>>
>> Depending on which MSI controller is in use (ITS or GICV2M),
>> we declare either:
>> - the ITS interrupt translation space (ITS_base + 0x10000),
>>    containing the GITS_TRANSLATOR or
>> - The GICV2M single frame, containing the MSI_SETSP_NS register.
>>
>> Signed-off-by: Eric Auger <eric.auger@redhat.com>
>> Message-id: 20200629070404.10969-6-eric.auger@redhat.com
>> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
>> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
>> ---
>>   include/hw/arm/virt.h |  7 +++++++
>>   hw/arm/virt.c         | 30 ++++++++++++++++++++++++++++++
>>   2 files changed, 37 insertions(+)
> 
> 
>>   static void create_gic(VirtMachineState *vms)
>> @@ -2198,8 +2200,36 @@ out:
>>   static void virt_machine_device_pre_plug_cb(HotplugHandler 
>> *hotplug_dev,
>>                                               DeviceState *dev, Error 
>> **errp)
>>   {
>> +    VirtMachineState *vms = VIRT_MACHINE(hotplug_dev);
>> +
>>       if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
>>           virt_memory_pre_plug(hotplug_dev, dev, errp);
>> +    } else if (object_dynamic_cast(OBJECT(dev), 
>> TYPE_VIRTIO_IOMMU_PCI)) {
>> +        hwaddr db_start = 0, db_end = 0;
>> +        char *resv_prop_str;
>> +
>> +        switch (vms->msi_controller) {
>> +        case VIRT_MSI_CTRL_NONE:
>> +            return;
>> +        case VIRT_MSI_CTRL_ITS:
>> +            /* GITS_TRANSLATER page */
>> +            db_start = base_memmap[VIRT_GIC_ITS].base + 0x10000;
>> +            db_end = base_memmap[VIRT_GIC_ITS].base +
>> +                     base_memmap[VIRT_GIC_ITS].size - 1;
>> +            break;
>> +        case VIRT_MSI_CTRL_GICV2M:
>> +            /* MSI_SETSPI_NS page */
>> +            db_start = base_memmap[VIRT_GIC_V2M].base;
>> +            db_end = db_start + base_memmap[VIRT_GIC_V2M].size - 1;
>> +            break;
>> +        }
>> +        resv_prop_str = g_strdup_printf("0x%"PRIx64":0x%"PRIx64":%u",
>> +                                        db_start, db_end,
>> +                                        VIRTIO_IOMMU_RESV_MEM_T_MSI);
>> +
>> +        qdev_prop_set_uint32(dev, "len-reserved-regions", 1);
> 
> Where is "len-reserved-regions" declared?
> 
> Since qdev_prop_set_uint32() uses &error_abort, isn't this call
> aborting the process? I am confused how this code path is exercised,
> what am I missing?

The call path is:

   qdev_prop_set_uint32 ->
     object_property_set_int ->
       object_property_set_qobject ->
         object_property_set ->
           object_property_find_err

So QEMU should abort displaying:

"Property 'virtio-iommu-pci.len-reserved-regions' not found".

>> +        qdev_prop_set_string(dev, "reserved-regions[0]", resv_prop_str);
>> +        g_free(resv_prop_str);
>>       }
>>   }