| Message ID | 20240318110442.3653997-4-andrew.cooper3@citrix.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | xen/arch: Simplify virtual_region setup | expand |
On 18.03.2024 12:04, Andrew Cooper wrote: > Given 3 statically initialised objects, its easy to link the list at build > time. There's no need to do it during runtime at boot (and with IRQs-off, > even). Hmm, technically that's correct, but isn't the overall result more fragile, in being more error prone if going forward someone found a need to alter things? Kind of supporting that view is also ... > --- > xen/common/virtual_region.c | 45 ++++++++++++++++++++++++------------- > 1 file changed, 30 insertions(+), 15 deletions(-) ... the diffstat of the change. It's perhaps also for a reason that ... > --- a/xen/common/virtual_region.c > +++ b/xen/common/virtual_region.c > @@ -15,8 +15,19 @@ extern const struct bug_frame > __start_bug_frames_2[], __stop_bug_frames_2[], > __start_bug_frames_3[], __stop_bug_frames_3[]; > > +/* > + * For the built-in regions, the double linked list can be constructed at > + * build time. Forward-declare the elements. > + */ > +static struct list_head virtual_region_list; > +static struct virtual_region core, core_init; > + > static struct virtual_region core = { > - .list = LIST_HEAD_INIT(core.list), > + .list = { > + .next = &core_init.list, > + .prev = &virtual_region_list, > + }, > + > .text_start = _stext, > .text_end = _etext, > .rodata_start = _srodata, > @@ -32,7 +43,11 @@ static struct virtual_region core = { > > /* Becomes irrelevant when __init sections are cleared. */ > static struct virtual_region core_init __initdata = { > - .list = LIST_HEAD_INIT(core_init.list), > + .list = { > + .next = &virtual_region_list, > + .prev = &core.list, > + }, > + > .text_start = _sinittext, > .text_end = _einittext, > > @@ -50,7 +65,10 @@ static struct virtual_region core_init __initdata = { > * > * All readers of virtual_region_list MUST use list_for_each_entry_rcu. > */ > -static LIST_HEAD(virtual_region_list); > +static struct list_head virtual_region_list = { > + .next = &core.list, > + .prev = &core_init.list, > +}; ... there's no pre-cooked construct to avoid any open-coding at least here. To clarify up front: I'm willing to be convinced otherwise, and I therefore might subsequently provide an ack. I'm also specifically not meaning this to be treated as "pending objection"; if another maintainer provides an ack, that's okay(ish) with me. Jan
On 18/03/2024 1:25 pm, Jan Beulich wrote: > On 18.03.2024 12:04, Andrew Cooper wrote: >> Given 3 statically initialised objects, its easy to link the list at build >> time. There's no need to do it during runtime at boot (and with IRQs-off, >> even). > Hmm, technically that's correct, but isn't the overall result more fragile, > in being more error prone if going forward someone found a need to alter > things? Kind of supporting that view is also ... > >> --- >> xen/common/virtual_region.c | 45 ++++++++++++++++++++++++------------- >> 1 file changed, 30 insertions(+), 15 deletions(-) > ... the diffstat of the change. It's perhaps also for a reason that ... > >> --- a/xen/common/virtual_region.c >> +++ b/xen/common/virtual_region.c >> @@ -15,8 +15,19 @@ extern const struct bug_frame >> __start_bug_frames_2[], __stop_bug_frames_2[], >> __start_bug_frames_3[], __stop_bug_frames_3[]; >> >> +/* >> + * For the built-in regions, the double linked list can be constructed at >> + * build time. Forward-declare the elements. >> + */ >> +static struct list_head virtual_region_list; >> +static struct virtual_region core, core_init; >> + >> static struct virtual_region core = { >> - .list = LIST_HEAD_INIT(core.list), >> + .list = { >> + .next = &core_init.list, >> + .prev = &virtual_region_list, >> + }, >> + >> .text_start = _stext, >> .text_end = _etext, >> .rodata_start = _srodata, >> @@ -32,7 +43,11 @@ static struct virtual_region core = { >> >> /* Becomes irrelevant when __init sections are cleared. */ >> static struct virtual_region core_init __initdata = { >> - .list = LIST_HEAD_INIT(core_init.list), >> + .list = { >> + .next = &virtual_region_list, >> + .prev = &core.list, >> + }, >> + >> .text_start = _sinittext, >> .text_end = _einittext, >> >> @@ -50,7 +65,10 @@ static struct virtual_region core_init __initdata = { >> * >> * All readers of virtual_region_list MUST use list_for_each_entry_rcu. >> */ >> -static LIST_HEAD(virtual_region_list); >> +static struct list_head virtual_region_list = { >> + .next = &core.list, >> + .prev = &core_init.list, >> +}; > ... there's no pre-cooked construct to avoid any open-coding at least > here. > > To clarify up front: I'm willing to be convinced otherwise, and I therefore > might subsequently provide an ack. I'm also specifically not meaning this > to be treated as "pending objection"; if another maintainer provides an ack, > that's okay(ish) with me. I think it's a very small price to pay in order to allow patch 4 to exist. If you can think of a nice way to express this with a pre-cooked construct then suggestions welcome, but it's a really complicated piece of metaprogramming to express in a nice way. ~Andrew
On 18.03.2024 14:54, Andrew Cooper wrote: > On 18/03/2024 1:25 pm, Jan Beulich wrote: >> On 18.03.2024 12:04, Andrew Cooper wrote: >>> Given 3 statically initialised objects, its easy to link the list at build >>> time. There's no need to do it during runtime at boot (and with IRQs-off, >>> even). >> Hmm, technically that's correct, but isn't the overall result more fragile, >> in being more error prone if going forward someone found a need to alter >> things? Kind of supporting that view is also ... >> >>> --- >>> xen/common/virtual_region.c | 45 ++++++++++++++++++++++++------------- >>> 1 file changed, 30 insertions(+), 15 deletions(-) >> ... the diffstat of the change. It's perhaps also for a reason that ... >> >>> --- a/xen/common/virtual_region.c >>> +++ b/xen/common/virtual_region.c >>> @@ -15,8 +15,19 @@ extern const struct bug_frame >>> __start_bug_frames_2[], __stop_bug_frames_2[], >>> __start_bug_frames_3[], __stop_bug_frames_3[]; >>> >>> +/* >>> + * For the built-in regions, the double linked list can be constructed at >>> + * build time. Forward-declare the elements. >>> + */ >>> +static struct list_head virtual_region_list; >>> +static struct virtual_region core, core_init; >>> + >>> static struct virtual_region core = { >>> - .list = LIST_HEAD_INIT(core.list), >>> + .list = { >>> + .next = &core_init.list, >>> + .prev = &virtual_region_list, >>> + }, >>> + >>> .text_start = _stext, >>> .text_end = _etext, >>> .rodata_start = _srodata, >>> @@ -32,7 +43,11 @@ static struct virtual_region core = { >>> >>> /* Becomes irrelevant when __init sections are cleared. */ >>> static struct virtual_region core_init __initdata = { >>> - .list = LIST_HEAD_INIT(core_init.list), >>> + .list = { >>> + .next = &virtual_region_list, >>> + .prev = &core.list, >>> + }, >>> + >>> .text_start = _sinittext, >>> .text_end = _einittext, >>> >>> @@ -50,7 +65,10 @@ static struct virtual_region core_init __initdata = { >>> * >>> * All readers of virtual_region_list MUST use list_for_each_entry_rcu. >>> */ >>> -static LIST_HEAD(virtual_region_list); >>> +static struct list_head virtual_region_list = { >>> + .next = &core.list, >>> + .prev = &core_init.list, >>> +}; >> ... there's no pre-cooked construct to avoid any open-coding at least >> here. >> >> To clarify up front: I'm willing to be convinced otherwise, and I therefore >> might subsequently provide an ack. I'm also specifically not meaning this >> to be treated as "pending objection"; if another maintainer provides an ack, >> that's okay(ish) with me. > > I think it's a very small price to pay in order to allow patch 4 to exist. > > If you can think of a nice way to express this with a pre-cooked > construct then suggestions welcome, but it's a really complicated piece > of metaprogramming to express in a nice way. I don't see any suitable pre-cooked construct, but something custom just for this file might be to have /* * For the built-in regions, the double linked list can be constructed at * build time. Forward-declare the elements. */ static struct list_head virtual_region_list; static struct virtual_region core, core_init; #define ENTRY1() { .next = &core_init.list, .prev = &virtual_region_list } #define ENTRY2() { .next = &virtual_region_list, .prev = &core.list } #define ENTRY3() { .next = &core.list, .prev = &core_init.list } such that they're all close together and hence the list arrangement can be easily seen. Sure, that'll still require each of the macros to be used exactly once. Maybe instead of numeric suffixes the name of the struct the macro is to be used in might help: #define ENTRY_HEAD() { .next = &core.list, .prev = &core_init.list } #define ENTRY_CORE() { .next = &core_init.list, .prev = &virtual_region_list } #define ENTRY_INIT() { .next = &virtual_region_list, .prev = &core.list } This way entries also come in list order. Jan
diff --git a/xen/common/virtual_region.c b/xen/common/virtual_region.c index eb9645daa99d..ad39bb74e15c 100644 --- a/xen/common/virtual_region.c +++ b/xen/common/virtual_region.c @@ -15,8 +15,19 @@ extern const struct bug_frame __start_bug_frames_2[], __stop_bug_frames_2[], __start_bug_frames_3[], __stop_bug_frames_3[]; +/* + * For the built-in regions, the double linked list can be constructed at + * build time. Forward-declare the elements. + */ +static struct list_head virtual_region_list; +static struct virtual_region core, core_init; + static struct virtual_region core = { - .list = LIST_HEAD_INIT(core.list), + .list = { + .next = &core_init.list, + .prev = &virtual_region_list, + }, + .text_start = _stext, .text_end = _etext, .rodata_start = _srodata, @@ -32,7 +43,11 @@ static struct virtual_region core = { /* Becomes irrelevant when __init sections are cleared. */ static struct virtual_region core_init __initdata = { - .list = LIST_HEAD_INIT(core_init.list), + .list = { + .next = &virtual_region_list, + .prev = &core.list, + }, + .text_start = _sinittext, .text_end = _einittext, @@ -50,7 +65,10 @@ static struct virtual_region core_init __initdata = { * * All readers of virtual_region_list MUST use list_for_each_entry_rcu. */ -static LIST_HEAD(virtual_region_list); +static struct list_head virtual_region_list = { + .next = &core.list, + .prev = &core_init.list, +}; static DEFINE_SPINLOCK(virtual_region_lock); static DEFINE_RCU_READ_LOCK(rcu_virtual_region_lock); @@ -73,15 +91,6 @@ const struct virtual_region *find_text_region(unsigned long addr) return region; } -void register_virtual_region(struct virtual_region *r) -{ - unsigned long flags; - - spin_lock_irqsave(&virtual_region_lock, flags); - list_add_tail_rcu(&r->list, &virtual_region_list); - spin_unlock_irqrestore(&virtual_region_lock, flags); -} - /* * Suggest inline so when !CONFIG_LIVEPATCH the function is not left * unreachable after init code is removed. @@ -96,6 +105,15 @@ static void inline remove_virtual_region(struct virtual_region *r) } #ifdef CONFIG_LIVEPATCH +void register_virtual_region(struct virtual_region *r) +{ + unsigned long flags; + + spin_lock_irqsave(&virtual_region_lock, flags); + list_add_tail_rcu(&r->list, &virtual_region_list); + spin_unlock_irqrestore(&virtual_region_lock, flags); +} + void unregister_virtual_region(struct virtual_region *r) { remove_virtual_region(r); @@ -155,9 +173,6 @@ void __init setup_virtual_regions(const struct exception_table_entry *start, { core_init.ex = core.ex = start; core_init.ex_end = core.ex_end = end; - - register_virtual_region(&core_init); - register_virtual_region(&core); } /*
Given 3 statically initialised objects, its easy to link the list at build time. There's no need to do it during runtime at boot (and with IRQs-off, even). As a consequence, register_virtual_region() can now move inside ifdef CONFIG_LIVEPATCH like unregister_virtual_region(). 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> CC: Stefano Stabellini <sstabellini@kernel.org> CC: Julien Grall <julien@xen.org> CC: Volodymyr Babchuk <Volodymyr_Babchuk@epam.com> CC: Bertrand Marquis <bertrand.marquis@arm.com> CC: Michal Orzel <michal.orzel@amd.com> CC: Oleksii Kurochko <oleksii.kurochko@gmail.com> CC: Shawn Anastasio <sanastasio@raptorengineering.com> CC: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> CC: Ross Lagerwall <ross.lagerwall@citrix.com> --- xen/common/virtual_region.c | 45 ++++++++++++++++++++++++------------- 1 file changed, 30 insertions(+), 15 deletions(-)