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

[Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>]

On 11/13/26 11:55 AM, Andrew Cooper wrote:
> On 13/11/2026 8:51 am, Oleksii Kurochko wrote:
> > 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).
> __read_mostly is indeed meant to reduce false sharing, but this patch
> does not introduce any.
>
> HYPFS_PARAM should have been read_mostly to begin with, and page
> alignment is greater th cache alignment so there's no sharing there either.
Thanks for clarifying. Then:

Reviewed-by: Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>

~ Oleksii