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

[Luca Fancellu <Luca.Fancellu@xxxxxxx>]

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


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

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

Cheers,
Luca