Re: [PATCH v2] vfio: Fix vfio_get_dev_region() trace event

[Alex Williamson]

On Fri, 3 Mar 2023 10:34:47 +0100
Philippe Mathieu-Daudé <philmd@linaro.org> wrote:

> On 3/3/23 08:43, Cédric Le Goater wrote:
> > From: Cédric Le Goater <clg@redhat.com>
> > 
> > Simply revert 'x8' to fix the typo and remove the ending '8'

s/revert/reverse/?  ie. transpose?  Nothing is being reverted afaict.

> > 
> > Fixes: e61a424f05 ("vfio: Create device specific region info helper")
> > Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1526
> > Signed-off-by: Cédric Le Goater <clg@redhat.com>
> > ---
> >   hw/vfio/trace-events | 2 +-
> >   1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/hw/vfio/trace-events b/hw/vfio/trace-events
> > index 669d9fe07c..28e82541a2 100644
> > --- a/hw/vfio/trace-events
> > +++ b/hw/vfio/trace-events
> > @@ -117,7 +117,7 @@ vfio_region_mmaps_set_enabled(const char *name, bool 
> > enabled) "Region %s mmaps e
> >   vfio_region_unmap(const char *name, unsigned long offset, unsigned long 
> > end) "Region %s unmap [0x%lx - 0x%lx]"
> >   vfio_region_sparse_mmap_header(const char *name, int index, int nr_areas) 
> > "Device %s region %d: %d sparse mmap entries"
> >   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_get_dev_region(const char *name, int index, uint32_t type, uint32_t 
> > subtype) "%s index %d, %08x/%08x"  
> 
> 8 digits might be over-engineered, but at least is future-proof :)
> 
> $ git grep -E 'VFIO_REGION_(SUB)?TYPE_' linux-headers/
> linux-headers/linux/vfio.h:322:#define VFIO_REGION_TYPE_PCI_VENDOR_TYPE 
> (1 << 31)

This alone justifies printing the full zero-prefixed hex value.  I tend
to prefer fixed width fields in trace logs, though it's less important
here than when we're streaming register accesses.  Thanks,

Alex