Hi Jan, thanks for your review,
> On 17 Feb 2025, at 10:50, Jan Beulich <[email protected]> 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 <[email protected]> > > ... 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? > >> --- 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)? > >> @@ -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? Cheers, Luca
