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

[Halil Pasic]

On Thu, 10 Feb 2022 10:55:13 +0100
Cornelia Huck <cohuck@redhat.com> wrote:

> On Wed, Feb 09 2022, Halil Pasic <pasic@linux.ibm.com> wrote:
> 
> > On Wed, 09 Feb 2022 18:24:56 +0100
> > Cornelia Huck <cohuck@redhat.com> wrote:
> >  
> >> On Wed, Feb 09 2022, Halil Pasic <pasic@linux.ibm.com> wrote:  
> >> > @@ -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;
> >> > +    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 
> >> > */    
> >> 
> >> I must admit that the more I stare at this code, the more confused I
> >> get. We run this function during device realization, and the reason that
> >> the feature bit might have gotten lost is that the ->get_features()
> >> device callback dropped it. This happens before the driver is actually
> >> involved; the check whether the *driver* dropped the feature is done
> >> during feature validation, which is another code path.   
> > [moved text from here]  
> >>   
> >> >          virtio_add_feature(&vdev->host_features, 
> >> > VIRTIO_F_IOMMU_PLATFORM); [Mark 1]  
> >
> >
> > Let us have a look at 
> > static int virtio_validate_features(VirtIODevice *vdev)                     
> >     
> > {                                                                           
> >     
> >     VirtioDeviceClass *k = VIRTIO_DEVICE_GET_CLASS(vdev);                   
> >     
> >                                                                             
> >     
> >     if (virtio_host_has_feature(vdev, VIRTIO_F_IOMMU_PLATFORM) &&           
> >     
> >         !virtio_vdev_has_feature(vdev, VIRTIO_F_IOMMU_PLATFORM)) {          
> >     
> >         return -EFAULT;                                                     
> >     [Mark 2]                  
> >     }                                                                       
> >     
> > [..]
> >
> > So were it not of the [Mark 1] we could not hit [Mark 2] if the feature
> > bit was lost because the ->get_features() callback dropped it. Yes,
> > feature negotiation is another code path, but the two are interdependent
> > in a non-trivial way. That is why I added that comment.  
> 
> Yes, of course we need to offer the bit to the driver in the first
> place. My point is that the code here is not what makes us fail
> FEATURES_OK; we won't even get to that point because the device will
> fail realization.

I disagree! Have you tested your hypothesis? Which line of code does
cause the device realization to fail? Where is that check?

> 
> >
> > [moved here]  
> >> So what we do
> >> here is failing device realization if a backend doesn't support
> >> IOMMU_PLATFORM, isn't it?  
> >
> > Not really. We fail the device realization if !vdev_has_iommu &&
> > vdev->dma_as != &address_space_memory, that is the device does not
> > support address translation, but we need it to support address
> > translation because ->dma_as != &address_space memory. If however  
> > ->dma_as == &address_space memory we carry on happily even if 
> > ->get_features() dropped  
> > IOMMU_PLATFORM, because we don't actually need an iova -> gpa
> > translation. This is the case with virtiofs confidential guests for
> > example.
> >  
> 
> Well yes, that's what I meant, I just did not spell out all of the
> conditions...
> 
> > But we still don't want the guest dropping ACCESS_PLATFORM, because it is
> > still mandatory, because the device won't operate correctly unless the
> > driver grants access to the pieces of memory that the device needs to
> > access. The underlying mechanism of granting access may not have
> > anything to do with an IOMMU though.
> >
> > Does it make sense now?  
> 
> The code yes, the comment no. What we are actually doing is failing
> realization so we don't end up offering a device without IOMMU_PLATFORM
> that would need it. 

I don't understand. That is only one of the possible cases IMHO.

Do you mean the check
        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;                                                         
            }                                                                   
        }
or something different? If yo mean that check, it does not cover all
cases where has_iommu.

Please note that the line in question is

    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);  
only conditional on has_iommu.

But we want the guest to *never* drop ACCESS_PLATFORM, regardless of 
vdev_has_iommu and ->dma_as.

Please also note that the comment 
/* Fail FEATURE_OK if the device tries to drop IOMMU_PLATFORM */
is intended to document why do we do 
virtio_add_feature(&vdev->host_features, VIRTIO_F_IOMMU_PLATFORM);  
_only_ and is not intended to document the entire code that follows:

        virtio_add_feature(&vdev->host_features, VIRTIO_F_IOMMU_PLATFORM);      
        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;                                                         
            }                                                                   
        }

Is that the source of the confusion? If yes, maybe I should add a blank
line after virtio_add_feature().

Regards,
Halil

> The code that fails FEATURES_OK if the driver
> dropped it is already in place.
> 
> I'd suggest a comment like
> 
> /* make sure that the device did not drop a required IOMMU_PLATFORM */
> 
> or so.
> 
>