[PATCH v2 0/3] Enable ESRTPS and simplify caching-mode=on check

[PATCH v2 0/3] Enable ESRTPS and simplify caching-mode=on check

[Zhenzhong Duan]

Hi

Patch1 enable ESRTPS to avoid three global invalidation request in guest
kernel.
Patch2 rework caching mode check with simpler code for VFIO device.
Patch3 fix a wrong parameter passing, I didn't add Fixed-by as they
       belong to many different commits

Thanks
Zhenzhong

Changelog:
v2:
- restore the original code for VDPA, patch2 only take effect for VFIO device
- add patch3 to fix a parameter passing bug

Zhenzhong Duan (3):
  intel_iommu: Enable Enhanced Set Root Table Pointer Support (ESRTPS)
  intel_iommu: Simplify caching mode check with VFIO device
  pci: Fix wrong parameter passing to pci_device_get_iommu_bus_devfn()

 hw/i386/intel_iommu_internal.h |  1 +
 hw/i386/intel_iommu.c          | 42 ++++++----------------------------
 hw/i386/pc.c                   | 20 ----------------
 hw/pci/pci.c                   | 18 +++++++--------
 4 files changed, 16 insertions(+), 65 deletions(-)


base-commit: 5ca676a4c78f14d01e4720eb6de786cf7f5b751d
-- 
2.47.1

[PATCH v2 1/3] intel_iommu: Enable Enhanced Set Root Table Pointer Support (ESRTPS)

[Zhenzhong Duan]

According to VTD spec rev 4.1 section 6.6:
"For implementations reporting the Enhanced Set Root Table Pointer Support
(ESRTPS) field as Clear, on a 'Set Root Table Pointer' operation, software
must perform a global invalidate of the context cache, PASID-cache (if
applicable), and IOTLB, in that order. This is required to ensure hardware
references only the remapping structures referenced by the new root table
pointer and not stale cached entries.

For implementations reporting the Enhanced Set Root Table Pointer Support
(ESRTPS) field as Set, as part of 'Set Root Table Pointer' operation,
hardware performs global invalidation on all DMA remapping translation
caches and hence software is not required to perform additional
invalidations"

We already implemented ESRTPS capability in vtd_handle_gcmd_srtp() by
calling vtd_reset_caches(), just set ESRTPS in DMAR_CAP_REG to avoid
unnecessary global invalidation requests of context, PASID-cache and
IOTLB from guest.

This change doesn't impact migration as the content of DMAR_CAP_REG is
migrated too.

Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
Reviewed-by: Clément Mathieu--Drif <clement.mathieu--drif@eviden.com>
---
 hw/i386/intel_iommu_internal.h | 1 +
 hw/i386/intel_iommu.c          | 2 +-
 2 files changed, 2 insertions(+), 1 deletion(-)

diff --git a/hw/i386/intel_iommu_internal.h b/hw/i386/intel_iommu_internal.h
index 360e937989..5dd92d388d 100644
--- a/hw/i386/intel_iommu_internal.h
+++ b/hw/i386/intel_iommu_internal.h
@@ -214,6 +214,7 @@
 #define VTD_CAP_DRAIN_WRITE         (1ULL << 54)
 #define VTD_CAP_DRAIN_READ          (1ULL << 55)
 #define VTD_CAP_FS1GP               (1ULL << 56)
+#define VTD_CAP_ESRTPS              (1ULL << 63)
 #define VTD_CAP_DRAIN               (VTD_CAP_DRAIN_READ | VTD_CAP_DRAIN_WRITE)
 #define VTD_CAP_CM                  (1ULL << 7)
 #define VTD_PASID_ID_SHIFT          20
diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
index 83c5e44413..f04300022e 100644
--- a/hw/i386/intel_iommu.c
+++ b/hw/i386/intel_iommu.c
@@ -4549,7 +4549,7 @@ static void vtd_cap_init(IntelIOMMUState *s)
 
     s->cap = VTD_CAP_FRO | VTD_CAP_NFR | VTD_CAP_ND |
              VTD_CAP_MAMV | VTD_CAP_PSI | VTD_CAP_SLLPS |
-             VTD_CAP_MGAW(s->aw_bits);
+             VTD_CAP_ESRTPS | VTD_CAP_MGAW(s->aw_bits);
     if (s->dma_drain) {
         s->cap |= VTD_CAP_DRAIN;
     }
-- 
2.47.1

[PATCH v2 2/3] intel_iommu: Simplify caching mode check with VFIO device

[Zhenzhong Duan]

In early days, we have different tricks to ensure caching-mode=on with VFIO
device:

28cf553afe ("intel_iommu: Sanity check vfio-pci config on machine init done")
c6cbc29d36 ("pc/q35: Disallow vfio-pci hotplug without VT-d caching mode")

There is also a patch of same purpose but for VDPA device:

b8d78277c0 ("intel-iommu: fail MAP notifier without caching mode")

Because without caching mode, MAP notifier won't work correctly since guest
won't send IOTLB update event when it establishes new mappings in the I/O page
tables.

