On 03/03/2022 07:35, Jan Beulich wrote:
> 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]>
Thanks.
>> --- 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.
I don't see why livepatches would need two sections - they're linked in
a similar way to Xen IIRC. Either way, if changes are needed, they
should be part of the livepatch work.
>> --- 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.
No - that was unintentional. I only merged the initcalls late during
development and forgot the wrapper.
I've adjusted to:
- return iommu_call(&iommu_ops, adjust_irq_affinities);
+ return iommu_adjust_irq_affinities();
> 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.
Yeah, I'd prefer not to fold cleanup into this bugfix, but there are
certainly improvements to be done.
~Andrew
