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

[Jan Beulich <jbeulich@suse.com>]

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