[PATCH] hw/vfio/common: Trace in which mode a IOMMU is opened

[PATCH] hw/vfio/common: Trace in which mode a IOMMU is opened

[Philippe Mathieu-Daudé]

One might want to check which IOMMU version the host kernel
provide. Add a trace event to see in which mode we opened
our container.

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 hw/vfio/common.c     | 19 ++++++++++++++-----
 hw/vfio/trace-events |  1 +
 2 files changed, 15 insertions(+), 5 deletions(-)

diff --git a/hw/vfio/common.c b/hw/vfio/common.c
index 0b3593b3c0..6b69a259c1 100644
--- a/hw/vfio/common.c
+++ b/hw/vfio/common.c
@@ -1157,15 +1157,24 @@ static void vfio_put_address_space(VFIOAddressSpace *space)
 static int vfio_get_iommu_type(VFIOContainer *container,
                                Error **errp)
 {
-    int iommu_types[] = { VFIO_TYPE1v2_IOMMU, VFIO_TYPE1_IOMMU,
-                          VFIO_SPAPR_TCE_v2_IOMMU, VFIO_SPAPR_TCE_IOMMU };
+    static const struct {
+        int type;
+        const char *name;
+    } iommu[] = {
+        {VFIO_TYPE1v2_IOMMU, "Type1 (v2)"},
+        {VFIO_TYPE1_IOMMU, "Type1 (v1)"},
+        {VFIO_SPAPR_TCE_v2_IOMMU, "sPAPR TCE (v2)"},
+        {VFIO_SPAPR_TCE_IOMMU, "sPAPR TCE (v1)"}
+    };
     int i;
 
-    for (i = 0; i < ARRAY_SIZE(iommu_types); i++) {
-        if (ioctl(container->fd, VFIO_CHECK_EXTENSION, iommu_types[i])) {
-            return iommu_types[i];
+    for (i = 0; i < ARRAY_SIZE(iommu); i++) {
+        if (ioctl(container->fd, VFIO_CHECK_EXTENSION, iommu[i].type)) {
+            trace_vfio_get_iommu_type(iommu[i].type, iommu[i].name);
+            return iommu[i].type;
         }
     }
+    trace_vfio_get_iommu_type(-1, "Not available or not supported");
     error_setg(errp, "No available IOMMU models");
     return -EINVAL;
 }
diff --git a/hw/vfio/trace-events b/hw/vfio/trace-events
index b1ef55a33f..8166c4c50d 100644
--- a/hw/vfio/trace-events
+++ b/hw/vfio/trace-events
@@ -115,6 +115,7 @@ vfio_region_sparse_mmap_header(const char *name, int index, int nr_areas) "Devic
 vfio_region_sparse_mmap_entry(int i, unsigned long start, unsigned long end) "sparse entry %d [0x%lx - 0x%lx]"
 vfio_get_dev_region(const char *name, int index, uint32_t type, uint32_t subtype) "%s index %d, %08x/%0x8"
 vfio_dma_unmap_overflow_workaround(void) ""
+vfio_get_iommu_type(int iommu_type, const char *iommu_name) "IOMMU type %d (%s)"
 
 # platform.c
 vfio_platform_base_device_init(char *name, int groupid) "%s belongs to group #%d"
-- 
2.21.3

Re: [PATCH] hw/vfio/common: Trace in which mode a IOMMU is opened

[Cornelia Huck]

On Tue, 26 May 2020 19:35:42 +0200
Philippe Mathieu-Daudé <philmd@redhat.com> wrote:

> One might want to check which IOMMU version the host kernel
> provide. Add a trace event to see in which mode we opened
> our container.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  hw/vfio/common.c     | 19 ++++++++++++++-----
>  hw/vfio/trace-events |  1 +
>  2 files changed, 15 insertions(+), 5 deletions(-)

Reviewed-by: Cornelia Huck <cohuck@redhat.com>

Re: [PATCH] hw/vfio/common: Trace in which mode a IOMMU is opened

[Auger Eric]

Hi Philippe,

On 5/26/20 7:35 PM, Philippe Mathieu-Daudé wrote:
> One might want to check which IOMMU version the host kernel
> provide. Add a trace event to see in which mode we opened
> our container.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  hw/vfio/common.c     | 19 ++++++++++++++-----
>  hw/vfio/trace-events |  1 +
>  2 files changed, 15 insertions(+), 5 deletions(-)
> 
> diff --git a/hw/vfio/common.c b/hw/vfio/common.c
> index 0b3593b3c0..6b69a259c1 100644
> --- a/hw/vfio/common.c
> +++ b/hw/vfio/common.c
> @@ -1157,15 +1157,24 @@ static void vfio_put_address_space(VFIOAddressSpace *space)
>  static int vfio_get_iommu_type(VFIOContainer *container,
>                                 Error **errp)
>  {
> -    int iommu_types[] = { VFIO_TYPE1v2_IOMMU, VFIO_TYPE1_IOMMU,
> -                          VFIO_SPAPR_TCE_v2_IOMMU, VFIO_SPAPR_TCE_IOMMU };
> +    static const struct {
> +        int type;
> +        const char *name;
> +    } iommu[] = {
> +        {VFIO_TYPE1v2_IOMMU, "Type1 (v2)"},
> +        {VFIO_TYPE1_IOMMU, "Type1 (v1)"},
> +        {VFIO_SPAPR_TCE_v2_IOMMU, "sPAPR TCE (v2)"},
> +        {VFIO_SPAPR_TCE_IOMMU, "sPAPR TCE (v1)"}
> +    };
>      int i;
>  
> -    for (i = 0; i < ARRAY_SIZE(iommu_types); i++) {
> -        if (ioctl(container->fd, VFIO_CHECK_EXTENSION, iommu_types[i])) {
> -            return iommu_types[i];
> +    for (i = 0; i < ARRAY_SIZE(iommu); i++) {
> +        if (ioctl(container->fd, VFIO_CHECK_EXTENSION, iommu[i].type)) {
> +            trace_vfio_get_iommu_type(iommu[i].type, iommu[i].name);
Just wondering why you want to trace the type as you now have the name
string.
> +            return iommu[i].type;
>          }
>      }
> +    trace_vfio_get_iommu_type(-1, "Not available or not supported");
nit: from a debugging pov, this may be not needed as
vfio_get_group/vfio_connect_container() fails and this leads to an error
output.

Thanks

Eric
>      error_setg(errp, "No available IOMMU models");
>      return -EINVAL;
>  }
> diff --git a/hw/vfio/trace-events b/hw/vfio/trace-events
> index b1ef55a33f..8166c4c50d 100644
> --- a/hw/vfio/trace-events
> +++ b/hw/vfio/trace-events
> @@ -115,6 +115,7 @@ vfio_region_sparse_mmap_header(const char *name, int index, int nr_areas) "Devic
>  vfio_region_sparse_mmap_entry(int i, unsigned long start, unsigned long end) "sparse entry %d [0x%lx - 0x%lx]"
>  vfio_get_dev_region(const char *name, int index, uint32_t type, uint32_t subtype) "%s index %d, %08x/%0x8"
>  vfio_dma_unmap_overflow_workaround(void) ""
> +vfio_get_iommu_type(int iommu_type, const char *iommu_name) "IOMMU type %d (%s)"
>  
>  # platform.c
>  vfio_platform_base_device_init(char *name, int groupid) "%s belongs to group #%d"
> 

Re: [PATCH] hw/vfio/common: Trace in which mode a IOMMU is opened

[Philippe Mathieu-Daudé]

On 5/27/20 9:08 AM, Auger Eric wrote:
> Hi Philippe,
> 
> On 5/26/20 7:35 PM, Philippe Mathieu-Daudé wrote:
>> One might want to check which IOMMU version the host kernel
>> provide. Add a trace event to see in which mode we opened
>> our container.
>>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>> ---
>>  hw/vfio/common.c     | 19 ++++++++++++++-----
>>  hw/vfio/trace-events |  1 +
>>  2 files changed, 15 insertions(+), 5 deletions(-)
>>
>> diff --git a/hw/vfio/common.c b/hw/vfio/common.c
>> index 0b3593b3c0..6b69a259c1 100644
>> --- a/hw/vfio/common.c
>> +++ b/hw/vfio/common.c
>> @@ -1157,15 +1157,24 @@ static void vfio_put_address_space(VFIOAddressSpace *space)
>>  static int vfio_get_iommu_type(VFIOContainer *container,
>>                                 Error **errp)
>>  {
>> -    int iommu_types[] = { VFIO_TYPE1v2_IOMMU, VFIO_TYPE1_IOMMU,
>> -                          VFIO_SPAPR_TCE_v2_IOMMU, VFIO_SPAPR_TCE_IOMMU };
>> +    static const struct {
>> +        int type;
>> +        const char *name;
>> +    } iommu[] = {
>> +        {VFIO_TYPE1v2_IOMMU, "Type1 (v2)"},
>> +        {VFIO_TYPE1_IOMMU, "Type1 (v1)"},
>> +        {VFIO_SPAPR_TCE_v2_IOMMU, "sPAPR TCE (v2)"},
>> +        {VFIO_SPAPR_TCE_IOMMU, "sPAPR TCE (v1)"}
>> +    };
>>      int i;
>>  
>> -    for (i = 0; i < ARRAY_SIZE(iommu_types); i++) {
>> -        if (ioctl(container->fd, VFIO_CHECK_EXTENSION, iommu_types[i])) {
>> -            return iommu_types[i];
>> +    for (i = 0; i < ARRAY_SIZE(iommu); i++) {
>> +        if (ioctl(container->fd, VFIO_CHECK_EXTENSION, iommu[i].type)) {
>> +            trace_vfio_get_iommu_type(iommu[i].type, iommu[i].name);
> Just wondering why you want to trace the type as you now have the name
> string.

You are right :)

>> +            return iommu[i].type;
>>          }
>>      }
>> +    trace_vfio_get_iommu_type(-1, "Not available or not supported");
> nit: from a debugging pov, this may be not needed as
> vfio_get_group/vfio_connect_container() fails and this leads to an error
> output.

Indeed.

Thanks for the review!

> 
> Thanks
> 
> Eric
>>      error_setg(errp, "No available IOMMU models");
>>      return -EINVAL;
>>  }
>> diff --git a/hw/vfio/trace-events b/hw/vfio/trace-events
>> index b1ef55a33f..8166c4c50d 100644
>> --- a/hw/vfio/trace-events
>> +++ b/hw/vfio/trace-events
>> @@ -115,6 +115,7 @@ vfio_region_sparse_mmap_header(const char *name, int index, int nr_areas) "Devic
>>  vfio_region_sparse_mmap_entry(int i, unsigned long start, unsigned long end) "sparse entry %d [0x%lx - 0x%lx]"
>>  vfio_get_dev_region(const char *name, int index, uint32_t type, uint32_t subtype) "%s index %d, %08x/%0x8"
>>  vfio_dma_unmap_overflow_workaround(void) ""
>> +vfio_get_iommu_type(int iommu_type, const char *iommu_name) "IOMMU type %d (%s)"
>>  
>>  # platform.c
>>  vfio_platform_base_device_init(char *name, int groupid) "%s belongs to group #%d"
>>
> 

Re: [PATCH] hw/vfio/common: Trace in which mode a IOMMU is opened

[Philippe Mathieu-Daudé]

On 5/27/20 9:43 AM, Philippe Mathieu-Daudé wrote:
> On 5/27/20 9:08 AM, Auger Eric wrote:
>> Hi Philippe,
>>
>> On 5/26/20 7:35 PM, Philippe Mathieu-Daudé wrote:
>>> One might want to check which IOMMU version the host kernel
>>> provide. Add a trace event to see in which mode we opened
>>> our container.
>>>
>>> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>>> ---
>>>  hw/vfio/common.c     | 19 ++++++++++++++-----
>>>  hw/vfio/trace-events |  1 +
>>>  2 files changed, 15 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/hw/vfio/common.c b/hw/vfio/common.c
>>> index 0b3593b3c0..6b69a259c1 100644
>>> --- a/hw/vfio/common.c
>>> +++ b/hw/vfio/common.c
>>> @@ -1157,15 +1157,24 @@ static void vfio_put_address_space(VFIOAddressSpace *space)
>>>  static int vfio_get_iommu_type(VFIOContainer *container,
>>>                                 Error **errp)
>>>  {
>>> -    int iommu_types[] = { VFIO_TYPE1v2_IOMMU, VFIO_TYPE1_IOMMU,
>>> -                          VFIO_SPAPR_TCE_v2_IOMMU, VFIO_SPAPR_TCE_IOMMU };
>>> +    static const struct {
>>> +        int type;
>>> +        const char *name;
>>> +    } iommu[] = {
>>> +        {VFIO_TYPE1v2_IOMMU, "Type1 (v2)"},
>>> +        {VFIO_TYPE1_IOMMU, "Type1 (v1)"},
>>> +        {VFIO_SPAPR_TCE_v2_IOMMU, "sPAPR TCE (v2)"},
>>> +        {VFIO_SPAPR_TCE_IOMMU, "sPAPR TCE (v1)"}
>>> +    };
>>>      int i;
>>>  
>>> -    for (i = 0; i < ARRAY_SIZE(iommu_types); i++) {
>>> -        if (ioctl(container->fd, VFIO_CHECK_EXTENSION, iommu_types[i])) {
>>> -            return iommu_types[i];
>>> +    for (i = 0; i < ARRAY_SIZE(iommu); i++) {
>>> +        if (ioctl(container->fd, VFIO_CHECK_EXTENSION, iommu[i].type)) {
>>> +            trace_vfio_get_iommu_type(iommu[i].type, iommu[i].name);
>> Just wondering why you want to trace the type as you now have the name
>> string.
> 
> You are right :)
> 
>>> +            return iommu[i].type;
>>>          }
>>>      }
>>> +    trace_vfio_get_iommu_type(-1, "Not available or not supported");
>> nit: from a debugging pov, this may be not needed as
>> vfio_get_group/vfio_connect_container() fails and this leads to an error
>> output.

But you can reach this for example using No-IOMMU. If you don't mind, I
find having this information in the trace log clearer.

> 
> Indeed.
> 
> Thanks for the review!
> 
>>
>> Thanks
>>
>> Eric
>>>      error_setg(errp, "No available IOMMU models");
>>>      return -EINVAL;
>>>  }
>>> diff --git a/hw/vfio/trace-events b/hw/vfio/trace-events
>>> index b1ef55a33f..8166c4c50d 100644
>>> --- a/hw/vfio/trace-events
>>> +++ b/hw/vfio/trace-events
>>> @@ -115,6 +115,7 @@ vfio_region_sparse_mmap_header(const char *name, int index, int nr_areas) "Devic
>>>  vfio_region_sparse_mmap_entry(int i, unsigned long start, unsigned long end) "sparse entry %d [0x%lx - 0x%lx]"
>>>  vfio_get_dev_region(const char *name, int index, uint32_t type, uint32_t subtype) "%s index %d, %08x/%0x8"
>>>  vfio_dma_unmap_overflow_workaround(void) ""
>>> +vfio_get_iommu_type(int iommu_type, const char *iommu_name) "IOMMU type %d (%s)"
>>>  
>>>  # platform.c
>>>  vfio_platform_base_device_init(char *name, int groupid) "%s belongs to group #%d"
>>>
>>
> 

Re: [PATCH] hw/vfio/common: Trace in which mode a IOMMU is opened

[Peter Xu]

On Wed, May 27, 2020 at 05:53:16PM +0200, Philippe Mathieu-Daudé wrote:
> >>> +    for (i = 0; i < ARRAY_SIZE(iommu); i++) {
> >>> +        if (ioctl(container->fd, VFIO_CHECK_EXTENSION, iommu[i].type)) {
> >>> +            trace_vfio_get_iommu_type(iommu[i].type, iommu[i].name);
> >> Just wondering why you want to trace the type as you now have the name
> >> string.
> > 
> > You are right :)
> > 
> >>> +            return iommu[i].type;
> >>>          }
> >>>      }
> >>> +    trace_vfio_get_iommu_type(-1, "Not available or not supported");
> >> nit: from a debugging pov, this may be not needed as
> >> vfio_get_group/vfio_connect_container() fails and this leads to an error
> >> output.
> 
> But you can reach this for example using No-IOMMU. If you don't mind, I
> find having this information in the trace log clearer.

I kinda agree with Eric - AFAICT QEMU vfio-pci don't work with no-iommu, then
it seems meaningless to trace it...

I'm not sure whether this trace is extremely helpful because syscalls like this
could be easily traced by things like strace or bpftrace as general tools (and
this information should be a one-time thing rather than dynamically changing),
no strong opinion though.  Also, if we want to dump something, maybe it's
better to do in vfio_init_container() after vfio_get_iommu_type() succeeded, so
we dump which container is enabled with what type of iommu.

Thanks,

-- 
Peter Xu

Re: [PATCH] hw/vfio/common: Trace in which mode a IOMMU is opened

[Philippe Mathieu-Daudé]

On 5/27/20 6:16 PM, Peter Xu wrote:
> On Wed, May 27, 2020 at 05:53:16PM +0200, Philippe Mathieu-Daudé wrote:
>>>>> +    for (i = 0; i < ARRAY_SIZE(iommu); i++) {
>>>>> +        if (ioctl(container->fd, VFIO_CHECK_EXTENSION, iommu[i].type)) {
>>>>> +            trace_vfio_get_iommu_type(iommu[i].type, iommu[i].name);
>>>> Just wondering why you want to trace the type as you now have the name
>>>> string.
>>>
>>> You are right :)
>>>
>>>>> +            return iommu[i].type;
>>>>>          }
>>>>>      }
>>>>> +    trace_vfio_get_iommu_type(-1, "Not available or not supported");
>>>> nit: from a debugging pov, this may be not needed as
>>>> vfio_get_group/vfio_connect_container() fails and this leads to an error
>>>> output.
>>
>> But you can reach this for example using No-IOMMU. If you don't mind, I
>> find having this information in the trace log clearer.
> 
> I kinda agree with Eric - AFAICT QEMU vfio-pci don't work with no-iommu, then
> it seems meaningless to trace it...
> 
> I'm not sure whether this trace is extremely helpful because syscalls like this
> could be easily traced by things like strace or bpftrace as general tools (and
> this information should be a one-time thing rather than dynamically changing),
> no strong opinion though.  Also, if we want to dump something, maybe it's
> better to do in vfio_init_container() after vfio_get_iommu_type() succeeded, so
> we dump which container is enabled with what type of iommu.

OK. I'm a recent VFIO user so maybe I am not using the good information.

This trace helps me while working on a new device feature, I didn't
thought about gathering it in a production because there I'd expect
things to work.

Now in my case what I want is to know is if I'm using a v1 or v2 type.
Maybe this information is already available in /proc or /sys and we
don't need this patch...

Re: [PATCH] hw/vfio/common: Trace in which mode a IOMMU is opened

[Peter Xu]

On Wed, May 27, 2020 at 06:27:38PM +0200, Philippe Mathieu-Daudé wrote:
> On 5/27/20 6:16 PM, Peter Xu wrote:
> > On Wed, May 27, 2020 at 05:53:16PM +0200, Philippe Mathieu-Daudé wrote:
> >>>>> +    for (i = 0; i < ARRAY_SIZE(iommu); i++) {
> >>>>> +        if (ioctl(container->fd, VFIO_CHECK_EXTENSION, iommu[i].type)) {
> >>>>> +            trace_vfio_get_iommu_type(iommu[i].type, iommu[i].name);
> >>>> Just wondering why you want to trace the type as you now have the name
> >>>> string.
> >>>
> >>> You are right :)
> >>>
> >>>>> +            return iommu[i].type;
> >>>>>          }
> >>>>>      }
> >>>>> +    trace_vfio_get_iommu_type(-1, "Not available or not supported");
> >>>> nit: from a debugging pov, this may be not needed as
> >>>> vfio_get_group/vfio_connect_container() fails and this leads to an error
> >>>> output.
> >>
> >> But you can reach this for example using No-IOMMU. If you don't mind, I
> >> find having this information in the trace log clearer.
> > 
> > I kinda agree with Eric - AFAICT QEMU vfio-pci don't work with no-iommu, then
> > it seems meaningless to trace it...
> > 
> > I'm not sure whether this trace is extremely helpful because syscalls like this
> > could be easily traced by things like strace or bpftrace as general tools (and
> > this information should be a one-time thing rather than dynamically changing),
> > no strong opinion though.  Also, if we want to dump something, maybe it's
> > better to do in vfio_init_container() after vfio_get_iommu_type() succeeded, so
> > we dump which container is enabled with what type of iommu.
> 
> OK. I'm a recent VFIO user so maybe I am not using the good information.
> 
> This trace helps me while working on a new device feature, I didn't
> thought about gathering it in a production because there I'd expect
> things to work.
> 
> Now in my case what I want is to know is if I'm using a v1 or v2 type.
> Maybe this information is already available in /proc or /sys and we
> don't need this patch...

I don't know such /proc or /sys, so maybe it's still useful. I guess Alex would
have the best judgement. The strace/bpftrace things are not really reasons I
found to nak this patch, but just something I thought first that could be
easier when any of us wants to peak at those information, probably something
just FYI. :-)

Thanks,

-- 
Peter Xu

Re: [PATCH] hw/vfio/common: Trace in which mode a IOMMU is opened

[Cornelia Huck]

On Wed, 27 May 2020 12:53:30 -0400
Peter Xu <peterx@redhat.com> wrote:

> On Wed, May 27, 2020 at 06:27:38PM +0200, Philippe Mathieu-Daudé wrote:
> > On 5/27/20 6:16 PM, Peter Xu wrote:  
> > > On Wed, May 27, 2020 at 05:53:16PM +0200, Philippe Mathieu-Daudé wrote:  
> > >>>>> +    for (i = 0; i < ARRAY_SIZE(iommu); i++) {
> > >>>>> +        if (ioctl(container->fd, VFIO_CHECK_EXTENSION, iommu[i].type)) {
> > >>>>> +            trace_vfio_get_iommu_type(iommu[i].type, iommu[i].name);  
> > >>>> Just wondering why you want to trace the type as you now have the name
> > >>>> string.  
> > >>>
> > >>> You are right :)
> > >>>  
> > >>>>> +            return iommu[i].type;
> > >>>>>          }
> > >>>>>      }
> > >>>>> +    trace_vfio_get_iommu_type(-1, "Not available or not supported");  
> > >>>> nit: from a debugging pov, this may be not needed as
> > >>>> vfio_get_group/vfio_connect_container() fails and this leads to an error
> > >>>> output.  
> > >>
> > >> But you can reach this for example using No-IOMMU. If you don't mind, I
> > >> find having this information in the trace log clearer.  
> > > 
> > > I kinda agree with Eric - AFAICT QEMU vfio-pci don't work with no-iommu, then
> > > it seems meaningless to trace it...
> > > 
> > > I'm not sure whether this trace is extremely helpful because syscalls like this
> > > could be easily traced by things like strace or bpftrace as general tools (and
> > > this information should be a one-time thing rather than dynamically changing),
> > > no strong opinion though.  Also, if we want to dump something, maybe it's
> > > better to do in vfio_init_container() after vfio_get_iommu_type() succeeded, so
> > > we dump which container is enabled with what type of iommu.  
> > 
> > OK. I'm a recent VFIO user so maybe I am not using the good information.
> > 
> > This trace helps me while working on a new device feature, I didn't
> > thought about gathering it in a production because there I'd expect
> > things to work.
> > 
> > Now in my case what I want is to know is if I'm using a v1 or v2 type.
> > Maybe this information is already available in /proc or /sys and we
> > don't need this patch...  
> 
> I don't know such /proc or /sys, so maybe it's still useful. I guess Alex would
> have the best judgement. The strace/bpftrace things are not really reasons I
> found to nak this patch, but just something I thought first that could be
> easier when any of us wants to peak at those information, probably something
> just FYI. :-)