Now with host IOMMU device interface between VFIO and vIOMMU, we can simplify
first two commits above with a small check in set_iommu_device(). This also
works for future IOMMUFD backed VDPA implementation which may also need caching
mode on. But for legacy VDPA we still need commit b8d78277c0 as it doesn't
use host IOMMU device interface.

For coldplug VFIO device:

  qemu-system-x86_64: -device vfio-pci,host=0000:3b:00.0,id=hostdev3,bus=root0,iommufd=iommufd0: vfio 0000:3b:00.0: Failed to set vIOMMU: Device assignment is not allowed without enabling caching-mode=on for Intel IOMMU.

For hotplug VFIO device:

  if "iommu=off" is configured in guest,
    Error: vfio 0000:3b:00.0: Failed to set vIOMMU: Device assignment is not allowed without enabling caching-mode=on for Intel IOMMU.
  else
    Error: vfio 0000:3b:00.0: memory listener initialization failed: Region vtd-00.0-dmar: device 01.00.0 requires caching mode: Operation not supported

The specialty for hotplug is due to the check in commit b8d78277c0 happen before
the check in set_iommu_device.

Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
---
 hw/i386/intel_iommu.c | 40 ++++++----------------------------------
 hw/i386/pc.c          | 20 --------------------
 2 files changed, 6 insertions(+), 54 deletions(-)

diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
index f04300022e..c634121514 100644
--- a/hw/i386/intel_iommu.c
+++ b/hw/i386/intel_iommu.c
@@ -85,13 +85,6 @@ struct vtd_iotlb_key {
 static void vtd_address_space_refresh_all(IntelIOMMUState *s);
 static void vtd_address_space_unmap(VTDAddressSpace *as, IOMMUNotifier *n);
 
-static void vtd_panic_require_caching_mode(void)
-{
-    error_report("We need to set caching-mode=on for intel-iommu to enable "
-                 "device assignment with IOMMU protection.");
-    exit(1);
-}
-
 static void vtd_define_quad(IntelIOMMUState *s, hwaddr addr, uint64_t val,
                             uint64_t wmask, uint64_t w1cmask)
 {
@@ -4378,6 +4371,12 @@ static bool vtd_dev_set_iommu_device(PCIBus *bus, void *opaque, int devfn,
 
     assert(hiod);
 
+    if (!s->caching_mode) {
+        error_setg(errp, "Device assignment is not allowed without enabling "
+                   "caching-mode=on for Intel IOMMU.");
+        return false;
+    }
+
     vtd_iommu_lock(s);
 
     if (g_hash_table_lookup(s->vtd_host_iommu_dev, &key)) {
@@ -4910,32 +4909,6 @@ static bool vtd_decide_config(IntelIOMMUState *s, Error **errp)
     return true;
 }
 
-static int vtd_machine_done_notify_one(Object *child, void *unused)
-{
-    IntelIOMMUState *iommu = INTEL_IOMMU_DEVICE(x86_iommu_get_default());
-
-    /*
-     * We hard-coded here because vfio-pci is the only special case
-     * here.  Let's be more elegant in the future when we can, but so
-     * far there seems to be no better way.
-     */
-    if (object_dynamic_cast(child, "vfio-pci") && !iommu->caching_mode) {
-        vtd_panic_require_caching_mode();
-    }
-
-    return 0;
-}
-
-static void vtd_machine_done_hook(Notifier *notifier, void *unused)
-{
-    object_child_foreach_recursive(object_get_root(),
-                                   vtd_machine_done_notify_one, NULL);
-}
-
-static Notifier vtd_machine_done_notify = {
-    .notify = vtd_machine_done_hook,
-};
-
 static void vtd_realize(DeviceState *dev, Error **errp)
 {
     MachineState *ms = MACHINE(qdev_get_machine());
@@ -4990,7 +4963,6 @@ static void vtd_realize(DeviceState *dev, Error **errp)
     pci_setup_iommu(bus, &vtd_iommu_ops, dev);
     /* Pseudo address space under root PCI bus. */
     x86ms->ioapic_as = vtd_host_dma_iommu(bus, s, Q35_PSEUDO_DEVFN_IOAPIC);
-    qemu_add_machine_init_done_notifier(&vtd_machine_done_notify);
 }
 
 static void vtd_class_init(ObjectClass *klass, const void *data)
diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index bc048a6d13..01cd9a67db 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -1720,25 +1720,6 @@ static void pc_machine_wakeup(MachineState *machine)
     cpu_synchronize_all_post_reset();
 }
 
-static bool pc_hotplug_allowed(MachineState *ms, DeviceState *dev, Error **errp)
-{
-    X86IOMMUState *iommu = x86_iommu_get_default();
-    IntelIOMMUState *intel_iommu;
-
-    if (iommu &&
-        object_dynamic_cast((Object *)iommu, TYPE_INTEL_IOMMU_DEVICE) &&
-        object_dynamic_cast((Object *)dev, "vfio-pci")) {
-        intel_iommu = INTEL_IOMMU_DEVICE(iommu);
-        if (!intel_iommu->caching_mode) {
-            error_setg(errp, "Device assignment is not allowed without "
-                       "enabling caching-mode=on for Intel IOMMU.");
-            return false;
-        }
-    }
-
-    return true;
-}
-
 static void pc_machine_class_init(ObjectClass *oc, const void *data)
 {
     MachineClass *mc = MACHINE_CLASS(oc);
@@ -1758,7 +1739,6 @@ static void pc_machine_class_init(ObjectClass *oc, const void *data)
     x86mc->apic_xrupt_override = true;
     assert(!mc->get_hotplug_handler);
     mc->get_hotplug_handler = pc_get_hotplug_handler;
-    mc->hotplug_allowed = pc_hotplug_allowed;
     mc->auto_enable_numa_with_memhp = true;
     mc->auto_enable_numa_with_memdev = true;
     mc->has_hotpluggable_cpus = true;
-- 
2.47.1

Re: [PATCH v2 2/3] intel_iommu: Simplify caching mode check with VFIO device

[CLEMENT MATHIEU--DRIF]

Hi zhenzhong,

Thanks for the respin.  
Just a few comments in the commit message.

On Sun, 2025-09-28 at 23:42 -0400, Zhenzhong Duan wrote:
> In early days, we have different tricks to ensure caching-mode=on with VFIO  

s/have/had

> device:
> 
> 28cf553afe ("intel_iommu: Sanity check vfio-pci config on machine init done")  
> c6cbc29d36 ("pc/q35: Disallow vfio-pci hotplug without VT-d caching mode")
> 
> There is also a patch of same purpose but for VDPA device:

s/of same/with the same

> 
> b8d78277c0 ("intel-iommu: fail MAP notifier without caching mode")
> 
> Because without caching mode, MAP notifier won't work correctly since guest  
> won't send IOTLB update event when it establishes new mappings in the I/O page  
> tables.
> 
> Now with host IOMMU device interface between VFIO and vIOMMU, we can simplify  
> first two commits above with a small check in set_iommu_device(). This also  
> works for future IOMMUFD backed VDPA implementation which may also need caching  
> mode on. But for legacy VDPA we still need commit b8d78277c0 as it doesn't  
> use host IOMMU device interface.

s/use host/use the host

> 
> For coldplug VFIO device:
> 
>   qemu-system-x86_64: -device vfio-pci,host=0000:3b:00.0,id=hostdev3,bus=root0,iommufd=iommufd0: vfio 0000:3b:00.0: Failed to set vIOMMU: Device assignment is not allowed without enabling caching-mode=on for Intel IOMMU.
> 
> For hotplug VFIO device:
> 
>   if "iommu=off" is configured in guest,  
>     Error: vfio 0000:3b:00.0: Failed to set vIOMMU: Device assignment is not allowed without enabling caching-mode=on for Intel IOMMU.  
>   else  
>     Error: vfio 0000:3b:00.0: memory listener initialization failed: Region vtd-00.0-dmar: device 01.00.0 requires caching mode: Operation not supported
> 
> The specialty for hotplug is due to the check in commit b8d78277c0 happen before  
> the check in set_iommu_device.
> 
> Signed-off-by: Zhenzhong Duan <[zhenzhong.duan@intel.com](mailto:zhenzhong.duan@intel.com)>  
> ---  
>  hw/i386/intel_iommu.c | 40 ++++++----------------------------------  
>  hw/i386/pc.c          | 20 --------------------  
>  2 files changed, 6 insertions(+), 54 deletions(-)
> 
> diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c  
> index f04300022e..c634121514 100644  
> --- a/hw/i386/intel_iommu.c  
> +++ b/hw/i386/intel_iommu.c  
> @@ -85,13 +85,6 @@ struct vtd_iotlb_key {  
>  static void vtd_address_space_refresh_all(IntelIOMMUState *s);  
>  static void vtd_address_space_unmap(VTDAddressSpace *as, IOMMUNotifier *n);  
>    
> -static void vtd_panic_require_caching_mode(void)  
> -{  
> -    error_report("We need to set caching-mode=on for intel-iommu to enable "  
> -                 "device assignment with IOMMU protection.");  
> -    exit(1);  
> -}  
> -  
>  static void vtd_define_quad(IntelIOMMUState *s, hwaddr addr, uint64_t val,  
>                              uint64_t wmask, uint64_t w1cmask)  
>  {  
> @@ -4378,6 +4371,12 @@ static bool vtd_dev_set_iommu_device(PCIBus *bus, void *opaque, int devfn,  
>    
>      assert(hiod);  
>    
> +    if (!s->caching_mode) {  
> +        error_setg(errp, "Device assignment is not allowed without enabling "  
> +                   "caching-mode=on for Intel IOMMU.");  
> +        return false;  
> +    }  
> +  
>      vtd_iommu_lock(s);  
>    
>      if (g_hash_table_lookup(s->vtd_host_iommu_dev, &key)) {  
> @@ -4910,32 +4909,6 @@ static bool vtd_decide_config(IntelIOMMUState *s, Error **errp)  
>      return true;  
>  }  
>    
> -static int vtd_machine_done_notify_one(Object *child, void *unused)  
> -{  
> -    IntelIOMMUState *iommu = INTEL_IOMMU_DEVICE(x86_iommu_get_default());  
> -  
> -    /*  
> -     * We hard-coded here because vfio-pci is the only special case  
> -     * here.  Let's be more elegant in the future when we can, but so  
> -     * far there seems to be no better way.  
> -     */  
> -    if (object_dynamic_cast(child, "vfio-pci") && !iommu->caching_mode) {  
> -        vtd_panic_require_caching_mode();  
> -    }  
> -  
> -    return 0;  
> -}  
> -  
> -static void vtd_machine_done_hook(Notifier *notifier, void *unused)  
> -{  
> -    object_child_foreach_recursive(object_get_root(),  
> -                                   vtd_machine_done_notify_one, NULL);  
> -}  
> -  
> -static Notifier vtd_machine_done_notify = {  
> -    .notify = vtd_machine_done_hook,  
> -};  
> -  
>  static void vtd_realize(DeviceState *dev, Error **errp)  
>  {  
>      MachineState *ms = MACHINE(qdev_get_machine());  
> @@ -4990,7 +4963,6 @@ static void vtd_realize(DeviceState *dev, Error **errp)  
>      pci_setup_iommu(bus, &vtd_iommu_ops, dev);  
>      /* Pseudo address space under root PCI bus. */  
>      x86ms->ioapic_as = vtd_host_dma_iommu(bus, s, Q35_PSEUDO_DEVFN_IOAPIC);  
> -    qemu_add_machine_init_done_notifier(&vtd_machine_done_notify);  
>  }  
>    
>  static void vtd_class_init(ObjectClass *klass, const void *data)  
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c  
> index bc048a6d13..01cd9a67db 100644  
> --- a/hw/i386/pc.c  
> +++ b/hw/i386/pc.c  
> @@ -1720,25 +1720,6 @@ static void pc_machine_wakeup(MachineState *machine)  
>      cpu_synchronize_all_post_reset();  
>  }  
>    
> -static bool pc_hotplug_allowed(MachineState *ms, DeviceState *dev, Error **errp)  
> -{  
> -    X86IOMMUState *iommu = x86_iommu_get_default();  
> -    IntelIOMMUState *intel_iommu;  
> -  
> -    if (iommu &&  
> -        object_dynamic_cast((Object *)iommu, TYPE_INTEL_IOMMU_DEVICE) &&  
> -        object_dynamic_cast((Object *)dev, "vfio-pci")) {  
> -        intel_iommu = INTEL_IOMMU_DEVICE(iommu);  
> -        if (!intel_iommu->caching_mode) {  
> -            error_setg(errp, "Device assignment is not allowed without "  
> -                       "enabling caching-mode=on for Intel IOMMU.");  
> -            return false;  
> -        }  
> -    }  
> -  
> -    return true;  
> -}  
> -  
>  static void pc_machine_class_init(ObjectClass *oc, const void *data)  
>  {  
>      MachineClass *mc = MACHINE_CLASS(oc);  
> @@ -1758,7 +1739,6 @@ static void pc_machine_class_init(ObjectClass *oc, const void *data)  
>      x86mc->apic_xrupt_override = true;  
>      assert(!mc->get_hotplug_handler);  
>      mc->get_hotplug_handler = pc_get_hotplug_handler;  
> -    mc->hotplug_allowed = pc_hotplug_allowed;  
>      mc->auto_enable_numa_with_memhp = true;  
>      mc->auto_enable_numa_with_memdev = true;  
>      mc->has_hotpluggable_cpus = true;

RE: [PATCH v2 2/3] intel_iommu: Simplify caching mode check with VFIO device

[Duan, Zhenzhong]

>-----Original Message-----
>From: CLEMENT MATHIEU--DRIF <clement.mathieu--drif@eviden.com>
>Subject: Re: [PATCH v2 2/3] intel_iommu: Simplify caching mode check with
>VFIO device
>
>Hi zhenzhong,
>
>Thanks for the respin.
>Just a few comments in the commit message.

Hi Clement,

I see Michael has helped on the suggested changes, thanks Michael.

BRs,
Zhenzhong

[PATCH v2 3/3] pci: Fix wrong parameter passing to pci_device_get_iommu_bus_devfn()

[Zhenzhong Duan]

The 2nd parameter of pci_device_get_iommu_bus_devfn() about root PCIBus
backed by an IOMMU for the PCI device, the 3rd is about aliased PCIBus
of the PCI device.

Meanwhile the 3rd and 4th parameters are optional, pass NULL if they
are not needed.

Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
---
 hw/pci/pci.c | 18 ++++++++----------
 1 file changed, 8 insertions(+), 10 deletions(-)

diff --git a/hw/pci/pci.c b/hw/pci/pci.c
index c3df9d6656..d5ed89aab7 100644
--- a/hw/pci/pci.c
+++ b/hw/pci/pci.c
@@ -2967,7 +2967,7 @@ int pci_iommu_init_iotlb_notifier(PCIDevice *dev, IOMMUNotifier *n,
     PCIBus *iommu_bus;
     int devfn;
 
-    pci_device_get_iommu_bus_devfn(dev, &bus, &iommu_bus, &devfn);
+    pci_device_get_iommu_bus_devfn(dev, &iommu_bus, &bus, &devfn);
     if (iommu_bus && iommu_bus->iommu_ops->init_iotlb_notifier) {
         iommu_bus->iommu_ops->init_iotlb_notifier(bus, iommu_bus->iommu_opaque,
                                                   devfn, n, fn, opaque);
@@ -3025,7 +3025,7 @@ int pci_pri_request_page(PCIDevice *dev, uint32_t pasid, bool priv_req,
         return -EPERM;
     }
 
-    pci_device_get_iommu_bus_devfn(dev, &bus, &iommu_bus, &devfn);
+    pci_device_get_iommu_bus_devfn(dev, &iommu_bus, &bus, &devfn);
     if (iommu_bus && iommu_bus->iommu_ops->pri_request_page) {
         return iommu_bus->iommu_ops->pri_request_page(bus,
                                                      iommu_bus->iommu_opaque,
@@ -3049,7 +3049,7 @@ int pci_pri_register_notifier(PCIDevice *dev, uint32_t pasid,
         return -EPERM;
     }
 
-    pci_device_get_iommu_bus_devfn(dev, &bus, &iommu_bus, &devfn);
+    pci_device_get_iommu_bus_devfn(dev, &iommu_bus, &bus, &devfn);
     if (iommu_bus && iommu_bus->iommu_ops->pri_register_notifier) {
         iommu_bus->iommu_ops->pri_register_notifier(bus,
                                                     iommu_bus->iommu_opaque,
@@ -3066,7 +3066,7 @@ void pci_pri_unregister_notifier(PCIDevice *dev, uint32_t pasid)
     PCIBus *iommu_bus;
     int devfn;
 
-    pci_device_get_iommu_bus_devfn(dev, &bus, &iommu_bus, &devfn);
+    pci_device_get_iommu_bus_devfn(dev, &iommu_bus, &bus, &devfn);
     if (iommu_bus && iommu_bus->iommu_ops->pri_unregister_notifier) {
         iommu_bus->iommu_ops->pri_unregister_notifier(bus,
                                                       iommu_bus->iommu_opaque,
@@ -3098,7 +3098,7 @@ ssize_t pci_ats_request_translation(PCIDevice *dev, uint32_t pasid,
         return -EPERM;
     }
 
-    pci_device_get_iommu_bus_devfn(dev, &bus, &iommu_bus, &devfn);
+    pci_device_get_iommu_bus_devfn(dev, &iommu_bus, &bus, &devfn);
     if (iommu_bus && iommu_bus->iommu_ops->ats_request_translation) {
         return iommu_bus->iommu_ops->ats_request_translation(bus,
                                                      iommu_bus->iommu_opaque,
@@ -3122,7 +3122,7 @@ int pci_iommu_register_iotlb_notifier(PCIDevice *dev, uint32_t pasid,
         return -EPERM;
     }
 
-    pci_device_get_iommu_bus_devfn(dev, &bus, &iommu_bus, &devfn);
+    pci_device_get_iommu_bus_devfn(dev, &iommu_bus, &bus, &devfn);
     if (iommu_bus && iommu_bus->iommu_ops->register_iotlb_notifier) {
         iommu_bus->iommu_ops->register_iotlb_notifier(bus,
                                            iommu_bus->iommu_opaque, devfn,
@@ -3144,7 +3144,7 @@ int pci_iommu_unregister_iotlb_notifier(PCIDevice *dev, uint32_t pasid,
         return -EPERM;
     }
 
-    pci_device_get_iommu_bus_devfn(dev, &bus, &iommu_bus, &devfn);
+    pci_device_get_iommu_bus_devfn(dev, &iommu_bus, &bus, &devfn);
     if (iommu_bus && iommu_bus->iommu_ops->unregister_iotlb_notifier) {
         iommu_bus->iommu_ops->unregister_iotlb_notifier(bus,
                                                         iommu_bus->iommu_opaque,
@@ -3158,11 +3158,9 @@ int pci_iommu_unregister_iotlb_notifier(PCIDevice *dev, uint32_t pasid,
 int pci_iommu_get_iotlb_info(PCIDevice *dev, uint8_t *addr_width,
                              uint32_t *min_page_size)
 {
-    PCIBus *bus;
     PCIBus *iommu_bus;
-    int devfn;
 
-    pci_device_get_iommu_bus_devfn(dev, &bus, &iommu_bus, &devfn);
+    pci_device_get_iommu_bus_devfn(dev, &iommu_bus, NULL, NULL);
     if (iommu_bus && iommu_bus->iommu_ops->get_iotlb_info) {
         iommu_bus->iommu_ops->get_iotlb_info(iommu_bus->iommu_opaque,
                                              addr_width, min_page_size);
-- 
2.47.1

Re: [PATCH v2 3/3] pci: Fix wrong parameter passing to pci_device_get_iommu_bus_devfn()

[CLEMENT MATHIEU--DRIF]

Hi Zhenzhong

Uh, good catch O.o

Reviewed-by: Clément Mathieu--Drif <clement.mathieu--drif@eviden.com>

On Sun, 2025-09-28 at 23:42 -0400, Zhenzhong Duan wrote:
> The 2nd parameter of pci_device_get_iommu_bus_devfn() about root PCIBus  
> backed by an IOMMU for the PCI device, the 3rd is about aliased PCIBus  
> of the PCI device.
> 
> Meanwhile the 3rd and 4th parameters are optional, pass NULL if they  
> are not needed.
> 
> Signed-off-by: Zhenzhong Duan <[zhenzhong.duan@intel.com](mailto:zhenzhong.duan@intel.com)>  
> ---  
>  hw/pci/pci.c | 18 ++++++++----------  
>  1 file changed, 8 insertions(+), 10 deletions(-)
> 
> diff --git a/hw/pci/pci.c b/hw/pci/pci.c  
> index c3df9d6656..d5ed89aab7 100644  
> --- a/hw/pci/pci.c  
> +++ b/hw/pci/pci.c  
> @@ -2967,7 +2967,7 @@ int pci_iommu_init_iotlb_notifier(PCIDevice *dev, IOMMUNotifier *n,  
>      PCIBus *iommu_bus;  
>      int devfn;  
>    
> -    pci_device_get_iommu_bus_devfn(dev, &bus, &iommu_bus, &devfn);  
> +    pci_device_get_iommu_bus_devfn(dev, &iommu_bus, &bus, &devfn);  
>      if (iommu_bus && iommu_bus->iommu_ops->init_iotlb_notifier) {  
>          iommu_bus->iommu_ops->init_iotlb_notifier(bus, iommu_bus->iommu_opaque,  
>                                                    devfn, n, fn, opaque);  
> @@ -3025,7 +3025,7 @@ int pci_pri_request_page(PCIDevice *dev, uint32_t pasid, bool priv_req,  
>          return -EPERM;  
>      }  
>    
> -    pci_device_get_iommu_bus_devfn(dev, &bus, &iommu_bus, &devfn);  
> +    pci_device_get_iommu_bus_devfn(dev, &iommu_bus, &bus, &devfn);  
>      if (iommu_bus && iommu_bus->iommu_ops->pri_request_page) {  
>          return iommu_bus->iommu_ops->pri_request_page(bus,  
>                                                       iommu_bus->iommu_opaque,  
> @@ -3049,7 +3049,7 @@ int pci_pri_register_notifier(PCIDevice *dev, uint32_t pasid,  
>          return -EPERM;  
>      }  
>    
> -    pci_device_get_iommu_bus_devfn(dev, &bus, &iommu_bus, &devfn);  
> +    pci_device_get_iommu_bus_devfn(dev, &iommu_bus, &bus, &devfn);  
>      if (iommu_bus && iommu_bus->iommu_ops->pri_register_notifier) {  
>          iommu_bus->iommu_ops->pri_register_notifier(bus,  
>                                                      iommu_bus->iommu_opaque,  
> @@ -3066,7 +3066,7 @@ void pci_pri_unregister_notifier(PCIDevice *dev, uint32_t pasid)  
>      PCIBus *iommu_bus;  
>      int devfn;  
>    
> -    pci_device_get_iommu_bus_devfn(dev, &bus, &iommu_bus, &devfn);  
> +    pci_device_get_iommu_bus_devfn(dev, &iommu_bus, &bus, &devfn);  
>      if (iommu_bus && iommu_bus->iommu_ops->pri_unregister_notifier) {  
>          iommu_bus->iommu_ops->pri_unregister_notifier(bus,  
>                                                        iommu_bus->iommu_opaque,  
> @@ -3098,7 +3098,7 @@ ssize_t pci_ats_request_translation(PCIDevice *dev, uint32_t pasid,  
>          return -EPERM;  
>      }  
>    
> -    pci_device_get_iommu_bus_devfn(dev, &bus, &iommu_bus, &devfn);  
> +    pci_device_get_iommu_bus_devfn(dev, &iommu_bus, &bus, &devfn);  
>      if (iommu_bus && iommu_bus->iommu_ops->ats_request_translation) {  
>          return iommu_bus->iommu_ops->ats_request_translation(bus,  
>                                                       iommu_bus->iommu_opaque,  
> @@ -3122,7 +3122,7 @@ int pci_iommu_register_iotlb_notifier(PCIDevice *dev, uint32_t pasid,  
>          return -EPERM;  
>      }  
>    
> -    pci_device_get_iommu_bus_devfn(dev, &bus, &iommu_bus, &devfn);  
> +    pci_device_get_iommu_bus_devfn(dev, &iommu_bus, &bus, &devfn);  
>      if (iommu_bus && iommu_bus->iommu_ops->register_iotlb_notifier) {  
>          iommu_bus->iommu_ops->register_iotlb_notifier(bus,  
>                                             iommu_bus->iommu_opaque, devfn,  
> @@ -3144,7 +3144,7 @@ int pci_iommu_unregister_iotlb_notifier(PCIDevice *dev, uint32_t pasid,  
>          return -EPERM;  
>      }  
>    
> -    pci_device_get_iommu_bus_devfn(dev, &bus, &iommu_bus, &devfn);  
> +    pci_device_get_iommu_bus_devfn(dev, &iommu_bus, &bus, &devfn);  
>      if (iommu_bus && iommu_bus->iommu_ops->unregister_iotlb_notifier) {  
>          iommu_bus->iommu_ops->unregister_iotlb_notifier(bus,  
>                                                          iommu_bus->iommu_opaque,  
> @@ -3158,11 +3158,9 @@ int pci_iommu_unregister_iotlb_notifier(PCIDevice *dev, uint32_t pasid,  
>  int pci_iommu_get_iotlb_info(PCIDevice *dev, uint8_t *addr_width,  
>                               uint32_t *min_page_size)  
>  {  
> -    PCIBus *bus;  
>      PCIBus *iommu_bus;  
> -    int devfn;  
>    
> -    pci_device_get_iommu_bus_devfn(dev, &bus, &iommu_bus, &devfn);  
> +    pci_device_get_iommu_bus_devfn(dev, &iommu_bus, NULL, NULL);  
>      if (iommu_bus && iommu_bus->iommu_ops->get_iotlb_info) {  
>          iommu_bus->iommu_ops->get_iotlb_info(iommu_bus->iommu_opaque,  
>                                               addr_width, min_page_size);

Re: [PATCH v2 3/3] pci: Fix wrong parameter passing to pci_device_get_iommu_bus_devfn()

[Cédric Le Goater]

On 9/29/25 05:42, Zhenzhong Duan wrote:
> The 2nd parameter of pci_device_get_iommu_bus_devfn() about root PCIBus
> backed by an IOMMU for the PCI device, the 3rd is about aliased PCIBus
> of the PCI device.
> 
> Meanwhile the 3rd and 4th parameters are optional, pass NULL if they
> are not needed.
> 
> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>

The commit log should mention potential consequences of this change.

Will this fix need to be backported ? up to ~9.1


Thanks,

C.



> ---
>   hw/pci/pci.c | 18 ++++++++----------
>   1 file changed, 8 insertions(+), 10 deletions(-)
> 
> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> index c3df9d6656..d5ed89aab7 100644
> --- a/hw/pci/pci.c
> +++ b/hw/pci/pci.c
> @@ -2967,7 +2967,7 @@ int pci_iommu_init_iotlb_notifier(PCIDevice *dev, IOMMUNotifier *n,
>       PCIBus *iommu_bus;
>       int devfn;
>   
> -    pci_device_get_iommu_bus_devfn(dev, &bus, &iommu_bus, &devfn);
> +    pci_device_get_iommu_bus_devfn(dev, &iommu_bus, &bus, &devfn);
>       if (iommu_bus && iommu_bus->iommu_ops->init_iotlb_notifier) {
>           iommu_bus->iommu_ops->init_iotlb_notifier(bus, iommu_bus->iommu_opaque,
>                                                     devfn, n, fn, opaque);
> @@ -3025,7 +3025,7 @@ int pci_pri_request_page(PCIDevice *dev, uint32_t pasid, bool priv_req,
>           return -EPERM;
>       }
>   
> -    pci_device_get_iommu_bus_devfn(dev, &bus, &iommu_bus, &devfn);
> +    pci_device_get_iommu_bus_devfn(dev, &iommu_bus, &bus, &devfn);
>       if (iommu_bus && iommu_bus->iommu_ops->pri_request_page) {
>           return iommu_bus->iommu_ops->pri_request_page(bus,
>                                                        iommu_bus->iommu_opaque,
> @@ -3049,7 +3049,7 @@ int pci_pri_register_notifier(PCIDevice *dev, uint32_t pasid,
>           return -EPERM;
>       }
>   
> -    pci_device_get_iommu_bus_devfn(dev, &bus, &iommu_bus, &devfn);
> +    pci_device_get_iommu_bus_devfn(dev, &iommu_bus, &bus, &devfn);
>       if (iommu_bus && iommu_bus->iommu_ops->pri_register_notifier) {
>           iommu_bus->iommu_ops->pri_register_notifier(bus,
>                                                       iommu_bus->iommu_opaque,
> @@ -3066,7 +3066,7 @@ void pci_pri_unregister_notifier(PCIDevice *dev, uint32_t pasid)
>       PCIBus *iommu_bus;
>       int devfn;
>   
> -    pci_device_get_iommu_bus_devfn(dev, &bus, &iommu_bus, &devfn);
> +    pci_device_get_iommu_bus_devfn(dev, &iommu_bus, &bus, &devfn);
>       if (iommu_bus && iommu_bus->iommu_ops->pri_unregister_notifier) {
>           iommu_bus->iommu_ops->pri_unregister_notifier(bus,
>                                                         iommu_bus->iommu_opaque,
> @@ -3098,7 +3098,7 @@ ssize_t pci_ats_request_translation(PCIDevice *dev, uint32_t pasid,
>           return -EPERM;
>       }
>   
> -    pci_device_get_iommu_bus_devfn(dev, &bus, &iommu_bus, &devfn);
> +    pci_device_get_iommu_bus_devfn(dev, &iommu_bus, &bus, &devfn);
>       if (iommu_bus && iommu_bus->iommu_ops->ats_request_translation) {
>           return iommu_bus->iommu_ops->ats_request_translation(bus,
>                                                        iommu_bus->iommu_opaque,
> @@ -3122,7 +3122,7 @@ int pci_iommu_register_iotlb_notifier(PCIDevice *dev, uint32_t pasid,
>           return -EPERM;
>       }
>   
> -    pci_device_get_iommu_bus_devfn(dev, &bus, &iommu_bus, &devfn);
> +    pci_device_get_iommu_bus_devfn(dev, &iommu_bus, &bus, &devfn);
>       if (iommu_bus && iommu_bus->iommu_ops->register_iotlb_notifier) {
>           iommu_bus->iommu_ops->register_iotlb_notifier(bus,
>                                              iommu_bus->iommu_opaque, devfn,
> @@ -3144,7 +3144,7 @@ int pci_iommu_unregister_iotlb_notifier(PCIDevice *dev, uint32_t pasid,
>           return -EPERM;
>       }
>   
> -    pci_device_get_iommu_bus_devfn(dev, &bus, &iommu_bus, &devfn);
> +    pci_device_get_iommu_bus_devfn(dev, &iommu_bus, &bus, &devfn);
>       if (iommu_bus && iommu_bus->iommu_ops->unregister_iotlb_notifier) {
>           iommu_bus->iommu_ops->unregister_iotlb_notifier(bus,
>                                                           iommu_bus->iommu_opaque,
> @@ -3158,11 +3158,9 @@ int pci_iommu_unregister_iotlb_notifier(PCIDevice *dev, uint32_t pasid,
>   int pci_iommu_get_iotlb_info(PCIDevice *dev, uint8_t *addr_width,
>                                uint32_t *min_page_size)
>   {
> -    PCIBus *bus;
>       PCIBus *iommu_bus;
> -    int devfn;
>   
> -    pci_device_get_iommu_bus_devfn(dev, &bus, &iommu_bus, &devfn);
> +    pci_device_get_iommu_bus_devfn(dev, &iommu_bus, NULL, NULL);
>       if (iommu_bus && iommu_bus->iommu_ops->get_iotlb_info) {
>           iommu_bus->iommu_ops->get_iotlb_info(iommu_bus->iommu_opaque,
>                                                addr_width, min_page_size);

RE: [PATCH v2 3/3] pci: Fix wrong parameter passing to pci_device_get_iommu_bus_devfn()

[Duan, Zhenzhong]

>-----Original Message-----
>From: Cédric Le Goater <clg@redhat.com>
>Subject: Re: [PATCH v2 3/3] pci: Fix wrong parameter passing to
>pci_device_get_iommu_bus_devfn()
>
>On 9/29/25 05:42, Zhenzhong Duan wrote:
>> The 2nd parameter of pci_device_get_iommu_bus_devfn() about root
>PCIBus
>> backed by an IOMMU for the PCI device, the 3rd is about aliased PCIBus
>> of the PCI device.
>>
>> Meanwhile the 3rd and 4th parameters are optional, pass NULL if they
>> are not needed.
>>
>> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
>
>The commit log should mention potential consequences of this change.

Without fix, the callback function may not be called as iommu_ops is set
for iommu_bus rather than bus. Luckily, there is no user of those
functions yet, so no real issue currently.

>
>Will this fix need to be backported ? up to ~9.1

I think no need, Clement could correct me if above explanation is wrong.

Thanks
Zhenzhong

Re: [PATCH v2 3/3] pci: Fix wrong parameter passing to pci_device_get_iommu_bus_devfn()

[CLEMENT MATHIEU--DRIF]

On Wed, 2025-10-08 at 10:15 +0000, Duan, Zhenzhong wrote:
> 
> 
> 
> > -----Original Message-----
> > From: Cédric Le Goater <[clg@redhat.com](mailto:clg@redhat.com)>
> > Subject: Re: [PATCH v2 3/3] pci: Fix wrong parameter passing to
> > pci_device_get_iommu_bus_devfn()
> > 
> > On 9/29/25 05:42, Zhenzhong Duan wrote:
> > 
> > > The 2nd parameter of pci_device_get_iommu_bus_devfn() about root
> > 
> > PCIBus
> > 
> > > backed by an IOMMU for the PCI device, the 3rd is about aliased PCIBus
> > > of the PCI device.
> > > 
> > > Meanwhile the 3rd and 4th parameters are optional, pass NULL if they
> > > are not needed.
> > > 
> > > Signed-off-by: Zhenzhong Duan <[zhenzhong.duan@intel.com](mailto:zhenzhong.duan@intel.com)>
> > 
> > 
> > The commit log should mention potential consequences of this change.
> 
> 
> Without fix, the callback function may not be called as iommu_ops is set  
> for iommu_bus rather than bus. Luckily, there is no user of those  
> functions yet, so no real issue currently.
> 
> 
> 
> > Will this fix need to be backported ? up to ~9.1
> 
> 
> I think no need, Clement could correct me if above explanation is wrong.

That's correct, thanks Zhenzhong

> 
> Thanks  
> Zhenzhong
>