On 02.03.2022 23:10, Andrew Cooper wrote: > The linker script collecting .init.rodata.* ahead of .init.rodata.cf_clobber > accidentally causes __initconst_cf_clobber to be a no-op. > > Rearrange the linker script to unbreak this. > > The IOMMU adjust_irq_affinities() hooks currently violate the safety > requirement for being cf_clobber, by also being __initcall(). > > Consolidate to a single initcall using iommu_call() (satisfying the cf_clobber > safety requirement), and also removes the dubious property that we'd call into > both vendors IOMMU drivers on boot, relying on the for_each_*() loops to be > empty for safety. > > With this fixed, an all-enabled build of Xen has 1681 endbr64's (1918 > including .init.text) with 382 (23%) being clobbered during boot. > > Signed-off-by: Andrew Cooper <[email protected]>
This will do for the immediate purpose, so: Reviewed-by: Jan Beulich <[email protected]> > I was unsure whether to go common or x86 spefific IOMMU code, so went with the > conservative option. The final hunk can trivially move if preferred. The hook is x86-specific, so the wrapper should be too (and the existing inline wrapper also is). > --- a/xen/arch/x86/xen.lds.S > +++ b/xen/arch/x86/xen.lds.S > @@ -210,6 +210,12 @@ SECTIONS > DECL_SECTION(.init.data) { > #endif > > + . = ALIGN(POINTER_ALIGN); > + __initdata_cf_clobber_start = .; > + *(.init.data.cf_clobber) > + *(.init.rodata.cf_clobber) > + __initdata_cf_clobber_end = .; > + > *(.init.rodata) > *(.init.rodata.*) I wonder if this shouldn't really be two sections. Live-patching will need to supply two ranges to apply_alternatives() anyway (one for each section, unless you want to start requiring to pass a linker script to "$(LD) -r" when generating live patches, just to fold the two sections), so in the core hypervisor we may want to follow suit. > --- a/xen/drivers/passthrough/x86/iommu.c > +++ b/xen/drivers/passthrough/x86/iommu.c > @@ -462,6 +462,12 @@ bool arch_iommu_use_permitted(const struct domain *d) > likely(!p2m_get_hostp2m(d)->global_logdirty)); > } > > +static int cf_check __init adjust_irq_affinities(void) > +{ > + return iommu_call(&iommu_ops, adjust_irq_affinities); > +} > +__initcall(adjust_irq_affinities); I assume it is intentional that you didn't re-use the inline wrapper, to avoid its (then non-__init) instantiation to stay with an ENDBR. Yet then you could at least _call_ that wrapper here, instead of open- coding it. And I further think the iommu_enabled checks should move out of the vendor functions, plus the hook also has no need anymore to have a return type of int. I guess I'll make a follow-on patch if you don't want to fold this in here. Jan
