Re: [PATCH v2 1/1] virtio-ccw: auto-manage VIRTIO_F_IOMMU_PLATFORM if PV

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

On Wed, 20 May 2020 12:23:24 -0400
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Fri, May 15, 2020 at 12:11:55AM +0200, Halil Pasic wrote:
> > The virtio specification tells that the device is to present
> > VIRTIO_F_ACCESS_PLATFORM (a.k.a. VIRTIO_F_IOMMU_PLATFORM) when the
> > device "can only access certain memory addresses with said access
> > specified and/or granted by the platform". This is the case for a
> > protected VMs, as the device can access only memory addresses that are
> > in pages that are currently shared (only the guest can share/unsare its
> > pages).
> > 
> > No VM, however, starts out as a protected VM, but some VMs may be
> > converted to protected VMs if the guest decides so.
> > 
> > Making the end user explicitly manage the VIRTIO_F_ACCESS_PLATFORM via
> > the property iommu_on is a minor disaster. Since the correctness of the
> > paravirtualized virtio devices depends (and thus in a sense the
> > correctness of the hypervisor) it, then the hypervisor should have the
> > last word about whether VIRTIO_F_ACCESS_PLATFORM is to be presented or
> > not.
> 
> So, how about this: switch iommu to on/off/auto.

Many thanks for the reveiw, and sorry about the delay on my side. We
have holidays here in Germany and I was not motivated enough up until
now to check on my mails.


I've actually played  with the thought of switching iommu_platform to 
'on/off/auto', but I didn't find an easy way to do it. I will look
again. This would be the first property of this kind in QEMU, or?

The 'on/off/auto' would be certainly much cleaner form user-interface
perspective. The downsides are that it is more invasive, and more
complicated. I'm afraid that it would also leave more possibilities for
user error.

>  Add a property with a
> reasonable name "allow protected"?  If set allow switch to protected
> memory and also set iommu auto to on by default.  If not set then don't.
>

I think we have "allow protected" already expressed via cpu models. I'm
also not sure how libvirt would react to the idea of a new machine
property for this. You did mean "allow protected" as machine property,
or?

AFAIU "allow protected" would be required for the !PV to PV switch, and
we would have to reject paravirtualized devices with iommu_platform='off'
on VM construction or hotplug (iommu_platform='auto/on' would be fine).

Could you please confirm that I understood this correctly?


> This will come handy for other things like migrating to hosts without
> protected memory support.
> 

This is already covered by cpu model AFAIK.

> 
> Also, virtio now calls this PLATFORM_ACCESS, maybe we should rename
> the property (keeping old one around for compat)?

You mean the like rename 'iommu_platform' to 'platform_access'? I like
the idea, but I'm not sure libvirt will like it as well. Boris any
opinions?

> I feel this will address lots of complaints ...
> 
> > Currently presenting a PV guest with a (paravirtualized) virtio-ccw
> > device has catastrophic consequences for the VM (after the hypervisors
> > access to protected memory). This is especially grave in case of device
> > hotplug (because in this case the guest is more likely to be in the
> > middle of something important).
> > 
> > Let us manage the VIRTIO_F_ACCESS_PLATFORM virtio feature automatically
> > for virtio-ccw devices, i.e. force it before we start the protected VM.
> > If the VM should cease to be protected, the original value is restored.
> > 
> > Signed-off-by: Halil Pasic <pasic@linux.ibm.com>
> 
> 
> I don't really understand things fully but it looks like you are
> changing features of a device.  If so this bothers me, resets
> happen at random times while driver is active, and we never
> expect features to change.
>

Changing the device features is IMHO all right because the features can
change only immediately after a system reset and before the first vCPU
is run. That is ensured by two facts.


First, the feature can only change when ms->pv changes. That is on the
first reset after the VM entered or left the "protected virtualization"
mode of operation. And that switch requires a system reset. Because the
PV switch is initiated by the guest, and the guest is rebooted as a
consequence, the guest will never observe the change in features.

By the way, when switching between PV and !PV the features of the
cpu (model) also change.

Second,  virtio_ccw_reset() -- the function that is modified -- does
not get called on a reset that is initiated via the transport. We have
virtio_ccw_reset_virtio() for that.

[..]

> >      VirtIOCCWDeviceClass *vdc = VIRTIO_CCW_DEVICE_GET_CLASS(dev);
> > +    S390CcwMachineState *ms = S390_CCW_MACHINE(qdev_get_machine());
> > +
> > +    /*
> > +     * An attempt to use a paravirt device without
> > VIRTIO_F_IOMMU_PLATFORM
> > +     * in PV, has catastrophic consequences for the VM. Let's force
> > +     * VIRTIO_F_IOMMU_PLATFORM not already specified.
> > +     */
> > +    if (ms->pv && !virtio_host_has_feature(vdev,
> > VIRTIO_F_IOMMU_PLATFORM)) {
> > +        virtio_add_feature(&vdev->host_features,
> > VIRTIO_F_IOMMU_PLATFORM);
> > +        dev->forced_iommu_platform = true;
> > +    } else if (!ms->pv && dev->forced_iommu_platform) {
> > +        virtio_clear_feature(&vdev->host_features,
> > VIRTIO_F_IOMMU_PLATFORM);
> > +        dev->forced_iommu_platform = false;
> > +    }
> >  
> >      virtio_ccw_reset_virtio(dev, vdev);
> >      if (vdc->parent_reset) {
> > diff --git a/hw/s390x/virtio-ccw.h b/hw/s390x/virtio-ccw.h
> > index 3453aa1f98..34ff7b0b4e 100644
> > --- a/hw/s390x/virtio-ccw.h
> > +++ b/hw/s390x/virtio-ccw.h
> > @@ -99,6 +99,7 @@ struct VirtioCcwDevice {
> >      IndAddr *summary_indicator;
> >      uint64_t ind_bit;
> >      bool force_revision_1;
> > +    bool forced_iommu_platform;
> >  };
> >  
> >  /* The maximum virtio revision we support. */
> > 
> 
> This seems unused. Why is it helpful?
> 

You mean the "base-commit: SHA-1"?

It is what the --base option of git format-patch generates, and it tells
what exact commit the series is based on. Can be useful when it is not
clear against which git subtree was developed, or for comparatively old
series. It hopefully also indicates what code level was tested by the
developer.


Thanks again!

Regards,
Halil

> 
> > base-commit: 0ffd3d64bd1bb8b84950e52159a0062fdab34628
> > -- 
> > 2.17.1
>