When Xen is built without HAS_PASSTHROUGH, there are some parts
in arm where iommu_* functions are called in the codebase, but
their implementation is under xen/drivers/passthrough that is
not built.

So provide some stub for these functions in order to build Xen
when !HAS_PASSTHROUGH, which is the case for example on systems
with MPU support.

For gnttab_need_iommu_mapping() in the Arm part, modify the macro
to use IS_ENABLED for the HAS_PASSTHROUGH Kconfig.

Fixes: 0388a5979b21 ("xen/arm: mpu: Introduce choice between MMU and MPU")
Signed-off-by: Luca Fancellu <luca.fancellu@arm.com>
---
v4 changes:
 - re-introduced stub for iommu_use_hap_pt, Stefano suggested
   it is ok to have it in iommu.h.
 - Reworded comment in iommu_domain_init from Jan suggestion

v3 Changes:
 - removed stub for iommu_use_hap_pt, another solution will be
   done for the instance in common arm code.
 - Moved a comment close to the macro it was referred to
 - add comment to iommu_domain_init() stub
 - modified commit message
 - Add fixes tag

v2 Changes:
 - modify gnttab_need_iommu_mapping to use IS_ENABLED
 - removed macro that didn't allow some of the parameter to be
   evaluated
 - Changed commit message
---
---
 xen/arch/arm/include/asm/grant_table.h |  5 +--
 xen/include/xen/iommu.h                | 50 +++++++++++++++++++++++++-
 2 files changed, 52 insertions(+), 3 deletions(-)

diff --git a/xen/arch/arm/include/asm/grant_table.h b/xen/arch/arm/include/asm/grant_table.h
index d3c518a926b9..c5d87b60c4df 100644
--- a/xen/arch/arm/include/asm/grant_table.h
+++ b/xen/arch/arm/include/asm/grant_table.h
@@ -73,8 +73,9 @@ int replace_grant_host_mapping(uint64_t gpaddr, mfn_t frame,
 #define gnttab_status_gfn(d, t, i)                                       \
     page_get_xenheap_gfn(gnttab_status_page(t, i))
 
-#define gnttab_need_iommu_mapping(d)                    \
-    (is_domain_direct_mapped(d) && is_iommu_enabled(d))
+#define gnttab_need_iommu_mapping(d)                                     \
+    (IS_ENABLED(CONFIG_HAS_PASSTHROUGH) && is_domain_direct_mapped(d) && \
+     is_iommu_enabled(d))
 
 #endif /* __ASM_GRANT_TABLE_H__ */
 /*
diff --git a/xen/include/xen/iommu.h b/xen/include/xen/iommu.h
index 77a514019cc6..5ac038521e23 100644
--- 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)
+{
+    /*
+     * Return as the real iommu_domain_init() would: Success when
+     * !is_iommu_enabled(), following from !iommu_enabled when !HAS_PASSTHROUGH
+     */
+    return 0;
+}
+
+static inline void iommu_hwdom_init(struct domain *d) {}
+
+static inline void iommu_domain_destroy(struct domain *d) {}
+
+#endif /* HAS_PASSTHROUGH */
+
 /*
  * The following flags are passed to map (applicable ones also to unmap)
  * operations, while some are passed back by lookup operations.
@@ -209,6 +233,8 @@ struct msi_msg;
 #ifdef CONFIG_HAS_DEVICE_TREE
 #include <xen/device_tree.h>
 
+#ifdef CONFIG_HAS_PASSTHROUGH
+
 int iommu_assign_dt_device(struct domain *d, struct dt_device_node *dev);
 int iommu_deassign_dt_device(struct domain *d, struct dt_device_node *dev);
 int iommu_dt_domain_init(struct domain *d);
@@ -238,6 +264,26 @@ int iommu_do_dt_domctl(struct xen_domctl *domctl, struct domain *d,
  */
 int iommu_remove_dt_device(struct dt_device_node *np);
 
+#else
+
+static inline int iommu_assign_dt_device(struct domain *d,
+                                         struct dt_device_node *dev)
+{
+    return -EINVAL;
+}
+
+static inline int iommu_add_dt_device(struct dt_device_node *np)
+{
+    return 1;
+}
+
+static inline int iommu_release_dt_devices(struct domain *d)
+{
+    return 0;
+}
+
+#endif /* HAS_PASSTHROUGH */
+
 #endif /* HAS_DEVICE_TREE */
 
 struct page_info;
