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

[Halil Pasic <pasic@linux.ibm.com>]

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:
> 
> > Unlike most virtio features ACCESS_PLATFORM is considered mandatory by
> > QEMU, 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.  
> 
> Maybe add
> 
> (see the "{Driver,Device} Requirements: Reserved Feature Bits" sections
> in the virtio spec)
> 
> ?

I can add that, but I doubt people will have trouble finding it anyway.
There are 6 mentions of ACCESS_PLATFORM in the spec, so unless somebody
is using the dead tree version...
[..]
> > @@ -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.

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

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?

> > -        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;
> >      }
> >  }
> >    
> 
>