[PATCH v2 0/2] Prerequisite patches for Arm64 MPU build

[PATCH v2 0/2] Prerequisite patches for Arm64 MPU build

[Luca Fancellu]

Hi all,

in order to build Xen for Arm64 with MPU support, there are few changes to
support the compilation of Arm code without HAS_PASSTHROUGH and some refactoring
around the start_xen for Arm.

Luca Fancellu (2):
  xen/passthrough: Provide stub functions when !HAS_PASSTHROUGH
  xen/arm: Restrict Kconfig configuration for LLC coloring

 xen/arch/arm/Kconfig                   |  2 +-
 xen/arch/arm/include/asm/grant_table.h |  5 +--
 xen/arch/arm/setup.c                   |  1 +
 xen/include/xen/iommu.h                | 48 ++++++++++++++++++++++++--
 4 files changed, 51 insertions(+), 5 deletions(-)

-- 
2.34.1

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

[Luca Fancellu]

When Xen is built without HAS_PASSTHROUGH, there are some parts
in arm and x86 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.

Signed-off-by: Luca Fancellu <luca.fancellu@arm.com>
---
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                | 48 ++++++++++++++++++++++++--
 2 files changed, 49 insertions(+), 4 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 b928c67e1995..3e49fef8544e 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,24 @@ 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 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 +229,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 +260,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;
@@ -381,17 +423,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)
 
+/* Does the IOMMU pagetable need to be kept synchronized with the P2M */
+#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

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

[Jan Beulich]

On 17.02.2025 11:27, Luca Fancellu wrote:
> When Xen is built without HAS_PASSTHROUGH, there are some parts
> in arm and x86 where iommu_* functions are called in the codebase,
> but their implementation is under xen/drivers/passthrough that is
> not built.

Why the mention of x86, where HAS_PASSTHROUGH is always selected?

> 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.

Is this fixing a build issue when MPU=y? If so, ...

> For gnttab_need_iommu_mapping() in the Arm part, modify the macro
> to use IS_ENABLED for the HAS_PASSTHROUGH Kconfig.
> 
> Signed-off-by: Luca Fancellu <luca.fancellu@arm.com>

... is there a Fixes: tag missing?

> --- 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,24 @@ 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 0;

Shouldn't this fail when is_iommu_enabled(d) is true? (The use of the
predicate here as well as in the real function is slightly strange, but
that's the way it is.)

> @@ -381,17 +423,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)
>  
> +/* Does the IOMMU pagetable need to be kept synchronized with the P2M */

This comment belongs to just ...

> +#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)

... this, not also iommu_use_hap_pt(). There's a connection between the
two, but that is unrelated to what the comment says. I'm also not clear
about the need for ...

>  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; })

... this change, i.e. the need for a stub. Afaics there's nothing in the
description to help understanding this need.

Jan

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

[Luca Fancellu]

Hi Jan,

thanks for your review,

> On 17 Feb 2025, at 10:50, Jan Beulich <jbeulich@suse.com> wrote:
> 
> On 17.02.2025 11:27, Luca Fancellu wrote:
>> When Xen is built without HAS_PASSTHROUGH, there are some parts
>> in arm and x86 where iommu_* functions are called in the codebase,
>> but their implementation is under xen/drivers/passthrough that is
>> not built.
> 
> Why the mention of x86, where HAS_PASSTHROUGH is always selected?

sure, I’ll remove x86

> 
>> 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.
> 
> Is this fixing a build issue when MPU=y? If so, ...
> 
>> For gnttab_need_iommu_mapping() in the Arm part, modify the macro
>> to use IS_ENABLED for the HAS_PASSTHROUGH Kconfig.
>> 
>> Signed-off-by: Luca Fancellu <luca.fancellu@arm.com>
> 
> ... is there a Fixes: tag missing?

right, I’ll add a tag, but I don’t expect it to be backported, also the MPU will still
have some changes needed before being able to build, should I put a tag even
if this is the case?

> 
>> --- 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,24 @@ 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 0;
> 
> Shouldn't this fail when is_iommu_enabled(d) is true? (The use of the
> predicate here as well as in the real function is slightly strange, but
> that's the way it is.)

Right, probably you know better this code than me, I started from the assumption
that when !HAS_PASSTHROUGH, 'iommu_enabled' is false.

