Re: [PATCH for-4.17] xen/arm: Support properly __ro_after_init on Arm

[Julien Grall <julien@xxxxxxx>]

On 17/08/2022 07:33, Penny Zheng wrote:
> Hi Julien
Hi Penny,

> > -----Original Message-----
> > From: Xen-devel <xen-devel-bounces@xxxxxxxxxxxxxxxxxxxx> On Behalf Of
> > Julien Grall
> > Sent: Wednesday, August 17, 2022 3:00 AM
> > To: xen-devel@xxxxxxxxxxxxxxxxxxxx
> > Cc: julien@xxxxxxx; Julien Grall <jgrall@xxxxxxxxxx>; Stefano Stabellini
> > <sstabellini@xxxxxxxxxx>; Bertrand Marquis <Bertrand.Marquis@xxxxxxx>;
> > Volodymyr Babchuk <Volodymyr_Babchuk@xxxxxxxx>
> > Subject: [PATCH for-4.17] xen/arm: Support properly __ro_after_init on Arm
> >
> > From: Julien Grall <jgrall@xxxxxxxxxx>
> >
> > __ro_after_init was introduced recently to prevent modifying some variables
> > after init.
> >
> > At the moment, on Arm, the variables will still be accessible because the
> > region permission is not updated.
> >
> > Address that, but moving the sections .data.ro_after_init out of .data and
> > then mark the region read-only once we finish to boot.
> >
> > Signed-off-by: Julien Grall <jgrall@xxxxxxxxxx>
> Reviewed-by: Penny Zheng <penny.zheng@xxxxxxx>
Thanks!

> > ---
> >
> > This patch is targeting Xen 4.17. There are quite a few arm specific variables
> > that could be switch to use __ro_after_init.
> >
> > This is not addressed by the commit. We could consider to switch some of
> > them for Xen 4.17. So the benefits for now is any common variables using
> > __ro_after_init.
> > ---
> >   xen/arch/arm/include/asm/setup.h |  2 ++
> >   xen/arch/arm/setup.c             | 14 ++++++++++++++
> >   xen/arch/arm/xen.lds.S           |  7 +++++++
> >   3 files changed, 23 insertions(+)
> >
> > diff --git a/xen/arch/arm/include/asm/setup.h
> > b/xen/arch/arm/include/asm/setup.h
> > index 2bb01ecfa88f..5815ccf8c5cc 100644
> > --- a/xen/arch/arm/include/asm/setup.h
> > +++ b/xen/arch/arm/include/asm/setup.h
> > @@ -137,6 +137,8 @@ u32 device_tree_get_u32(const void *fdt, int node,
> > int map_range_to_domain(const struct dt_device_node *dev,
> >                           u64 addr, u64 len, void *data);
> >
> > +extern const char __ro_after_init_start[], __ro_after_init_end[];
> > +
> >   #endif
> >   /*
> >    * Local variables:
> > diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c index
> > 500307edc08d..5bde321b9d07 100644
> > --- a/xen/arch/arm/setup.c
> > +++ b/xen/arch/arm/setup.c
> > @@ -75,10 +75,24 @@ domid_t __read_mostly max_init_domid;
> >
> >   static __used void init_done(void)
> >   {
> > +    int rc;
> > +
> >       /* Must be done past setting system_state. */
> >       unregister_init_virtual_region();
> >
> >       free_init_memory();
> > +
> > +    /*
> > +     * We have finished to boot. Mark the section .data.ro_after_init
> > +     * read-only.
> > +     */
> Nit: Maybe it is finish + doing, could be wrong, feel free to change or not~~
I will update.

Cheers,

--
Julien Grall