hw/vfio/pci.h | 1 + hw/vfio/container.c | 4 ++-- hw/vfio/device.c | 2 +- hw/vfio/iommufd.c | 4 ++-- hw/vfio/listener.c | 4 ++-- hw/vfio/pci.c | 9 +++++++++ 6 files changed, 17 insertions(+), 7 deletions(-)
Introduce helper vfio_pci_from_vfio_device() to transform from VFIODevice
to VFIOPCIDevice, also to hide low level VFIO_DEVICE_TYPE_PCI type check.
Suggested-by: Cédric Le Goater <clg@redhat.com>
Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
---
v3: add one line comment to the helper
v2: move helper to hw/vfio/pci.[hc]
rename with vfio_pci_ prefix
hw/vfio/pci.h | 1 +
hw/vfio/container.c | 4 ++--
hw/vfio/device.c | 2 +-
hw/vfio/iommufd.c | 4 ++--
hw/vfio/listener.c | 4 ++--
hw/vfio/pci.c | 9 +++++++++
6 files changed, 17 insertions(+), 7 deletions(-)
diff --git a/hw/vfio/pci.h b/hw/vfio/pci.h
index 81465a8214..53842cb149 100644
--- a/hw/vfio/pci.h
+++ b/hw/vfio/pci.h
@@ -219,6 +219,7 @@ void vfio_pci_write_config(PCIDevice *pdev,
uint64_t vfio_vga_read(void *opaque, hwaddr addr, unsigned size);
void vfio_vga_write(void *opaque, hwaddr addr, uint64_t data, unsigned size);
+VFIOPCIDevice *vfio_pci_from_vfio_device(VFIODevice *vbasedev);
void vfio_sub_page_bar_update_mappings(VFIOPCIDevice *vdev);
bool vfio_opt_rom_in_denylist(VFIOPCIDevice *vdev);
bool vfio_config_quirk_setup(VFIOPCIDevice *vdev, Error **errp);
diff --git a/hw/vfio/container.c b/hw/vfio/container.c
index 3e13feaa74..134ddccc52 100644
--- a/hw/vfio/container.c
+++ b/hw/vfio/container.c
@@ -1087,7 +1087,7 @@ static int vfio_legacy_pci_hot_reset(VFIODevice *vbasedev, bool single)
/* Prep dependent devices for reset and clear our marker. */
QLIST_FOREACH(vbasedev_iter, &group->device_list, next) {
if (!vbasedev_iter->dev->realized ||
- vbasedev_iter->type != VFIO_DEVICE_TYPE_PCI) {
+ !vfio_pci_from_vfio_device(vbasedev_iter)) {
continue;
}
tmp = container_of(vbasedev_iter, VFIOPCIDevice, vbasedev);
@@ -1172,7 +1172,7 @@ out:
QLIST_FOREACH(vbasedev_iter, &group->device_list, next) {
if (!vbasedev_iter->dev->realized ||
- vbasedev_iter->type != VFIO_DEVICE_TYPE_PCI) {
+ !vfio_pci_from_vfio_device(vbasedev_iter)) {
continue;
}
tmp = container_of(vbasedev_iter, VFIOPCIDevice, vbasedev);
diff --git a/hw/vfio/device.c b/hw/vfio/device.c
index 52a1996dc4..08f12ac31f 100644
--- a/hw/vfio/device.c
+++ b/hw/vfio/device.c
@@ -129,7 +129,7 @@ static inline const char *action_to_str(int action)
static const char *index_to_str(VFIODevice *vbasedev, int index)
{
- if (vbasedev->type != VFIO_DEVICE_TYPE_PCI) {
+ if (!vfio_pci_from_vfio_device(vbasedev)) {
return NULL;
}
diff --git a/hw/vfio/iommufd.c b/hw/vfio/iommufd.c
index 48c590b6a9..8c27222f75 100644
--- a/hw/vfio/iommufd.c
+++ b/hw/vfio/iommufd.c
@@ -737,8 +737,8 @@ iommufd_cdev_dep_get_realized_vpdev(struct vfio_pci_dependent_device *dep_dev,
}
vbasedev_tmp = iommufd_cdev_pci_find_by_devid(dep_dev->devid);
- if (!vbasedev_tmp || !vbasedev_tmp->dev->realized ||
- vbasedev_tmp->type != VFIO_DEVICE_TYPE_PCI) {
+ if (!vfio_pci_from_vfio_device(vbasedev_tmp) ||
+ !vbasedev_tmp->dev->realized) {
return NULL;
}
diff --git a/hw/vfio/listener.c b/hw/vfio/listener.c
index f498e23a93..903dfd8bf2 100644
--- a/hw/vfio/listener.c
+++ b/hw/vfio/listener.c
@@ -450,7 +450,7 @@ static void vfio_device_error_append(VFIODevice *vbasedev, Error **errp)
* MMIO region mapping failures are not fatal but in this case PCI
* peer-to-peer transactions are broken.
*/
- if (vbasedev && vbasedev->type == VFIO_DEVICE_TYPE_PCI) {
+ if (vfio_pci_from_vfio_device(vbasedev)) {
error_append_hint(errp, "%s: PCI peer-to-peer transactions "
"on BARs are not supported.\n", vbasedev->name);
}
@@ -751,7 +751,7 @@ static bool vfio_section_is_vfio_pci(MemoryRegionSection *section,
owner = memory_region_owner(section->mr);
QLIST_FOREACH(vbasedev, &bcontainer->device_list, container_next) {
- if (vbasedev->type != VFIO_DEVICE_TYPE_PCI) {
+ if (!vfio_pci_from_vfio_device(vbasedev)) {
continue;
}
pcidev = container_of(vbasedev, VFIOPCIDevice, vbasedev);
diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
index 4fa692c1a3..85761544ba 100644
--- a/hw/vfio/pci.c
+++ b/hw/vfio/pci.c
@@ -2824,6 +2824,15 @@ static int vfio_pci_load_config(VFIODevice *vbasedev, QEMUFile *f)
return ret;
}
+/* Transform from VFIODevice to VFIOPCIDevice. Return NULL if fails. */
+VFIOPCIDevice *vfio_pci_from_vfio_device(VFIODevice *vbasedev)
+{
+ if (vbasedev && vbasedev->type == VFIO_DEVICE_TYPE_PCI) {
+ return container_of(vbasedev, VFIOPCIDevice, vbasedev);
+ }
+ return NULL;
+}
+
void vfio_sub_page_bar_update_mappings(VFIOPCIDevice *vdev)
{
PCIDevice *pdev = &vdev->pdev;
--
2.47.1
On 8/1/25 04:35, Zhenzhong Duan wrote: > Introduce helper vfio_pci_from_vfio_device() to transform from VFIODevice > to VFIOPCIDevice, also to hide low level VFIO_DEVICE_TYPE_PCI type check. > > Suggested-by: Cédric Le Goater <clg@redhat.com> > Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com> > --- > v3: add one line comment to the helper > v2: move helper to hw/vfio/pci.[hc] > rename with vfio_pci_ prefix > > hw/vfio/pci.h | 1 + > hw/vfio/container.c | 4 ++-- > hw/vfio/device.c | 2 +- > hw/vfio/iommufd.c | 4 ++-- > hw/vfio/listener.c | 4 ++-- > hw/vfio/pci.c | 9 +++++++++ > 6 files changed, 17 insertions(+), 7 deletions(-) Reviewed-by: Cédric Le Goater <clg@redhat.com> Thanks, C.
Zhenzhong, On 8/1/25 07:42, Cédric Le Goater wrote: > On 8/1/25 04:35, Zhenzhong Duan wrote: >> Introduce helper vfio_pci_from_vfio_device() to transform from VFIODevice >> to VFIOPCIDevice, also to hide low level VFIO_DEVICE_TYPE_PCI type check. >> >> Suggested-by: Cédric Le Goater <clg@redhat.com> >> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com> >> --- >> v3: add one line comment to the helper >> v2: move helper to hw/vfio/pci.[hc] >> rename with vfio_pci_ prefix >> >> hw/vfio/pci.h | 1 + >> hw/vfio/container.c | 4 ++-- >> hw/vfio/device.c | 2 +- >> hw/vfio/iommufd.c | 4 ++-- >> hw/vfio/listener.c | 4 ++-- >> hw/vfio/pci.c | 9 +++++++++ >> 6 files changed, 17 insertions(+), 7 deletions(-) > > > Reviewed-by: Cédric Le Goater <clg@redhat.com> > > Thanks, > > C. I have modified your patch with : +/** + * vfio_pci_from_vfio_device: Transform from VFIODevice to + * VFIOPCIDevice + * + * This function checks if the given @vbasedev is a VFIO PCI device. + * If it is, it returns the containing VFIOPCIDevice. + * + * @vbasedev: The VFIODevice to transform + * + * Return: The VFIOPCIDevice on success, NULL on failure. + */ See https://github.com/legoater/qemu/tree/vfio-10.2. I don't think it's really necessary, as these are internal APIs and none are documented, but Philippe seems keen on it. I guess he plans to volunteer to document the rest ;) No need to resend a v4. Thanks, C.
On Fri, 1 Aug 2025 at 16:47, Cédric Le Goater <clg@redhat.com> wrote: > I have modified your patch with : > > +/** > + * vfio_pci_from_vfio_device: Transform from VFIODevice to > + * VFIOPCIDevice > + * > + * This function checks if the given @vbasedev is a VFIO PCI device. > + * If it is, it returns the containing VFIOPCIDevice. > + * > + * @vbasedev: The VFIODevice to transform > + * > + * Return: The VFIOPCIDevice on success, NULL on failure. > + */ > > See https://github.com/legoater/qemu/tree/vfio-10.2. > > I don't think it's really necessary, as these are internal APIs and > none are documented, but Philippe seems keen on it. I guess he plans > to volunteer to document the rest ;) This is one of those cases where we have a rule in place that we apply on code review for newly added prototypes in header files, as an exercise in trying to at least not make an existing problem worse :-) I think Philippe's intent was just to say "put that one line comment by the prototype in the .h file rather than by the implementation in the .c file", rather than to require a full-on doc-comment format comment, though I do appreciate having one. thanks -- PMM
On 8/1/25 17:56, Peter Maydell wrote: > On Fri, 1 Aug 2025 at 16:47, Cédric Le Goater <clg@redhat.com> wrote: >> I have modified your patch with : >> >> +/** >> + * vfio_pci_from_vfio_device: Transform from VFIODevice to >> + * VFIOPCIDevice >> + * >> + * This function checks if the given @vbasedev is a VFIO PCI device. >> + * If it is, it returns the containing VFIOPCIDevice. >> + * >> + * @vbasedev: The VFIODevice to transform >> + * >> + * Return: The VFIOPCIDevice on success, NULL on failure. >> + */ >> >> See https://github.com/legoater/qemu/tree/vfio-10.2. >> >> I don't think it's really necessary, as these are internal APIs and >> none are documented, but Philippe seems keen on it. I guess he plans >> to volunteer to document the rest ;) > > This is one of those cases where we have a rule in place > that we apply on code review for newly added prototypes > in header files, as an exercise in trying to at least not > make an existing problem worse :-) Sure. May be, this is something we could add to checkpatch.pl then ? This would reduce the resends for simple things. > I think Philippe's intent was just to say "put that one > line comment by the prototype in the .h file rather than > by the implementation in the .c file", rather than to > require a full-on doc-comment format comment, though > I do appreciate having one. I've tried to reorganize the VFIO code to define some software subcomponents and layers in previous versions. This to handle recent (rather significant) changes and also because vfio-pci was initially a device and is gradually evolving into a kind of library. I'd also like to address documentation, so my comment was self-interested :) as it's a pretty broad topic; any help is welcome ! I should find a way to generate some of it to start with. At least. Thanks, C.
>-----Original Message----- >From: Cédric Le Goater <clg@redhat.com> >Subject: Re: [PATCH v3] vfio: Introduce helper vfio_pci_from_vfio_device() > >Zhenzhong, > >On 8/1/25 07:42, Cédric Le Goater wrote: >> On 8/1/25 04:35, Zhenzhong Duan wrote: >>> Introduce helper vfio_pci_from_vfio_device() to transform from >VFIODevice >>> to VFIOPCIDevice, also to hide low level VFIO_DEVICE_TYPE_PCI type >check. >>> >>> Suggested-by: Cédric Le Goater <clg@redhat.com> >>> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com> >>> --- >>> v3: add one line comment to the helper >>> v2: move helper to hw/vfio/pci.[hc] >>> rename with vfio_pci_ prefix >>> >>> hw/vfio/pci.h | 1 + >>> hw/vfio/container.c | 4 ++-- >>> hw/vfio/device.c | 2 +- >>> hw/vfio/iommufd.c | 4 ++-- >>> hw/vfio/listener.c | 4 ++-- >>> hw/vfio/pci.c | 9 +++++++++ >>> 6 files changed, 17 insertions(+), 7 deletions(-) >> >> >> Reviewed-by: Cédric Le Goater <clg@redhat.com> >> >> Thanks, >> >> C. > >I have modified your patch with : > >+/** >+ * vfio_pci_from_vfio_device: Transform from VFIODevice to >+ * VFIOPCIDevice >+ * >+ * This function checks if the given @vbasedev is a VFIO PCI device. >+ * If it is, it returns the containing VFIOPCIDevice. >+ * >+ * @vbasedev: The VFIODevice to transform >+ * >+ * Return: The VFIOPCIDevice on success, NULL on failure. >+ */ > >See https://github.com/legoater/qemu/tree/vfio-10.2. Thanks Cédric, looks better. > >I don't think it's really necessary, as these are internal APIs and >none are documented, but Philippe seems keen on it. I guess he plans >to volunteer to document the rest ;) > >No need to resend a v4. Got it. BRs, Zhenzhong
On 1/8/25 04:35, Zhenzhong Duan wrote:
> Introduce helper vfio_pci_from_vfio_device() to transform from VFIODevice
> to VFIOPCIDevice, also to hide low level VFIO_DEVICE_TYPE_PCI type check.
>
> Suggested-by: Cédric Le Goater <clg@redhat.com>
> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
> ---
> v3: add one line comment to the helper
> v2: move helper to hw/vfio/pci.[hc]
> rename with vfio_pci_ prefix
>
> hw/vfio/pci.h | 1 +
> hw/vfio/container.c | 4 ++--
> hw/vfio/device.c | 2 +-
> hw/vfio/iommufd.c | 4 ++--
> hw/vfio/listener.c | 4 ++--
> hw/vfio/pci.c | 9 +++++++++
> 6 files changed, 17 insertions(+), 7 deletions(-)
>
> diff --git a/hw/vfio/pci.h b/hw/vfio/pci.h
> index 81465a8214..53842cb149 100644
> --- a/hw/vfio/pci.h
> +++ b/hw/vfio/pci.h
> @@ -219,6 +219,7 @@ void vfio_pci_write_config(PCIDevice *pdev,
> uint64_t vfio_vga_read(void *opaque, hwaddr addr, unsigned size);
> void vfio_vga_write(void *opaque, hwaddr addr, uint64_t data, unsigned size);
>
[*]
> +VFIOPCIDevice *vfio_pci_from_vfio_device(VFIODevice *vbasedev);
> void vfio_sub_page_bar_update_mappings(VFIOPCIDevice *vdev);
> bool vfio_opt_rom_in_denylist(VFIOPCIDevice *vdev);
> bool vfio_config_quirk_setup(VFIOPCIDevice *vdev, Error **errp);
Quoting https://lore.kernel.org/qemu-devel/87bjpl25e6.fsf@draig.linaro.org/:
Generally APIs to the rest of QEMU should be documented in the
headers. Comments on individual functions or internal details are
fine to live in the C files.
> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
> index 4fa692c1a3..85761544ba 100644
> --- a/hw/vfio/pci.c
> +++ b/hw/vfio/pci.c
> @@ -2824,6 +2824,15 @@ static int vfio_pci_load_config(VFIODevice *vbasedev, QEMUFile *f)
> return ret;
> }
>
> +/* Transform from VFIODevice to VFIOPCIDevice. Return NULL if fails. */
So this comment preferably goes in [*]. Otherwise,
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> +VFIOPCIDevice *vfio_pci_from_vfio_device(VFIODevice *vbasedev)
> +{
> + if (vbasedev && vbasedev->type == VFIO_DEVICE_TYPE_PCI) {
> + return container_of(vbasedev, VFIOPCIDevice, vbasedev);
> + }
> + return NULL;
> +}
>-----Original Message-----
>From: Philippe Mathieu-Daudé <philmd@linaro.org>
>Subject: Re: [PATCH v3] vfio: Introduce helper vfio_pci_from_vfio_device()
>
>On 1/8/25 04:35, Zhenzhong Duan wrote:
>> Introduce helper vfio_pci_from_vfio_device() to transform from VFIODevice
>> to VFIOPCIDevice, also to hide low level VFIO_DEVICE_TYPE_PCI type
>check.
>>
>> Suggested-by: Cédric Le Goater <clg@redhat.com>
>> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
>> ---
>> v3: add one line comment to the helper
>> v2: move helper to hw/vfio/pci.[hc]
>> rename with vfio_pci_ prefix
>>
>> hw/vfio/pci.h | 1 +
>> hw/vfio/container.c | 4 ++--
>> hw/vfio/device.c | 2 +-
>> hw/vfio/iommufd.c | 4 ++--
>> hw/vfio/listener.c | 4 ++--
>> hw/vfio/pci.c | 9 +++++++++
>> 6 files changed, 17 insertions(+), 7 deletions(-)
>>
>> diff --git a/hw/vfio/pci.h b/hw/vfio/pci.h
>> index 81465a8214..53842cb149 100644
>> --- a/hw/vfio/pci.h
>> +++ b/hw/vfio/pci.h
>> @@ -219,6 +219,7 @@ void vfio_pci_write_config(PCIDevice *pdev,
>> uint64_t vfio_vga_read(void *opaque, hwaddr addr, unsigned size);
>> void vfio_vga_write(void *opaque, hwaddr addr, uint64_t data, unsigned
>size);
>>
>
>[*]
>
>> +VFIOPCIDevice *vfio_pci_from_vfio_device(VFIODevice *vbasedev);
>> void vfio_sub_page_bar_update_mappings(VFIOPCIDevice *vdev);
>> bool vfio_opt_rom_in_denylist(VFIOPCIDevice *vdev);
>> bool vfio_config_quirk_setup(VFIOPCIDevice *vdev, Error **errp);
>
>Quoting
>https://lore.kernel.org/qemu-devel/87bjpl25e6.fsf@draig.linaro.org/:
>
> Generally APIs to the rest of QEMU should be documented in the
> headers. Comments on individual functions or internal details are
> fine to live in the C files.
I see, thanks
>
>> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
>> index 4fa692c1a3..85761544ba 100644
>> --- a/hw/vfio/pci.c
>> +++ b/hw/vfio/pci.c
>> @@ -2824,6 +2824,15 @@ static int vfio_pci_load_config(VFIODevice
>*vbasedev, QEMUFile *f)
>> return ret;
>> }
>>
>> +/* Transform from VFIODevice to VFIOPCIDevice. Return NULL if fails. */
>
>So this comment preferably goes in [*]. Otherwise,
FYI, Cedric helped to make the suggested change, so I'll not send v4.
https://github.com/legoater/qemu/commit/d0ec15af9a5c0a8a9c1a7a71903eb4e00cae71b1
BRs,
Zhenzhong
>
>Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>
>> +VFIOPCIDevice *vfio_pci_from_vfio_device(VFIODevice *vbasedev)
>> +{
>> + if (vbasedev && vbasedev->type == VFIO_DEVICE_TYPE_PCI) {
>> + return container_of(vbasedev, VFIOPCIDevice, vbasedev);
>> + }
>> + return NULL;
>> +}
© 2016 - 2025 Red Hat, Inc.