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

[Michael S. Tsirkin]

On Fri, Jan 28, 2022 at 08:02:39AM -0300, Daniel Henrique Barboza wrote:
> 
> 
> On 1/27/22 23:29, Halil Pasic wrote:
> > On Thu, 27 Jan 2022 18:34:23 -0300
> > Daniel Henrique Barboza <danielhb413@gmail.com> wrote:
> > 
> > > On 1/27/22 10:28, Halil Pasic wrote:
> > > > ping^2
> > > > 
> > > > Also adding Brijesh and Daniel, as I believe you guys should be
> > > > interested in this, and I'm yet to receive review.
> > > > 
> > > > @Brijesh, Daniel: Can you confirm that AMD (SEV) and Power are affected
> > > > too, and that the fix works for your platforms as well?
> > > 
> > > I failed to find a host that has Power secure execution support. I'll 
> > > keep looking.
> > > 
> > > 
> > > Meanwhile I have to mention that this patch re-introduced the problem 
> > > that Kevin's
> > > commit fixed.
> 
> [...]
> 
> > > 
> > > I made a little experiment with upstream and reverting Kevin's patch and 
> > > the result is
> > > the same, meaning that this is the original bug [1] Kevin fixed back 
> > > then. Note that [1]
> > > was reported on x86, meaning that this particular issue seems to be arch 
> > > agnostic.
> > 
> > We don't have this problem on s390, so it ain't entirely arch agnostic.
> 
> It is arch agnostic in a way that it relies on iommu_platform support being 
> true to this
> specific device (vhost-user-fs-pci) instead of some particularity of the 
> machine.

But it is specific to VMs with an IOMMU or other ways to restrict access
such as cgs, right?
Without a vIOMMU or cgs in the VM the ACCESS_PLATFORM flag is a nop for
the guest, it doesn't affect anything except slowing things down
somewhat, right?

> > 
> > > 
> > > 
> > > My point here is that your patch fixes the situation for s390x, and 
> > > Brijesh already chimed
> > > in claiming that it fixed for AMD SEV, but it reintroduced a bug. I 
> > > believe you should
> > > include this test case with vhost-user in your testing to figure out a 
> > > way to fix what
> > > is needed without adding this particular regression.
> > 
> > Can you help me with this? IMHO the big problem is that iommu_platform
> > is used for two distinct things. I've described that in the commit
> > message.
> > 
> > We may be able to differentiate between the two using ->dma_as, but for
> > that it needs to be set up correctly: whenever you require translation
> > it should be something different than address_space_memory. The question
> > is why do you require translation but don't have your ->dma_as set up
> > properly? It can be a guest thing, i.e. guest just assumes it has to do
> > bus addresses, while it actually does not have to, or we indeed do have
> > an IOMMU which polices the devices access to the guest memory, but for
> > some strange reason we failed to set up ->dma_as to reflect that.
> 
> 
> I have 2 suggestions. First is to separate how we interpret iommu_platform. I 
> find it
> hard to do this properly without creating a new flag/command line option.

We do want to switch to call it access_platform at some point anyway.
When we do, we can make it only mean the guest flag.