is_iommu_enabled(d) checks if the domain structure ‘options’ field has
XEN_DOMCTL_CDF_iommu, this flag is set on domain creation when ‘iommu_enabled'
is true on arm and x86.

So when !HAS_PASSTHROUGH can we assume is_iommu_enabled(d) give false?
Or shall we return for example the value of is_iommu_enabled(d)?

> 
>> @@ -381,17 +423,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)
>> 
>> +/* Does the IOMMU pagetable need to be kept synchronized with the P2M */
> 
> This comment belongs to just ...
> 
>> +#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)
> 
> ... this, not also iommu_use_hap_pt().

I’ll move that close to need_iommu_pt_sync(d)

> There's a connection between the
> two, but that is unrelated to what the comment says. I'm also not clear
> about the need for ...
> 
>> 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; })
> 
> ... this change, i.e. the need for a stub. Afaics there's nothing in the
> description to help understanding this need.

Ok, so in arch/arm/p2m.c the function p2m_set_way_flush() uses this,
so I provided a stub, do you think I should provide something in the
commit message to explain that or shold I try to find another way in order to
don’t provide this stub?

Cheers,
Luca

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

[Luca Fancellu]

>>> +static inline int iommu_domain_init(struct domain *d, unsigned int opts)
>>> +{
>>> +    return 0;
>> 
>> Shouldn't this fail when is_iommu_enabled(d) is true? (The use of the
>> predicate here as well as in the real function is slightly strange, but
>> that's the way it is.)
> 
> Right, probably you know better this code than me, I started from the assumption
> that when !HAS_PASSTHROUGH, 'iommu_enabled' is false.
> 
> is_iommu_enabled(d) checks if the domain structure ‘options’ field has
> XEN_DOMCTL_CDF_iommu, this flag is set on domain creation when ‘iommu_enabled'
> is true on arm and x86.
> 
> So when !HAS_PASSTHROUGH can we assume is_iommu_enabled(d) give false?
> Or shall we return for example the value of is_iommu_enabled(d)?

Sorry, just a clarification here, I don’t mean return the value of is_iommu_enabled straight away,
but use this to compute the return value of the stub.

Cheers,
Luca

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

[Jan Beulich]

On 17.02.2025 12:55, Luca Fancellu wrote:
>> On 17 Feb 2025, at 10:50, Jan Beulich <jbeulich@suse.com> wrote:
>> On 17.02.2025 11:27, Luca Fancellu wrote:
>>> When Xen is built without HAS_PASSTHROUGH, there are some parts
>>> in arm and x86 where iommu_* functions are called in the codebase,
>>> but their implementation is under xen/drivers/passthrough that is
>>> not built.
>>
>> Why the mention of x86, where HAS_PASSTHROUGH is always selected?
> 
> sure, I’ll remove x86
> 
>>
>>> 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.
>>
>> Is this fixing a build issue when MPU=y? If so, ...
>>
>>> For gnttab_need_iommu_mapping() in the Arm part, modify the macro
>>> to use IS_ENABLED for the HAS_PASSTHROUGH Kconfig.
>>>
>>> Signed-off-by: Luca Fancellu <luca.fancellu@arm.com>
>>
>> ... is there a Fixes: tag missing?
> 
> right, I’ll add a tag, but I don’t expect it to be backported, also the MPU will still
> have some changes needed before being able to build, should I put a tag even
> if this is the case?

If you're fixing an issue an earlier commit introduced, it's always a
good idea to add a Fixes: tag. That'll also help reviewers and
observers.

>>> --- 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,24 @@ 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 0;
>>
>> Shouldn't this fail when is_iommu_enabled(d) is true? (The use of the
>> predicate here as well as in the real function is slightly strange, but
>> that's the way it is.)
> 
> Right, probably you know better this code than me, I started from the assumption
> that when !HAS_PASSTHROUGH, 'iommu_enabled' is false.
> 
> is_iommu_enabled(d) checks if the domain structure ‘options’ field has
> XEN_DOMCTL_CDF_iommu, this flag is set on domain creation when ‘iommu_enabled'
> is true on arm and x86.
> 
> So when !HAS_PASSTHROUGH can we assume is_iommu_enabled(d) give false?
> Or shall we return for example the value of is_iommu_enabled(d)?

Since HAS_PASSTHROUGH being selected conditionally a (pretty) new, I
fear that assumptions shouldn't be made. It's possible the stub could
remain as is, yet even then - if only for documentation purposes - I'd
suggest to have some ASSERT() there. In the end it all depends on how
XEN_DOMCTL_CDF_iommu is handled when !HAS_PASSTHROUGH.

>>> @@ -381,17 +423,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)
>>>
>>> +/* Does the IOMMU pagetable need to be kept synchronized with the P2M */
>>
>> This comment belongs to just ...
>>
>>> +#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)
>>
>> ... this, not also iommu_use_hap_pt().
> 
> I’ll move that close to need_iommu_pt_sync(d)
> 
>> There's a connection between the
>> two, but that is unrelated to what the comment says. I'm also not clear
>> about the need for ...
>>
>>> 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; })
>>
>> ... this change, i.e. the need for a stub. Afaics there's nothing in the
>> description to help understanding this need.
> 
> Ok, so in arch/arm/p2m.c the function p2m_set_way_flush() uses this,
> so I provided a stub, do you think I should provide something in the
> commit message to explain that or shold I try to find another way in order to
> don’t provide this stub?

