RE: [PATCH v4 14/24] xen/domctl: wrap pci-subset iommu-related domctl op with CONFIG_MGMT_HYPERCALLS

["Penny, Zheng" <penny.zheng@xxxxxxx>]

[Public]

> -----Original Message-----
> From: Penny, Zheng <penny.zheng@xxxxxxx>
> Sent: Friday, November 21, 2025 6:58 PM
> To: xen-devel@xxxxxxxxxxxxxxxxxxxx
> Cc: Huang, Ray <Ray.Huang@xxxxxxx>; grygorii_strashko@xxxxxxxx; Penny,
> Zheng <penny.zheng@xxxxxxx>; Jan Beulich <jbeulich@xxxxxxxx>; Andrew
> Cooper <andrew.cooper3@xxxxxxxxxx>; Roger Pau Monné <roger.pau@xxxxxxxxxx>;
> Andryuk, Jason <Jason.Andryuk@xxxxxxx>; Daniel P. Smith
> <dpsmith@xxxxxxxxxxxxxxxxxxxx>
> 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@xxxxxxx>
> ---
> 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