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

[Cornelia Huck]

On Mon, Feb 07 2022, 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.

s/ACCESS_PATFORM/ACCESS_PLATFORM/

>> 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.

It's in section 6.1 "Driver Requirements: Reserved Feature Bits": "A
driver SHOULD accept VIRTIO_F_ACCESS_PLATFORM if it is offered" and
section 6.2 "Device Requirements: Reserved Feature Bits": "A device MAY
fail to operate further if VIRTIO_F_ACCESS_PLATFORM is not accepted."

That said, I'm not sure the wording in the spec translates to
"mandatory"... if the driver fails to accept the bit, the device can
choose to not work with the driver, but it's not forced to. There are
other instances where the device may reject FEATURES_OK (e.g. when the
driver does not accept a feature that is a pre-req for another feature),
I'd say it is up to the device whether something is mandatory or not. If
the device/setup cannot work without it, it certainly is mandatory, but
the driver only knows when FEATURES_OK is rejected without the feature.

OTOH, the decision to make it mandatory is certainly sound, and covered
by the spec. As the driver must be prepared for the device failing to
accept FEATURES_OK, we can make it mandatory here -- we should just not
say that it is considered mandatory from a spec standpoint. The spec
allows to make it mandatory, and we make it mandatory in our
implementation.

>
>
>> 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 thought it should be "If ACCESS_PLATFORM is offered not because..." ?

>
>
>> 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.

It might make it harder to add code at the tail end of the function in
the future, though.

>
>
> Thanks,
>
>
> Daniel
>
>
>
>> +    if (has_iommu) {
>> +        vdev_has_iommu = virtio_host_has_feature(vdev, 
>> VIRTIO_F_IOMMU_PLATFORM);
>> +        /* Fail FEATURE_OK if the device tries to drop IOMMU_PLATFORM */
>>           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,
>> +        if (klass->get_dma_as) {
>> +            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");
>> +                return;
>> +            }
>>           }
>> -    } else {
>> -        vdev->dma_as = &address_space_memory;
>>       }
>>   }
>>   
>> 
>> base-commit: da89f242b4b774a25eaa16be125cf3e17299c127