Re: [PATCH] xen/CET: Fix __initconst_cf_clobber

[Jan Beulich <jbeulich@xxxxxxxx>]

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 <andrew.cooper3@xxxxxxxxxx>

This will do for the immediate purpose, so:
Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>

> 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