| Message ID | 20250218095130.2666580-2-luca.fancellu@arm.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Prerequisite patches for Arm64 MPU build | expand |
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. > @@ -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? 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. Jan
> On 19 Feb 2025, at 12:45, Jan Beulich <jbeulich@suse.com> 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
On 19.02.2025 14:06, Luca Fancellu wrote: >> On 19 Feb 2025, at 12:45, Jan Beulich <jbeulich@suse.com> 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
> On 19 Feb 2025, at 13:30, Jan Beulich <jbeulich@suse.com> wrote: > > On 19.02.2025 14:06, Luca Fancellu wrote: >>> On 19 Feb 2025, at 12:45, Jan Beulich <jbeulich@suse.com> 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
On 19.02.2025 16:25, Luca Fancellu wrote: >> On 19 Feb 2025, at 13:30, Jan Beulich <jbeulich@suse.com> wrote: >> On 19.02.2025 14:06, Luca Fancellu wrote: >>>> On 19 Feb 2025, at 12:45, Jan Beulich <jbeulich@suse.com> wrote: >>>> 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. I think I indicated earlier that I'd like the Arm maintainers to voice their preference. Doing it in iommu.h may be okay, but also may not be. Yet to decide that very Arm use of the macro needs taking into account, and I lack context there. >>> 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. Yes, except that this wasn't said anywhere. Also if you mean to take care of this macro here, then in full please. I.e. either don't touch that area of the header at all, or provide (wherever suitable) a stub alongside moving the #ifdef. Jan
diff --git a/xen/arch/arm/include/asm/grant_table.h b/xen/arch/arm/include/asm/grant_table.h index d3c518a926b9..c5d87b60c4df 100644 --- a/xen/arch/arm/include/asm/grant_table.h +++ b/xen/arch/arm/include/asm/grant_table.h @@ -73,8 +73,9 @@ int replace_grant_host_mapping(uint64_t gpaddr, mfn_t frame, #define gnttab_status_gfn(d, t, i) \ page_get_xenheap_gfn(gnttab_status_page(t, i)) -#define gnttab_need_iommu_mapping(d) \ - (is_domain_direct_mapped(d) && is_iommu_enabled(d)) +#define gnttab_need_iommu_mapping(d) \ + (IS_ENABLED(CONFIG_HAS_PASSTHROUGH) && is_domain_direct_mapped(d) && \ + is_iommu_enabled(d)) #endif /* __ASM_GRANT_TABLE_H__ */ /* diff --git a/xen/include/xen/iommu.h b/xen/include/xen/iommu.h index 77a514019cc6..b6ba3adcbc8a 100644 --- 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; +} + +static inline void iommu_hwdom_init(struct domain *d) {} + +static inline void iommu_domain_destroy(struct domain *d) {} + +#endif /* HAS_PASSTHROUGH */ + /* * The following flags are passed to map (applicable ones also to unmap) * operations, while some are passed back by lookup operations. @@ -209,6 +233,8 @@ struct msi_msg; #ifdef CONFIG_HAS_DEVICE_TREE #include <xen/device_tree.h> +#ifdef CONFIG_HAS_PASSTHROUGH + int iommu_assign_dt_device(struct domain *d, struct dt_device_node *dev); int iommu_deassign_dt_device(struct domain *d, struct dt_device_node *dev); int iommu_dt_domain_init(struct domain *d); @@ -238,6 +264,26 @@ int iommu_do_dt_domctl(struct xen_domctl *domctl, struct domain *d, */ int iommu_remove_dt_device(struct dt_device_node *np); +#else + +static inline int iommu_assign_dt_device(struct domain *d, + struct dt_device_node *dev) +{ + return -EINVAL; +} + +static inline int iommu_add_dt_device(struct dt_device_node *np) +{ + return 1; +} + +static inline int iommu_release_dt_devices(struct domain *d) +{ + return 0; +} + +#endif /* HAS_PASSTHROUGH */ + #endif /* HAS_DEVICE_TREE */ struct page_info; @@ -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,
When Xen is built without HAS_PASSTHROUGH, there are some parts in arm where iommu_* functions are called in the codebase, but their implementation is under xen/drivers/passthrough that is not built. 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. For gnttab_need_iommu_mapping() in the Arm part, modify the macro to use IS_ENABLED for the HAS_PASSTHROUGH Kconfig. Fixes: 0388a5979b21 ("xen/arm: mpu: Introduce choice between MMU and MPU") Signed-off-by: Luca Fancellu <luca.fancellu@arm.com> --- v3 Changes: - removed stub for iommu_use_hap_pt, another solution will be done for the instance in common arm code. - Moved a comment close to the macro it was referred to - add comment to iommu_domain_init() stub - modified commit message - Add fixes tag v2 Changes: - modify gnttab_need_iommu_mapping to use IS_ENABLED - removed macro that didn't allow some of the parameter to be evaluated - Changed commit message --- --- xen/arch/arm/include/asm/grant_table.h | 5 +-- xen/include/xen/iommu.h | 48 +++++++++++++++++++++++++- 2 files changed, 50 insertions(+), 3 deletions(-)