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

[Matthew Rosato]

On 1/17/22 9:51 AM, Thomas Huth wrote:
> On 14/01/2022 21.38, Matthew Rosato wrote:
...
> >   static void s390_pcihost_plug(HotplugHandler *hotplug_dev, 
> > DeviceState *dev,
> >                                 Error **errp)
> >   {
> >       S390pciState *s = S390_PCI_HOST_BRIDGE(hotplug_dev);
> >       PCIDevice *pdev = NULL;
> >       S390PCIBusDevice *pbdev = NULL;
> > +    int rc;
> >       if (object_dynamic_cast(OBJECT(dev), TYPE_PCI_BRIDGE)) {
> >           PCIBridge *pb = PCI_BRIDGE(dev);
> > @@ -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.
> > +             */
> > +            if (pbdev->interp && 
> > !s390_has_feat(S390_FEAT_ZPCI_INTERP)) {
> > +                    DPRINTF("zPCI interpretation facilities 
> > missing.\n");
> > +                    pbdev->interp = false;
> > +                }
>
> Wrong indentation in the above three lines.

Thanks

> > +            if (pbdev->interp) {
> > +                rc = s390_pci_interp_plug(s, pbdev);
> > +                if (rc) {
> > +                    error_setg(errp, "zpci interp plug failed: %d", rc);
>
> The error message is a little bit scarce for something that might be 
> presented to the user - maybe write at least "interpretation" instead of 
> "interp" ?
Good point, I'll re-word to something like "Plug failed for zPCI device 
in interpretation mode: %d"

> > +                    return;
> > +                }
> > +            }
> >               pbdev->iommu->dma_limit = s390_pci_start_dma_count(s, 
> > pbdev);
> >               /* Fill in CLP information passed via the vfio region */
> >               s390_pci_get_clp_info(pbdev);
> > +            if (!pbdev->interp) {
> > +                /* Do vfio passthrough but intercept for I/O */
> > +                pbdev->fh |= FH_SHM_VFIO;
> > +            }
> >           } else {
> >               pbdev->fh |= FH_SHM_EMUL;
> > +            /* Always intercept emulated devices */
> > +            pbdev->interp = false;
> >           }
>
>   Thomas