1. Patchew
  2. QEMU
  3. View series

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

Cédric Le Goater posted 1 patch 1 year, 1 month ago
Failed in applying to current master (apply log)
Maintainers: Alex Williamson <alex.williamson@redhat.com>, "Cédric Le Goater" <clg@redhat.com>
hw/vfio/trace-events | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
[PATCH v2] vfio: Fix vfio_get_dev_region() trace event
Posted by Cédric Le Goater 1 year, 1 month ago 
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"
 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
-- 
2.39.2
Re: [PATCH v2] vfio: Fix vfio_get_dev_region() trace event
Posted by Philippe Mathieu-Daudé 1 year, 1 month ago 
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>
Re: [PATCH v2] vfio: Fix vfio_get_dev_region() trace event
Posted by Alex Williamson 1 year, 1 month ago 
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
Re: [PATCH v2] vfio: Fix vfio_get_dev_region() trace event
Posted by Cédric Le Goater 1 year, 1 month ago 
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
>
Re: [PATCH v2] vfio: Fix vfio_get_dev_region() trace event
Posted by Cédric Le Goater 1 year, 1 month ago 
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>
>
Re: [PATCH v2] vfio: Fix vfio_get_dev_region() trace event
Posted by Philippe Mathieu-Daudé 1 year, 1 month ago 
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

Patchew 2.1-devel-0870f84

© 2016 - 2024 Red Hat, Inc.