Re: [PATCH v3 10/10] s390x/pci: get zPCI function info from host

From: Matthew Rosato <mjrosato@linux.ibm.com>
To: Cornelia Huck <cohuck@redhat.com>
Cc: thuth@redhat.com, pmorel@linux.ibm.com, schnelle@linux.ibm.com,
	rth@twiddle.net, david@redhat.com, pasic@linux.ibm.com,
	borntraeger@de.ibm.com, mst@redhat.com, pbonzini@redhat.com,
	alex.williamson@redhat.com, qemu-s390x@nongnu.org,
	qemu-devel@nongnu.org, kvm@vger.kernel.org
Subject: Re: [PATCH v3 10/10] s390x/pci: get zPCI function info from host
Date: Fri, 9 Oct 2020 12:10:29 -0400	[thread overview]
Message-ID: <6b5f9372-325c-9136-e5c5-4ff885ea8e15@linux.ibm.com> (raw)
In-Reply-To: <20201009174807.6d800999.cohuck@redhat.com>

On 10/9/20 11:48 AM, Cornelia Huck wrote:
> On Wed,  7 Oct 2020 15:04:15 -0400
> Matthew Rosato <mjrosato@linux.ibm.com> wrote:
> 
>> We use the capability chains of the VFIO_DEVICE_GET_INFO ioctl to retrieve
>> the CLP information that the kernel exports.
>>
>> To be compatible with previous kernel versions we fall back on previous
>> predefined values, same as the emulation values, when the ioctl is found
>> to not support capability chains. If individual CLP capabilities are not
>> found, we fall back on default values for only those capabilities missing
>> from the chain.
>>
>> This patch is based on work previously done by Pierre Morel.
>>
>> Signed-off-by: Matthew Rosato <mjrosato@linux.ibm.com>
>> ---
>>   hw/s390x/meson.build             |   1 +
>>   hw/s390x/s390-pci-bus.c          |  10 +-
>>   hw/s390x/s390-pci-vfio.c         | 197 +++++++++++++++++++++++++++++++++++++++
>>   include/hw/s390x/s390-pci-bus.h  |   1 +
>>   include/hw/s390x/s390-pci-clp.h  |  12 ++-
>>   include/hw/s390x/s390-pci-vfio.h |  19 ++++
>>   6 files changed, 233 insertions(+), 7 deletions(-)
>>   create mode 100644 hw/s390x/s390-pci-vfio.c
>>   create mode 100644 include/hw/s390x/s390-pci-vfio.h
> 
> (...)
> 
>> diff --git a/hw/s390x/s390-pci-vfio.c b/hw/s390x/s390-pci-vfio.c
>> new file mode 100644
>> index 0000000..43684c6
>> --- /dev/null
>> +++ b/hw/s390x/s390-pci-vfio.c
>> @@ -0,0 +1,197 @@
>> +/*
>> + * s390 vfio-pci interfaces
>> + *
>> + * Copyright 2020 IBM Corp.
>> + * Author(s): Matthew Rosato <mjrosato@linux.ibm.com>
>> + *
>> + * This work is licensed under the terms of the GNU GPL, version 2 or (at
>> + * your option) any later version. See the COPYING file in the top-level
>> + * directory.
>> + */
>> +
>> +#include <sys/ioctl.h>
>> +#include <linux/vfio.h>
>> +#include <linux/vfio_zdev.h>
>> +
>> +#include "qemu/osdep.h"
>> +#include "hw/s390x/s390-pci-bus.h"
>> +#include "hw/s390x/s390-pci-clp.h"
>> +#include "hw/s390x/s390-pci-vfio.h"
>> +#include "hw/vfio/pci.h"
>> +
>> +#ifndef DEBUG_S390PCI_VFIO
>> +#define DEBUG_S390PCI_VFIO  0
>> +#endif
>> +
>> +#define DPRINTF(fmt, ...)                                          \
>> +    do {                                                           \
>> +        if (DEBUG_S390PCI_VFIO) {                                  \
>> +            fprintf(stderr, "S390pci-vfio: " fmt, ## __VA_ARGS__); \
>> +        }                                                          \
>> +    } while (0)
> 
> Not really a fan of DPRINTF. Can you maybe use trace events instead?
> 

Sure, I was just continuing what -inst.c and -bus.c do today.  I'll 
remove DPRINTF here and look at what trace-events make sense, with a 
note to convert the rest of s390-pci* to use trace events at some later 
time.

> Other than that, looks good to me.
> 

Thanks!  Assuming nobody has further comments, I'll plan to send both 
this set (with the change above) and the 's390x/pci: Accomodate vfio DMA 
limiting' set merged together with a single linux header sync once 
5.10-rc1 is available, sound OK?