| Message ID | 20200201003303.2363081-7-dwmw2@infradead.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Early cleanups and bug fixes in preparation for live update | expand |
Hi David, On 01/02/2020 01:33, 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 mbi->mods_count > is what find_first_bit() will return when it doesn't find anything. > > Signed-off-by: David Woodhouse <dwmw@amazon.co.uk> > --- > xen/arch/x86/setup.c | 9 +++++---- > 1 file changed, 5 insertions(+), 4 deletions(-) > > diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c > index 5f68a1308f..10209e6bfb 100644 > --- a/xen/arch/x86/setup.c > +++ b/xen/arch/x86/setup.c > @@ -687,7 +687,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; > @@ -1793,6 +1793,9 @@ 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 ) IIUC, the check on initrdx > 0 was pointless as bit 0 will always be cleared (it is used for dom0 kernel). It might be worth mentionning it in the commit message. Something like: "The initrd can never be in the first module (i.e initrdidx == 0) as this is always used by the dom0 kernel. So the check can be simplify." Acked-by: Julien Grall <julien@xen.org> > + initrd = mod + initrdidx; > + > if ( bitmap_weight(module_map, mbi->mods_count) > 1 ) > printk(XENLOG_WARNING > "Multiple initrd candidates, picking module #%u\n", > @@ -1817,9 +1820,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 ) > Cheers,
On 01.02.2020 01:33, David Woodhouse wrote: > From: David Woodhouse <dwmw@amazon.co.uk> > > Remove a ternary operator that made my brain hurt. Personally I'd prefer the code to stay as is, but if Andrew agrees with this being an improvement, then I also wouldn't want to stand in the way. If it is to go in I have a few small adjustment requests: > Replace it with something simpler that makes it somewhat clearer that > the check for initrdidx < mbi->mods_count is because mbi->mods_count > is what find_first_bit() will return when it doesn't find anything. Especially in light of the recent XSA-307 I'd like to ask that we avoid imprecise statements like this: Afaict find_first_bit() may also validly return any value larger than the passed in bitmap length. > @@ -1793,6 +1793,9 @@ 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", Since this if() is tightly coupled with the find_first_bit() invocation, I'd like to ask for there to not be a blank line in between. Jan
Hi Jan, On 21/02/2020 16:59, Jan Beulich wrote: > On 01.02.2020 01:33, David Woodhouse wrote: >> From: David Woodhouse <dwmw@amazon.co.uk> >> >> Remove a ternary operator that made my brain hurt. > > Personally I'd prefer the code to stay as is, but if Andrew agrees > with this being an improvement, then I also wouldn't want to stand > in the way. If it is to go in I have a few small adjustment requests: > >> Replace it with something simpler that makes it somewhat clearer that >> the check for initrdidx < mbi->mods_count is because mbi->mods_count >> is what find_first_bit() will return when it doesn't find anything. > > Especially in light of the recent XSA-307 I'd like to ask that we > avoid imprecise statements like this: Afaict find_first_bit() may > also validly return any value larger than the passed in bitmap > length. Is it? I though that all the callers are now returning 'size' in all the error cases. Cheers,
On 24.02.2020 14:31, Julien Grall wrote: > On 21/02/2020 16:59, Jan Beulich wrote: >> On 01.02.2020 01:33, David Woodhouse wrote: >>> From: David Woodhouse <dwmw@amazon.co.uk> >>> >>> Remove a ternary operator that made my brain hurt. >> >> Personally I'd prefer the code to stay as is, but if Andrew agrees >> with this being an improvement, then I also wouldn't want to stand >> in the way. If it is to go in I have a few small adjustment requests: >> >>> Replace it with something simpler that makes it somewhat clearer that >>> the check for initrdidx < mbi->mods_count is because mbi->mods_count >>> is what find_first_bit() will return when it doesn't find anything. >> >> Especially in light of the recent XSA-307 I'd like to ask that we >> avoid imprecise statements like this: Afaict find_first_bit() may >> also validly return any value larger than the passed in bitmap >> length. > > Is it? I though that all the callers are now returning 'size' in all the > error cases. Taking (part of) the x86 example: >#define find_next_bit(addr, size, off) ({ \ > unsigned int r__; \ > const unsigned long *a__ = (addr); \ > unsigned int s__ = (size); \ > unsigned int o__ = (off); \ > if ( o__ >= s__ ) \ > r__ = s__; \ This is what did get adjusted, guaranteeing "size" to be returned for too large an "offset". > else if ( __builtin_constant_p(size) && s__ <= BITS_PER_LONG ) \ > r__ = o__ + __scanbit(*(const unsigned long *)(a__) >> o__, s__); \ Yet this was (deliberately) left untouched. Without s__ getting replaced by s__ - o__ it may still produce a value larger than "size", though. Further, even if all current implementations obeyed by the more strict rule, this still wouldn't mean callers are actually permitted to assume such. The more strict rule would then also need to be written down, such that it won't get violated again in the future (by changes to an existing arch's implementation, or by a new port). Jan
Hi Jan, On 25/02/2020 12:34, Jan Beulich wrote: > On 24.02.2020 14:31, Julien Grall wrote: >> On 21/02/2020 16:59, Jan Beulich wrote: >>> On 01.02.2020 01:33, David Woodhouse wrote: >>>> From: David Woodhouse <dwmw@amazon.co.uk> >>>> >>>> Remove a ternary operator that made my brain hurt. >>> >>> Personally I'd prefer the code to stay as is, but if Andrew agrees >>> with this being an improvement, then I also wouldn't want to stand >>> in the way. If it is to go in I have a few small adjustment requests: >>> >>>> Replace it with something simpler that makes it somewhat clearer that >>>> the check for initrdidx < mbi->mods_count is because mbi->mods_count >>>> is what find_first_bit() will return when it doesn't find anything. >>> >>> Especially in light of the recent XSA-307 I'd like to ask that we >>> avoid imprecise statements like this: Afaict find_first_bit() may >>> also validly return any value larger than the passed in bitmap >>> length. >> >> Is it? I though that all the callers are now returning 'size' in all the >> error cases. > > Taking (part of) the x86 example: > >> #define find_next_bit(addr, size, off) ({ \ >> unsigned int r__; \ >> const unsigned long *a__ = (addr); \ >> unsigned int s__ = (size); \ >> unsigned int o__ = (off); \ >> if ( o__ >= s__ ) \ >> r__ = s__; \ > > This is what did get adjusted, guaranteeing "size" to be returned for > too large an "offset". > >> else if ( __builtin_constant_p(size) && s__ <= BITS_PER_LONG ) \ >> r__ = o__ + __scanbit(*(const unsigned long *)(a__) >> o__, s__); \ > > Yet this was (deliberately) left untouched. Without s__ getting > replaced by s__ - o__ it may still produce a value larger than > "size", though. Ah I missed this part. > > Further, even if all current implementations obeyed by the more > strict rule, this still wouldn't mean callers are actually permitted > to assume such. The more strict rule would then also need to be > written down, such that it won't get violated again in the future > (by changes to an existing arch's implementation, or by a new port To be honest, the rule should be written down in any case. The current one is not necessarily an obvious one and also differ from what Linux folks can expect. Regarding future port, the number of architectures in Linux using custom bitops are fairly limited (AFAICT only arm32 and unicore32). All the rest (including x86) using a generic implementation. On Xen, Arm64 is already using the generic implementation. Is there any particular concern to use it for x86 as well? If not, I can pull a patch to use the generic implementation on x86/arm32. This would solve the discrenpancies in find_*_bit implementations. Cheers,
On 26.02.2020 08:13, Julien Grall wrote: > On 25/02/2020 12:34, Jan Beulich wrote: >> Further, even if all current implementations obeyed by the more >> strict rule, this still wouldn't mean callers are actually permitted >> to assume such. The more strict rule would then also need to be >> written down, such that it won't get violated again in the future >> (by changes to an existing arch's implementation, or by a new port > > To be honest, the rule should be written down in any case. The current > one is not necessarily an obvious one and also differ from what Linux > folks can expect. I think we should stick to the more relaxed rule in any event, unless there's a strong reason to enforce the more strict one. Much (but not all) of Linux code looks to assume the more relaxed rule too, likely also for historical reasons (when the implementation on e.g. x86-64 still followed the more relaxed model). > Regarding future port, the number of architectures in Linux using custom > bitops are fairly limited (AFAICT only arm32 and unicore32). All the > rest (including x86) using a generic implementation. > > On Xen, Arm64 is already using the generic implementation. Is there any > particular concern to use it for x86 as well? According to the (over 10 years old) commit updating Linux x86 this way, the generic implementation was even faster. If that was the case today and for our implementation as well, then I think it would be very nice if we updated. If, otoh, data isn't as clear, then further consideration may be needed. Andrew, do you have any thoughts either way? > If not, I can pull a patch to use the generic implementation on > x86/arm32. This would solve the discrenpancies in find_*_bit > implementations. This is orthogonal to the issue discussed - as said, I think code using the functions would still better assume the more relaxed model. Jan
diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c index 5f68a1308f..10209e6bfb 100644 --- a/xen/arch/x86/setup.c +++ b/xen/arch/x86/setup.c @@ -687,7 +687,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; @@ -1793,6 +1793,9 @@ 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", @@ -1817,9 +1820,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 )