Re: [PATCH v2 4/9] s390x/pci: enable for load/store intepretation

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

On 1/31/22 9:46 AM, Pierre Morel wrote:
> 
> 
> On 1/14/22 21:38, 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.
> 
> I wonder if we should not explain here that having interpretation as a 
> default and silently fall back to interception allows backward 
> compatibility while allowing performence be chosing by default.
> (You can say it better as I do :) )

Good suggestion, I'll think of something to add to the commit message.


>> @@ -1022,12 +1068,33 @@ static void s390_pcihost_plug(HotplugHandler 
>> *hotplug_dev, DeviceState *dev,
>>           set_pbdev_info(pbdev);
>>           if (object_dynamic_cast(OBJECT(dev), "vfio-pci")) {
>> -            pbdev->fh |= FH_SHM_VFIO;
>> +            /*
>> +             * By default, interpretation is always requested; if the 
>> available
>> +             * facilities indicate it is not available, fallback to the
>> +             * intercept model.
> 
> s/intercept/interception/ ?
> 

OK

>> +             */
>> +            if (pbdev->interp && 
>> !s390_has_feat(S390_FEAT_ZPCI_INTERP)) {
>> +                    DPRINTF("zPCI interpretation facilities 
>> missing.\n");
>> +                    pbdev->interp = false;
>> +                }
>> +            if (pbdev->interp) {
>> +                rc = s390_pci_interp_plug(s, pbdev);
>> +                if (rc) {
>> +                    error_setg(errp, "zpci interp plug failed: %d", rc);
>> +                    return;
>> +                }
>> +            }
> 
> 
> Can't we rearrange that as
> if (pbdev->interp) {
>      if (s390_has_feat) {
>      } else {
>      }
> }

Yep, sure

...

> 
> LGTM
> With the corrections proposed by Thomas.
> Mine... you see what you prefer.
> 
> Reviewed-by: Pierre Morel <pmorel@linux.ibm.com>

Thanks!