Finding another way would be preferred, but isn't a requirement. Looking
at p2m_set_way_flush() I for one can't figure how page table arrangements
can matter there. Nor can I see how "flush by VA" fits with MPU (where,
aiui, there's no real notion of VA). So yes, if this can't be done
differently to avoid the need for the stub, something will imo want saying
in the description.

Jan

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

[Luca Fancellu]

> 
>>>> --- 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,24 @@ 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 0;
>>> 
>>> Shouldn't this fail when is_iommu_enabled(d) is true? (The use of the
>>> predicate here as well as in the real function is slightly strange, but
>>> that's the way it is.)
>> 
>> Right, probably you know better this code than me, I started from the assumption
>> that when !HAS_PASSTHROUGH, 'iommu_enabled' is false.
>> 
>> is_iommu_enabled(d) checks if the domain structure ‘options’ field has
>> XEN_DOMCTL_CDF_iommu, this flag is set on domain creation when ‘iommu_enabled'
>> is true on arm and x86.
>> 
>> So when !HAS_PASSTHROUGH can we assume is_iommu_enabled(d) give false?
>> Or shall we return for example the value of is_iommu_enabled(d)?
> 
> Since HAS_PASSTHROUGH being selected conditionally a (pretty) new, I
> fear that assumptions shouldn't be made. It's possible the stub could
> remain as is, yet even then - if only for documentation purposes - I'd
> suggest to have some ASSERT() there. In the end it all depends on how
> XEN_DOMCTL_CDF_iommu is handled when !HAS_PASSTHROUGH.

I’ve tried to add an ASSERT(!is_iommu_enabled(d)); but it’s not building, I’m starting to think there
is some reason why I can’t do that but I didn’t figure out why, I’ve added the inclusion for xen/sched.h,
but it still says implicit declaration of function ‘is_iommu_enabled’…

But I could assert for !iommu_enabled: I checked into common/domain.c, sanitise_domain_config,
if a domain is called with XEN_DOMCTL_CDF_iommu set, the function would fail if !iommu_enabled,
so I would say that the stub returns the expected value (0) since for sure iommu_enabled is false and
there cannot be a domain with that flag set that has the iommu_enabled=true under !HAS_PASSTHROUGH.

But would it be ok to add this assert (ASSERT(!iommu_enabled);) even if we know that iommu_enabled
is false, since !HAS_PASSTHROUGH ?

Please let me know your thoughts on this.

Cheers,
Luca

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

[Jan Beulich]

On 17.02.2025 17:14, 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,24 @@ 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 0;
>>>>
>>>> Shouldn't this fail when is_iommu_enabled(d) is true? (The use of the
>>>> predicate here as well as in the real function is slightly strange, but
>>>> that's the way it is.)
>>>
>>> Right, probably you know better this code than me, I started from the assumption
>>> that when !HAS_PASSTHROUGH, 'iommu_enabled' is false.
>>>
>>> is_iommu_enabled(d) checks if the domain structure ‘options’ field has
>>> XEN_DOMCTL_CDF_iommu, this flag is set on domain creation when ‘iommu_enabled'
>>> is true on arm and x86.
>>>
>>> So when !HAS_PASSTHROUGH can we assume is_iommu_enabled(d) give false?
>>> Or shall we return for example the value of is_iommu_enabled(d)?
>>
>> Since HAS_PASSTHROUGH being selected conditionally a (pretty) new, I
>> fear that assumptions shouldn't be made. It's possible the stub could
>> remain as is, yet even then - if only for documentation purposes - I'd
>> suggest to have some ASSERT() there. In the end it all depends on how
>> XEN_DOMCTL_CDF_iommu is handled when !HAS_PASSTHROUGH.
> 
> I’ve tried to add an ASSERT(!is_iommu_enabled(d)); but it’s not building, I’m starting to think there
> is some reason why I can’t do that but I didn’t figure out why, I’ve added the inclusion for xen/sched.h,
> but it still says implicit declaration of function ‘is_iommu_enabled’…

Well, xen/sched.h includes xen/iommu.h. Hence when you make the latter
include xen/sched.h, that'll have a meaningful effect on use sites
of xen/iommu.h; wherever xen/sched.h is used the nested #include will
do nothing due to the include guard.

> But I could assert for !iommu_enabled: I checked into common/domain.c, sanitise_domain_config,
> if a domain is called with XEN_DOMCTL_CDF_iommu set, the function would fail if !iommu_enabled,
> so I would say that the stub returns the expected value (0) since for sure iommu_enabled is false and
> there cannot be a domain with that flag set that has the iommu_enabled=true under !HAS_PASSTHROUGH.
> 
> But would it be ok to add this assert (ASSERT(!iommu_enabled);) even if we know that iommu_enabled
> is false, since !HAS_PASSTHROUGH ?

Such an assertion then isn't very useful, imo. Since, as you say,
sanitise_domain_config() properly covers the !HAS_PASSTHROUGH case even
for cases like the MPU one, I think the code is fine then. A brief
comment might be nice ...

Jan

[PATCH v2 2/2] xen/arm: Restrict Kconfig configuration for LLC coloring

[Luca Fancellu]

LLC coloring can be used only on MMU system, move the code
that selects it from ARM_64 to MMU and add the ARM_64
dependency.

While there, add a clarification comment in the startup
code related to the LLC coloring, because boot_fdt_info()
is required to be called before llc_coloring_init(), because
it parses the memory banks from the DT, but to discover that
the developer needs to dig into the function.

Signed-off-by: Luca Fancellu <luca.fancellu@arm.com>
---
v2 changes:
 - dropped part of the v1 code, now this one is simpler, I will
   discuss better how to design a common boot flow for MPU and
   implement on another patch.

---
---
 xen/arch/arm/Kconfig | 2 +-
 xen/arch/arm/setup.c | 1 +
 2 files changed, 2 insertions(+), 1 deletion(-)

diff --git a/xen/arch/arm/Kconfig b/xen/arch/arm/Kconfig
index a26d3e11827c..ffdff1f0a36c 100644
--- a/xen/arch/arm/Kconfig
+++ b/xen/arch/arm/Kconfig
@@ -8,7 +8,6 @@ config ARM_64
 	depends on !ARM_32
 	select 64BIT
 	select HAS_FAST_MULTIPLY
-	select HAS_LLC_COLORING if !NUMA
 
 config ARM
 	def_bool y
@@ -76,6 +75,7 @@ choice
 
 config MMU
 	bool "MMU"
+	select HAS_LLC_COLORING if !NUMA && ARM_64
 	select HAS_PMAP
 	select HAS_VMAP
 	select HAS_PASSTHROUGH
diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
index c1f2d1b89d43..91fa579e73e5 100644
--- a/xen/arch/arm/setup.c
+++ b/xen/arch/arm/setup.c
@@ -328,6 +328,7 @@ void asmlinkage __init start_xen(unsigned long fdt_paddr)
                              (paddr_t)(uintptr_t)(_end - _start), false);
     BUG_ON(!xen_bootmodule);
 
