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

[Eric Auger <eric.auger@linaro.org>]

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 <alex.williamson@redhat.com>
>>>> + *
>>>> + * 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 (alex@neocleus.com)
>>>> + *  Copyright (c) 2007, Neocleus, Guy Zana (guy@neocleus.com)
>>>> + *  Copyright (C) 2008, Qumranet, Amit Shah (amit.shah@qumranet.com)
>>>> + *  Copyright (C) 2008, Red Hat, Amit Shah (amit.shah@redhat.com)
>>>> + *  Copyright (C) 2008, IBM, Muli Ben-Yehuda (muli@il.ibm.com)
>>>> + */
>>>> +#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?

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.

Thanks in advance

Best Regards

Eric
> 
> Alex
>