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

[Daniel Henrique Barboza]

On 2/7/22 10:41, Cornelia Huck wrote:
> On Mon, Feb 07 2022, Daniel Henrique Barboza <danielhb413@gmail.com> wrote:
>
> > On 2/3/22 13:45, Halil Pasic wrote:
> > > Unlike most virtio features ACCESS_PATFORM is considered mandatory, i.e.
>
> s/ACCESS_PATFORM/ACCESS_PLATFORM/
>
> > > 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.
>
> It's in section 6.1 "Driver Requirements: Reserved Feature Bits": "A
> driver SHOULD accept VIRTIO_F_ACCESS_PLATFORM if it is offered" and
> section 6.2 "Device Requirements: Reserved Feature Bits": "A device MAY
> fail to operate further if VIRTIO_F_ACCESS_PLATFORM is not accepted."

If we provide this info in the commit message we can skip adding a doc link.

> That said, I'm not sure the wording in the spec translates to
> "mandatory"... if the driver fails to accept the bit, the device can
> choose to not work with the driver, but it's not forced to. There are
> other instances where the device may reject FEATURES_OK (e.g. when the
> driver does not accept a feature that is a pre-req for another feature),
> I'd say it is up to the device whether something is mandatory or not. If
> the device/setup cannot work without it, it certainly is mandatory, but
> the driver only knows when FEATURES_OK is rejected without the feature.
>
> OTOH, the decision to make it mandatory is certainly sound, and covered
> by the spec. As the driver must be prepared for the device failing to
> accept FEATURES_OK, we can make it mandatory here -- we should just not
> say that it is considered mandatory from a spec standpoint. The spec
> allows to make it mandatory, and we make it mandatory in our
> implementation.

Fair point.

> > > 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".
>
> I thought it should be "If ACCESS_PLATFORM is offered not because..." ?
>
> > > 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.
>
> It might make it harder to add code at the tail end of the function in
> the future, though.

True. I suggested that based on an assumption that the "!has_iommu" case is 
something
that is already covered and we don't need to bother about it. IMO it's fine to 
keep
the existing if/else code just in case we change our minds later on.


Thanks,


Daniel





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