Re: [Qemu-devel] [PATCH v5 03/10] hw/vfio/pci: introduce VFIODevice

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 2643 bytes --]

On Sat, Aug 09, 2014 at 03:25:42PM +0100, Eric Auger wrote:
> Introduce the VFIODevice struct that is going to be shared by
> VFIOPCIDevice and VFIOPlatformDevice.
> 
> Additional fields will be added there later on for review
> convenience.
> 
> the group's device_list becomes a list of VFIODevice
> 
> This obliges to rework the reset_handler which becomes generic and
> calls VFIODevice ops that are specialized in each parent object.
> Also functions that iterate on this list must take care that the
> devices can be something else than VFIOPCIDevice. The type is used
> to discriminate them.
> 
> we profit from this step to change the prototype of
> vfio_unmask_intx, vfio_mask_intx, vfio_disable_irqindex which now
> apply to VFIODevice. They are renamed as *_irqindex.
> The index is passed as parameter to anticipate their usage for
> platform IRQs
> 
> Signed-off-by: Eric Auger <eric.auger@linaro.org>
> 
> ---
> 
> v4->v5:
> - fix style issues
> - in vfio_initfn, rework allocation of vdev->vbasedev.name and
>   replace snprintf by g_strdup_printf
> ---
>  hw/vfio/pci.c | 239 +++++++++++++++++++++++++++++++++++-----------------------
>  1 file changed, 146 insertions(+), 93 deletions(-)
> 
> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
> index c2cdd73..ae827c5 100644
> --- a/hw/vfio/pci.c
> +++ b/hw/vfio/pci.c
> @@ -56,6 +56,11 @@
>  #define VFIO_ALLOW_KVM_MSI 1
>  #define VFIO_ALLOW_KVM_MSIX 1
>  
> +enum {
> +    VFIO_DEVICE_TYPE_PCI = 0,
> +    VFIO_DEVICE_TYPE_PLATFORM = 1,
> +};
> +
>  struct VFIOPCIDevice;
>  
>  typedef struct VFIOQuirk {
> @@ -193,9 +198,27 @@ typedef struct VFIOMSIXInfo {
>      void *mmap;
>  } VFIOMSIXInfo;
>  
> +typedef struct VFIODeviceOps VFIODeviceOps;
> +
> +typedef struct VFIODevice {
> +    QLIST_ENTRY(VFIODevice) next;
> +    struct VFIOGroup *group;
> +    char *name;
> +    int fd;
> +    int type;

I'm assuming this takes values from the enum above.  Is this actually
necessary as a field, or could the same information be derived from
the device's QOM class information?

> +    bool reset_works;
> +    bool needs_reset;
> +    VFIODeviceOps *ops;
> +} VFIODevice;
> +
> +struct VFIODeviceOps {
> +    bool (*vfio_compute_needs_reset)(VFIODevice *vdev);
> +    int (*vfio_hot_reset_multi)(VFIODevice *vdev);
> +};

Shouldn't these be methods in the QOM class, rather than a separate
ops structure?

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]