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

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

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)
>>   {
> 
> (...)
>