Re: [PATCH v3 1/1] virtio: fix the condition for iommu_platform not supported

[Daniel Henrique Barboza <danielhb413@gmail.com>]

On 2/2/22 13:23, Halil Pasic wrote:
> On Wed, 2 Feb 2022 10:24:51 -0300
> Daniel Henrique Barboza <danielhb413@gmail.com> wrote:
> 
>> On 2/1/22 22:15, Halil Pasic wrote:
>>> On Tue, 1 Feb 2022 16:31:22 -0300
>>> Daniel Henrique Barboza <danielhb413@gmail.com> wrote:
>>>    
>>>> On 2/1/22 15:33, Halil Pasic wrote:
>>>>> On Tue, 1 Feb 2022 12:36:25 -0300
>>>>> Daniel Henrique Barboza <danielhb@linux.ibm.com> wrote:
>>>>>       
>>>>>>> +    vdev_has_iommu = virtio_host_has_feature(vdev, VIRTIO_F_IOMMU_PLATFORM);
>>>>>>>          if (klass->get_dma_as != NULL && has_iommu) {
>>>>>>>              virtio_add_feature(&vdev->host_features, VIRTIO_F_IOMMU_PLATFORM);
>>>>>>>              vdev->dma_as = klass->get_dma_as(qbus->parent);
>>>>>>> +        if (!vdev_has_iommu && vdev->dma_as != &address_space_memory) {
>>>>>>> +            error_setg(errp,
>>>>>>> +                       "iommu_platform=true is not supported by the device");
>>>>>>> +        }
>>>>>>
>>>>>>      
>>>>>>>          } else {
>>>>>>>              vdev->dma_as = &address_space_memory;
>>>>>>>          }
>>>>>>
>>>>>>
>>>>>> I struggled to understand what this 'else' clause was doing and I assumed that it was
>>>>>> wrong. Searching through the ML I learned that this 'else' clause is intended to handle
>>>>>> legacy virtio devices that doesn't support the DMA API (introduced in 8607f5c3072caeebb)
>>>>>> and thus shouldn't set  VIRTIO_F_IOMMU_PLATFORM.
>>>>>>
>>>>>>
>>>>>> My suggestion, if a v4 is required for any other reason, is to add a small comment in this
>>>>>> 'else' clause explaining that this is the legacy virtio devices condition and those devices
>>>>>> don't set F_IOMMU_PLATFORM. This would make the code easier to read for a virtio casual like
>>>>>> myself.
>>>>>
>>>>> I do not agree that this is about legacy virtio. In my understanding
>>>>> virtio-ccw simply does not need translation because CCW devices use
>>>>> guest physical addresses as per architecture. It may be considered
>>>>> legacy stuff form PCI perspective, but I don't think it is legacy
>>>>> in general.
>>>>
>>>>
>>>> I wasn't talking about virtio-ccw. I was talking about this piece of code:
>>>>
>>>>
>>>>        if (klass->get_dma_as != NULL && has_iommu) {
>>>>            virtio_add_feature(&vdev->host_features, VIRTIO_F_IOMMU_PLATFORM);
>>>>            vdev->dma_as = klass->get_dma_as(qbus->parent);
>>>>        } else {
>>>>            vdev->dma_as = &address_space_memory;
>>>>        }
>>>>
>>>>
>>>> I suggested something like this:
>>>>
>>>>
>>>>
>>>>        if (klass->get_dma_as != NULL && has_iommu) {
>>>>            virtio_add_feature(&vdev->host_features, VIRTIO_F_IOMMU_PLATFORM);
>>>>            vdev->dma_as = klass->get_dma_as(qbus->parent);
>>>>        } else {
>>>>            /*
>>>>             * We don't force VIRTIO_F_IOMMU_PLATFORM for legacy devices, i.e.
>>>>             * devices that don't implement klass->get_dma_as, regardless of
>>>>             * 'has_iommu' setting.
>>>>             */
>>>>            vdev->dma_as = &address_space_memory;
>>>>        }
>>>>
>>>>
>>>> At least from my reading of commits 8607f5c3072 and 2943b53f682 this seems to be
>>>> the case. I spent some time thinking that this IF/ELSE was wrong because I wasn't
>>>> aware of this history.
>>>
>>> With virtio-ccw we take the else branch because we don't implement
>>> ->get_dma_as(). I don't consider all the virtio-ccw to be legacy.
>>>
>>> IMHO there are two ways to think about this:
>>> a) The commit that introduced this needs a fix which implemets
>>> get_dma_as() for virtio-ccw in a way that it simply returns
>>> address_space_memory.
>>> b) The presence of ->get_dma_as() is not indicative of "legacy".
>>>
>>> BTW in virtospeak "legacy" has a special meaning: pre-1.0 virtio. Do you
>>> mean that legacy. And if I read the virtio-pci code correctly
>>> ->get_dma_as is set for legacy, transitional and modern devices alike.
>>
>>
>> Oh ok. I'm not well versed into virtiospeak. My "legacy" comment was a poor choice of
>> word for the situation.
>>
>> We can ignore the "legacy" bit. My idea/suggestion is to put a comment at that point
>> explaining the logic behind into not forcing VIRTIO_F_IOMMU_PLATFORM in devices that
>> doesn't implement ->get_dma_as().
>>
>> I am assuming that this is an intended design that was introduced by 2943b53f682
>> ("virtio: force VIRTIO_F_IOMMU_PLATFORM"), meaning that the implementation of the
>> ->get_dma_as is being used as a parameter to force the feature in the device. And with
>> this code:
>>
>>
>>       if (klass->get_dma_as != NULL && has_iommu) {
>>           virtio_add_feature(&vdev->host_features, VIRTIO_F_IOMMU_PLATFORM);
>>           vdev->dma_as = klass->get_dma_as(qbus->parent);
>>       } else {
>>           vdev->dma_as = &address_space_memory;
>>       }
>>
>> It is possible that we have 2 vdev devices where ->dma_as = &address_space_memory, but one
>> of them is sitting in a bus where "klass->get_dma_as(qbus->parent) = &address_space_memory",
>> and this device will have VIRTIO_F_IOMMU_PLATFORM forced onto it and the former won't.
>>
>>
>> If this is not an intended design I can only speculate how to fix it. Forcing VIRTIO_F_IOMMU_PLATFORM
>> in all devices, based only on has_iommu, can break stuff. Setting VIRTIO_F_IOMMU_PLATFORM only
>> if "vdev->dma_as != &address_space_memory" make some sense but I am fairly certain it will
>> break stuff the other way. Or perhaps the fix is something else entirely.
>>
>>
>>
>>
>>>
>>> IMHO the important thing to figure out is what impact that
>>> virtio_add_feature(&vdev->host_features, VIRTIO_F_IOMMU_PLATFORM);
>>> in the first branch (of the if-else) has. IMHO if one examines the
>>> commits 8607f5c307 ("virtio: convert to use DMA api") and 2943b53f68
>>> ("virtio: force VIRTIO_F_IOMMU_PLATFORM") very carefully, one will
>>> probably reach the conclusion that the objective of the latter, is
>>> to prevent the guest form not negotiating the IOMMU_PLATFORM feature
>>> (clearing it as part of the feature negotiation) and trying to use
>>> the device without that feature. In other words, virtio features are
>>> usually optional for the guest for the sake of compatibility, but
>>> IOMMU_PLATFORM is not: for very good reasons. Neither the commit message
>>> nor the patch does mention legacy anywhere.
>>>
>>> In my opinion not forcing the guest to negotiate IOMMU_PLATFORM when
>>> ->get_dma_as() is not set is at least unfortunate. Please observe, that
>>> virtio-pci is not affected by this omission because for virtio-pci
>>> devices ->get_dma_as != NULL always holds. And what is the deal for
>>> devices that don't implement get_dma_as() (and don't need address
>>> translation)? If iommu_platform=on is justified (no user error) then
>>> the device does not have access to the entire guest memory. Which
>>> means it more than likely needs cooperation form the guest (driver).
>>> So detecting that the guest does not support IOMMU_PLATFORM and failing
>>> gracefully via virtio_validate_features() instead of carrying on
>>> in good faith and failing in ugly ways when the host attempts to access
>>> guest memory to which it does not have access to. If we assume user
>>> error, that is the host can access at least all the memory it needs
>>> to access to make that device work, then it is probably still a
>>> good idea to fail the device and thus help the user correct his
>>> error.
>>
>> Yeah, this go back on what I've said about 2943b53f682 up there. There are assumptions
>> being made on the ->get_dma_as() existence that aren't clear.
>>
> 
> I agree. The commit message does not explain.
> 
>>
>>>
>>> IMHO the best course of action is
>>> diff --git a/hw/virtio/virtio-bus.c b/hw/virtio/virtio-bus.c
>>> index 34f5a0a664..1d0eb16d1c 100644
>>> --- a/hw/virtio/virtio-bus.c
>>> +++ b/hw/virtio/virtio-bus.c
>>> @@ -80,7 +80,6 @@ void virtio_bus_device_plugged(VirtIODevice *vdev, Error **errp)
>>>    
>>>        vdev_has_iommu = virtio_host_has_feature(vdev, VIRTIO_F_IOMMU_PLATFORM);
>>>        if (klass->get_dma_as != NULL && has_iommu) {
>>> -        virtio_add_feature(&vdev->host_features, VIRTIO_F_IOMMU_PLATFORM);
>>>            vdev->dma_as = klass->get_dma_as(qbus->parent);
>>>            if (!vdev_has_iommu && vdev->dma_as != &address_space_memory) {
>>>                error_setg(errp,
>>> @@ -89,6 +88,7 @@ void virtio_bus_device_plugged(VirtIODevice *vdev, Error **errp)
>>>        } else {
>>>            vdev->dma_as = &address_space_memory;
>>>        }
>>> +    virtio_add_feature(&vdev->host_features, VIRTIO_F_IOMMU_PLATFORM);
>>>    }
>>
>>
>> I am fairly confident that forcing VIRTIO_F_IOMMU_PLATFORM all around, based on has_iommu
> 
> Yes I should have made that conditional on has_iommu. It was very late
> when I finished that email.
> 
>> alone, will have consequences all around. This code has been around for almost 5 years and a
>> lot of stuff has been developed on top of it.
>>
> 
> Do you have any particular examples in mind?
> 
>> All that said, if this is the proper way of fixing it I'd say to do it now, document it properly
>> and fix the breakages as they come along. The alternative - hacking around and around a codebase
>> that might not be solid - is worse in the long run.
> 
> IMHO this is a good discussion you triggered. But I see it out of scope
> for the bug I'm trying to fix.

Agree. I'll re-ack the patch given that I did it from another email that isn't
in QEMU devel. All this discussion is pertinent to a separated work.


Thanks,


Daniel


> 
> I can post a proper patch for "IOMMU_PLATFORM is non-negotiable for
> all guests" and we can have proper review and discussion on that. But
> I would like the bug I'm working on here fixed first. There are
> people that want to use virtiofs with confidential guests, and
> we should really make sure they can.
> 
> Regards,
> Halil