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

From: "Cédric Le Goater" <clg@redhat.com>
To: "Philippe Mathieu-Daudé" <philmd@linaro.org>,
	"Cédric Le Goater" <clg@kaod.org>,
	qemu-devel@nongnu.org
Cc: Alex Williamson <alex.williamson@redhat.com>
Subject: Re: [PATCH v2] vfio: Fix vfio_get_dev_region() trace event
Date: Fri, 3 Mar 2023 10:45:01 +0100	[thread overview]
Message-ID: <f4f9fe64-0e79-08e7-7833-fd5c897424a8@redhat.com> (raw)
In-Reply-To: <93a2b734-d3b1-27e8-32f4-d46342dc6b00@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>
> 