+    /* This parses memory banks needed for LLC coloring */
     fdt_size = boot_fdt_info(device_tree_flattened, fdt_paddr);
 
     cmdline = boot_fdt_cmdline(device_tree_flattened);
-- 
2.34.1

Re: [PATCH v2 2/2] xen/arm: Restrict Kconfig configuration for LLC coloring

[Orzel, Michal]

On 17/02/2025 11:27, Luca Fancellu wrote:
> 
> 
> LLC coloring can be used only on MMU system, move the code
> that selects it from ARM_64 to MMU and add the ARM_64
> dependency.
> 
> While there, add a clarification comment in the startup
> code related to the LLC coloring, because boot_fdt_info()
> is required to be called before llc_coloring_init(), because
> it parses the memory banks from the DT, but to discover that
> the developer needs to dig into the function.
Well, if at all such requirement would better be expressed using ASSERT in
get_xen_paddr(). The reason is ...

> 
> Signed-off-by: Luca Fancellu <luca.fancellu@arm.com>
> ---
> v2 changes:
>  - dropped part of the v1 code, now this one is simpler, I will
>    discuss better how to design a common boot flow for MPU and
>    implement on another patch.
> 
> ---
> ---
>  xen/arch/arm/Kconfig | 2 +-
>  xen/arch/arm/setup.c | 1 +
>  2 files changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/xen/arch/arm/Kconfig b/xen/arch/arm/Kconfig
> index a26d3e11827c..ffdff1f0a36c 100644
> --- a/xen/arch/arm/Kconfig
> +++ b/xen/arch/arm/Kconfig
> @@ -8,7 +8,6 @@ config ARM_64
>         depends on !ARM_32
>         select 64BIT
>         select HAS_FAST_MULTIPLY
> -       select HAS_LLC_COLORING if !NUMA
> 
>  config ARM
>         def_bool y
> @@ -76,6 +75,7 @@ choice
> 
>  config MMU
>         bool "MMU"
> +       select HAS_LLC_COLORING if !NUMA && ARM_64
>         select HAS_PMAP
>         select HAS_VMAP
>         select HAS_PASSTHROUGH
> diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
> index c1f2d1b89d43..91fa579e73e5 100644
> --- a/xen/arch/arm/setup.c
> +++ b/xen/arch/arm/setup.c
> @@ -328,6 +328,7 @@ void asmlinkage __init start_xen(unsigned long fdt_paddr)
>                               (paddr_t)(uintptr_t)(_end - _start), false);
>      BUG_ON(!xen_bootmodule);
> 
> +    /* This parses memory banks needed for LLC coloring */
this comment is confusing. It reads as if boot_fdt_info was here only for LLC
coloring. Moreover, if you add such comment here, why not adding a comment above
boot_fdt_cmdline and cmdline_parse which are hard dependency for LLC coloring
code to read LLC cmdline options parsed by llc_coloring_init?

