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

[Jan Beulich <jbeulich@xxxxxxxx>]

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