Function iommu_do_pci_domctl() is the main entry for pci-subset
iommu-related domctl-op, and shall be wrapped with CONFIG_MGMT_HYPERCALLS.
Tracking its calling chain, the following functions shall all be wrapped
with CONFIG_MGMT_HYPERCALLS:
- iommu_do_pci_domctl
  - iommu_get_device_group
    - xsm_get_device_group
    - iommu_ops.get_device_group_id
      - amd_iommu_group_id/intel_iommu_group_id
  - device_assigned
  - xsm_assign_device
  - assign_device
    - iommu_ops.assign_device
      - intel_iommu_assign_device/amd_iommu_assign_device
  - xsm_deassign_device
  - deassign_device
    - iommu_ops.reassign_device
      - reassign_device_ownership/reassign_device
Otherwise all the functions will become unreachable when MGMT_HYPERCALLS=n,
and hence violating Misra rule 2.1

Signed-off-by: Penny Zheng <Penny.Zheng@amd.com>
---
v1 -> v2:
- adapt to changes of "unify DOMCTL to MGMT_HYPERCALLS"
- wrap XEN_DOMCTL_assign_device{test_assign_device,deassign_device,
 get_device_group}-case transiently
---
v2 -> v3:
- make PCI_PASSTHROUGH(, then HAS_VPCI_GUEST_SUPPORT) depend on MGMT_HYPERCALLS
- add wrapping for iommu_remove_dt_device/iommu_dt_device_is_assigned_locked/
arm_smmu_detach_dev/arm_smmu_domain_remove_master
- fold commit
"xen/xsm: wrap xsm-iommu-related functions with CONFIG_MGMT_HYPERCALLS" in
- fix overly long #ifdef
- add missing wrapping in xsm/dummy.h
- address "violating Misra rule 2.1" in commit message
- remove transient wrapping of
XEN_DOMCTL_assign_device{test_assign_device,deassign_device,get_device_group}-case
---
v3 -> v4:
- move codes to wrap with a single #ifdef
- split into PCI related subset and DT subset
---
 xen/drivers/passthrough/amd/pci_amd_iommu.c | 20 ++++----
 xen/drivers/passthrough/pci.c               | 52 +++++++++++----------
 xen/drivers/passthrough/vtd/iommu.c         |  6 ++-
 xen/include/xsm/dummy.h                     |  6 ++-
 xen/include/xsm/xsm.h                       | 12 +++--
 xen/xsm/dummy.c                             |  6 ++-
 xen/xsm/flask/hooks.c                       | 12 +++--
 7 files changed, 68 insertions(+), 46 deletions(-)

diff --git a/xen/drivers/passthrough/amd/pci_amd_iommu.c b/xen/drivers/passthrough/amd/pci_amd_iommu.c
index 3a14770855..576b36af92 100644
--- a/xen/drivers/passthrough/amd/pci_amd_iommu.c
+++ b/xen/drivers/passthrough/amd/pci_amd_iommu.c
@@ -461,6 +461,7 @@ static void amd_iommu_disable_domain_device(const struct domain *domain,
         spin_unlock_irqrestore(&iommu->lock, flags);
 }
 
+#ifdef CONFIG_MGMT_HYPERCALLS
 static int cf_check reassign_device(
     struct domain *source, struct domain *target, u8 devfn,
     struct pci_dev *pdev)
@@ -551,6 +552,14 @@ static int cf_check amd_iommu_assign_device(
     return rc;
 }
 
+static int cf_check amd_iommu_group_id(u16 seg, u8 bus, u8 devfn)
+{
+    unsigned int bdf = PCI_BDF(bus, devfn);
+
+    return (bdf < ivrs_bdf_entries) ? get_dma_requestor_id(seg, bdf) : bdf;
+}
+#endif /* CONFIG_MGMT_HYPERCALLS */
+
 static void cf_check amd_iommu_clear_root_pgtable(struct domain *d)
 {
     struct domain_iommu *hd = dom_iommu(d);
@@ -698,13 +707,6 @@ static int cf_check amd_iommu_remove_device(u8 devfn, struct pci_dev *pdev)
     return 0;
 }
 