> 
> My second suggestion is, well .... I think it's proved that s390x-PV and AMD 
> SEV are
> being impacted (and probably Power secure guests as well), so why not check 
> for
> confidential guest support to skip that check entirely? Something like this 
> patch:
> 
> diff --git a/hw/virtio/virtio-bus.c b/hw/virtio/virtio-bus.c
> index d23db98c56..4305fdd1b7 100644
> --- a/hw/virtio/virtio-bus.c
> +++ b/hw/virtio/virtio-bus.c
> @@ -29,6 +29,7 @@
>  #include "hw/virtio/virtio-bus.h"
>  #include "hw/virtio/virtio.h"
>  #include "exec/address-spaces.h"
> +#include "hw/boards.h"
>  /* #define DEBUG_VIRTIO_BUS */
> @@ -42,6 +43,7 @@ do { printf("virtio_bus: " fmt , ## __VA_ARGS__); } while 
> (0)
>  /* A VirtIODevice is being plugged */
>  void virtio_bus_device_plugged(VirtIODevice *vdev, Error **errp)
>  {
> +    MachineState *machine = MACHINE(qdev_get_machine());
>      DeviceState *qdev = DEVICE(vdev);
>      BusState *qbus = BUS(qdev_get_parent_bus(qdev));
>      VirtioBusState *bus = VIRTIO_BUS(qbus);
> @@ -69,7 +71,18 @@ void virtio_bus_device_plugged(VirtIODevice *vdev, Error 
> **errp)
>          return;
>      }
> -    if (has_iommu && !virtio_host_has_feature(vdev, 
> VIRTIO_F_IOMMU_PLATFORM)) {
> +    /*
> +     * Confidential guest technologies such as AMD SEV and s390x-PV relies
> +     * on device/hypervisor offering _F_ACCESS_PLATFORM so the guest grants
> +     * access to the portions of memory the device needs to see. For these
> +     * guests, _F_ACCESS_PLATFORM is about the restricted access to memory,
> +     * but not about infering whether iommu_platform is supported in the
> +     * device.
> +     *
> +     * Skip this check for these guests by checking machine->cgs.
> +     */
> +    if (!machine->cgs && has_iommu &&
> +        !virtio_host_has_feature(vdev, VIRTIO_F_IOMMU_PLATFORM)) {
>          error_setg(errp, "iommu_platform=true is not supported by the 
> device");
>          return;
>      }
> -- 
> 2.34.1

In fact I proposed setting _F_ACCESS_PLATFORM automatically in the past,



> 
> This will not break anything for non-secure guests and, granted that 
> machine->cgs is already
> set at this point, this will fix the problem for s390x-PV and AMD SEV. And we 
> won't have to
> dive deep into a virtio-bus feature negotiation saga because of something 
> that can be easily
> handled for machine->cgs guests only.
> 
> If this patch works for you and Brijesh I believe this is a good option.
> 
> 
> 
> Thanks,
> 
> 
> Daniel
> 
> 
> 
> > 
> > @Michael: what is your opinion?
> > 
> > > 
> > > 
> > > In fact, I have a feeling that this is not the first time this kind of 
> > > situation is discussed
> > > around here. This reminds me of [2] and a discussion about the order 
> > > virtiofs features
> > > are negotiated versus when/how QEMU inits the devices.
> > > 
> > > 
> > > 
> > > [1] https://bugzilla.redhat.com/show_bug.cgi?id=1935019
> > > [2] https://lists.gnu.org/archive/html/qemu-devel/2021-02/msg05644.html
> > > 
> > > 
> > > Thanks,
> > > 
> > > 
> > > Daniel
> > > 
> > > 
> > > > 
> > > > Regards,
> > > > Halil
> > > > 
> > > > On Tue, 25 Jan 2022 11:21:12 +0100
> > > > Halil Pasic <pasic@linux.ibm.com> wrote:
> > > > > ping
> > > > > 
> > > > > On Mon, 17 Jan 2022 13:02:38 +0100
> > > > > Halil Pasic <pasic@linux.ibm.com> wrote:
> > > > > > The commit 04ceb61a40 ("virtio: Fail if iommu_platform is 
> > > > > > requested, but
> > > > > > unsupported") claims to fail the device hotplug when iommu_platform
> > > > > > is requested, but not supported by the (vhost) device. On the first
> > > > > > glance the condition for detecting that situation looks perfect, but
> > > > > > because a certain peculiarity of virtio_platform it ain't.
> > > > > > 
> > > > > > In fact the aforementioned commit introduces a regression. It breaks
> > > > > > virtio-fs support for Secure Execution, and most likely also for 
> > > > > > AMD SEV
> > > > > > or any other confidential guest scenario that relies encrypted guest
> > > > > > memory.  The same also applies to any other vhost device that does 
> > > > > > not
> > > > > > support _F_ACCESS_PLATFORM.
> > > > > > 
> > > > > > The peculiarity is that iommu_platform and _F_ACCESS_PLATFORM 
> > > > > > collates
> > > > > > "device can not access all of the guest RAM" and "iova != gpa, thus
> > > > > > device needs to translate iova".
> > > > > > 
> > > > > > Confidential guest technologies currently rely on the 
> > > > > > device/hypervisor
> > > > > > offering _F_ACCESS_PLATFORM, so that, after the feature has been
> > > > > > negotiated, the guest  grants access to the portions of memory the
> > > > > > device needs to see. So in for confidential guests, generally,
> > > > > > _F_ACCESS_PLATFORM is about the restricted access to memory, but not
> > > > > > about the addresses used being something else than guest physical
> > > > > > addresses.
> > > > > > 
> > > > > > This is the very reason for which commit f7ef7e6e3b ("vhost: 
> > > > > > correctly
> > > > > > turn on VIRTIO_F_IOMMU_PLATFORM") for, which fences 
> > > > > > _F_ACCESS_PLATFORM
> > > > > > form the vhost device that does not need it, because on the vhost
> > > > > > interface it only means "I/O address translation is needed".
> > > > > > 
> > > > > > This patch takes inspiration from f7ef7e6e3b ("vhost: correctly 
> > > > > > turn on
> > > > > > VIRTIO_F_IOMMU_PLATFORM"), and uses the same condition for 
> > > > > > detecting the
> > > > > > situation when _F_ACCESS_PLATFORM is requested, but no I/O 
> > > > > > translation
> > > > > > by the device, and thus no device capability is needed. In this
> > > > > > situation claiming that the device does not support 
> > > > > > iommu_plattform=on
> > > > > > is counter-productive. So let us stop doing that!
> > > > > > 
> > > > > > Signed-off-by: Halil Pasic <pasic@linux.ibm.com>
> > > > > > Reported-by: Jakob Naucke <Jakob.Naucke@ibm.com>
> > > > > > Fixes: 04ceb61a40 ("virtio: Fail if iommu_platform is requested, but
> > > > > > unsupported")
> > > > > > Cc: Kevin Wolf <kwolf@redhat.com>
> > > > > > Cc: qemu-stable@nongnu.org
> > > > > > 
> > > > > > ---
> > > > > > 
> > > > > > v1->v2:
> > > > > > * Commit message tweaks. Most notably fixed commit SHA (Michael)
> > > > > > 
> > > > > > ---
> > > > > >    hw/virtio/virtio-bus.c | 11 ++++++-----
> > > > > >    1 file changed, 6 insertions(+), 5 deletions(-)
> > > > > > 
> > > > > > diff --git a/hw/virtio/virtio-bus.c b/hw/virtio/virtio-bus.c
> > > > > > index d23db98c56..c1578f3de2 100644
> > > > > > --- a/hw/virtio/virtio-bus.c
> > > > > > +++ b/hw/virtio/virtio-bus.c
> > > > > > @@ -69,11 +69,6 @@ void virtio_bus_device_plugged(VirtIODevice 
> > > > > > *vdev, Error **errp)
> > > > > >            return;
> > > > > >        }
> > > > > > -    if (has_iommu && !virtio_host_has_feature(vdev, 
> > > > > > VIRTIO_F_IOMMU_PLATFORM)) {
> > > > > > -        error_setg(errp, "iommu_platform=true is not supported by 
> > > > > > the device");
> > > > > > -        return;
> > > > > > -    }
> > > > > > -
> > > > > >        if (klass->device_plugged != NULL) {
> > > > > >            klass->device_plugged(qbus->parent, &local_err);
> > > > > >        }
> > > > > > @@ -88,6 +83,12 @@ void virtio_bus_device_plugged(VirtIODevice 
> > > > > > *vdev, Error **errp)
> > > > > >        } else {
> > > > > >            vdev->dma_as = &address_space_memory;
> > > > > >        }
> > > > > > +
> > > > > > +    if (has_iommu && vdev->dma_as != &address_space_memory
> > > > > > +                  && !virtio_host_has_feature(vdev, 
> > > > > > VIRTIO_F_IOMMU_PLATFORM)) {
> > > > > > +        error_setg(errp, "iommu_platform=true is not supported by 
> > > > > > the device");
> > > > > > +        return;
> > > > > > +    }
> > > > > >    }
> > > > > >    /* Reset the virtio_bus */
> > > > > > 
> > > > > > base-commit: 6621441db50d5bae7e34dbd04bf3c57a27a71b32
> > > > 
> > > 
> >