~Michal

Re: [PATCH v2 2/2] xen/arm: Restrict Kconfig configuration for LLC coloring

[Luca Fancellu]

Hi Michal,

> On 17 Feb 2025, at 12:55, Orzel, Michal <michal.orzel@amd.com> wrote:
> 
> 
> 
> On 17/02/2025 11:27, Luca Fancellu wrote:
>> 
>> 
>> LLC coloring can be used only on MMU system, move the code
>> that selects it from ARM_64 to MMU and add the ARM_64
>> dependency.
>> 
>> While there, add a clarification comment in the startup
>> code related to the LLC coloring, because boot_fdt_info()
>> is required to be called before llc_coloring_init(), because
>> it parses the memory banks from the DT, but to discover that
>> the developer needs to dig into the function.
> Well, if at all such requirement would better be expressed using ASSERT in
> get_xen_paddr().

Ok, you mean asserting that mem ( bootinfo_get_mem() ) is not empty?

> The reason is ...
> 
>> 
>> Signed-off-by: Luca Fancellu <luca.fancellu@arm.com>
>> ---
>> v2 changes:
>> - dropped part of the v1 code, now this one is simpler, I will
>>   discuss better how to design a common boot flow for MPU and
>>   implement on another patch.
>> 
>> ---
>> ---
>> xen/arch/arm/Kconfig | 2 +-
>> xen/arch/arm/setup.c | 1 +
>> 2 files changed, 2 insertions(+), 1 deletion(-)
>> 
>> diff --git a/xen/arch/arm/Kconfig b/xen/arch/arm/Kconfig
>> index a26d3e11827c..ffdff1f0a36c 100644
>> --- a/xen/arch/arm/Kconfig
>> +++ b/xen/arch/arm/Kconfig
>> @@ -8,7 +8,6 @@ config ARM_64
>>        depends on !ARM_32
>>        select 64BIT
>>        select HAS_FAST_MULTIPLY
>> -       select HAS_LLC_COLORING if !NUMA
>> 
>> config ARM
>>        def_bool y
>> @@ -76,6 +75,7 @@ choice
>> 
>> config MMU
>>        bool "MMU"
>> +       select HAS_LLC_COLORING if !NUMA && ARM_64
>>        select HAS_PMAP
>>        select HAS_VMAP
>>        select HAS_PASSTHROUGH
>> diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
>> index c1f2d1b89d43..91fa579e73e5 100644
>> --- a/xen/arch/arm/setup.c
>> +++ b/xen/arch/arm/setup.c
>> @@ -328,6 +328,7 @@ void asmlinkage __init start_xen(unsigned long fdt_paddr)
>>                              (paddr_t)(uintptr_t)(_end - _start), false);
>>     BUG_ON(!xen_bootmodule);
>> 
>> +    /* This parses memory banks needed for LLC coloring */
> this comment is confusing. It reads as if boot_fdt_info was here only for LLC
> coloring. Moreover, if you add such comment here, why not adding a comment above
> boot_fdt_cmdline and cmdline_parse which are hard dependency for LLC coloring
> code to read LLC cmdline options parsed by llc_coloring_init?