Personally, I find traces to be quite handy, and it's nice if you can
just enable more of them if they are in your debugging workflow anyway.
Probably boils down to a matter of preference :)

Re: [PATCH] hw/vfio/common: Trace in which mode a IOMMU is opened

[Peter Xu]

On Wed, May 27, 2020 at 07:06:51PM +0200, Cornelia Huck wrote:
> Personally, I find traces to be quite handy, and it's nice if you can
> just enable more of them if they are in your debugging workflow anyway.
> Probably boils down to a matter of preference :)

Totally agree.  I am actually a heavy user of QEMU tracing system, just like
the rest of the tracing tools all over the world... :)

IMHO the difference between a tracepoint and a manual printf() is majorly the
reusablility part - if a debugging printf() is likely to be reused in the
future, then it is a good tracepoint candidate.

Thanks,

-- 
Peter Xu

Re: [PATCH] hw/vfio/common: Trace in which mode a IOMMU is opened

[Alex Williamson]

On Wed, 27 May 2020 12:53:30 -0400
Peter Xu <peterx@redhat.com> wrote:

> On Wed, May 27, 2020 at 06:27:38PM +0200, Philippe Mathieu-Daudé wrote:
> > On 5/27/20 6:16 PM, Peter Xu wrote:  
> > > On Wed, May 27, 2020 at 05:53:16PM +0200, Philippe Mathieu-Daudé wrote:  
> > >>>>> +    for (i = 0; i < ARRAY_SIZE(iommu); i++) {
> > >>>>> +        if (ioctl(container->fd, VFIO_CHECK_EXTENSION, iommu[i].type)) {
> > >>>>> +            trace_vfio_get_iommu_type(iommu[i].type, iommu[i].name);  
> > >>>> Just wondering why you want to trace the type as you now have the name
> > >>>> string.  
> > >>>
> > >>> You are right :)
> > >>>  
> > >>>>> +            return iommu[i].type;
> > >>>>>          }
> > >>>>>      }
> > >>>>> +    trace_vfio_get_iommu_type(-1, "Not available or not supported");  
> > >>>> nit: from a debugging pov, this may be not needed as
> > >>>> vfio_get_group/vfio_connect_container() fails and this leads to an error
> > >>>> output.  
> > >>
> > >> But you can reach this for example using No-IOMMU. If you don't mind, I
> > >> find having this information in the trace log clearer.  
> > > 
> > > I kinda agree with Eric - AFAICT QEMU vfio-pci don't work with no-iommu, then
> > > it seems meaningless to trace it...
> > > 
> > > I'm not sure whether this trace is extremely helpful because syscalls like this
> > > could be easily traced by things like strace or bpftrace as general tools (and
> > > this information should be a one-time thing rather than dynamically changing),
> > > no strong opinion though.  Also, if we want to dump something, maybe it's
> > > better to do in vfio_init_container() after vfio_get_iommu_type() succeeded, so
> > > we dump which container is enabled with what type of iommu.  
> > 
> > OK. I'm a recent VFIO user so maybe I am not using the good information.
> > 
> > This trace helps me while working on a new device feature, I didn't
> > thought about gathering it in a production because there I'd expect
> > things to work.
> > 
> > Now in my case what I want is to know is if I'm using a v1 or v2 type.
> > Maybe this information is already available in /proc or /sys and we
> > don't need this patch...  

You're using v2 unless you're on a very old kernel.

> I don't know such /proc or /sys, so maybe it's still useful. I guess Alex would
> have the best judgement. The strace/bpftrace things are not really reasons I
> found to nak this patch, but just something I thought first that could be
> easier when any of us wants to peak at those information, probably something
> just FYI. :-)

I appreciate good trace code, but I don't appreciate code bloat for the
sake of tracing, which is what I'd consider the name fields here.  Do
it in the trace-event or require that the user needs to cross reference
the header to turn the integer type into a name themselves.  Thanks,

Alex