-static int cf_check amd_iommu_group_id(u16 seg, u8 bus, u8 devfn)
-{
-    unsigned int bdf = PCI_BDF(bus, devfn);
-
-    return (bdf < ivrs_bdf_entries) ? get_dma_requestor_id(seg, bdf) : bdf;
-}
-
 #include <asm/io_apic.h>
 
 static void amd_dump_page_table_level(struct page_info *pg, int level,
@@ -772,14 +774,16 @@ static const struct iommu_ops __initconst_cf_clobber _iommu_ops = {
     .quarantine_init = amd_iommu_quarantine_init,
     .add_device = amd_iommu_add_device,
     .remove_device = amd_iommu_remove_device,
-    .assign_device  = amd_iommu_assign_device,
     .teardown = amd_iommu_domain_destroy,
     .clear_root_pgtable = amd_iommu_clear_root_pgtable,
     .map_page = amd_iommu_map_page,
     .unmap_page = amd_iommu_unmap_page,
     .iotlb_flush = amd_iommu_flush_iotlb_pages,
+#ifdef CONFIG_MGMT_HYPERCALLS
+    .assign_device  = amd_iommu_assign_device,
     .reassign_device = reassign_device,
     .get_device_group_id = amd_iommu_group_id,
+#endif
     .enable_x2apic = iov_enable_xt,
     .update_ire_from_apic = amd_iommu_ioapic_update_ire,
     .update_ire_from_msi = amd_iommu_msi_msg_update_ire,
diff --git a/xen/drivers/passthrough/pci.c b/xen/drivers/passthrough/pci.c
index 422e45f5a6..8ab229bfe7 100644
--- a/xen/drivers/passthrough/pci.c
+++ b/xen/drivers/passthrough/pci.c
@@ -878,6 +878,7 @@ int pci_remove_device(u16 seg, u8 bus, u8 devfn)
     return ret;
 }
 
+#ifdef CONFIG_MGMT_HYPERCALLS
 /* Caller should hold the pcidevs_lock */
 static int deassign_device(struct domain *d, uint16_t seg, uint8_t bus,
                            uint8_t devfn)
@@ -946,7 +947,6 @@ static int deassign_device(struct domain *d, uint16_t seg, uint8_t bus,
     return ret;
 }
 
-#ifdef CONFIG_MGMT_HYPERCALLS
 int pci_release_devices(struct domain *d)
 {
     int combined_ret;
@@ -1484,6 +1484,7 @@ static int iommu_remove_device(struct pci_dev *pdev)
     return iommu_call(hd->platform_ops, remove_device, devfn, pci_to_dev(pdev));
 }
 
+#ifdef CONFIG_MGMT_HYPERCALLS
 static int device_assigned(u16 seg, u8 bus, u8 devfn)
 {
     struct pci_dev *pdev;
@@ -1648,30 +1649,6 @@ static int iommu_get_device_group(
     return i;
 }
 
-void iommu_dev_iotlb_flush_timeout(struct domain *d, struct pci_dev *pdev)
-{
-    pcidevs_lock();
-
-    disable_ats_device(pdev);
-
-    ASSERT(pdev->domain);
-    if ( d != pdev->domain )
-    {
-        pcidevs_unlock();
-        return;
-    }
-
-    pdev->broken = true;
-
-    if ( !d->is_shutting_down && printk_ratelimit() )
-        printk(XENLOG_ERR "dom%d: ATS device %pp flush failed\n",
-               d->domain_id, &pdev->sbdf);
-    if ( !is_hardware_domain(d) )
-        domain_crash(d);
-
-    pcidevs_unlock();
-}
-
 int iommu_do_pci_domctl(
     struct xen_domctl *domctl, struct domain *d,
     XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
@@ -1805,6 +1782,31 @@ int iommu_do_pci_domctl(
 
     return ret;
 }
+#endif /* CONFIG_MGMT_HYPERCALLS */
+
+void iommu_dev_iotlb_flush_timeout(struct domain *d, struct pci_dev *pdev)
+{
+    pcidevs_lock();
+
+    disable_ats_device(pdev);
+
+    ASSERT(pdev->domain);
+    if ( d != pdev->domain )
+    {
+        pcidevs_unlock();
+        return;
+    }
+
+    pdev->broken = true;
+
+    if ( !d->is_shutting_down && printk_ratelimit() )
+        printk(XENLOG_ERR "dom%d: ATS device %pp flush failed\n",
+               d->domain_id, &pdev->sbdf);
+    if ( !is_hardware_domain(d) )
+        domain_crash(d);
+
+    pcidevs_unlock();
+}
 
 struct segment_iter {
     int (*handler)(struct pci_dev *pdev, void *arg);
diff --git a/xen/drivers/passthrough/vtd/iommu.c b/xen/drivers/passthrough/vtd/iommu.c
index 90f36ac22b..ad2e657bca 100644
--- a/xen/drivers/passthrough/vtd/iommu.c
+++ b/xen/drivers/passthrough/vtd/iommu.c
@@ -2742,6 +2742,7 @@ static int __init cf_check vtd_setup(void)
     return ret;
 }
 
+#ifdef CONFIG_MGMT_HYPERCALLS
 static int cf_check reassign_device_ownership(
     struct domain *source,
     struct domain *target,
@@ -2937,6 +2938,7 @@ static int cf_check intel_iommu_group_id(u16 seg, u8 bus, u8 devfn)
 
     return PCI_BDF(bus, devfn);
 }
+#endif /* CONFIG_MGMT_HYPERCALLS */
 
 static int __must_check cf_check vtd_suspend(void)
 {
@@ -3245,14 +3247,16 @@ static const struct iommu_ops __initconst_cf_clobber vtd_ops = {
     .add_device = intel_iommu_add_device,
     .enable_device = intel_iommu_enable_device,
     .remove_device = intel_iommu_remove_device,
-    .assign_device  = intel_iommu_assign_device,
     .teardown = iommu_domain_teardown,
     .clear_root_pgtable = iommu_clear_root_pgtable,
     .map_page = intel_iommu_map_page,
     .unmap_page = intel_iommu_unmap_page,
     .lookup_page = intel_iommu_lookup_page,
+#ifdef CONFIG_MGMT_HYPERCALLS
+    .assign_device  = intel_iommu_assign_device,
     .reassign_device = reassign_device_ownership,
     .get_device_group_id = intel_iommu_group_id,
+#endif
     .enable_x2apic = intel_iommu_enable_eim,
     .disable_x2apic = intel_iommu_disable_eim,
     .update_ire_from_apic = io_apic_write_remap_rte,
diff --git a/xen/include/xsm/dummy.h b/xen/include/xsm/dummy.h
index a598d74f1f..83972d36b7 100644
--- a/xen/include/xsm/dummy.h
+++ b/xen/include/xsm/dummy.h
@@ -407,7 +407,8 @@ static XSM_INLINE int cf_check xsm_get_vnumainfo(
     return xsm_default_action(action, current->domain, d);
 }
 
-#if defined(CONFIG_HAS_PASSTHROUGH) && defined(CONFIG_HAS_PCI)
+#if defined(CONFIG_HAS_PASSTHROUGH) && defined(CONFIG_MGMT_HYPERCALLS)
+#ifdef CONFIG_HAS_PCI
 static XSM_INLINE int cf_check xsm_get_device_group(
     XSM_DEFAULT_ARG uint32_t machine_bdf)
 {
@@ -429,7 +430,8 @@ static XSM_INLINE int cf_check xsm_deassign_device(
     return xsm_default_action(action, current->domain, d);
 }
 
-#endif /* HAS_PASSTHROUGH && HAS_PCI */
+#endif /* CONFIG_HAS_PCI */
+#endif /* HAS_PASSTHROUGH && MGMT_HYPERCALLS */
 
 #if defined(CONFIG_HAS_PASSTHROUGH) && defined(CONFIG_HAS_DEVICE_TREE_DISCOVERY)
 static XSM_INLINE int cf_check xsm_assign_dtdevice(
diff --git a/xen/include/xsm/xsm.h b/xen/include/xsm/xsm.h
index 154a4b8a92..f2e92645ef 100644
--- a/xen/include/xsm/xsm.h
+++ b/xen/include/xsm/xsm.h
@@ -123,11 +123,13 @@ struct xsm_ops {
     int (*pci_config_permission)(struct domain *d, uint32_t machine_bdf,
                                  uint16_t start, uint16_t end, uint8_t access);
 
-#if defined(CONFIG_HAS_PASSTHROUGH) && defined(CONFIG_HAS_PCI)
+#if defined(CONFIG_HAS_PASSTHROUGH) && defined(CONFIG_MGMT_HYPERCALLS)
+#ifdef CONFIG_HAS_PCI
     int (*get_device_group)(uint32_t machine_bdf);
     int (*assign_device)(struct domain *d, uint32_t machine_bdf);
     int (*deassign_device)(struct domain *d, uint32_t machine_bdf);
-#endif
+#endif /* CONFIG_HAS_PCI */
+#endif /* HAS_PASSTHROUGH && MGMT_HYPERCALLS */
 
 #if defined(CONFIG_HAS_PASSTHROUGH) && defined(CONFIG_HAS_DEVICE_TREE_DISCOVERY)
     int (*assign_dtdevice)(struct domain *d, const char *dtpath);
@@ -524,7 +526,8 @@ static inline int xsm_pci_config_permission(
     return alternative_call(xsm_ops.pci_config_permission, d, machine_bdf, start, end, access);
 }
 
-#if defined(CONFIG_HAS_PASSTHROUGH) && defined(CONFIG_HAS_PCI)
+#if defined(CONFIG_HAS_PASSTHROUGH) && defined(CONFIG_MGMT_HYPERCALLS)
+#ifdef CONFIG_HAS_PCI
 static inline int xsm_get_device_group(xsm_default_t def, uint32_t machine_bdf)
 {
     return alternative_call(xsm_ops.get_device_group, machine_bdf);
@@ -541,7 +544,8 @@ static inline int xsm_deassign_device(
 {
     return alternative_call(xsm_ops.deassign_device, d, machine_bdf);
 }
-#endif /* HAS_PASSTHROUGH && HAS_PCI) */
+#endif /* CONFIG_HAS_PCI */
+#endif /* HAS_PASSTHROUGH && MGMT_HYPERCALLS */
 
 #if defined(CONFIG_HAS_PASSTHROUGH) && defined(CONFIG_HAS_DEVICE_TREE_DISCOVERY)
 static inline int xsm_assign_dtdevice(
diff --git a/xen/xsm/dummy.c b/xen/xsm/dummy.c
index 9774bb3bdb..0026a0963b 100644
--- a/xen/xsm/dummy.c
+++ b/xen/xsm/dummy.c
@@ -80,11 +80,13 @@ static const struct xsm_ops __initconst_cf_clobber dummy_ops = {
     .pci_config_permission         = xsm_pci_config_permission,
     .get_vnumainfo                 = xsm_get_vnumainfo,
 
-#if defined(CONFIG_HAS_PASSTHROUGH) && defined(CONFIG_HAS_PCI)
+#if defined(CONFIG_HAS_PASSTHROUGH) && defined(CONFIG_MGMT_HYPERCALLS)
+#ifdef CONFIG_HAS_PCI
     .get_device_group              = xsm_get_device_group,
     .assign_device                 = xsm_assign_device,
     .deassign_device               = xsm_deassign_device,
-#endif
+#endif /* CONFIG_HAS_PCI */
+#endif /* HAS_PASSTHROUGH && MGMT_HYPERCALLS */
 
 #if defined(CONFIG_HAS_PASSTHROUGH) && defined(CONFIG_HAS_DEVICE_TREE_DISCOVERY)
     .assign_dtdevice               = xsm_assign_dtdevice,
diff --git a/xen/xsm/flask/hooks.c b/xen/xsm/flask/hooks.c
index 9b63c516e6..805a9a528e 100644
--- a/xen/xsm/flask/hooks.c
+++ b/xen/xsm/flask/hooks.c
@@ -1388,7 +1388,8 @@ static int cf_check flask_mem_sharing(struct domain *d)
 }
 #endif
 
-#if defined(CONFIG_HAS_PASSTHROUGH) && defined(CONFIG_HAS_PCI)
+#if defined(CONFIG_HAS_PASSTHROUGH) && defined(CONFIG_MGMT_HYPERCALLS)
+#ifdef CONFIG_HAS_PCI
 static int cf_check flask_get_device_group(uint32_t machine_bdf)
 {
     uint32_t rsid;
@@ -1459,7 +1460,8 @@ static int cf_check flask_deassign_device(
 
     return avc_current_has_perm(rsid, SECCLASS_RESOURCE, RESOURCE__REMOVE_DEVICE, NULL);
 }
-#endif /* HAS_PASSTHROUGH && HAS_PCI */
+#endif /* CONFIG_HAS_PCI */
+#endif /* HAS_PASSTHROUGH && MGMT_HYPERCALLS */
 
 #if defined(CONFIG_HAS_PASSTHROUGH) && defined(CONFIG_HAS_DEVICE_TREE_DISCOVERY)
 static int flask_test_assign_dtdevice(const char *dtpath)
@@ -1987,11 +1989,13 @@ static const struct xsm_ops __initconst_cf_clobber flask_ops = {
     .remove_from_physmap = flask_remove_from_physmap,
     .map_gmfn_foreign = flask_map_gmfn_foreign,
 
-#if defined(CONFIG_HAS_PASSTHROUGH) && defined(CONFIG_HAS_PCI)
+#if defined(CONFIG_HAS_PASSTHROUGH) && defined(CONFIG_MGMT_HYPERCALLS)
+#ifdef CONFIG_HAS_PCI
     .get_device_group = flask_get_device_group,
     .assign_device = flask_assign_device,
     .deassign_device = flask_deassign_device,
-#endif
+#endif /* CONFIG_HAS_PCI */
+#endif /* HAS_PASSTHROUGH && MGMT_HYPERCALLS */
 
 #if defined(CONFIG_HAS_PASSTHROUGH) && defined(CONFIG_HAS_DEVICE_TREE_DISCOVERY)
     .assign_dtdevice = flask_assign_dtdevice,
-- 
2.34.1

[Public]

> -----Original Message-----
> From: Penny, Zheng <penny.zheng@amd.com>
> Sent: Friday, November 21, 2025 6:58 PM
> To: xen-devel@lists.xenproject.org
> Cc: Huang, Ray <Ray.Huang@amd.com>; grygorii_strashko@epam.com; Penny,
> Zheng <penny.zheng@amd.com>; Jan Beulich <jbeulich@suse.com>; Andrew
> Cooper <andrew.cooper3@citrix.com>; Roger Pau Monné <roger.pau@citrix.com>;
> Andryuk, Jason <Jason.Andryuk@amd.com>; Daniel P. Smith
> <dpsmith@apertussolutions.com>
> Subject: [PATCH v4 14/24] xen/domctl: wrap pci-subset iommu-related domctl op
> with CONFIG_MGMT_HYPERCALLS
>
> Function iommu_do_pci_domctl() is the main entry for pci-subset iommu-related
> domctl-op, and shall be wrapped with CONFIG_MGMT_HYPERCALLS.
> Tracking its calling chain, the following functions shall all be wrapped with
> CONFIG_MGMT_HYPERCALLS:
> - iommu_do_pci_domctl
>   - iommu_get_device_group
>     - xsm_get_device_group
>     - iommu_ops.get_device_group_id
>       - amd_iommu_group_id/intel_iommu_group_id
>   - device_assigned
>   - xsm_assign_device
>   - assign_device
>     - iommu_ops.assign_device
>       - intel_iommu_assign_device/amd_iommu_assign_device
>   - xsm_deassign_device
>   - deassign_device
>     - iommu_ops.reassign_device
>       - reassign_device_ownership/reassign_device
> Otherwise all the functions will become unreachable when
> MGMT_HYPERCALLS=n, and hence violating Misra rule 2.1
>
> Signed-off-by: Penny Zheng <Penny.Zheng@amd.com>
> ---
> v1 -> v2:
> - adapt to changes of "unify DOMCTL to MGMT_HYPERCALLS"
> - wrap XEN_DOMCTL_assign_device{test_assign_device,deassign_device,
>  get_device_group}-case transiently
> ---
> v2 -> v3:
> - make PCI_PASSTHROUGH(, then HAS_VPCI_GUEST_SUPPORT) depend on
> MGMT_HYPERCALLS
> - add wrapping for
> iommu_remove_dt_device/iommu_dt_device_is_assigned_locked/
> arm_smmu_detach_dev/arm_smmu_domain_remove_master
> - fold commit
> "xen/xsm: wrap xsm-iommu-related functions with
> CONFIG_MGMT_HYPERCALLS" in
> - fix overly long #ifdef
> - add missing wrapping in xsm/dummy.h
> - address "violating Misra rule 2.1" in commit message
> - remove transient wrapping of
> XEN_DOMCTL_assign_device{test_assign_device,deassign_device,get_device_gr
> oup}-case
> ---
> v3 -> v4:
> - move codes to wrap with a single #ifdef
> - split into PCI related subset and DT subset
> ---
>  xen/drivers/passthrough/amd/pci_amd_iommu.c | 20 ++++----
>  xen/drivers/passthrough/pci.c               | 52 +++++++++++----------
>  xen/drivers/passthrough/vtd/iommu.c         |  6 ++-
>  xen/include/xsm/dummy.h                     |  6 ++-
>  xen/include/xsm/xsm.h                       | 12 +++--
>  xen/xsm/dummy.c                             |  6 ++-
>  xen/xsm/flask/hooks.c                       | 12 +++--
>  7 files changed, 68 insertions(+), 46 deletions(-)
>
> diff --git a/xen/drivers/passthrough/amd/pci_amd_iommu.c
> b/xen/drivers/passthrough/amd/pci_amd_iommu.c
> index 3a14770855..576b36af92 100644
> --- a/xen/drivers/passthrough/amd/pci_amd_iommu.c
> +++ b/xen/drivers/passthrough/amd/pci_amd_iommu.c
> @@ -461,6 +461,7 @@ static void amd_iommu_disable_domain_device(const
> struct domain *domain,
>          spin_unlock_irqrestore(&iommu->lock, flags);  }
>
> +#ifdef CONFIG_MGMT_HYPERCALLS
>  static int cf_check reassign_device(
>      struct domain *source, struct domain *target, u8 devfn,
>      struct pci_dev *pdev)
> @@ -551,6 +552,14 @@ static int cf_check amd_iommu_assign_device(
>      return rc;
>  }
>
> +static int cf_check amd_iommu_group_id(u16 seg, u8 bus, u8 devfn) {
> +    unsigned int bdf = PCI_BDF(bus, devfn);
> +
> +    return (bdf < ivrs_bdf_entries) ? get_dma_requestor_id(seg, bdf) :
> +bdf; } #endif /* CONFIG_MGMT_HYPERCALLS */
> +
>  static void cf_check amd_iommu_clear_root_pgtable(struct domain *d)  {
>      struct domain_iommu *hd = dom_iommu(d); @@ -698,13 +707,6 @@ static int
> cf_check amd_iommu_remove_device(u8 devfn, struct pci_dev *pdev)
>      return 0;
>  }
>
> -static int cf_check amd_iommu_group_id(u16 seg, u8 bus, u8 devfn) -{
> -    unsigned int bdf = PCI_BDF(bus, devfn);
> -
> -    return (bdf < ivrs_bdf_entries) ? get_dma_requestor_id(seg, bdf) : bdf;
> -}
> -
>  #include <asm/io_apic.h>
>
>  static void amd_dump_page_table_level(struct page_info *pg, int level, @@ -
> 772,14 +774,16 @@ static const struct iommu_ops __initconst_cf_clobber
> _iommu_ops = {
>      .quarantine_init = amd_iommu_quarantine_init,
>      .add_device = amd_iommu_add_device,
>      .remove_device = amd_iommu_remove_device,
> -    .assign_device  = amd_iommu_assign_device,
>      .teardown = amd_iommu_domain_destroy,
>      .clear_root_pgtable = amd_iommu_clear_root_pgtable,
>      .map_page = amd_iommu_map_page,
>      .unmap_page = amd_iommu_unmap_page,
>      .iotlb_flush = amd_iommu_flush_iotlb_pages,
> +#ifdef CONFIG_MGMT_HYPERCALLS
> +    .assign_device  = amd_iommu_assign_device,
>      .reassign_device = reassign_device,
>      .get_device_group_id = amd_iommu_group_id,
> +#endif

FWIS, Alejandro has come up a more clever way to DCE these kinds of op, staying conditionally as callback. Here, I just took this commit as example to show the methodology:
```
         .assign_device  = IS_ENABLED(CONFIG_MGMT_HYPERCALLS)
                                        ? amd_iommu_assign_device
                                        : NULL,
```
The compiler has enough visibility to know that static(amd_iommu_assign_device()) is used, and is droppable when MGMT_HYPERCALLS=n. So there is no need to do ifdef-wrapping around these statics now. Later when jason's "--gc-section" patch serie in,  --gc-section will help linker identify them unused when MGMT_HYPERCALLS=n, then remove them automatically.

If we all agreed to use above methodology to do DCE.
Alejandro also recommended that since we will do this assignments in enough places in this patch serie, we probably want something like MAYBE_OP() somewhere in xen/macros.h:

#define MAYBE_OP(c, fn) (IS_ENABLED(c) ? fn : NULL)

I'd like to listen from your opinions on whether I shall do such update for v5, since it is quite a big update

> diff --git a/xen/xsm/dummy.c b/xen/xsm/dummy.c index 9774bb3bdb..0026a0963b
> 100644
> --- a/xen/xsm/dummy.c
> +++ b/xen/xsm/dummy.c
> @@ -80,11 +80,13 @@ static const struct xsm_ops __initconst_cf_clobber
> dummy_ops = {
>      .pci_config_permission         = xsm_pci_config_permission,
>      .get_vnumainfo                 = xsm_get_vnumainfo,
>
> -#if defined(CONFIG_HAS_PASSTHROUGH) && defined(CONFIG_HAS_PCI)
> +#if defined(CONFIG_HAS_PASSTHROUGH) &&
> defined(CONFIG_MGMT_HYPERCALLS)
> +#ifdef CONFIG_HAS_PCI
>      .get_device_group              = xsm_get_device_group,
>      .assign_device                 = xsm_assign_device,
>      .deassign_device               = xsm_deassign_device,
> -#endif

The same thing we shall do for XSM-changes too:
        .get_device_group              = IS_ENABLED(CONFIG_MGMT_HYPERCALLS)
                                                ? xsm_get_device_group
                                                : NULL,

Many thanks
Penny Zheng

On 04.02.2026 08:50, Penny, Zheng wrote:
>> -----Original Message-----
>> From: Penny, Zheng <penny.zheng@amd.com>
>> Sent: Friday, November 21, 2025 6:58 PM
>>[...]
>> 772,14 +774,16 @@ static const struct iommu_ops __initconst_cf_clobber
>> _iommu_ops = {
>>      .quarantine_init = amd_iommu_quarantine_init,
>>      .add_device = amd_iommu_add_device,
>>      .remove_device = amd_iommu_remove_device,
>> -    .assign_device  = amd_iommu_assign_device,
>>      .teardown = amd_iommu_domain_destroy,
>>      .clear_root_pgtable = amd_iommu_clear_root_pgtable,
>>      .map_page = amd_iommu_map_page,
>>      .unmap_page = amd_iommu_unmap_page,
>>      .iotlb_flush = amd_iommu_flush_iotlb_pages,
>> +#ifdef CONFIG_MGMT_HYPERCALLS
>> +    .assign_device  = amd_iommu_assign_device,
>>      .reassign_device = reassign_device,
>>      .get_device_group_id = amd_iommu_group_id,
>> +#endif
> 
> FWIS, Alejandro has come up a more clever way to DCE these kinds of op, staying conditionally as callback. Here, I just took this commit as example to show the methodology:
> ```
>          .assign_device  = IS_ENABLED(CONFIG_MGMT_HYPERCALLS)
>                                         ? amd_iommu_assign_device
>                                         : NULL,
> ```
> The compiler has enough visibility to know that static(amd_iommu_assign_device()) is used, and is droppable when MGMT_HYPERCALLS=n. So there is no need to do ifdef-wrapping around these statics now. Later when jason's "--gc-section" patch serie in,  --gc-section will help linker identify them unused when MGMT_HYPERCALLS=n, then remove them automatically.

I fear I don't see why --gc-sections would make a difference when, for static
functions, the compiler already is in the position of removing the functions.

> If we all agreed to use above methodology to do DCE.
> Alejandro also recommended that since we will do this assignments in enough places in this patch serie, we probably want something like MAYBE_OP() somewhere in xen/macros.h:
> 
> #define MAYBE_OP(c, fn) (IS_ENABLED(c) ? fn : NULL)
> 
> I'd like to listen from your opinions on whether I shall do such update for v5, since it is quite a big update

Well, already there I did raise my concern of leaving around function pointer
fields in structures which will only ever be NULL. If respective fields are
removed altogether, there's no risk whatsoever that an accidental use may be
overlooked - the build would simply fail when making such an attempt. Calls
through NULL are privilege escalation XSAs when PV guests can somehow
leverage them, and use of altcall patching would still only downgrade them
to DoS XSAs.

Jan

[Public]

> -----Original Message-----
> From: Jan Beulich <jbeulich@suse.com>
> Sent: Wednesday, February 4, 2026 4:09 PM
> To: Penny, Zheng <penny.zheng@amd.com>
> Cc: Huang, Ray <Ray.Huang@amd.com>; grygorii_strashko@epam.com; Andrew
> Cooper <andrew.cooper3@citrix.com>; Roger Pau Monné <roger.pau@citrix.com>;
> Andryuk, Jason <Jason.Andryuk@amd.com>; Daniel P. Smith
> <dpsmith@apertussolutions.com>; xen-devel@lists.xenproject.org; Garcia Vallejo,
> Alejandro <Alejandro.GarciaVallejo@amd.com>; Stabellini, Stefano
> <stefano.stabellini@amd.com>
> Subject: Re: [PATCH v4 14/24] xen/domctl: wrap pci-subset iommu-related domctl
> op with CONFIG_MGMT_HYPERCALLS
>
> On 04.02.2026 08:50, Penny, Zheng wrote:
> >> -----Original Message-----
> >> From: Penny, Zheng <penny.zheng@amd.com>
> >> Sent: Friday, November 21, 2025 6:58 PM [...]
> >> 772,14 +774,16 @@ static const struct iommu_ops
> >>__initconst_cf_clobber  _iommu_ops = {
> >>      .quarantine_init = amd_iommu_quarantine_init,
> >>      .add_device = amd_iommu_add_device,
> >>      .remove_device = amd_iommu_remove_device,
> >> -    .assign_device  = amd_iommu_assign_device,
> >>      .teardown = amd_iommu_domain_destroy,
> >>      .clear_root_pgtable = amd_iommu_clear_root_pgtable,
> >>      .map_page = amd_iommu_map_page,
> >>      .unmap_page = amd_iommu_unmap_page,
> >>      .iotlb_flush = amd_iommu_flush_iotlb_pages,
> >> +#ifdef CONFIG_MGMT_HYPERCALLS
> >> +    .assign_device  = amd_iommu_assign_device,
> >>      .reassign_device = reassign_device,
> >>      .get_device_group_id = amd_iommu_group_id,
> >> +#endif
> >
> > FWIS, Alejandro has come up a more clever way to DCE these kinds of op,
> staying conditionally as callback. Here, I just took this commit as example to show
> the methodology:
> > ```
> >          .assign_device  = IS_ENABLED(CONFIG_MGMT_HYPERCALLS)
> >                                         ? amd_iommu_assign_device
> >                                         : NULL, ``` The compiler has
> > enough visibility to know that static(amd_iommu_assign_device()) is used, and is
> droppable when MGMT_HYPERCALLS=n. So there is no need to do ifdef-wrapping
> around these statics now. Later when jason's "--gc-section" patch serie in,  --gc-
> section will help linker identify them unused when MGMT_HYPERCALLS=n, then
> remove them automatically.
>
> I fear I don't see why --gc-sections would make a difference when, for static
> functions, the compiler already is in the position of removing the functions.
>

I may misunderstand the DCE process. With this change, we don't need to put #ifdef CONFIG_MGMT_HYPERCALLS around the statics (e.g. amd_iommu_assign_device()). So preprocessor will not help us remove them.
But if we enable --gc-sections flag, it will help us remove them in linker time, Without --gc-section, they will become unreachable codes.

> > If we all agreed to use above methodology to do DCE.
> > Alejandro also recommended that since we will do this assignments in enough
> places in this patch serie, we probably want something like MAYBE_OP()
> somewhere in xen/macros.h:
> >
> > #define MAYBE_OP(c, fn) (IS_ENABLED(c) ? fn : NULL)
> >
> > I'd like to listen from your opinions on whether I shall do such
> > update for v5, since it is quite a big update
>
> Well, already there I did raise my concern of leaving around function pointer fields in
> structures which will only ever be NULL. If respective fields are removed altogether,
> there's no risk whatsoever that an accidental use may be overlooked - the build
> would simply fail when making such an attempt. Calls through NULL are privilege
> escalation XSAs when PV guests can somehow leverage them, and use of altcall
> patching would still only downgrade them to DoS XSAs.
>

Understood. Conditional NULL pointer fields are better be eliminated way down to the structure definition stage.

> Jan

On 04.02.2026 09:23, Penny, Zheng wrote:
>> -----Original Message-----
>> From: Jan Beulich <jbeulich@suse.com>
>> Sent: Wednesday, February 4, 2026 4:09 PM
>>
>> On 04.02.2026 08:50, Penny, Zheng wrote:
>>>> -----Original Message-----
>>>> From: Penny, Zheng <penny.zheng@amd.com>
>>>> Sent: Friday, November 21, 2025 6:58 PM [...]
>>>> 772,14 +774,16 @@ static const struct iommu_ops
>>>> __initconst_cf_clobber  _iommu_ops = {
>>>>      .quarantine_init = amd_iommu_quarantine_init,
>>>>      .add_device = amd_iommu_add_device,
>>>>      .remove_device = amd_iommu_remove_device,
>>>> -    .assign_device  = amd_iommu_assign_device,
>>>>      .teardown = amd_iommu_domain_destroy,
>>>>      .clear_root_pgtable = amd_iommu_clear_root_pgtable,
>>>>      .map_page = amd_iommu_map_page,
>>>>      .unmap_page = amd_iommu_unmap_page,
>>>>      .iotlb_flush = amd_iommu_flush_iotlb_pages,
>>>> +#ifdef CONFIG_MGMT_HYPERCALLS
>>>> +    .assign_device  = amd_iommu_assign_device,
>>>>      .reassign_device = reassign_device,
>>>>      .get_device_group_id = amd_iommu_group_id,
>>>> +#endif
>>>
>>> FWIS, Alejandro has come up a more clever way to DCE these kinds of op,
>> staying conditionally as callback. Here, I just took this commit as example to show
>> the methodology:
>>> ```
>>>          .assign_device  = IS_ENABLED(CONFIG_MGMT_HYPERCALLS)
>>>                                         ? amd_iommu_assign_device
>>>                                         : NULL, ``` The compiler has
>>> enough visibility to know that static(amd_iommu_assign_device()) is used, and is
>> droppable when MGMT_HYPERCALLS=n. So there is no need to do ifdef-wrapping
>> around these statics now. Later when jason's "--gc-section" patch serie in,  --gc-
>> section will help linker identify them unused when MGMT_HYPERCALLS=n, then
>> remove them automatically.
>>
>> I fear I don't see why --gc-sections would make a difference when, for static
>> functions, the compiler already is in the position of removing the functions.
> 
> I may misunderstand the DCE process. With this change, we don't need to put #ifdef CONFIG_MGMT_HYPERCALLS around the statics (e.g. amd_iommu_assign_device()). So preprocessor will not help us remove them.

But the compiler's optimization passes can, when they observe that a static
function, while referenced by source code, is referenced only in branches of
conditionals which are compile-time false. Without link-time optimization
that's really about all DCE can do.

Jan

On 21.11.2025 11:57, Penny Zheng wrote:
>  xen/drivers/passthrough/amd/pci_amd_iommu.c | 20 ++++----
>  xen/drivers/passthrough/pci.c               | 52 +++++++++++----------
>  xen/drivers/passthrough/vtd/iommu.c         |  6 ++-
>  xen/include/xsm/dummy.h                     |  6 ++-
>  xen/include/xsm/xsm.h                       | 12 +++--
>  xen/xsm/dummy.c                             |  6 ++-
>  xen/xsm/flask/hooks.c                       | 12 +++--
>  7 files changed, 68 insertions(+), 46 deletions(-)

With this diffstat and there being quite a few HAS_PCI under
xen/drivers/passthrough/arm/, what's the (PCI) deal there?
> @@ -772,14 +774,16 @@ static const struct iommu_ops __initconst_cf_clobber _iommu_ops = {
>      .quarantine_init = amd_iommu_quarantine_init,
>      .add_device = amd_iommu_add_device,
>      .remove_device = amd_iommu_remove_device,
> -    .assign_device  = amd_iommu_assign_device,
>      .teardown = amd_iommu_domain_destroy,
>      .clear_root_pgtable = amd_iommu_clear_root_pgtable,
>      .map_page = amd_iommu_map_page,
>      .unmap_page = amd_iommu_unmap_page,
>      .iotlb_flush = amd_iommu_flush_iotlb_pages,
> +#ifdef CONFIG_MGMT_HYPERCALLS
> +    .assign_device  = amd_iommu_assign_device,
>      .reassign_device = reassign_device,
>      .get_device_group_id = amd_iommu_group_id,
> +#endif

You don't zap the hooks themselves, i.e. they end up being NULL now in
the (still only hypothetical, provided the Kconfig change will be adjusted)
case of MGMT_HYPERCALLS=n. I understand the former two hooks are still
needed for DT, but at least .get_device_group_id should be properly dealt
with in xen/iommu.h right away, imo. This would then also clarify already
here that that's the plan for the other two hooks as well.

> --- a/xen/include/xsm/dummy.h
> +++ b/xen/include/xsm/dummy.h
> @@ -407,7 +407,8 @@ static XSM_INLINE int cf_check xsm_get_vnumainfo(
>      return xsm_default_action(action, current->domain, d);
>  }
>  
> -#if defined(CONFIG_HAS_PASSTHROUGH) && defined(CONFIG_HAS_PCI)
> +#if defined(CONFIG_HAS_PASSTHROUGH) && defined(CONFIG_MGMT_HYPERCALLS)
> +#ifdef CONFIG_HAS_PCI

Why the separate #ifdef? Can't that be folded with the #if? Are there further
changes to be put inside the outer #if? (Applies again further down as well.)

>  static XSM_INLINE int cf_check xsm_get_device_group(
>      XSM_DEFAULT_ARG uint32_t machine_bdf)
>  {
> @@ -429,7 +430,8 @@ static XSM_INLINE int cf_check xsm_deassign_device(
>      return xsm_default_action(action, current->domain, d);
>  }
>  
> -#endif /* HAS_PASSTHROUGH && HAS_PCI */
> +#endif /* CONFIG_HAS_PCI */
> +#endif /* HAS_PASSTHROUGH && MGMT_HYPERCALLS */
>  
>  #if defined(CONFIG_HAS_PASSTHROUGH) && defined(CONFIG_HAS_DEVICE_TREE_DISCOVERY)
>  static XSM_INLINE int cf_check xsm_assign_dtdevice(

The DT counterpart, otoh, is separate anyway.

Jan

[Public]

> -----Original Message-----
> From: Jan Beulich <jbeulich@suse.com>
> Sent: Tuesday, November 25, 2025 11:37 PM
> To: Penny, Zheng <penny.zheng@amd.com>
> Cc: Huang, Ray <Ray.Huang@amd.com>; grygorii_strashko@epam.com; Andrew
> Cooper <andrew.cooper3@citrix.com>; Roger Pau Monné <roger.pau@citrix.com>;
> Andryuk, Jason <Jason.Andryuk@amd.com>; Daniel P. Smith
> <dpsmith@apertussolutions.com>; xen-devel@lists.xenproject.org
> Subject: Re: [PATCH v4 14/24] xen/domctl: wrap pci-subset iommu-related domctl
> op with CONFIG_MGMT_HYPERCALLS
>
> On 21.11.2025 11:57, Penny Zheng wrote:
> >  xen/drivers/passthrough/amd/pci_amd_iommu.c | 20 ++++----
> >  xen/drivers/passthrough/pci.c               | 52 +++++++++++----------
> >  xen/drivers/passthrough/vtd/iommu.c         |  6 ++-
> >  xen/include/xsm/dummy.h                     |  6 ++-
> >  xen/include/xsm/xsm.h                       | 12 +++--
> >  xen/xsm/dummy.c                             |  6 ++-
> >  xen/xsm/flask/hooks.c                       | 12 +++--
> >  7 files changed, 68 insertions(+), 46 deletions(-)
>
> With this diffstat and there being quite a few HAS_PCI under
> xen/drivers/passthrough/arm/, what's the (PCI) deal there?

In arm, we have the following select chain:
HAS_VPCI_GUEST_SUPPORT if PCI_PASSTHROUGH -> HAS_VPCI -> HAS_PCI
So if we make PCI_PASSTHROUGH later depend on MGMT_HYPERCALLS, the PCI-subset for arm will also be guarded too.
I'll add description in commit message

> > --- a/xen/include/xsm/dummy.h
> > +++ b/xen/include/xsm/dummy.h
> > @@ -407,7 +407,8 @@ static XSM_INLINE int cf_check xsm_get_vnumainfo(
> >      return xsm_default_action(action, current->domain, d);  }
> >
> > -#if defined(CONFIG_HAS_PASSTHROUGH) && defined(CONFIG_HAS_PCI)
> > +#if defined(CONFIG_HAS_PASSTHROUGH) &&
> > +defined(CONFIG_MGMT_HYPERCALLS) #ifdef CONFIG_HAS_PCI
>
> Why the separate #ifdef? Can't that be folded with the #if? Are there further changes
> to be put inside the outer #if? (Applies again further down as well.)
>

Because later in DT-subset, we want something like the following:
```
#if defined(CONFIG_HAS_PASSTHROUGH) && defined(CONFIG_MGMT_HYPERCALLS)
#ifdef CONFIG_HAS_PCI
...
#endif
#ifdef CONFIG_HAS_DEVICE_TREE_DISCOVERY
...
#endif
#endif /* HAS_PASSTHROUGH  && MGMT_HYPERCALLS  */
Letting HAS_PASSTHROUGH  && MGMT_HYPERCALLS being outer wrapper and a separate #ifdef CONFIG_HAS_PCI here could avoid extra changes in DT commit.

> >  static XSM_INLINE int cf_check xsm_get_device_group(
> >      XSM_DEFAULT_ARG uint32_t machine_bdf)  { @@ -429,7 +430,8 @@
> > static XSM_INLINE int cf_check xsm_deassign_device(
> >      return xsm_default_action(action, current->domain, d);  }
> >
> > -#endif /* HAS_PASSTHROUGH && HAS_PCI */
> > +#endif /* CONFIG_HAS_PCI */
> > +#endif /* HAS_PASSTHROUGH && MGMT_HYPERCALLS */
> >
> >  #if defined(CONFIG_HAS_PASSTHROUGH) &&
> > defined(CONFIG_HAS_DEVICE_TREE_DISCOVERY)
> >  static XSM_INLINE int cf_check xsm_assign_dtdevice(
>
> The DT counterpart, otoh, is separate anyway.
>
> Jan