Yeah I get your point, do you think we should just go with the assert or maybe add a comment
on top of llc_coloring_init() to say it needs to be called after boot_fdt_info and boot_fdt_cmdline
in order to work? Also because the assert in get_xen_paddr (llc-coloring.c) won’t be compiled on
a setup not having cache coloring

Cheers,
Luca

> 
> ~Michal
> 

Re: [PATCH v2 2/2] xen/arm: Restrict Kconfig configuration for LLC coloring

[Orzel, Michal]

On 17/02/2025 14:15, Luca Fancellu wrote:
> 
> 
> Hi Michal,
> 
>> On 17 Feb 2025, at 12:55, Orzel, Michal <michal.orzel@amd.com> wrote:
>>
>>
>>
>> On 17/02/2025 11:27, Luca Fancellu wrote:
>>>
>>>
>>> LLC coloring can be used only on MMU system, move the code
>>> that selects it from ARM_64 to MMU and add the ARM_64
>>> dependency.
>>>
>>> While there, add a clarification comment in the startup
>>> code related to the LLC coloring, because boot_fdt_info()
>>> is required to be called before llc_coloring_init(), because
>>> it parses the memory banks from the DT, but to discover that
>>> the developer needs to dig into the function.
>> Well, if at all such requirement would better be expressed using ASSERT in
>> get_xen_paddr().
> 
> Ok, you mean asserting that mem ( bootinfo_get_mem() ) is not empty?
> 
>> The reason is ...
>>
>>>
>>> Signed-off-by: Luca Fancellu <luca.fancellu@arm.com>
>>> ---
>>> v2 changes:
>>> - dropped part of the v1 code, now this one is simpler, I will
>>>   discuss better how to design a common boot flow for MPU and
>>>   implement on another patch.
>>>
>>> ---
>>> ---
>>> xen/arch/arm/Kconfig | 2 +-
>>> xen/arch/arm/setup.c | 1 +
>>> 2 files changed, 2 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/xen/arch/arm/Kconfig b/xen/arch/arm/Kconfig
>>> index a26d3e11827c..ffdff1f0a36c 100644
>>> --- a/xen/arch/arm/Kconfig
>>> +++ b/xen/arch/arm/Kconfig
>>> @@ -8,7 +8,6 @@ config ARM_64
>>>        depends on !ARM_32
>>>        select 64BIT
>>>        select HAS_FAST_MULTIPLY
>>> -       select HAS_LLC_COLORING if !NUMA
>>>
>>> config ARM
>>>        def_bool y
>>> @@ -76,6 +75,7 @@ choice
>>>
>>> config MMU
>>>        bool "MMU"
>>> +       select HAS_LLC_COLORING if !NUMA && ARM_64
>>>        select HAS_PMAP
>>>        select HAS_VMAP
>>>        select HAS_PASSTHROUGH
>>> diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
>>> index c1f2d1b89d43..91fa579e73e5 100644
>>> --- a/xen/arch/arm/setup.c
>>> +++ b/xen/arch/arm/setup.c
>>> @@ -328,6 +328,7 @@ void asmlinkage __init start_xen(unsigned long fdt_paddr)
>>>                              (paddr_t)(uintptr_t)(_end - _start), false);
>>>     BUG_ON(!xen_bootmodule);
>>>
>>> +    /* This parses memory banks needed for LLC coloring */
>> this comment is confusing. It reads as if boot_fdt_info was here only for LLC
>> coloring. Moreover, if you add such comment here, why not adding a comment above
>> boot_fdt_cmdline and cmdline_parse which are hard dependency for LLC coloring
>> code to read LLC cmdline options parsed by llc_coloring_init?
> 
> Yeah I get your point, do you think we should just go with the assert or maybe add a comment
> on top of llc_coloring_init() to say it needs to be called after boot_fdt_info and boot_fdt_cmdline
> in order to work? Also because the assert in get_xen_paddr (llc-coloring.c) won’t be compiled on
> a setup not having cache coloring
TBH I would not do anything. I assume such comment would target developers. Then
why are we special casing LLC coloring and not for example boot_fdt_cmdline that
also needs to be called after boot_fdt_info to parse legacy location for
cmdline? There are dozens of examples in start_xen where we rely on a specific
order and developer always needs to check if rearranging is possible. Adding a
single comment for LLC would not improve the situation and would just result in
inconsistency leading to confusion. That's why I would only consider adding an
ASSERT but in this case, there are more things than memory bank that LLC init
relies on.