@@ -383,17 +429,19 @@ 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,
                     XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl);
 #else
+#define iommu_use_hap_pt(d)     ({ (void)(d); false; })
+
 #define need_iommu_pt_sync(d)     ({ (void)(d); false; })
 
 static inline int iommu_do_domctl(struct xen_domctl *domctl, struct domain *d,
-- 
2.34.1

Hi Luca,

On 07/03/2025 07:58, Luca Fancellu wrote:
> When Xen is built without HAS_PASSTHROUGH, there are some parts
> in arm where iommu_* functions are called in the codebase, but
> their implementation is under xen/drivers/passthrough that is
> not built.
> 
> So provide some stub for these functions in order to build Xen
> when !HAS_PASSTHROUGH, which is the case for example on systems
> with MPU support.
> 
> For gnttab_need_iommu_mapping() in the Arm part, modify the macro
> to use IS_ENABLED for the HAS_PASSTHROUGH Kconfig.
> 
> Fixes: 0388a5979b21 ("xen/arm: mpu: Introduce choice between MMU and MPU")
> Signed-off-by: Luca Fancellu <luca.fancellu@arm.com>
> ---
> v4 changes:
>   - re-introduced stub for iommu_use_hap_pt, Stefano suggested
>     it is ok to have it in iommu.h.
>   - Reworded comment in iommu_domain_init from Jan suggestion
> 
> v3 Changes:
>   - removed stub for iommu_use_hap_pt, another solution will be
>     done for the instance in common arm code.
>   - Moved a comment close to the macro it was referred to
>   - add comment to iommu_domain_init() stub
>   - modified commit message
>   - Add fixes tag
> 
> v2 Changes:
>   - modify gnttab_need_iommu_mapping to use IS_ENABLED
>   - removed macro that didn't allow some of the parameter to be
>     evaluated
>   - Changed commit message
> ---
> ---
>   xen/arch/arm/include/asm/grant_table.h |  5 +--
>   xen/include/xen/iommu.h                | 50 +++++++++++++++++++++++++-
>   2 files changed, 52 insertions(+), 3 deletions(-)
> 
> diff --git a/xen/arch/arm/include/asm/grant_table.h b/xen/arch/arm/include/asm/grant_table.h
> index d3c518a926b9..c5d87b60c4df 100644
> --- a/xen/arch/arm/include/asm/grant_table.h
> +++ b/xen/arch/arm/include/asm/grant_table.h
> @@ -73,8 +73,9 @@ int replace_grant_host_mapping(uint64_t gpaddr, mfn_t frame,
>   #define gnttab_status_gfn(d, t, i)                                       \
>       page_get_xenheap_gfn(gnttab_status_page(t, i))
>   
> -#define gnttab_need_iommu_mapping(d)                    \
> -    (is_domain_direct_mapped(d) && is_iommu_enabled(d))
> +#define gnttab_need_iommu_mapping(d)                                     \
> +    (IS_ENABLED(CONFIG_HAS_PASSTHROUGH) && is_domain_direct_mapped(d) && \
> +     is_iommu_enabled(d))
>   
>   #endif /* __ASM_GRANT_TABLE_H__ */
>   /*
> diff --git a/xen/include/xen/iommu.h b/xen/include/xen/iommu.h
> index 77a514019cc6..5ac038521e23 100644
> --- 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)
> +{
> +    /*
> +     * Return as the real iommu_domain_init() would: Success when
> +     * !is_iommu_enabled(), following from !iommu_enabled when !HAS_PASSTHROUGH
> +     */
> +    return 0;
> +}
> +
> +static inline void iommu_hwdom_init(struct domain *d) {}
> +
> +static inline void iommu_domain_destroy(struct domain *d) {}
> +
> +#endif /* HAS_PASSTHROUGH */
> +
>   /*
>    * The following flags are passed to map (applicable ones also to unmap)
>    * operations, while some are passed back by lookup operations.
> @@ -209,6 +233,8 @@ struct msi_msg;
>   #ifdef CONFIG_HAS_DEVICE_TREE
>   #include <xen/device_tree.h>
>   
> +#ifdef CONFIG_HAS_PASSTHROUGH
> +
>   int iommu_assign_dt_device(struct domain *d, struct dt_device_node *dev);
>   int iommu_deassign_dt_device(struct domain *d, struct dt_device_node *dev);
>   int iommu_dt_domain_init(struct domain *d);
> @@ -238,6 +264,26 @@ int iommu_do_dt_domctl(struct xen_domctl *domctl, struct domain *d,
>    */
>   int iommu_remove_dt_device(struct dt_device_node *np);
>   
> +#else
> +
> +static inline int iommu_assign_dt_device(struct domain *d,
> +                                         struct dt_device_node *dev)
> +{
> +    return -EINVAL;
> +}
> +
> +static inline int iommu_add_dt_device(struct dt_device_node *np)
> +{
> +    return 1;

I would suggest to add a comment explain what 1 means. IIRC, this means 
"no iommu" present.

Other than that:

Acked-by: Julien Grall <jgrall@amazon.com>

Cheers,

-- 
Julien Grall

On 07.03.2025 10:09, Julien Grall wrote:
> Hi Luca,
> 
> On 07/03/2025 07:58, Luca Fancellu wrote:
>> When Xen is built without HAS_PASSTHROUGH, there are some parts
>> in arm where iommu_* functions are called in the codebase, but
>> their implementation is under xen/drivers/passthrough that is
>> not built.
>>
>> So provide some stub for these functions in order to build Xen
>> when !HAS_PASSTHROUGH, which is the case for example on systems
>> with MPU support.
>>
>> For gnttab_need_iommu_mapping() in the Arm part, modify the macro
>> to use IS_ENABLED for the HAS_PASSTHROUGH Kconfig.
>>
>> Fixes: 0388a5979b21 ("xen/arm: mpu: Introduce choice between MMU and MPU")
>> Signed-off-by: Luca Fancellu <luca.fancellu@arm.com>
>> ---
>> v4 changes:
>>   - re-introduced stub for iommu_use_hap_pt, Stefano suggested
>>     it is ok to have it in iommu.h.
>>   - Reworded comment in iommu_domain_init from Jan suggestion
>>
>> v3 Changes:
>>   - removed stub for iommu_use_hap_pt, another solution will be
>>     done for the instance in common arm code.
>>   - Moved a comment close to the macro it was referred to
>>   - add comment to iommu_domain_init() stub
>>   - modified commit message
>>   - Add fixes tag
>>
>> v2 Changes:
>>   - modify gnttab_need_iommu_mapping to use IS_ENABLED
>>   - removed macro that didn't allow some of the parameter to be
>>     evaluated
>>   - Changed commit message
>> ---
>> ---
>>   xen/arch/arm/include/asm/grant_table.h |  5 +--
>>   xen/include/xen/iommu.h                | 50 +++++++++++++++++++++++++-
>>   2 files changed, 52 insertions(+), 3 deletions(-)
>>
>> diff --git a/xen/arch/arm/include/asm/grant_table.h b/xen/arch/arm/include/asm/grant_table.h
>> index d3c518a926b9..c5d87b60c4df 100644
>> --- a/xen/arch/arm/include/asm/grant_table.h
>> +++ b/xen/arch/arm/include/asm/grant_table.h
>> @@ -73,8 +73,9 @@ int replace_grant_host_mapping(uint64_t gpaddr, mfn_t frame,
>>   #define gnttab_status_gfn(d, t, i)                                       \
>>       page_get_xenheap_gfn(gnttab_status_page(t, i))
>>   
>> -#define gnttab_need_iommu_mapping(d)                    \
>> -    (is_domain_direct_mapped(d) && is_iommu_enabled(d))
>> +#define gnttab_need_iommu_mapping(d)                                     \
>> +    (IS_ENABLED(CONFIG_HAS_PASSTHROUGH) && is_domain_direct_mapped(d) && \
>> +     is_iommu_enabled(d))
>>   
>>   #endif /* __ASM_GRANT_TABLE_H__ */
>>   /*
>> diff --git a/xen/include/xen/iommu.h b/xen/include/xen/iommu.h
>> index 77a514019cc6..5ac038521e23 100644
>> --- 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)
>> +{
>> +    /*
>> +     * Return as the real iommu_domain_init() would: Success when
>> +     * !is_iommu_enabled(), following from !iommu_enabled when !HAS_PASSTHROUGH
>> +     */
>> +    return 0;
>> +}
>> +
>> +static inline void iommu_hwdom_init(struct domain *d) {}
>> +
>> +static inline void iommu_domain_destroy(struct domain *d) {}
>> +
>> +#endif /* HAS_PASSTHROUGH */
>> +
>>   /*
>>    * The following flags are passed to map (applicable ones also to unmap)
>>    * operations, while some are passed back by lookup operations.
>> @@ -209,6 +233,8 @@ struct msi_msg;
>>   #ifdef CONFIG_HAS_DEVICE_TREE
>>   #include <xen/device_tree.h>
>>   
>> +#ifdef CONFIG_HAS_PASSTHROUGH
>> +
>>   int iommu_assign_dt_device(struct domain *d, struct dt_device_node *dev);
>>   int iommu_deassign_dt_device(struct domain *d, struct dt_device_node *dev);
>>   int iommu_dt_domain_init(struct domain *d);
>> @@ -238,6 +264,26 @@ int iommu_do_dt_domctl(struct xen_domctl *domctl, struct domain *d,
>>    */
>>   int iommu_remove_dt_device(struct dt_device_node *np);
>>   
>> +#else
>> +
>> +static inline int iommu_assign_dt_device(struct domain *d,
>> +                                         struct dt_device_node *dev)
>> +{
>> +    return -EINVAL;
>> +}
>> +
>> +static inline int iommu_add_dt_device(struct dt_device_node *np)
>> +{
>> +    return 1;
> 
> I would suggest to add a comment explain what 1 means. IIRC, this means 
> "no iommu" present.
> 
> Other than that:
> 
> Acked-by: Julien Grall <jgrall@amazon.com>

Then also
Acked-by: Jan Beulich <jbeulich@suse.com>

Jan

Hi Julien,

> On 7 Mar 2025, at 09:09, Julien Grall <julien@xen.org> wrote:
> 
> Hi Luca,
> 
> On 07/03/2025 07:58, Luca Fancellu wrote:
>> When Xen is built without HAS_PASSTHROUGH, there are some parts
>> in arm where iommu_* functions are called in the codebase, but
>> their implementation is under xen/drivers/passthrough that is
>> not built.
>> So provide some stub for these functions in order to build Xen
>> when !HAS_PASSTHROUGH, which is the case for example on systems
>> with MPU support.
>> For gnttab_need_iommu_mapping() in the Arm part, modify the macro
>> to use IS_ENABLED for the HAS_PASSTHROUGH Kconfig.
>> Fixes: 0388a5979b21 ("xen/arm: mpu: Introduce choice between MMU and MPU")
>> Signed-off-by: Luca Fancellu <luca.fancellu@arm.com>
>> ---
>> v4 changes:
>>  - re-introduced stub for iommu_use_hap_pt, Stefano suggested
>>    it is ok to have it in iommu.h.
>>  - Reworded comment in iommu_domain_init from Jan suggestion
>> v3 Changes:
>>  - removed stub for iommu_use_hap_pt, another solution will be
>>    done for the instance in common arm code.
>>  - Moved a comment close to the macro it was referred to
>>  - add comment to iommu_domain_init() stub
>>  - modified commit message
>>  - Add fixes tag
>> v2 Changes:
>>  - modify gnttab_need_iommu_mapping to use IS_ENABLED
>>  - removed macro that didn't allow some of the parameter to be
>>    evaluated
>>  - Changed commit message
>> ---
>> ---
>>  xen/arch/arm/include/asm/grant_table.h |  5 +--
>>  xen/include/xen/iommu.h                | 50 +++++++++++++++++++++++++-
>>  2 files changed, 52 insertions(+), 3 deletions(-)
>> diff --git a/xen/arch/arm/include/asm/grant_table.h b/xen/arch/arm/include/asm/grant_table.h
>> index d3c518a926b9..c5d87b60c4df 100644
>> --- a/xen/arch/arm/include/asm/grant_table.h
>> +++ b/xen/arch/arm/include/asm/grant_table.h
>> @@ -73,8 +73,9 @@ int replace_grant_host_mapping(uint64_t gpaddr, mfn_t frame,
>>  #define gnttab_status_gfn(d, t, i)                                       \
>>      page_get_xenheap_gfn(gnttab_status_page(t, i))
>>  -#define gnttab_need_iommu_mapping(d)                    \
>> -    (is_domain_direct_mapped(d) && is_iommu_enabled(d))
>> +#define gnttab_need_iommu_mapping(d)                                     \
>> +    (IS_ENABLED(CONFIG_HAS_PASSTHROUGH) && is_domain_direct_mapped(d) && \
>> +     is_iommu_enabled(d))
>>    #endif /* __ASM_GRANT_TABLE_H__ */
>>  /*
>> diff --git a/xen/include/xen/iommu.h b/xen/include/xen/iommu.h
>> index 77a514019cc6..5ac038521e23 100644
>> --- 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)
>> +{
>> +    /*
>> +     * Return as the real iommu_domain_init() would: Success when
>> +     * !is_iommu_enabled(), following from !iommu_enabled when !HAS_PASSTHROUGH
>> +     */
>> +    return 0;
>> +}
>> +
>> +static inline void iommu_hwdom_init(struct domain *d) {}
>> +
>> +static inline void iommu_domain_destroy(struct domain *d) {}
>> +
>> +#endif /* HAS_PASSTHROUGH */
>> +
>>  /*
>>   * The following flags are passed to map (applicable ones also to unmap)
>>   * operations, while some are passed back by lookup operations.
>> @@ -209,6 +233,8 @@ struct msi_msg;
>>  #ifdef CONFIG_HAS_DEVICE_TREE
>>  #include <xen/device_tree.h>
>>  +#ifdef CONFIG_HAS_PASSTHROUGH
>> +
>>  int iommu_assign_dt_device(struct domain *d, struct dt_device_node *dev);
>>  int iommu_deassign_dt_device(struct domain *d, struct dt_device_node *dev);
>>  int iommu_dt_domain_init(struct domain *d);
>> @@ -238,6 +264,26 @@ int iommu_do_dt_domctl(struct xen_domctl *domctl, struct domain *d,
>>   */
>>  int iommu_remove_dt_device(struct dt_device_node *np);
>>  +#else
>> +
>> +static inline int iommu_assign_dt_device(struct domain *d,
>> +                                         struct dt_device_node *dev)
>> +{
>> +    return -EINVAL;
>> +}
>> +
>> +static inline int iommu_add_dt_device(struct dt_device_node *np)
>> +{
>> +    return 1;
> 
> I would suggest to add a comment explain what 1 means. IIRC, this means "no iommu" present.

Would it be ok something like in iommu_domain_init:

/*
 * Returns as the real iommu_add_dt_device() would: Error “no iommu" because
 * !iommu_enabled due to the fact that !HAS_PASSTHROUGH
 */

> 
> Other than that:
> 
> Acked-by: Julien Grall <jgrall@amazon.com>
> 
> Cheers,
> 
> -- 
> Julien Grall
> 

On 07.03.2025 10:20, Luca Fancellu wrote:
> Hi Julien,
> 
>> On 7 Mar 2025, at 09:09, Julien Grall <julien@xen.org> wrote:
>>
>> Hi Luca,
>>
>> On 07/03/2025 07:58, Luca Fancellu wrote:
>>> When Xen is built without HAS_PASSTHROUGH, there are some parts
>>> in arm where iommu_* functions are called in the codebase, but
>>> their implementation is under xen/drivers/passthrough that is
>>> not built.
>>> So provide some stub for these functions in order to build Xen
>>> when !HAS_PASSTHROUGH, which is the case for example on systems
>>> with MPU support.
>>> For gnttab_need_iommu_mapping() in the Arm part, modify the macro
>>> to use IS_ENABLED for the HAS_PASSTHROUGH Kconfig.
>>> Fixes: 0388a5979b21 ("xen/arm: mpu: Introduce choice between MMU and MPU")
>>> Signed-off-by: Luca Fancellu <luca.fancellu@arm.com>
>>> ---
>>> v4 changes:
>>>  - re-introduced stub for iommu_use_hap_pt, Stefano suggested
>>>    it is ok to have it in iommu.h.
>>>  - Reworded comment in iommu_domain_init from Jan suggestion
>>> v3 Changes:
>>>  - removed stub for iommu_use_hap_pt, another solution will be
>>>    done for the instance in common arm code.
>>>  - Moved a comment close to the macro it was referred to
>>>  - add comment to iommu_domain_init() stub
>>>  - modified commit message
>>>  - Add fixes tag
>>> v2 Changes:
>>>  - modify gnttab_need_iommu_mapping to use IS_ENABLED
>>>  - removed macro that didn't allow some of the parameter to be
>>>    evaluated
>>>  - Changed commit message
>>> ---
>>> ---
>>>  xen/arch/arm/include/asm/grant_table.h |  5 +--
>>>  xen/include/xen/iommu.h                | 50 +++++++++++++++++++++++++-
>>>  2 files changed, 52 insertions(+), 3 deletions(-)
>>> diff --git a/xen/arch/arm/include/asm/grant_table.h b/xen/arch/arm/include/asm/grant_table.h
>>> index d3c518a926b9..c5d87b60c4df 100644
>>> --- a/xen/arch/arm/include/asm/grant_table.h
>>> +++ b/xen/arch/arm/include/asm/grant_table.h
>>> @@ -73,8 +73,9 @@ int replace_grant_host_mapping(uint64_t gpaddr, mfn_t frame,
>>>  #define gnttab_status_gfn(d, t, i)                                       \
>>>      page_get_xenheap_gfn(gnttab_status_page(t, i))
>>>  -#define gnttab_need_iommu_mapping(d)                    \
>>> -    (is_domain_direct_mapped(d) && is_iommu_enabled(d))
>>> +#define gnttab_need_iommu_mapping(d)                                     \
>>> +    (IS_ENABLED(CONFIG_HAS_PASSTHROUGH) && is_domain_direct_mapped(d) && \
>>> +     is_iommu_enabled(d))
>>>    #endif /* __ASM_GRANT_TABLE_H__ */
>>>  /*
>>> diff --git a/xen/include/xen/iommu.h b/xen/include/xen/iommu.h
>>> index 77a514019cc6..5ac038521e23 100644
>>> --- 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)
>>> +{
>>> +    /*
>>> +     * Return as the real iommu_domain_init() would: Success when
>>> +     * !is_iommu_enabled(), following from !iommu_enabled when !HAS_PASSTHROUGH
>>> +     */
>>> +    return 0;
>>> +}
>>> +
>>> +static inline void iommu_hwdom_init(struct domain *d) {}
>>> +
>>> +static inline void iommu_domain_destroy(struct domain *d) {}
>>> +
>>> +#endif /* HAS_PASSTHROUGH */
>>> +
>>>  /*
>>>   * The following flags are passed to map (applicable ones also to unmap)
>>>   * operations, while some are passed back by lookup operations.
>>> @@ -209,6 +233,8 @@ struct msi_msg;
>>>  #ifdef CONFIG_HAS_DEVICE_TREE
>>>  #include <xen/device_tree.h>
>>>  +#ifdef CONFIG_HAS_PASSTHROUGH
>>> +
>>>  int iommu_assign_dt_device(struct domain *d, struct dt_device_node *dev);
>>>  int iommu_deassign_dt_device(struct domain *d, struct dt_device_node *dev);
>>>  int iommu_dt_domain_init(struct domain *d);
>>> @@ -238,6 +264,26 @@ int iommu_do_dt_domctl(struct xen_domctl *domctl, struct domain *d,
>>>   */
>>>  int iommu_remove_dt_device(struct dt_device_node *np);
>>>  +#else
>>> +
>>> +static inline int iommu_assign_dt_device(struct domain *d,
>>> +                                         struct dt_device_node *dev)
>>> +{
>>> +    return -EINVAL;
>>> +}
>>> +
>>> +static inline int iommu_add_dt_device(struct dt_device_node *np)
>>> +{
>>> +    return 1;
>>
>> I would suggest to add a comment explain what 1 means. IIRC, this means "no iommu" present.
> 
> Would it be ok something like in iommu_domain_init:
> 
> /*
>  * Returns as the real iommu_add_dt_device() would: Error “no iommu" because
>  * !iommu_enabled due to the fact that !HAS_PASSTHROUGH
>  */

We had been there before, hadn't we? Personally I find the suggested text
hard to follow. How about

    /*
     * !HAS_PASSTHROUGH => !iommu_enabled (see the non-stub
     * iommu_add_dt_device())
     */

If that's too terse, adding a few words should be fine.

Jan

Hi Jan,

>>>> +
>>>> +static inline int iommu_domain_init(struct domain *d, unsigned int opts)
>>>> +{
>>>> +    /*
>>>> +     * Return as the real iommu_domain_init() would: Success when
>>>> +     * !is_iommu_enabled(), following from !iommu_enabled when !HAS_PASSTHROUGH
>>>> +     */
>>>> +    return 0;
>>>> +}
>>>> +
>>>> +static inline void iommu_hwdom_init(struct domain *d) {}
>>>> +
>>>> +static inline void iommu_domain_destroy(struct domain *d) {}
>>>> +
>>>> +#endif /* HAS_PASSTHROUGH */
>>>> +
>>>> /*
>>>>  * The following flags are passed to map (applicable ones also to unmap)
>>>>  * operations, while some are passed back by lookup operations.
>>>> @@ -209,6 +233,8 @@ struct msi_msg;
>>>> #ifdef CONFIG_HAS_DEVICE_TREE
>>>> #include <xen/device_tree.h>
>>>> +#ifdef CONFIG_HAS_PASSTHROUGH
>>>> +
>>>> int iommu_assign_dt_device(struct domain *d, struct dt_device_node *dev);
>>>> int iommu_deassign_dt_device(struct domain *d, struct dt_device_node *dev);
>>>> int iommu_dt_domain_init(struct domain *d);
>>>> @@ -238,6 +264,26 @@ int iommu_do_dt_domctl(struct xen_domctl *domctl, struct domain *d,
>>>>  */
>>>> int iommu_remove_dt_device(struct dt_device_node *np);
>>>> +#else
>>>> +
>>>> +static inline int iommu_assign_dt_device(struct domain *d,
>>>> +                                         struct dt_device_node *dev)
>>>> +{
>>>> +    return -EINVAL;
>>>> +}
>>>> +
>>>> +static inline int iommu_add_dt_device(struct dt_device_node *np)
>>>> +{
>>>> +    return 1;
>>> 
>>> I would suggest to add a comment explain what 1 means. IIRC, this means "no iommu" present.
>> 
>> Would it be ok something like in iommu_domain_init:
>> 
>> /*
>> * Returns as the real iommu_add_dt_device() would: Error “no iommu" because
>> * !iommu_enabled due to the fact that !HAS_PASSTHROUGH
>> */
> 
> We had been there before, hadn't we? Personally I find the suggested text
> hard to follow.

well, I’ve taken your suggestion for iommu_domain_init and adapted here, 
maybe the adaptation didn’t meet your criteria then :)

But ...

> How about
> 
>    /*
>     * !HAS_PASSTHROUGH => !iommu_enabled (see the non-stub
>     * iommu_add_dt_device())
>     */
> 
> If that's too terse, adding a few words should be fine.

as long as you are happy with that, I’m happy as well, please let me know
if you want a v5 with this or if you would do the modificaiton on commit.

Cheers,
Luca

On 07.03.2025 12:23, Luca Fancellu wrote:
>>>>> +
>>>>> +static inline int iommu_domain_init(struct domain *d, unsigned int opts)
>>>>> +{
>>>>> +    /*
>>>>> +     * Return as the real iommu_domain_init() would: Success when
>>>>> +     * !is_iommu_enabled(), following from !iommu_enabled when !HAS_PASSTHROUGH
>>>>> +     */
>>>>> +    return 0;
>>>>> +}
>>>>> +
>>>>> +static inline void iommu_hwdom_init(struct domain *d) {}
>>>>> +
>>>>> +static inline void iommu_domain_destroy(struct domain *d) {}
>>>>> +
>>>>> +#endif /* HAS_PASSTHROUGH */
>>>>> +
>>>>> /*
>>>>>  * The following flags are passed to map (applicable ones also to unmap)
>>>>>  * operations, while some are passed back by lookup operations.
>>>>> @@ -209,6 +233,8 @@ struct msi_msg;
>>>>> #ifdef CONFIG_HAS_DEVICE_TREE
>>>>> #include <xen/device_tree.h>
>>>>> +#ifdef CONFIG_HAS_PASSTHROUGH
>>>>> +
>>>>> int iommu_assign_dt_device(struct domain *d, struct dt_device_node *dev);
>>>>> int iommu_deassign_dt_device(struct domain *d, struct dt_device_node *dev);
>>>>> int iommu_dt_domain_init(struct domain *d);
>>>>> @@ -238,6 +264,26 @@ int iommu_do_dt_domctl(struct xen_domctl *domctl, struct domain *d,
>>>>>  */
>>>>> int iommu_remove_dt_device(struct dt_device_node *np);
>>>>> +#else
>>>>> +
>>>>> +static inline int iommu_assign_dt_device(struct domain *d,
>>>>> +                                         struct dt_device_node *dev)
>>>>> +{
>>>>> +    return -EINVAL;
>>>>> +}
>>>>> +
>>>>> +static inline int iommu_add_dt_device(struct dt_device_node *np)
>>>>> +{
>>>>> +    return 1;
>>>>
>>>> I would suggest to add a comment explain what 1 means. IIRC, this means "no iommu" present.
>>>
>>> Would it be ok something like in iommu_domain_init:
>>>
>>> /*
>>> * Returns as the real iommu_add_dt_device() would: Error “no iommu" because
>>> * !iommu_enabled due to the fact that !HAS_PASSTHROUGH
>>> */
>>
>> We had been there before, hadn't we? Personally I find the suggested text
>> hard to follow.
> 
> well, I’ve taken your suggestion for iommu_domain_init and adapted here, 
> maybe the adaptation didn’t meet your criteria then :)

I wasn't overly happy with that other comment either.

> But ...
> 
>> How about
>>
>>    /*
>>     * !HAS_PASSTHROUGH => !iommu_enabled (see the non-stub
>>     * iommu_add_dt_device())
>>     */
>>
>> If that's too terse, adding a few words should be fine.
> 
> as long as you are happy with that, I’m happy as well, please let me know
> if you want a v5 with this or if you would do the modificaiton on commit.

I first need to get to looking at v4 as a whole.

Jan

On 07/03/2025 09:20, Luca Fancellu wrote:
>> On 7 Mar 2025, at 09:09, Julien Grall <julien@xen.org> wrote:
>>>   /*
>>>    * The following flags are passed to map (applicable ones also to unmap)
>>>    * operations, while some are passed back by lookup operations.
>>> @@ -209,6 +233,8 @@ struct msi_msg;
>>>   #ifdef CONFIG_HAS_DEVICE_TREE
>>>   #include <xen/device_tree.h>
>>>   +#ifdef CONFIG_HAS_PASSTHROUGH
>>> +
>>>   int iommu_assign_dt_device(struct domain *d, struct dt_device_node *dev);
>>>   int iommu_deassign_dt_device(struct domain *d, struct dt_device_node *dev);
>>>   int iommu_dt_domain_init(struct domain *d);
>>> @@ -238,6 +264,26 @@ int iommu_do_dt_domctl(struct xen_domctl *domctl, struct domain *d,
>>>    */
>>>   int iommu_remove_dt_device(struct dt_device_node *np);
>>>   +#else
>>> +
>>> +static inline int iommu_assign_dt_device(struct domain *d,
>>> +                                         struct dt_device_node *dev)
>>> +{
>>> +    return -EINVAL;
>>> +}
>>> +
>>> +static inline int iommu_add_dt_device(struct dt_device_node *np)
>>> +{
>>> +    return 1;
>>
>> I would suggest to add a comment explain what 1 means. IIRC, this means "no iommu" present.
> 
> Would it be ok something like in iommu_domain_init:
> 
> /*
>   * Returns as the real iommu_add_dt_device() would: Error “no iommu" because
>   * !iommu_enabled due to the fact that !HAS_PASSTHROUGH
>   */

That's fine with me. The iommu code is technically maintained by Jan, so 
I will wait for him to confirm he is fine with the patch before committing.

Cheers,

-- 
Julien Grall