Re: [RFC PATCH 1/1] virtio: fix feature negotiation for ACCESS_PLATFORM

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

On 2/7/22 11:46, Halil Pasic wrote:
> On Mon, 7 Feb 2022 08:46:34 -0300
> Daniel Henrique Barboza <danielhb413@gmail.com> wrote:
> 
>> On 2/3/22 13:45, Halil Pasic wrote:
>>> Unlike most virtio features ACCESS_PATFORM is considered mandatory, i.e.
>>> the driver must accept it if offered by the device. The virtio
>>> specification says that the driver SHOULD accept the ACCESS_PLATFORM
>>> feature if offered, and that the device MAY fail to operate if
>>> ACCESS_PLATFORM was offered but not negotiated.
>>>
>>> While a SHOULD ain't exactly a MUST, we are certainly allowed to fail
>>> the device when the driver fences ACCESS_PLATFORM. With commit
>>
>>
>> I believe a link to the virtio specification where this is being mentioned would
>> be good to have in the commit message.
> 
> I can add that if Michael agrees, and if the patch is deemed worthy.
>>
>>
>>> 2943b53f68 ("virtio: force VIRTIO_F_IOMMU_PLATFORM") we already made the
>>> decision to do so whenever the get_dma_as() callback is implemented (by
>>> the bus), which in practice means for the entirety of virtio-pci.
>>>
>>> That means, if the device needs to translate I/O addresses, then
>>> ACCESS_PLATFORM is mandatory. The aforementioned commit tells us
>>> in the commit message that this is for security reasons.
>>>
>>> If ACCESS_PLATFORM is offered not we want the device to utilize an
>>
>> I think you meant "If ACCESS_PLATFORM is offered".
> 
> I'm missing because. I.e. s/not/not becasue/
>>
>>
>>> IOMMU and do address translation, but because the device does not have
>>> access to the entire guest RAM, and needs the driver to grant access
>>> to the bits it needs access to (e.g. confidential guest support), we
>>> still require the guest to have the corresponding logic and to accept
>>> ACCESS_PLATFORM. If the driver does not accept ACCESS_PLATFORM, then
>>> things are bound to go wrong, and we may see failures much less graceful
>>> than failing the device because the driver didn't negotiate
>>> ACCESS_PLATFORM.
>>>
>>> So let us make ACCESS_PLATFORM mandatory for the driver regardless
>>> of whether the get_dma_as() callback is implemented or not.
>>>
>>> Signed-off-by: Halil Pasic <pasic@linux.ibm.com>
>>> Fixes: 2943b53f68 ("virtio: force VIRTIO_F_IOMMU_PLATFORM")
>>>
>>> ---
>>> This patch is based on:
>>> https://www.mail-archive.com/qemu-devel@nongnu.org/msg866199.html
>>>
>>> During the review of "virtio: fix the condition for iommu_platform not
>>> supported" Daniel raised the question why do we "force IOMMU_PLATFORM"
>>> iff has_iommu && !!klass->get_dma_as. My answer to that was, that
>>> this logic ain't right.
>>>
>>> While at it I used the opportunity to re-organize the code a little
>>> and provide an explanatory comment.
>>> ---
>>>    hw/virtio/virtio-bus.c | 17 ++++++++++-------
>>>    1 file changed, 10 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/hw/virtio/virtio-bus.c b/hw/virtio/virtio-bus.c
>>> index fbf0dd14b8..359430eb1c 100644
>>> --- a/hw/virtio/virtio-bus.c
>>> +++ b/hw/virtio/virtio-bus.c
>>> @@ -78,16 +78,19 @@ void virtio_bus_device_plugged(VirtIODevice *vdev, Error **errp)
>>>            return;
>>>        }
>>>    
>>> -    vdev_has_iommu = virtio_host_has_feature(vdev, VIRTIO_F_IOMMU_PLATFORM);
>>> -    if (klass->get_dma_as != NULL && has_iommu) {
>>> +    vdev->dma_as = &address_space_memory;
>>
>> At this point you can also do:
>>
>>      if (!has_iommu) {
>>          return;
>>      }
>>
>> and the rest of the code will have one less indentation level.
> 
> I have considered this and decided against it. The reason why is
> if that approach is taken, we can't really add more code to the
> end of the function. An early return is good if we want to
> abort the function with an error. My point is !has_iommu does
> not necessarily mean we are done: after a block that handles
> the has_iommu situation, in future, there could be a block that
> handles something different.

And that's fine, but the way this patch is changing it I'm not sure it's better
than what we already have. Today we have:

if (has_iommu) {
   (... assign vdev->dma_as in some cases ...)
} else {
    vdev->dma_as = &address_space_memory;
}


Your patch is doing:

vdev->dma_as = &address_space_memory;

if (has_iommu) {
   (... assign vdev->dma_as in some cases ...)
}


You got rid of an 'else', but ended up adding a double "vdev->dma_as =" assignment
depending on the case (has_iommu = true and klass->get_dma_as != NULL). This is why
I proposed the early exit.

If we're worried about adding more code in the future might as well leave the existing
if/else as is.
        


> 
> Would this patch work for power? Or are there valid scenarios that
> it breaks? I'm asking, because you voiced concern regarding this before.


I'll test it when I have an opportunity and let you know.


Thanks,


Daniel

> 
> Thanks for your feedback!
> 
> Halil