Re: [PATCH v3 5/5] s390x/pci: Honor DMA limits set by vfio

[Matthew Rosato]

On 9/16/20 7:05 AM, Cornelia Huck wrote:
> On Tue, 15 Sep 2020 15:14:43 -0400
> Matthew Rosato <mjrosato@linux.ibm.com> wrote:
>
> > When an s390 guest is using lazy unmapping, it can result in a very
> > large number of oustanding DMA requests, far beyond the default
> > limit configured for vfio.  Let's track DMA usage similar to vfio
> > in the host, and trigger the guest to flush their DMA mappings
> > before vfio runs out.
> >
> > Signed-off-by: Matthew Rosato <mjrosato@linux.ibm.com>
> > ---
> >   hw/s390x/s390-pci-bus.c  | 56 +++++++++++++++++++++++++++++++++++++++++++-----
> >   hw/s390x/s390-pci-bus.h  |  9 ++++++++
> >   hw/s390x/s390-pci-inst.c | 34 +++++++++++++++++++++++------
> >   hw/s390x/s390-pci-inst.h |  3 +++
> >   4 files changed, 91 insertions(+), 11 deletions(-)
>
> (...)
>
> > @@ -737,6 +740,41 @@ static void s390_pci_iommu_free(S390pciState *s, PCIBus 
> > *bus, int32_t devfn)
> >       object_unref(OBJECT(iommu));
> >   }
> >   
> > +static S390PCIDMACount *s390_start_dma_count(S390pciState *s, VFIODevice *vdev)
>
> Should these go into the new vfio-related file?
>
> > +{
> > +    int id = vdev->group->container->fd;
> > +    S390PCIDMACount *cnt;
> > +    uint32_t avail;
> > +
> > +    if (!s390_pci_update_dma_avail(id, &avail)) {
> > +        return NULL;
> > +    }
> > +
> > +    QTAILQ_FOREACH(cnt, &s->zpci_dma_limit, link) {
> > +        if (cnt->id  == id) {
> > +            cnt->users++;
> > +            return cnt;
> > +        }
> > +    }
> > +
> > +    cnt = g_new0(S390PCIDMACount, 1);
> > +    cnt->id = id;
> > +    cnt->users = 1;
> > +    cnt->avail = avail;
> > +    QTAILQ_INSERT_TAIL(&s->zpci_dma_limit, cnt, link);
> > +    return cnt;
> > +}
> > +
> > +static void s390_end_dma_count(S390pciState *s, S390PCIDMACount *cnt)
> > +{
> > +    assert(cnt);
> > +
> > +    cnt->users--;
> > +    if (cnt->users == 0) {
> > +        QTAILQ_REMOVE(&s->zpci_dma_limit, cnt, link);
> > +    }
> > +}
> > +
> >   static void s390_pcihost_realize(DeviceState *dev, Error **errp)
> >   {
> >       PCIBus *b;
> > @@ -764,6 +802,7 @@ static void s390_pcihost_realize(DeviceState *dev, Error 
> > **errp)
> >       s->bus_no = 0;
> >       QTAILQ_INIT(&s->pending_sei);
> >       QTAILQ_INIT(&s->zpci_devs);
> > +    QTAILQ_INIT(&s->zpci_dma_limit);
> >   
> >       css_register_io_adapters(CSS_IO_ADAPTER_PCI, true, false,
> >                                S390_ADAPTER_SUPPRESSIBLE, errp);
> > @@ -902,6 +941,7 @@ static void s390_pcihost_plug(HotplugHandler *hotplug_dev, 
> > DeviceState *dev,
> >   {
> >       S390pciState *s = S390_PCI_HOST_BRIDGE(hotplug_dev);
> >       PCIDevice *pdev = NULL;
> > +    VFIOPCIDevice *vpdev = NULL;
> >       S390PCIBusDevice *pbdev = NULL;
> >   
> >       if (object_dynamic_cast(OBJECT(dev), TYPE_PCI_BRIDGE)) {
> > @@ -941,17 +981,20 @@ static void s390_pcihost_plug(HotplugHandler 
> > *hotplug_dev, DeviceState *dev,
> >               }
> >           }
> >   
> > +        pbdev->pdev = pdev;
> > +        pbdev->iommu = s390_pci_get_iommu(s, pci_get_bus(pdev), pdev->devfn);
> > +        pbdev->iommu->pbdev = pbdev;
> > +        pbdev->state = ZPCI_FS_DISABLED;
> > +
> >           if (object_dynamic_cast(OBJECT(dev), "vfio-pci")) {
> >               pbdev->fh |= FH_SHM_VFIO;
> > +            vpdev = container_of(pbdev->pdev, VFIOPCIDevice, pdev);
> > +            pbdev->iommu->dma_limit = s390_start_dma_count(s,
> > +                                                           &vpdev->vbasedev);
>
> I think you can just pass s and pbdev to that function... that would
> move dealing with vfio specifics from this file.

I had considered this as well, should have went with my gut -- I'll move 
them.

> >           } else {
> >               pbdev->fh |= FH_SHM_EMUL;
> >           }
> >   
> > -        pbdev->pdev = pdev;
> > -        pbdev->iommu = s390_pci_get_iommu(s, pci_get_bus(pdev), pdev->devfn);
> > -        pbdev->iommu->pbdev = pbdev;
> > -        pbdev->state = ZPCI_FS_DISABLED;
> > -
> >           if (s390_pci_msix_init(pbdev)) {
> >               error_setg(errp, "MSI-X support is mandatory "
> >                          "in the S390 architecture");
>
> (...)
>
> > diff --git a/hw/s390x/s390-pci-inst.c b/hw/s390x/s390-pci-inst.c
> > index 2f7a7d7..cc34b17 100644
> > --- a/hw/s390x/s390-pci-inst.c
> > +++ b/hw/s390x/s390-pci-inst.c
> > @@ -32,6 +32,9 @@
> >           }                                                          \
> >       } while (0)
> >   
> > +#define inc_dma_avail(iommu) if (iommu->dma_limit) iommu->dma_limit->avail++;
>
> I was thinking more of something like
>
> static inline void inc_dma_avail(S390PCIIOMMU *iommu)
> {
>      if (iommu->dma_limit) {
>          iommu->dma_limit->avail++;
>      }
> }

Ah, I read the 'lowercase' and missed the 'inline function' part of your 
previous comment, sorry.  Will change.

> > +#define dec_dma_avail(iommu) if (iommu->dma_limit) iommu->dma_limit->avail--;
> > +
> >   static void s390_set_status_code(CPUS390XState *env,
> >                                    uint8_t r, uint64_t status_code)
> >   {
>
> (...)