Re: [RFC v3 06/19] vfio-user: Define type vfio_user_pci_dev_info

[John Johnson]

> On Nov 19, 2021, at 2:42 PM, Alex Williamson <alex.williamson@redhat.com> 
> wrote:
> 
> On Mon,  8 Nov 2021 16:46:34 -0800
> John Johnson <john.g.johnson@oracle.com> wrote:
> 
>> New class for vfio-user with its class and instance
>> constructors and destructors, and its pci ops.
>> 
>> Signed-off-by: Elena Ufimtseva <elena.ufimtseva@oracle.com>
>> Signed-off-by: John G Johnson <john.g.johnson@oracle.com>
>> Signed-off-by: Jagannathan Raman <jag.raman@oracle.com>
>> ---
>> hw/vfio/pci.h   |  9 ++++++
>> hw/vfio/pci.c   | 97 
>> +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>> hw/vfio/Kconfig | 10 ++++++
>> 3 files changed, 116 insertions(+)
>> 
>> diff --git a/hw/vfio/pci.h b/hw/vfio/pci.h
>> index bbc78aa..08ac647 100644
>> --- a/hw/vfio/pci.h
>> +++ b/hw/vfio/pci.h
>> @@ -187,6 +187,15 @@ struct VFIOKernPCIDevice {
>>     VFIOPCIDevice device;
>> };
>> 
>> +#define TYPE_VFIO_USER_PCI "vfio-user-pci"
>> +OBJECT_DECLARE_SIMPLE_TYPE(VFIOUserPCIDevice, VFIO_USER_PCI)
>> +
>> +struct VFIOUserPCIDevice {
>> +    VFIOPCIDevice device;
>> +    char *sock_name;
>> +    bool secure_dma; /* disable shared mem for DMA */
>> +};
>> +
>> /* Use uin32_t for vendor & device so PCI_ANY_ID expands and cannot match hw 
>> */
>> static inline bool vfio_pci_is(VFIOPCIDevice *vdev, uint32_t vendor, 
>> uint32_t device)
>> {
>> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
>> index 6e2ce35..fa3e028 100644
>> --- a/hw/vfio/pci.c
>> +++ b/hw/vfio/pci.c
>> @@ -19,6 +19,7 @@
>>  */
>> 
>> #include "qemu/osdep.h"
>> +#include CONFIG_DEVICES
>> #include <linux/vfio.h>
>> #include <sys/ioctl.h>
>> 
>> @@ -3438,3 +3439,99 @@ struct VFIOValidOps vfio_pci_valid_ops = {
>>     .validate_get_region_info = vfio_pci_valid_region_info,
>>     .validate_get_irq_info = vfio_pci_valid_irq_info,
>> };
>> +
>> +
>> +#ifdef CONFIG_VFIO_USER_PCI
>> +
>> +/*
>> + * vfio-user routines.
>> + */
>> +
>> +/*
>> + * Emulated devices don't use host hot reset
>> + */
>> +static int vfio_user_pci_no_reset(VFIODevice *vbasedev)
>> +{
>> +    error_printf("vfio-user - no hot reset\n");
>> +    return 0;
>> +}
>> +
>> +static void vfio_user_pci_not_needed(VFIODevice *vbasedev)
>> +{
>> +    vbasedev->needs_reset = false;
>> +}
> 
> Seems like we should make some of these optional rather than stubbing
> dummy functions.
> 

        ok

>> +
>> +static VFIODeviceOps vfio_user_pci_ops = {
>> +    .vfio_compute_needs_reset = vfio_user_pci_not_needed,
>> +    .vfio_hot_reset_multi = vfio_user_pci_no_reset,
>> +    .vfio_eoi = vfio_intx_eoi,
>> +    .vfio_get_object = vfio_pci_get_object,
>> +    .vfio_save_config = vfio_pci_save_config,
>> +    .vfio_load_config = vfio_pci_load_config,
>> +};
>> +
>> +static void vfio_user_pci_realize(PCIDevice *pdev, Error **errp)
>> +{
>> +    ERRP_GUARD();
>> +    VFIOUserPCIDevice *udev = VFIO_USER_PCI(pdev);
>> +    VFIOPCIDevice *vdev = VFIO_PCI_BASE(pdev);
>> +    VFIODevice *vbasedev = &vdev->vbasedev;
>> +
>> +    /*
>> +     * TODO: make option parser understand SocketAddress
>> +     * and use that instead of having scalar options
>> +     * for each socket type.
>> +     */
>> +    if (!udev->sock_name) {
>> +        error_setg(errp, "No socket specified");
>> +        error_append_hint(errp, "Use -device 
>> vfio-user-pci,socket=<name>\n");
>> +        return;
>> +    }
>> +
>> +    vbasedev->name = g_strdup_printf("VFIO user <%s>", udev->sock_name);
>> +    vbasedev->dev = DEVICE(vdev);
>> +    vbasedev->fd = -1;
>> +    vbasedev->type = VFIO_DEVICE_TYPE_PCI;
>> +    vbasedev->no_mmap = false;
> 
> Why hard coded rather than a property?  This is a useful debugging
> feature to be able to trap all device accesses.  The device should work
> either way.
> 

        As I said in the earlier property reply, I thought these were
HW workarounds.



>> +    vbasedev->ops = &vfio_user_pci_ops;
>> +    vbasedev->valid_ops = &vfio_pci_valid_ops;
>> +
>> +}
>> +
>> +static void vfio_user_instance_finalize(Object *obj)
>> +{
>> +}
>> +
>> +static Property vfio_user_pci_dev_properties[] = {
>> +    DEFINE_PROP_STRING("socket", VFIOUserPCIDevice, sock_name),
>> +    DEFINE_PROP_BOOL("secure-dma", VFIOUserPCIDevice, secure_dma, false),
> 
> Add this when it means something.  Thanks,
> 

        This can be moved to the path that uses ’secure_dma”

                                                JJ