Re: [PATCH v3 22/28] xen/domctl: wrap iommu-related domctl op with CONFIG_MGMT_HYPERCALLS

[Jan Beulich <jbeulich@xxxxxxxx>]

On 13.10.2025 12:15, Penny Zheng wrote:
> Function iommu_do_domctl() is the main entry for all 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
>     - amd_iommu_group_id/intel_iommu_group_id
>   - device_assigned
>   - assign_device
>     - intel_iommu_assign_device/amd_iommu_assign_device
>   - deassign_device
>     - reassign_device_ownership/reassign_device

Could this PCI related subset and ...

> - make PCI_PASSTHROUGH depend on MGMT_HYPERCALLS
> - iommu_do_dt_domctl
>   - iommu_deassign_dt_device
>     - arm_smmu_reassign_dev
>       - arm_smmu_deassign_dev
>         - arm_smmu_detach_dev
>           - arm_smmu_domain_remove_master
>     - ipmmu_reassign_device
>       - ipmmu_deassign_device
>         - ipmmu_detach_device
>   - iommu_remove_dt_device
>     - iommu_dt_device_is_assigned_locked
>   - dt_find_node_by_gpath

... this DT related subset become separate (prereq) patches? Doing so may also 
reduce
the number of acks you need to collect on individual patches.

The bullet point in between looks unrelated in this list; it's not about any 
function,
after all. In fact I was about to complain that the aspect isn't mentioned in 
the
description, until I spotted the misplaced line.

> - xsm_get_device_group
> - xsm_assign_device
> - xsm_deassign_device
> - xsm_assign_dtdevice
> - xsm_deassign_dtdevice
> 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

Is this correct, though? Isn't PCI pass-through and vPCI guest support also
possible in dom0less / hyperlaunch?

> --- 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)
> @@ -550,6 +551,7 @@ static int cf_check amd_iommu_assign_device(
>  
>      return rc;
>  }
> +#endif /* CONFIG_MGMT_HYPERCALLS */
>  
>  static void cf_check amd_iommu_clear_root_pgtable(struct domain *d)
>  {
> @@ -698,12 +700,14 @@ static int cf_check amd_iommu_remove_device(u8 devfn, 
> struct pci_dev *pdev)
>      return 0;
>  }
>  
> +#ifdef CONFIG_MGMT_HYPERCALLS
>  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 */

Might be worth moving some code and ...

> @@ -772,14 +776,18 @@ 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,
> +#ifdef CONFIG_MGMT_HYPERCALLS
>      .assign_device  = amd_iommu_assign_device,
> +#endif
>      .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
>      .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,

... some fields / initializers, such that we get away with a single #ifdef each.

> --- a/xen/drivers/passthrough/pci.c
> +++ b/xen/drivers/passthrough/pci.c
> @@ -877,6 +877,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)
> @@ -945,7 +946,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;
> @@ -1483,6 +1483,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;
> @@ -1646,6 +1647,7 @@ static int iommu_get_device_group(
>  
>      return i;
>  }
> +#endif /* CONFIG_MGMT_HYPERCALLS */
>  
>  void iommu_dev_iotlb_flush_timeout(struct domain *d, struct pci_dev *pdev)
>  {
> @@ -1671,6 +1673,7 @@ void iommu_dev_iotlb_flush_timeout(struct domain *d, 
> struct pci_dev *pdev)
>      pcidevs_unlock();
>  }
>  
> +#ifdef CONFIG_MGMT_HYPERCALLS
>  int iommu_do_pci_domctl(
>      struct xen_domctl *domctl, struct domain *d,
>      XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
> @@ -1804,6 +1807,7 @@ int iommu_do_pci_domctl(
>  
>      return ret;
>  }
> +#endif /* CONFIG_MGMT_HYPERCALLS */

Same here - the helpers of iommu_do_pci_domctl() would likely best move 
immediately
ahead of it, so that all can be covered with a single #ifdef.

Jan