Re: [PATCH 07/12] s390x/pci: enable for load/store intepretation

[Matthew Rosato]

On 12/15/21 2:44 AM, Pierre Morel wrote:
> On 12/7/21 22:04, Matthew Rosato wrote:
> > Use the associated vfio feature ioctl to enable interpretation for 
> > devices
> > when requested.  As part of this process, we must use the host function
> > handle rather than a QEMU-generated one -- this is provided as part of 
> > the
> > ioctl payload.
> >
> > Signed-off-by: Matthew Rosato <mjrosato@linux.ibm.com>
> > ---
> >   hw/s390x/s390-pci-bus.c          | 69 +++++++++++++++++++++++++++++++-
> >   hw/s390x/s390-pci-inst.c         | 63 ++++++++++++++++++++++++++++-
> >   hw/s390x/s390-pci-vfio.c         | 55 +++++++++++++++++++++++++
> >   include/hw/s390x/s390-pci-bus.h  |  1 +
> >   include/hw/s390x/s390-pci-vfio.h | 15 +++++++
> >   5 files changed, 201 insertions(+), 2 deletions(-)
> >
> > diff --git a/hw/s390x/s390-pci-bus.c b/hw/s390x/s390-pci-bus.c
> > index 01b58ebc70..451bd32d92 100644
> > --- a/hw/s390x/s390-pci-bus.c
> > +++ b/hw/s390x/s390-pci-bus.c
> > @@ -971,12 +971,57 @@ static void 
> > s390_pci_update_subordinate(PCIDevice *dev, uint32_t nr)
> >       }
> >   }
> > +static int s390_pci_interp_plug(S390pciState *s, S390PCIBusDevice 
> > *pbdev)
> > +{
> > +    uint32_t idx;
> > +    int rc;
> > +
> > +    rc = s390_pci_probe_interp(pbdev);
> > +    if (rc) {
> > +        return rc;
> > +    }
> > +
> > +    rc = s390_pci_update_passthrough_fh(pbdev);
> > +    if (rc) {
> > +        return rc;
> > +    }
> > +
> > +    /*
> > +     * The host device is in an enabled state, but the device must
> > +     * begin as disabled for the guest so mask off the enable bit
> > +     * from the passthrough handle.
> > +     */
>
> I think you should explain why the device must be seen disabled for the 
> guest.
> AFAIU it is because we need the guest to issue a CLP_ENABLE for us to 
> activate interpretation.
> This is due to the choice of activate/deactivate interpretation during 
> device enable/disable.

Not completely.  While it is true that we need the guest to issue the 
CLP_ENABLE to activate interpretation with the way this is currently 
implemented, existing code also starts all plugged devices with a QEMU 
internal state of

pbdev->state = ZPCI_FS_DISABLED;

and a disabled pbdev->fh, thus expecting a subsequent CLP ENABLE from 
the guest when/if it decides to use the device.

If we were to set the handle to an enabled state here, we must also 
udpate the pbdev state, introducing a difference in how we present 
intercept/emulated devices vs interpreted devices during plug.  I don't 
think we want to do that -- but I can improve this comment here to try 
and better explain that all devices begin plugged as disabled to the guest


> Just curious: why not activate/deactivate interpretation on plug/unplug?

I think it would be possible to do so (while still showing a disabled 
handle initially), but my thinking is that tying these actions to guest 
CLP disable/enable will also be useful later when looking at trying to 
better-handle error events from the guest e.g. hot reset.