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

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

On Tue, 9 Jun 2020 08:44:02 +0200
Cornelia Huck <cohuck@redhat.com> wrote:

> On Mon, 8 Jun 2020 19:00:45 +0200
> Halil Pasic <pasic@linux.ibm.com> wrote:
> 
> 
> > > I'm also not 100% sure about migration... would it make sense to
> > > discuss all of this in the context of the cross-arch patchset? It seems
> > > power has similar issues.
> > >   
> > 
> > I'm going to definitely have a good look at that. What I think special
> > about s390 is that F_ACCESS_PLATFORM is hurting us because all IO needs
> > to go through ZONE_DMA (this is a problem of the implementation that
> > stemming form a limitation of the DMA API, upstream didn't let me
> > fix it). 
> 
> My understanding is that power runs into similar issues, but I don't
> know much about power, so I might be entirely wrong :)
> 

I will come back to you once I've figured that patch-set out.

[..]

> > > > @@ -874,6 +874,20 @@ static void virtio_ccw_reset(DeviceState *d)
> > > >      VirtioCcwDevice *dev = VIRTIO_CCW_DEVICE(d);
> > > >      VirtIODevice *vdev = virtio_bus_get_device(&dev->bus);
> > > >      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 (vdev->access_platform_auto && ms->pv) {
> > > > +        virtio_add_feature(&vdev->host_features, VIRTIO_F_IOMMU_PLATFORM);
> > > > +        vdev->access_platform = ON_OFF_AUTO_ON;
> > > > +    } else if (vdev->access_platform_auto) {
> > > > +        virtio_clear_feature(&vdev->host_features, VIRTIO_F_IOMMU_PLATFORM);
> > > > +        vdev->access_platform = ON_OFF_AUTO_OFF;
> > > > +    }  
> > > 
> > > If the consequences are so dire, we really should disallow adding a
> > > device of IOMMU_PLATFORM off if pv is on.  
> > 
> > I totally agree. My previous patch didn't have the problem because there
> > we just forced what we need.
> > 
> > > 
> > > (Can we disallow transition to pv if it is off? Maybe with the machine
> > > property approach from the cross-arch patchset?)  
> > 
> > No we can't disallow transition because for hotplug that already
> > happened.
> 
> I mean, can a virtio devices without IOMMU_PLATFORM act as a transition
> blocker (i.e. an attempt by a guest to move to pv would fail?)
>

I don't know. Janosch could answer that, but he is on vacation. Adding
Claudio maybe he can answer. My understanding is, that while it might be
possible, it is ugly at best. The ability to do a transition is
indicated by a CPU model feature. Indicating the feature to the guest and
then failing the transition sounds wrong to me.
 
[..]

> > 
> > Right, this is more about the machine than the transport. I was thinking
> > of a machine hook, but decided to discuss the more basic stuff first
> > (i.e. is it OK to change the property after stuff is realized).
> > 
> > This would already fix the most pressing issue which is virtio-ccw. I
> > didn't even bother checking if virtio-pci works with PV out of the box,
> > or if something needs to be done there. My bet is that it does not work.
> 
> I agree, virtio-pci + pv is unlikely to work. But if at all possible,
> I'd prefer a general solution anyway, as other architectures care about
> virtio-pci.
> 
> 

I'm with you. I hoped to get changing features on the fly approved first
before setting out to solve this. But I will look into it.

Regards,
Halil