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

[Matthew Rosato]

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!