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@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Wei Liu <wl@xen.org>

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.
---
 xen/arch/x86/xen.lds.S                   | 12 ++++++------
 xen/drivers/passthrough/amd/iommu_init.c |  1 -
 xen/drivers/passthrough/vtd/iommu.c      |  1 -
 xen/drivers/passthrough/x86/iommu.c      |  6 ++++++
 4 files changed, 12 insertions(+), 8 deletions(-)

diff --git a/xen/arch/x86/xen.lds.S b/xen/arch/x86/xen.lds.S
index 83def6541ebd..b15e5b67e4a4 100644
--- 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.*)
 
@@ -224,12 +230,6 @@ SECTIONS
        *(.initcall1.init)
        __initcall_end = .;
 
-       . = ALIGN(POINTER_ALIGN);
-       __initdata_cf_clobber_start = .;
-       *(.init.data.cf_clobber)
-       *(.init.rodata.cf_clobber)
-       __initdata_cf_clobber_end = .;
-
        *(.init.data)
        *(.init.data.rel)
        *(.init.data.rel.*)
diff --git a/xen/drivers/passthrough/amd/iommu_init.c b/xen/drivers/passthrough/amd/iommu_init.c
index 657c7f619a51..2e5bffa732e7 100644
--- a/xen/drivers/passthrough/amd/iommu_init.c
+++ b/xen/drivers/passthrough/amd/iommu_init.c
@@ -831,7 +831,6 @@ int cf_check iov_adjust_irq_affinities(void)
 
     return 0;
 }
-__initcall(iov_adjust_irq_affinities);
 
 /*
  * Family15h Model 10h-1fh erratum 746 (IOMMU Logging May Stall Translations)
diff --git a/xen/drivers/passthrough/vtd/iommu.c b/xen/drivers/passthrough/vtd/iommu.c
index 6a65ba1d8271..f70d51580657 100644
--- a/xen/drivers/passthrough/vtd/iommu.c
+++ b/xen/drivers/passthrough/vtd/iommu.c
@@ -2119,7 +2119,6 @@ static int cf_check adjust_vtd_irq_affinities(void)
 
     return 0;
 }
-__initcall(adjust_vtd_irq_affinities);
 
 static int __must_check init_vtd_hw(bool resume)
 {
diff --git a/xen/drivers/passthrough/x86/iommu.c b/xen/drivers/passthrough/x86/iommu.c
index 58a422fb5f88..6ef580215bc2 100644
--- 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);
+
 /*
  * Local variables:
  * mode: C
-- 
2.11.0

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@citrix.com>

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

> 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

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 <andrew.cooper3@citrix.com>
> This will do for the immediate purpose, so:
> Reviewed-by: Jan Beulich <jbeulich@suse.com>

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

On 03.03.2022 11:29, Andrew Cooper wrote:
> On 03/03/2022 07:35, Jan Beulich wrote:
>> On 02.03.2022 23:10, Andrew Cooper wrote:
>>> --- 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.

Live patch objects being relocatable ones, their loading logic works with
section boundaries. Hence there'll be two sections of interest, the
boundaries of which are independent and hence need passing as separate
values.

Jan