Re: [PATCH 5/5] hw/arm/virt: Let the virtio-iommu bypass MSIs

[Auger Eric]

Hi Jean-Philippe,

On 5/8/20 10:21 AM, Jean-Philippe Brucker wrote:
> On Thu, May 07, 2020 at 04:32:01PM +0200, Eric Auger wrote:
>> 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. The ITS does MSI translation
>> anyway.
> 
> Makes sense. I think we have to check that the ITS is used before
> declaring the resv region, though. 

When using gicv2m, the guest will see
> this MSI region and deduce that it doesn't need to create a SW mapping for
> MSIs (might be wrong, I haven't tested it yet).

Yes you're right. I think any MSI region hides the SW MSI one. So I will
test the GIC type beforehand.

Also I will extend the series to handle the GICv2M backdoor

To me the problem is similar and the iommu subsystem will map the GICV2M
MSI doorbell as well. From a security pov, there is no difference
inbetween the 2 solutions. Anyway the doorbell is reachable by any
assigned device, would it be mapped or not. And given the GICv2M does
not perform any interrupt translation, an assigned device can trigger
MSIs on another userspace driver.

Thanks

Eric
> 
>> Signed-off-by: Eric Auger <address@hidden>
>> ---
>>  hw/arm/virt.c | 5 +++++
>>  1 file changed, 5 insertions(+)
>>
>> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
>> index 171e6908ec..6a041e97a5 100644
>> --- a/hw/arm/virt.c
>> +++ b/hw/arm/virt.c
>> @@ -2138,6 +2138,11 @@ static void 
>> virt_machine_device_pre_plug_cb(HotplugHandler *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)) {
>> +        /* we declare a VIRTIO_IOMMU_RESV_MEM_T_MSI region */
>> +        qdev_prop_set_uint32(dev, "len-reserved-regions", 1);
>> +        qdev_prop_set_string(dev, "reserved-regions[0]",
>> +                             "0x8090000, 0x809FFFF, 1");
> 
> Maybe worth adding a comment saying this is the GITS_TRANSLATER page?
> 
> Thanks,
> Jean
>