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

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

On 09/01/2014 07:41 PM, Alexander Graf wrote:
> 
> 
>> Am 01.09.2014 um 18:31 schrieb 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?
> 
> 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 ...

Thanks

Eric
> 
> 
> Alex
> 
>>
>> Thanks in advance
>>
>> Best Regards
>>
>> Eric
>>>
>>> Alex
>>