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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 17.02.2025 12:55, Luca Fancellu wrote:
>> 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?

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