Re: [Qemu-devel] [PATCH v5 06/10] hw/vfio: create common module

[Alex Williamson]

On Tue, 2014-09-02 at 09:13 +0200, Eric Auger wrote:
> On 09/01/2014 07:41 PM, Alexander Graf wrote:
> > 
> > 
> >> Am 01.09.2014 um 18:31 schrieb Eric Auger <address@hidden>:
> >>
> >>> On 08/13/2014 09:59 PM, Alex Williamson wrote:
> >>>> On Tue, 2014-08-12 at 08:09 +0200, Eric Auger wrote:
> >>>>> On 08/11/2014 09:25 PM, Alex Williamson wrote:
> >>>>>> On Sat, 2014-08-09 at 15:25 +0100, Eric Auger wrote:
> >>>>>> diff --git a/include/hw/vfio/vfio-common.h 
> >>>>>> b/include/hw/vfio/vfio-common.h
> >>>>>> new file mode 100644
> >>>>>> index 0000000..4684ee5
> >>>>>> --- /dev/null
> >>>>>> +++ b/include/hw/vfio/vfio-common.h
> >>>>>> @@ -0,0 +1,151 @@
> >>>>>> +/*
> >>>>>> + * common header for vfio based device assignment support
> >>>>>> + *
> >>>>>> + * Copyright Red Hat, Inc. 2012
> >>>>>> + *
> >>>>>> + * Authors:
> >>>>>> + *  Alex Williamson <address@hidden>
> >>>>>> + *
> >>>>>> + * This work is licensed under the terms of the GNU GPL, version 2.  
> >>>>>> See
> >>>>>> + * the COPYING file in the top-level directory.
> >>>>>> + *
> >>>>>> + * Based on qemu-kvm device-assignment:
> >>>>>> + *  Adapted for KVM by Qumranet.
> >>>>>> + *  Copyright (c) 2007, Neocleus, Alex Novik (address@hidden)
> >>>>>> + *  Copyright (c) 2007, Neocleus, Guy Zana (address@hidden)
> >>>>>> + *  Copyright (C) 2008, Qumranet, Amit Shah (address@hidden)
> >>>>>> + *  Copyright (C) 2008, Red Hat, Amit Shah (address@hidden)
> >>>>>> + *  Copyright (C) 2008, IBM, Muli Ben-Yehuda (address@hidden)
> >>>>>> + */
> >>>>>> +#ifndef HW_VFIO_VFIO_COMMON_H
> >>>>>> +#define HW_VFIO_VFIO_COMMON_H
> >>>>>> +
> >>>>>> +#include "qemu-common.h"
> >>>>>> +#include "exec/address-spaces.h"
> >>>>>> +#include "exec/memory.h"
> >>>>>> +#include "qemu/queue.h"
> >>>>>> +#include "qemu/notify.h"
> >>>>>> +
> >>>>>> +/*#define DEBUG_VFIO*/
> >>>>>> +#ifdef DEBUG_VFIO
> >>>>>> +#define DPRINTF(fmt, ...) \
> >>>>>> +    do { fprintf(stderr, "vfio: " fmt, ## __VA_ARGS__); } while (0)
> >>>>>> +#else
> >>>>>> +#define DPRINTF(fmt, ...) \
> >>>>>> +    do { } while (0)
> >>>>>> +#endif
> >>>>>
> >>>>>
> >>>>> DPRINTF also need to be renamed to avoid conflicting namespace issues.
> >>>> Ji Alex,
> >>>>
> >>>> OK.
> >>>>
> >>>> As I am going to touch at traces,
> >>>> - are you OK if I use the new .name field to simply format strings?
> >>>
> >>> Sure, that's fine.
> >>>
> >>>>    DPRINTF("%s(%04x:%02x:%02x.%x) Pin %c\n", __func__, vdev->host.domain,
> >>>>            vdev->host.bus, vdev->host.slot, vdev->host.function,
> >>>>            'A' + vdev->intx.pin);
> >>>> - Also Alex was suggesting to use trace points. What is your position
> >>>> about that? Also I am not 100% sure of what it consists in? is it trace
> >>>> events as documented in docs/tracing.txt
> >>>
> >>> I think it would be a great conversion, but it's not required.  Thanks,
> >>
> >> Hi Alex,
> >>
> >> I am currently progressing on the conversion to trace points (I did it
> >> for platform and common and now do the job for PCI). I wonder whether it
> >> makes sense I convert all DPRINTF into trace-points or only convert a
> >> subset (state transitions, ...). Would you accept a mixture of DPRINTFs
> >> and trace-points or do you advise to convert everything?
> > 
> > Yeah, it's perfectly good to even just nit introduce new dprintfs.
> ok thanks
> > 
> >>
> >> Also the tracing.txt doc says we should use the name of the function as
> >> prefix. That being said it could be interesting to trace all pci* or all
> >> platform* and wildcard seems to work fine to select the trace-events. So
> >> my second question is would you accept using pci_<function_name>_* as a
> >> generic pattern.
> > 
> > Not sure - maybe be more explicit and call it vfio_pci_...?
> well. maybe as a first draft I will follow the tracing.txt guideline and
> you will tell me, both Alex's, what you think of the outcome. Anyway it
> is not a big deal then to change ...

I haven't touched tracing yet, so I'll defer to you and agraf for now ;)
Thanks,

Alex