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

[Michael S. Tsirkin]

On Wed, Feb 02, 2022 at 02:15:47AM +0100, 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.
> 
> 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.
> 
> 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);
>  }
> 
> which would be a separate patch, as this is a separate issue. Jason,
> Michael, Connie, what do you think?

Do you mean just force VIRTIO_F_IOMMU_PLATFORM for everyone?
Or am I misreading the patch?


> Regards,
> Halil