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

[Matthew Rosato <mjrosato@linux.ibm.com>]

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.