Re: [PATCH v3 1/2] xen/passthrough: Provide stub functions when !HAS_PASSTHROUGH

[Jan Beulich <jbeulich@xxxxxxxx>]

On 18.02.2025 10:51, Luca Fancellu wrote:
> --- a/xen/include/xen/iommu.h
> +++ b/xen/include/xen/iommu.h
> @@ -110,6 +110,8 @@ extern int8_t iommu_hwdom_reserved;
>  
>  extern unsigned int iommu_dev_iotlb_timeout;
>  
> +#ifdef CONFIG_HAS_PASSTHROUGH
> +
>  int iommu_setup(void);
>  int iommu_hardware_setup(void);
>  
> @@ -122,6 +124,28 @@ int arch_iommu_domain_init(struct domain *d);
>  void arch_iommu_check_autotranslated_hwdom(struct domain *d);
>  void arch_iommu_hwdom_init(struct domain *d);
>  
> +#else
> +
> +static inline int iommu_setup(void)
> +{
> +    return -ENODEV;
> +}
> +
> +static inline int iommu_domain_init(struct domain *d, unsigned int opts)
> +{
> +    /*
> +     * When !HAS_PASSTHROUGH, iommu_enabled is set to false and the expected
> +     * behaviour of this function is to return success in that case.
> +     */
> +    return 0;
> +}

Hmm. Would the function be anywhere near likely to do anything else than
what it's expected to do? My original concern here was with "opts"
perhaps asking for something that cannot be supported. But that was wrong
thinking on my part. Here what you do is effectively open-code what the
real iommu_domain_init() would do: Return success when !is_iommu_enabled().
Which in turn follows from !iommu_enabled when !HAS_PASSTHROUGH.

On that basis I'd be okay if the comment was dropped again. Else it imo
wants re-wording to get closer to the explanation above.

> @@ -383,12 +429,12 @@ struct domain_iommu {
>  #define iommu_set_feature(d, f)   set_bit(f, dom_iommu(d)->features)
>  #define iommu_clear_feature(d, f) clear_bit(f, dom_iommu(d)->features)
>  
> +#ifdef CONFIG_HAS_PASSTHROUGH
>  /* Are we using the domain P2M table as its IOMMU pagetable? */
>  #define iommu_use_hap_pt(d)       (IS_ENABLED(CONFIG_HVM) && \
>                                     dom_iommu(d)->hap_pt_share)
>  
>  /* Does the IOMMU pagetable need to be kept synchronized with the P2M */
> -#ifdef CONFIG_HAS_PASSTHROUGH
>  #define need_iommu_pt_sync(d)     (dom_iommu(d)->need_sync)
>  
>  int iommu_do_domctl(struct xen_domctl *domctl, struct domain *d,

Coming back to my v2 comment: Why exactly is this change needed here? If
things build fine without the macro being defined when !HAS_PASSTHROUGH,
surely they will also build fine with it being defined? As per the
respective revlog entry, this change looks to belong into whatever is
going to be done to deal with the one Arm use of the macro. Or maybe
it's unneeded altogether.

Jan