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

[Luca Fancellu <Luca.Fancellu@xxxxxxx>]

Hi Jan,

thanks for your review,

> On 17 Feb 2025, at 10:50, Jan Beulich <jbeulich@xxxxxxxx> 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@xxxxxxx>
> 
> ... 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