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

[Luca Fancellu <Luca.Fancellu@xxxxxxx>]

> On 19 Feb 2025, at 13:30, Jan Beulich <jbeulich@xxxxxxxx> wrote:
> 
> 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"
> 
> ?

nothing wrong, I’ll use that, thanks for confirming.

> 
>>>> @@ -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?

yes, before building there are other bits needed, this is only a small step 
towards that.

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

ok, yes the justification is that MPU system will be built with 
!HAS_PASSTHROUGH,
but there is a common function (p2m_set_way_flush) to MMU/MPU subsystem that
I would like to keep common, to do so I have to hide the macro in this 
particular
configuration and afterwards I have two choices:

1) provide a stub implementation on the Arm side
2) provide a stub implementation in iommu.h

number 2 felt better because it could be applied on any Xen configuration 
without
HAS_PASSTHROUGH, even if at the moment there is only MPU.

Number 1 let the possibility for the specific configuration to choose what to 
do in absence
of HAS_PASSTHROUGH.

Now I would like your view on what would be acceptable here.

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

Ok, in this particular case, as I explained above, this macro is one of the 
thing preventing
Arm MPU side to build, otherwise I wouldn’t have touched it.

Cheers,
Luca