Re: [PATCH] xen/link: Don't separate .data.read_mostly from .data

[Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>]

On 11/12/26 7:58 PM, Andrew Cooper wrote:
> This appears to be an error in x86 which got copied into all other
> architectures.  .data.read_mostly is an internal detail, and none of the
> similar sections get exposed externally.
>
> Simply place .data.read_mostly in .data.
>
> No functional change.
>
> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> ---
> CC: Jan Beulich <jbeulich@xxxxxxxx>
> CC: Roger Pau Monné <roger@xxxxxxxxxxxxxx>
> CC: Teddy Astie <teddy.astie@xxxxxxxxxx>
> CC: Stefano Stabellini <sstabellini@xxxxxxxxxx>
> CC: Julien Grall <julien@xxxxxxx>
> CC: Volodymyr Babchuk <Volodymyr_Babchuk@xxxxxxxx>
> CC: Bertrand Marquis <bertrand.marquis@xxxxxxx>
> CC: Michal Orzel <michal.orzel@xxxxxxx>
> CC: Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>
> CC: Baptiste Le Duc <baptiste.le-duc@xxxxxxxxxx>
> CC: Timothy Pearson <tpearson@xxxxxxxxxxxxxxxxxxxxx>
> CC: Ross Lagerwall <ross.lagerwall@xxxxxxxxxx>
>
> This is to simplfy the script checking that xen.efi's section
> attributes/alignments are correct for UEFI SecureBoot.
> ---
>   xen/arch/arm/xen.lds.S   | 8 +++-----
>   xen/arch/ppc/xen.lds.S   | 8 +++-----
>   xen/arch/riscv/xen.lds.S | 7 ++-----
>   xen/arch/x86/xen.lds.S   | 4 +---
>   4 files changed, 9 insertions(+), 18 deletions(-)
>
> diff --git a/xen/arch/arm/xen.lds.S b/xen/arch/arm/xen.lds.S
> index d1bb6c62d208..8d0954c90afd 100644
> --- a/xen/arch/arm/xen.lds.S
> +++ b/xen/arch/arm/xen.lds.S
> @@ -86,16 +86,14 @@ SECTIONS
>         __ro_after_init_end = .;
>     } : text
>   
> -  .data.read_mostly : {
> -       *(.data.read_mostly)
> -  } :text
> -
>     . = ALIGN(SMP_CACHE_BYTES);
>     .data : {                    /* Data */
> -       *(.data.page_aligned)
> +       *(.data.read_mostly)
>   
>          HYPFS_PARAM
>   
> +       *(.data.page_aligned)
> +
>          *(.data .data.*)
>          CONSTRUCTORS
>     } :text
> diff --git a/xen/arch/ppc/xen.lds.S b/xen/arch/ppc/xen.lds.S
> index cddb70855307..b2159279169c 100644
> --- a/xen/arch/ppc/xen.lds.S
> +++ b/xen/arch/ppc/xen.lds.S
> @@ -77,16 +77,14 @@ SECTIONS
>           __ro_after_init_end = .;
>       } :text
>   
> -    DECL_SECTION(.data.read_mostly) {
> -        *(.data.read_mostly)
> -    } :text
> -
>       . = ALIGN(PAGE_SIZE);
>       DECL_SECTION(.data) {                    /* Data */
> -        *(.data.page_aligned)
> +        *(.data.read_mostly)
>   
>           HYPFS_PARAM
>   
> +        *(.data.page_aligned)
> +
>           *(.data .data.*)
>           CONSTRUCTORS
>       } :text
> diff --git a/xen/arch/riscv/xen.lds.S b/xen/arch/riscv/xen.lds.S
> index 250ee3f36f4f..d1c5ce59c90d 100644
> --- a/xen/arch/riscv/xen.lds.S
> +++ b/xen/arch/riscv/xen.lds.S
> @@ -84,16 +84,13 @@ SECTIONS
>           __ro_after_init_end = .;
>       } : text
>   
> -    .data.read_mostly : {
> -        *(.data.read_mostly)
> -    } :text
> -
>       . = ALIGN(PAGE_SIZE);
>       .data : {                    /* Data */
> -        *(.data.page_aligned)
> +        *(.data.read_mostly)
>   
>           HYPFS_PARAM
>   
> +        *(.data.page_aligned)
>           *(.sdata .sdata.*)
>           *(.data .data.*)
>           CONSTRUCTORS
> diff --git a/xen/arch/x86/xen.lds.S b/xen/arch/x86/xen.lds.S
> index 925848980219..99802aed4e13 100644
> --- a/xen/arch/x86/xen.lds.S
> +++ b/xen/arch/x86/xen.lds.S
> @@ -295,13 +295,11 @@ SECTIONS
>   
>     __2M_rwdata_start = .;       /* Start of 2M superpages, mapped RW. */
>     . = ALIGN(SMP_CACHE_BYTES);
> -  DECL_SECTION(.data.read_mostly) {
> +  DECL_SECTION(.data) {
>          *(.data.read_mostly)
>   
>          HYPFS_PARAM
> -  } PHDR(text)
>   
> -  DECL_SECTION(.data) {
>          *(.data.page_aligned)
>          *(.data .data.*)
>     } PHDR(text)
The ordering is preserved, but the alignment between the two isn't.
Today the ALIGN() before .data sits between .data.read_mostly and
regular .data.  After this patch it sits before .data.read_mostly, and
the only thing separating it from *(.sdata/.data) is
*(.data.page_aligned), which has no inputs on RISC-V, PPC or Arm with
CONFIG_MPU. So the tail of .data.read_mostly will share a cache line
with ordinary .data, which is the false sharing __read_mostly is meant
to avoid (IIUC).

Could you add ". = ALIGN(SMP_CACHE_BYTES);" after HYPFS_PARAM, so the
separation doesn't depend on whether an arch has any
.data.page_aligned?

~ Oleksii