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

[Alex Williamson <alex.williamson@redhat.com>]

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