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

[Cédric Le Goater]

On 3/3/23 16:42, Alex Williamson wrote:
> 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.

Transpose. Like Ctrl+T under Emacs.

> > > 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