~Michal

Re: [PATCH v2 2/2] xen/arm: Restrict Kconfig configuration for LLC coloring

[Luca Fancellu]

>>>> diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
>>>> index c1f2d1b89d43..91fa579e73e5 100644
>>>> --- a/xen/arch/arm/setup.c
>>>> +++ b/xen/arch/arm/setup.c
>>>> @@ -328,6 +328,7 @@ void asmlinkage __init start_xen(unsigned long fdt_paddr)
>>>>                             (paddr_t)(uintptr_t)(_end - _start), false);
>>>>    BUG_ON(!xen_bootmodule);
>>>> 
>>>> +    /* This parses memory banks needed for LLC coloring */
>>> this comment is confusing. It reads as if boot_fdt_info was here only for LLC
>>> coloring. Moreover, if you add such comment here, why not adding a comment above
>>> boot_fdt_cmdline and cmdline_parse which are hard dependency for LLC coloring
>>> code to read LLC cmdline options parsed by llc_coloring_init?
>> 
>> Yeah I get your point, do you think we should just go with the assert or maybe add a comment
>> on top of llc_coloring_init() to say it needs to be called after boot_fdt_info and boot_fdt_cmdline
>> in order to work? Also because the assert in get_xen_paddr (llc-coloring.c) won’t be compiled on
>> a setup not having cache coloring
> TBH I would not do anything. I assume such comment would target developers. Then
> why are we special casing LLC coloring and not for example boot_fdt_cmdline that
> also needs to be called after boot_fdt_info to parse legacy location for
> cmdline? There are dozens of examples in start_xen where we rely on a specific
> order and developer always needs to check if rearranging is possible. Adding a
> single comment for LLC would not improve the situation and would just result in
> inconsistency leading to confusion. That's why I would only consider adding an
> ASSERT but in this case, there are more things than memory bank that LLC init
> relies on.

ok, yes of course there are a lot of things that rely on the right order of calls, however I felt
that an optional feature like LLC would benefit for the extra documentation. In other cases
rearranging calls could lead to Xen not booting, but in this case (as it happened to me) it
was not immediately clear.

Anyway if that’s your preference I will drop the comment, I would not add the assert in this
commit as it feels not the right place, but I can see that in get_xen_paddr if mem is empty
we would not touch paddr and have a panic later, so we would notice if something happen.

Cheers,
Luca