| Message ID | 20230303074330.2609377-1-clg@kaod.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] vfio: Fix vfio_get_dev_region() trace event | expand |
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' > > 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) linux-headers/linux/vfio.h:323:#define VFIO_REGION_TYPE_PCI_VENDOR_MASK (0xffff) linux-headers/linux/vfio.h:324:#define VFIO_REGION_TYPE_GFX (1) linux-headers/linux/vfio.h:325:#define VFIO_REGION_TYPE_CCW (2) linux-headers/linux/vfio.h:326:#define VFIO_REGION_TYPE_MIGRATION_DEPRECATED (3) linux-headers/linux/vfio.h:328:/* sub-types for VFIO_REGION_TYPE_PCI_* */ linux-headers/linux/vfio.h:331:#define VFIO_REGION_SUBTYPE_INTEL_IGD_OPREGION (1) linux-headers/linux/vfio.h:332:#define VFIO_REGION_SUBTYPE_INTEL_IGD_HOST_CFG (2) linux-headers/linux/vfio.h:333:#define VFIO_REGION_SUBTYPE_INTEL_IGD_LPC_CFG (3) linux-headers/linux/vfio.h:341:#define VFIO_REGION_SUBTYPE_NVIDIA_NVLINK2_RAM (1) linux-headers/linux/vfio.h:350:#define VFIO_REGION_SUBTYPE_IBM_NVLINK2_ATSD (1) linux-headers/linux/vfio.h:352:/* sub-types for VFIO_REGION_TYPE_GFX */ linux-headers/linux/vfio.h:353:#define VFIO_REGION_SUBTYPE_GFX_EDID (1) linux-headers/linux/vfio.h:402:/* sub-types for VFIO_REGION_TYPE_CCW */ linux-headers/linux/vfio.h:403:#define VFIO_REGION_SUBTYPE_CCW_ASYNC_CMD (1) linux-headers/linux/vfio.h:404:#define VFIO_REGION_SUBTYPE_CCW_SCHIB (2) linux-headers/linux/vfio.h:405:#define VFIO_REGION_SUBTYPE_CCW_CRW (3) linux-headers/linux/vfio.h:407:/* sub-types for VFIO_REGION_TYPE_MIGRATION */ linux-headers/linux/vfio.h:408:#define VFIO_REGION_SUBTYPE_MIGRATION_DEPRECATED (1) Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
On 3/3/23 10:34, Philippe Mathieu-Daudé 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' >> >> 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 :) well, yes, but it also depends on other possible outputs from the OS or, more important, from gdb when dumping memory. It it better if they match. Since it is an hex number, may be I should add a '0x' prefix also ? That's a lot of versions for a 2 bytes patch ! :) Thanks, C. > > $ git grep -E 'VFIO_REGION_(SUB)?TYPE_' linux-headers/ > linux-headers/linux/vfio.h:322:#define VFIO_REGION_TYPE_PCI_VENDOR_TYPE (1 << 31) > linux-headers/linux/vfio.h:323:#define VFIO_REGION_TYPE_PCI_VENDOR_MASK (0xffff) > linux-headers/linux/vfio.h:324:#define VFIO_REGION_TYPE_GFX (1) > linux-headers/linux/vfio.h:325:#define VFIO_REGION_TYPE_CCW (2) > linux-headers/linux/vfio.h:326:#define VFIO_REGION_TYPE_MIGRATION_DEPRECATED (3) > linux-headers/linux/vfio.h:328:/* sub-types for VFIO_REGION_TYPE_PCI_* */ > linux-headers/linux/vfio.h:331:#define VFIO_REGION_SUBTYPE_INTEL_IGD_OPREGION (1) > linux-headers/linux/vfio.h:332:#define VFIO_REGION_SUBTYPE_INTEL_IGD_HOST_CFG (2) > linux-headers/linux/vfio.h:333:#define VFIO_REGION_SUBTYPE_INTEL_IGD_LPC_CFG (3) > linux-headers/linux/vfio.h:341:#define VFIO_REGION_SUBTYPE_NVIDIA_NVLINK2_RAM (1) > linux-headers/linux/vfio.h:350:#define VFIO_REGION_SUBTYPE_IBM_NVLINK2_ATSD (1) > linux-headers/linux/vfio.h:352:/* sub-types for VFIO_REGION_TYPE_GFX */ > linux-headers/linux/vfio.h:353:#define VFIO_REGION_SUBTYPE_GFX_EDID (1) > linux-headers/linux/vfio.h:402:/* sub-types for VFIO_REGION_TYPE_CCW */ > linux-headers/linux/vfio.h:403:#define VFIO_REGION_SUBTYPE_CCW_ASYNC_CMD (1) > linux-headers/linux/vfio.h:404:#define VFIO_REGION_SUBTYPE_CCW_SCHIB (2) > linux-headers/linux/vfio.h:405:#define VFIO_REGION_SUBTYPE_CCW_CRW (3) > linux-headers/linux/vfio.h:407:/* sub-types for VFIO_REGION_TYPE_MIGRATION */ > linux-headers/linux/vfio.h:408:#define VFIO_REGION_SUBTYPE_MIGRATION_DEPRECATED (1) > > Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org> >
On 3/3/23 10:45, Cédric Le Goater wrote: > On 3/3/23 10:34, Philippe Mathieu-Daudé 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' >>> >>> 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 :) > > well, yes, but it also depends on other possible outputs from the OS or, > more important, from gdb when dumping memory. It it better if they match. > > Since it is an hex number, may be I should add a '0x' prefix also ? That's > a lot of versions for a 2 bytes patch ! :) Per the coding style if you don't want to use the 0x prefix for readability, add a "(in hex)" hint: https://qemu-project.gitlab.io/qemu/devel/style.html#trace-events-style
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
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 >
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" vfio_dma_unmap_overflow_workaround(void) "" vfio_get_dirty_bitmap(int fd, uint64_t iova, uint64_t size, uint64_t bitmap_size, uint64_t start) "container fd=%d, iova=0x%"PRIx64" size= 0x%"PRIx64" bitmap_size=0x%"PRIx64" start=0x%"PRIx64 vfio_iommu_map_dirty_notify(uint64_t iova_start, uint64_t iova_end) "iommu dirty @ 0x%"PRIx64" - 0x%"PRIx64