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

[Michael S. Tsirkin]

On Wed, Feb 02, 2022 at 05:23:53PM +0100, 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.
> 
> 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

I think I second that.

-- 
MST