| Message ID | 20240419100217.12072-2-roger.pau@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | livepatch: make symbol resolution more robust | expand |
On 19/04/2024 11:02 am, Roger Pau Monne wrote: > Start by declaring the beginning and end of the init section. > > No functional change intended. > > Requested-by: Andrew Cooper <andrew.cooper3@citrix.com> > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> TYVM for doing this. There's a lot of cleanup which can follow on for it. Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com> although if anyone has a better name than sections.h then speak now.
On 19.04.2024 12:08, Andrew Cooper wrote: > On 19/04/2024 11:02 am, Roger Pau Monne wrote: >> Start by declaring the beginning and end of the init section. >> >> No functional change intended. >> >> Requested-by: Andrew Cooper <andrew.cooper3@citrix.com> >> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> > > TYVM for doing this. There's a lot of cleanup which can follow on for it. > > Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com> > > although if anyone has a better name than sections.h then speak now. For what is put there now, and for any other section bounds markers the name is fine with me. I'm not presently convinced though we want to put __read_mostly and friends there. Jan
On 19/04/2024 11:12 am, Jan Beulich wrote: > On 19.04.2024 12:08, Andrew Cooper wrote: >> On 19/04/2024 11:02 am, Roger Pau Monne wrote: >>> Start by declaring the beginning and end of the init section. >>> >>> No functional change intended. >>> >>> Requested-by: Andrew Cooper <andrew.cooper3@citrix.com> >>> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> >> TYVM for doing this. There's a lot of cleanup which can follow on for it. >> >> Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com> >> >> although if anyone has a better name than sections.h then speak now. > For what is put there now, and for any other section bounds markers the > name is fine with me. I'm not presently convinced though we want to put > __read_mostly and friends there. Well that's exactly what I intend to clean up into it, because it's far better in sections.h than (duplicated per arch) cache.h (Also I intend to strip down kernel.h for the other major sections too.) ~Andrew
On 19.04.2024 12:16, Andrew Cooper wrote: > On 19/04/2024 11:12 am, Jan Beulich wrote: >> On 19.04.2024 12:08, Andrew Cooper wrote: >>> On 19/04/2024 11:02 am, Roger Pau Monne wrote: >>>> Start by declaring the beginning and end of the init section. >>>> >>>> No functional change intended. >>>> >>>> Requested-by: Andrew Cooper <andrew.cooper3@citrix.com> >>>> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> >>> TYVM for doing this. There's a lot of cleanup which can follow on for it. >>> >>> Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com> >>> >>> although if anyone has a better name than sections.h then speak now. >> For what is put there now, and for any other section bounds markers the >> name is fine with me. I'm not presently convinced though we want to put >> __read_mostly and friends there. > > Well that's exactly what I intend to clean up into it, because it's far > better in sections.h than (duplicated per arch) cache.h The duplication per arch has had a patch pending for a long time, which you've been blocking. What you're suggesting is not only a very different sections-related use of the header (which is probably okay), but also requires touching a fair part of the code base (each and every .c file using __read_mostly). That's what I'd like to avoid. Yet what I could live with if it's not me needing to perform this tedious work (hence why I didn't want to adjust my patch along these lines). > (Also I intend to strip down kernel.h for the other major sections too.) I certainly have no issue with this; I was in fact already expecting that to happen as a follow-on step. Jan
On 19.04.2024 12:02, Roger Pau Monne wrote: > Start by declaring the beginning and end of the init section. > > No functional change intended. > > Requested-by: Andrew Cooper <andrew.cooper3@citrix.com> > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> > --- > xen/arch/arm/mmu/setup.c | 3 +-- > xen/arch/x86/setup.c | 3 +-- > xen/include/xen/sections.h | 17 +++++++++++++++++ > 3 files changed, 19 insertions(+), 4 deletions(-) > create mode 100644 xen/include/xen/sections.h Noticing a few things only after committing / pushing: For one, I should have waited for an Arm ack. I hope that's not a big issue. > --- a/xen/arch/arm/mmu/setup.c > +++ b/xen/arch/arm/mmu/setup.c > @@ -7,6 +7,7 @@ > > #include <xen/init.h> > #include <xen/libfdt/libfdt.h> > +#include <xen/sections.h> > #include <xen/sizes.h> > #include <xen/vmap.h> > > @@ -62,8 +63,6 @@ vaddr_t directmap_virt_start __read_mostly; > unsigned long directmap_base_pdx __read_mostly; > #endif > > -extern char __init_begin[], __init_end[]; Then I wonder why it was this rather than ... > --- a/xen/arch/x86/setup.c > +++ b/xen/arch/x86/setup.c > @@ -5,6 +5,7 @@ > #include <xen/param.h> > #include <xen/sched.h> > #include <xen/domain.h> > +#include <xen/sections.h> > #include <xen/serial.h> > #include <xen/softirq.h> > #include <xen/acpi.h> > @@ -309,8 +310,6 @@ void __init discard_initial_images(void) > initial_images = NULL; > } > > -extern unsigned char __init_begin[], __init_end[]; ... this ... > --- /dev/null > +++ b/xen/include/xen/sections.h > @@ -0,0 +1,17 @@ > +/* SPDX-License-Identifier: GPL-2.0-only */ > + > +#ifndef __XEN_SECTIONS_H__ > +#define __XEN_SECTIONS_H__ > + > +/* SAF-0-safe */ > +extern char __init_begin[], __init_end[]; ... that was moved. "unsigned char" is generally preferred over declaring binary stuff "strings". I further wonder why the opportunity wasn't taken to make both array-of-const. And finally I'm slightly puzzled by the SAF comment appearing with no mention at all in the description; of course I don't mind its addition. Jan
On Tue, Apr 23, 2024 at 02:25:16PM +0200, Jan Beulich wrote: > On 19.04.2024 12:02, Roger Pau Monne wrote: > Then I wonder why it was this rather than ... > > > --- a/xen/arch/x86/setup.c > > +++ b/xen/arch/x86/setup.c > > @@ -5,6 +5,7 @@ > > #include <xen/param.h> > > #include <xen/sched.h> > > #include <xen/domain.h> > > +#include <xen/sections.h> > > #include <xen/serial.h> > > #include <xen/softirq.h> > > #include <xen/acpi.h> > > @@ -309,8 +310,6 @@ void __init discard_initial_images(void) > > initial_images = NULL; > > } > > > > -extern unsigned char __init_begin[], __init_end[]; > > ... this ... > > > --- /dev/null > > +++ b/xen/include/xen/sections.h > > @@ -0,0 +1,17 @@ > > +/* SPDX-License-Identifier: GPL-2.0-only */ > > + > > +#ifndef __XEN_SECTIONS_H__ > > +#define __XEN_SECTIONS_H__ > > + > > +/* SAF-0-safe */ > > +extern char __init_begin[], __init_end[]; > > ... that was moved. "unsigned char" is generally preferred over > declaring binary stuff "strings". OK, noted. > I further wonder why the opportunity > wasn't taken to make both array-of-const. Hm, yes. The sections are mapped RWX IIRC, but there's no reason to not make the markers const. > And finally I'm slightly > puzzled by the SAF comment appearing with no mention at all in the > description; of course I don't mind its addition. I could have noted it in the commit message indeed, sorry. Thanks, Roger.
diff --git a/xen/arch/arm/mmu/setup.c b/xen/arch/arm/mmu/setup.c index c0cb17ca2ecf..f4bb424c3c91 100644 --- a/xen/arch/arm/mmu/setup.c +++ b/xen/arch/arm/mmu/setup.c @@ -7,6 +7,7 @@ #include <xen/init.h> #include <xen/libfdt/libfdt.h> +#include <xen/sections.h> #include <xen/sizes.h> #include <xen/vmap.h> @@ -62,8 +63,6 @@ vaddr_t directmap_virt_start __read_mostly; unsigned long directmap_base_pdx __read_mostly; #endif -extern char __init_begin[], __init_end[]; - /* Checking VA memory layout alignment. */ static void __init __maybe_unused build_assertions(void) { diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c index 86cd8b999774..dd4d1b2887ee 100644 --- a/xen/arch/x86/setup.c +++ b/xen/arch/x86/setup.c @@ -5,6 +5,7 @@ #include <xen/param.h> #include <xen/sched.h> #include <xen/domain.h> +#include <xen/sections.h> #include <xen/serial.h> #include <xen/softirq.h> #include <xen/acpi.h> @@ -309,8 +310,6 @@ void __init discard_initial_images(void) initial_images = NULL; } -extern unsigned char __init_begin[], __init_end[]; - static void __init init_idle_domain(void) { scheduler_init(); diff --git a/xen/include/xen/sections.h b/xen/include/xen/sections.h new file mode 100644 index 000000000000..b6cb5604c285 --- /dev/null +++ b/xen/include/xen/sections.h @@ -0,0 +1,17 @@ +/* SPDX-License-Identifier: GPL-2.0-only */ + +#ifndef __XEN_SECTIONS_H__ +#define __XEN_SECTIONS_H__ + +/* SAF-0-safe */ +extern char __init_begin[], __init_end[]; + +#endif /* !__XEN_SECTIONS_H__ */ +/* + * Local variables: + * mode: C + * c-file-style: "BSD" + * c-basic-offset: 4 + * indent-tabs-mode: nil + * End: + */
Start by declaring the beginning and end of the init section. No functional change intended. Requested-by: Andrew Cooper <andrew.cooper3@citrix.com> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> --- xen/arch/arm/mmu/setup.c | 3 +-- xen/arch/x86/setup.c | 3 +-- xen/include/xen/sections.h | 17 +++++++++++++++++ 3 files changed, 19 insertions(+), 4 deletions(-) create mode 100644 xen/include/xen/sections.h