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

[Daniel Henrique Barboza]

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.


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


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


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