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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 19.02.2025 14:06, Luca Fancellu wrote:
>> On 19 Feb 2025, at 12:45, Jan Beulich <jbeulich@xxxxxxxx> wrote:
>> On 18.02.2025 10:51, 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,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)
>>> +{
>>> +    /*
>>> +     * When !HAS_PASSTHROUGH, iommu_enabled is set to false and the 
>>> expected
>>> +     * behaviour of this function is to return success in that case.
>>> +     */
>>> +    return 0;
>>> +}
>>
>> Hmm. Would the function be anywhere near likely to do anything else than
>> what it's expected to do? My original concern here was with "opts"
>> perhaps asking for something that cannot be supported. But that was wrong
>> thinking on my part. Here what you do is effectively open-code what the
>> real iommu_domain_init() would do: Return success when !is_iommu_enabled().
>> Which in turn follows from !iommu_enabled when !HAS_PASSTHROUGH.
>>
>> On that basis I'd be okay if the comment was dropped again. Else it imo
>> wants re-wording to get closer to the explanation above.
> 
> Would it be ok for you a comment saying:
> “This stub returns the same as the real iommu_domain_init()
>  function: success when !is_iommu_enabled(), which value is based on 
> iommu_enabled
> that is false when !HAS_PASSTHROUGH"

I'm sorry, but this is too verbose for my taste. What's wrong with the more
terse

"Return as the real iommu_domain_init() would: Success when
 !is_iommu_enabled(), following from !iommu_enabled when !HAS_PASSTHROUGH"

?

>>> @@ -383,12 +429,12 @@ 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,
>>
>> Coming back to my v2 comment: Why exactly is this change needed here? If
>> things build fine without the macro being defined when !HAS_PASSTHROUGH,
>> surely they will also build fine with it being defined?
> 
> I’ve defined an empty stub on an header included only on MPU systems for the
> p2m module, this is why it is building

But that wasn't part of the patch, was it? I.e. with this series alone
applied, things still don't build?

> I didn’t modify p2m_set_way_flush() which lives in arm common code, because
> it will be used also on MPU systems (R82 has MPU at EL2 but MMU/MPU at EL1)
> and I would like to stay the same and be used by MMU/MPU subsystems.
> 
>> As per the
>> respective revlog entry, this change looks to belong into whatever is
>> going to be done to deal with the one Arm use of the macro. Or maybe
>> it's unneeded altogether.
> 
> I didn’t understand that you were opposing to protecting iommu_use_hap_pt() 
> when
> !HAS_PASSTHROUGH, I thought you were referring only to the stub in the #else
> branch.
> Can I ask why?

Sure. And no, I'm not against the extra protection. I'm against unnecessary
code churn. That is, any such a re-arrangement wants to have some kind of
justification.

> in any case when !HAS_PASSTHROUGH, this macro is not usable
> since dom_iommu() is resolved to a membed that doesn’t exist in the 
> configuration,
> am I missing something?

You very likely aren't, yet the macro's presence also does no harm. We
have lots of macros and declarations which are usable only in certain
configurations. Sometimes this just happens to be that way, sometimes it's
actually deliberate (e.g. to facilitate DCE).

Jan