| Message ID | c22d4a40717c7d2fad243c244619d2882ad5baf2.camel@infradead.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | x86/setup: Dom0 creation cleanups | expand |
On 18.03.2020 12:46, David Woodhouse wrote: > From: David Woodhouse <dwmw@amazon.co.uk> > > Remove a ternary operator that made my brain hurt. My position towards this hasn't changed, just ftr. > Replace it with something simpler that makes it somewhat clearer that > the check for initrdidx < mbi->mods_count is because larger values are > what find_first_bit() will return when it doesn't find anything. > > Also drop the explicit check for module #0 since that would be the > dom0 kernel and the corresponding bit is always clear in module_map. > > Signed-off-by: David Woodhouse <dwmw@amazon.co.uk> > Acked-by: Julien Grall <julien@xen.org> Strictly speaking this is not a valid tag here, only R-b would be. Jan
Hi, On 18/03/2020 11:51, Jan Beulich wrote: > On 18.03.2020 12:46, David Woodhouse wrote: >> From: David Woodhouse <dwmw@amazon.co.uk> >> >> Remove a ternary operator that made my brain hurt. > > My position towards this hasn't changed, just ftr. > >> Replace it with something simpler that makes it somewhat clearer that >> the check for initrdidx < mbi->mods_count is because larger values are >> what find_first_bit() will return when it doesn't find anything. >> >> Also drop the explicit check for module #0 since that would be the >> dom0 kernel and the corresponding bit is always clear in module_map. >> >> Signed-off-by: David Woodhouse <dwmw@amazon.co.uk> >> Acked-by: Julien Grall <julien@xen.org> > > Strictly speaking this is not a valid tag here, only R-b would be. I can't find any rule in our code base preventing a non-maintainer to add its "acked-by" tag. But if you want to play at this game, my tag is technically valid because "THE REST" englobes the full Xen codebase (Note the * in the MAINTAINERS file). We happen to not be CCed by scripts/get_maintainers.pl because *you* were not happy to be spammed... So we modified the scripts. In this particular case, I stand with the acked-by because I am ready to take the blame if something goes wrong with the patch. Such meaning is is not conveyed with "reviewed-by". Cheers,
On Wed, 2020-03-18 at 12:51 +0100, Jan Beulich wrote: > On 18.03.2020 12:46, David Woodhouse wrote: > > From: David Woodhouse <dwmw@amazon.co.uk> > > > > Remove a ternary operator that made my brain hurt. > > My position towards this hasn't changed, just ftr. Your position was not clearly stated. In https://lists.xenproject.org/archives/html/xen-devel/2020-02/msg01664.html you indicated that you preferred for it to remain as-is but you did not even seem to be disputing that the code is simpler and easier for the reader to understand after my cleanup. I was left wondering if your position was merely that you *liked* making my brain hurt? :) > > Replace it with something simpler that makes it somewhat clearer that > > the check for initrdidx < mbi->mods_count is because larger values are > > what find_first_bit() will return when it doesn't find anything. > > > > Also drop the explicit check for module #0 since that would be the > > dom0 kernel and the corresponding bit is always clear in module_map. > > > > Signed-off-by: David Woodhouse <dwmw@amazon.co.uk> > > Acked-by: Julien Grall <julien@xen.org> > > Strictly speaking this is not a valid tag here, only R-b would be.
On 18.03.2020 13:12, Julien Grall wrote: > Hi, > > On 18/03/2020 11:51, Jan Beulich wrote: >> On 18.03.2020 12:46, David Woodhouse wrote: >>> From: David Woodhouse <dwmw@amazon.co.uk> >>> >>> Remove a ternary operator that made my brain hurt. >> >> My position towards this hasn't changed, just ftr. >> >>> Replace it with something simpler that makes it somewhat clearer that >>> the check for initrdidx < mbi->mods_count is because larger values are >>> what find_first_bit() will return when it doesn't find anything. >>> >>> Also drop the explicit check for module #0 since that would be the >>> dom0 kernel and the corresponding bit is always clear in module_map. >>> >>> Signed-off-by: David Woodhouse <dwmw@amazon.co.uk> >>> Acked-by: Julien Grall <julien@xen.org> >> >> Strictly speaking this is not a valid tag here, only R-b would be. > > I can't find any rule in our code base preventing a non-maintainer to add its "acked-by" tag. I could have said "meaningful" instead of "valid": A patch is not supposed to go in without a direct maintainer's ack, unless there's a reason to invoke the nested maintainership rules. That's my understanding at least. > But if you want to play at this game, my tag is technically valid > because "THE REST" englobes the full Xen codebase (Note the * in > the MAINTAINERS file). Note the nested maintainership wording in that file, which was added pretty recently. If that wording isn't clear enough, perhaps we can further refine it? Jan
On 18.03.2020 13:33, David Woodhouse wrote: > On Wed, 2020-03-18 at 12:51 +0100, Jan Beulich wrote: >> On 18.03.2020 12:46, David Woodhouse wrote: >>> From: David Woodhouse <dwmw@amazon.co.uk> >>> >>> Remove a ternary operator that made my brain hurt. >> >> My position towards this hasn't changed, just ftr. > > Your position was not clearly stated. In > https://lists.xenproject.org/archives/html/xen-devel/2020-02/msg01664.html > you indicated that you preferred for it to remain as-is but you did not > even seem to be disputing that the code is simpler and easier for the > reader to understand after my cleanup. > > I was left wondering if your position was merely that you *liked* > making my brain hurt? :) Ehem. I would have thought indicating that you'd need Andrew's ack was clear enough a sign that I wouldn't want to give mine. I'm sorry if this wasn't the case. Jan
On 18/03/2020 13:20, Jan Beulich wrote: > On 18.03.2020 13:12, Julien Grall wrote: >> Hi, >> >> On 18/03/2020 11:51, Jan Beulich wrote: >>> On 18.03.2020 12:46, David Woodhouse wrote: >>>> From: David Woodhouse <dwmw@amazon.co.uk> >>>> >>>> Remove a ternary operator that made my brain hurt. >>> >>> My position towards this hasn't changed, just ftr. >>> >>>> Replace it with something simpler that makes it somewhat clearer that >>>> the check for initrdidx < mbi->mods_count is because larger values are >>>> what find_first_bit() will return when it doesn't find anything. >>>> >>>> Also drop the explicit check for module #0 since that would be the >>>> dom0 kernel and the corresponding bit is always clear in module_map. >>>> >>>> Signed-off-by: David Woodhouse <dwmw@amazon.co.uk> >>>> Acked-by: Julien Grall <julien@xen.org> >>> >>> Strictly speaking this is not a valid tag here, only R-b would be. >> >> I can't find any rule in our code base preventing a non-maintainer to add its "acked-by" tag. > > I could have said "meaningful" instead of "valid": A patch is not > supposed to go in without a direct maintainer's ack, unless there's > a reason to invoke the nested maintainership rules. That's my > understanding at least. I still don't see why you are not happy with my tag here the more I don't think David or I ever claimed my acked-by was sufficient for the patch to be merged. With my tag I acknowledged the patch. I could also have ignored it and you would have complained that nobody help you reviewing patches... > >> But if you want to play at this game, my tag is technically valid >> because "THE REST" englobes the full Xen codebase (Note the * in >> the MAINTAINERS file). > > Note the nested maintainership wording in that file, which was added > pretty recently. If that wording isn't clear enough, perhaps we can > further refine it? The wording is clear enough, but it still doesn't prevent me to add my acked-by. Cheers,
On 18.03.2020 14:29, Julien Grall wrote: > > > On 18/03/2020 13:20, Jan Beulich wrote: >> On 18.03.2020 13:12, Julien Grall wrote: >>> Hi, >>> >>> On 18/03/2020 11:51, Jan Beulich wrote: >>>> On 18.03.2020 12:46, David Woodhouse wrote: >>>>> From: David Woodhouse <dwmw@amazon.co.uk> >>>>> >>>>> Remove a ternary operator that made my brain hurt. >>>> >>>> My position towards this hasn't changed, just ftr. >>>> >>>>> Replace it with something simpler that makes it somewhat clearer that >>>>> the check for initrdidx < mbi->mods_count is because larger values are >>>>> what find_first_bit() will return when it doesn't find anything. >>>>> >>>>> Also drop the explicit check for module #0 since that would be the >>>>> dom0 kernel and the corresponding bit is always clear in module_map. >>>>> >>>>> Signed-off-by: David Woodhouse <dwmw@amazon.co.uk> >>>>> Acked-by: Julien Grall <julien@xen.org> >>>> >>>> Strictly speaking this is not a valid tag here, only R-b would be. >>> >>> I can't find any rule in our code base preventing a non-maintainer to add its "acked-by" tag. >> >> I could have said "meaningful" instead of "valid": A patch is not >> supposed to go in without a direct maintainer's ack, unless there's >> a reason to invoke the nested maintainership rules. That's my >> understanding at least. > > I still don't see why you are not happy with my tag here > the more I don't think David or I ever claimed my acked-by > was sufficient for the patch to be merged. I didn't say I'm not happy with it. I merely tried to state a fact, for the avoidance of doubt. > With my tag I acknowledged the patch. I could also have > ignored it and you would have complained that nobody help > you reviewing patches... An R-b would have achieved the same effect. Jan
On Wed, Mar 18, 2020 at 11:46:06AM +0000, David Woodhouse wrote: > From: David Woodhouse <dwmw@amazon.co.uk> > > Remove a ternary operator that made my brain hurt. > > Replace it with something simpler that makes it somewhat clearer that > the check for initrdidx < mbi->mods_count is because larger values are > what find_first_bit() will return when it doesn't find anything. > > Also drop the explicit check for module #0 since that would be the > dom0 kernel and the corresponding bit is always clear in module_map. > > Signed-off-by: David Woodhouse <dwmw@amazon.co.uk> > Acked-by: Julien Grall <julien@xen.org> Reviewed-by: Wei Liu <wl@xen.org> I think this is a fine improvement. It is more straightforward to follow. Wei.
diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c index c87040c890..2986cf5a3a 100644 --- a/xen/arch/x86/setup.c +++ b/xen/arch/x86/setup.c @@ -688,7 +688,7 @@ void __init noreturn __start_xen(unsigned long mbi_p) char *cmdline, *kextra, *loader; unsigned int initrdidx, num_parked = 0; multiboot_info_t *mbi; - module_t *mod; + module_t *mod, *initrd = NULL; unsigned long nr_pages, raw_max_page, modules_headroom, module_map[1]; int i, j, e820_warn = 0, bytes = 0; bool acpi_boot_table_init_done = false, relocated = false; @@ -1798,6 +1798,8 @@ void __init noreturn __start_xen(unsigned long mbi_p) xen_processor_pmbits |= XEN_PROCESSOR_PM_CX; initrdidx = find_first_bit(module_map, mbi->mods_count); + if ( initrdidx < mbi->mods_count ) + initrd = mod + initrdidx; if ( bitmap_weight(module_map, mbi->mods_count) > 1 ) printk(XENLOG_WARNING "Multiple initrd candidates, picking module #%u\n", @@ -1822,9 +1824,7 @@ void __init noreturn __start_xen(unsigned long mbi_p) * We're going to setup domain0 using the module(s) that we stashed safely * above our heap. The second module, if present, is an initrd ramdisk. */ - if ( construct_dom0(dom0, mod, modules_headroom, - (initrdidx > 0) && (initrdidx < mbi->mods_count) - ? mod + initrdidx : NULL, cmdline) != 0) + if ( construct_dom0(dom0, mod, modules_headroom, initrd, cmdline) != 0 ) panic("Could not set up DOM0 guest OS\n"); if ( cpu_has_smap )