| Message ID | On7D3GbE8WWWH3f-1bSvUFQDxFcHP3yg6NdfvffgKqPRjQmJKsPBKsPgCCEEHbt9r2A3yxvf3gARonkKF9M_n1f3UfLEFpnZ29J2-Jc35ls=@trmm.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | arch/x86/setup.c: Ignore early boot parameters like no-real-mode | expand |
On 12/08/2020 18:42, Trammell Hudson wrote: > There are parameters in xen/arch/x86/boot/cmdline.c that > are only used early in the boot process, so handlers are > necessary to avoid an "Unknown command line option" in > dmesg. > > This also updates ignore_param() to generate a temporary > variable name so that the macro can be used more than once > per file. > > Signed-off-by: Trammell hudson <hudson@trmm.net> Good spot. However, the use of __LINE__ creates problems for livepatch builds, as it causes the binary diffing tools to believe these changed, based on a change earlier in the file. Instead of opencoding TEMP_NAME(), can we borrow Linux's __UNIQUE_ID() infrastructure? __COUNTER__ appears to have existed for ages, and exists in all of our supported compilers. If you want, I can sort that out as a prereq patch, and rebase this one on top? ~Andrew
On Wednesday, August 12, 2020 8:16 PM, Andrew Cooper <andrew.cooper3@citrix.com> wrote: > However, the use of LINE creates problems for livepatch builds, as > it causes the binary diffing tools to believe these changed, based on a > change earlier in the file. Ah, I hadn't considered that. Makes sense that the deterministic __COUNTER__ would be better for many uses. However... One concern is that the __COUNTER__ is per compilation unit, which I think would mean that every file would conflict by creating __setup_str_ign_0 for the first one, __setup_str_ign_1 for the next, etc. Unless they are static scoped or have a variable-name-friendly unique prefix, they aren't suitable for globals that share a namespace. Another is that each evaluation increments it, so the macro would need some tricks to avoid double-evaluation since it both defines __setup_str_ign_XYZ and references it in the __kparam structure. This is in contrast to __LINE__, which is constant in the macro and based on the line where it was invoked so the double evaluation is not a problem. > Instead of opencoding TEMP_NAME(), can we borrow Linux's __UNIQUE_ID() > infrastructure? COUNTER appears to have existed for ages, and > exists in all of our supported compilers. I'm definitely in favor of borrowing it from Linux, although subject to those two caveats. > If you want, I can sort that out as a prereq patch, and rebase this one > on top? That sounds fine to me. They really are two separate patches. -- Trammell
On 12/08/2020 20:06, Trammell Hudson wrote: > On Wednesday, August 12, 2020 8:16 PM, Andrew Cooper <andrew.cooper3@citrix.com> wrote: > >> However, the use of LINE creates problems for livepatch builds, as >> it causes the binary diffing tools to believe these changed, based on a >> change earlier in the file. > Ah, I hadn't considered that. Makes sense that the > deterministic __COUNTER__ would be better for many uses. > However... > > One concern is that the __COUNTER__ is per compilation unit, > which I think would mean that every file would conflict by > creating __setup_str_ign_0 for the first one, __setup_str_ign_1 > for the next, etc. Unless they are static scoped or have a > variable-name-friendly unique prefix, they aren't > suitable for globals that share a namespace. That's fine. Something else which exists in our tangle of a build system is some symbol munging (behind CONFIG_ENFORCE_UNIQUE_SYMBOLS) which takes any static variable and prepends the relative filename, so the end result is something which is properly unique. In some copious free, this wants refining to "every ambiguous static variable", seeing as most static variables aren't ambiguous, but that is an incremental improvement. > > Another is that each evaluation increments it, so the macro > would need some tricks to avoid double-evaluation since it > both defines __setup_str_ign_XYZ and references it in the > __kparam structure. This is in contrast to __LINE__, > which is constant in the macro and based on the line where > it was invoked so the double evaluation is not a problem. > >> Instead of opencoding TEMP_NAME(), can we borrow Linux's __UNIQUE_ID() >> infrastructure? COUNTER appears to have existed for ages, and >> exists in all of our supported compilers. > I'm definitely in favor of borrowing it from Linux, although > subject to those two caveats. > >> If you want, I can sort that out as a prereq patch, and rebase this one >> on top? > That sounds fine to me. They really are two separate patches. I think we're fine caveat wise. I'll try and find some time tomorrow. ~Andrew
On 13/08/2020 00:35, Andrew Cooper wrote: > On 12/08/2020 20:06, Trammell Hudson wrote: >> On Wednesday, August 12, 2020 8:16 PM, Andrew Cooper <andrew.cooper3@citrix.com> wrote: >>> If you want, I can sort that out as a prereq patch, and rebase this one >>> on top? >> That sounds fine to me. They really are two separate patches. > I think we're fine caveat wise. I'll try and find some time tomorrow. So the __UNIQUE_ID() plan doesn't work, as a consequence of the logic inside ignore_param() to shuffle the string name into initdata. As everything is in .initdata, it doesn't actually interact with LIVEPATCH. I've committed this patch with an extra note to try and prevent TEMP_NAME() being used in wider contexts. ~Andrew
diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c index c9b6af8..4b15e06 100644 --- a/xen/arch/x86/setup.c +++ b/xen/arch/x86/setup.c @@ -679,6 +679,15 @@ static void __init noreturn reinit_bsp_stack(void) reset_stack_and_jump_nolp(init_done); } +/* + * x86 early command line parsing in xen/arch/x86/boot/cmdline.c + * has options that are only used during the very initial boot process, + * so they can be ignored now. + */ +ignore_param("no-real-mode"); +ignore_param("edd"); +ignore_param("edid"); + /* * Some scripts add "placeholder" to work around a grub error where it ate the * first parameter. diff --git a/xen/include/xen/param.h b/xen/include/xen/param.h index c2fd075..b77f7f2 100644 --- a/xen/include/xen/param.h +++ b/xen/include/xen/param.h @@ -35,6 +35,10 @@ extern const struct kernel_param __setup_start[], __setup_end[]; __attribute__((__aligned__(1))) char #define __kparam __param(__initsetup) +#define __TEMP_NAME(base,line) base##_##line +#define _TEMP_NAME(base,line) __TEMP_NAME(base,line) +#define TEMP_NAME(base) _TEMP_NAME(base,__LINE__) + #define custom_param(_name, _var) \ __setup_str __setup_str_##_var[] = _name; \ __kparam __setup_##_var = \ @@ -71,9 +75,9 @@ extern const struct kernel_param __setup_start[], __setup_end[]; .len = sizeof(_var), \ .par.var = &_var } #define ignore_param(_name) \ - __setup_str setup_str_ign[] = _name; \ - __kparam setup_ign = \ - { .name = setup_str_ign, \ + __setup_str TEMP_NAME(__setup_str_ign)[] = _name; \ + __kparam TEMP_NAME(__setup_ign) = \ + { .name = TEMP_NAME(__setup_str_ign), \ .type = OPT_IGNORE } #ifdef CONFIG_HYPFS
There are parameters in xen/arch/x86/boot/cmdline.c that are only used early in the boot process, so handlers are necessary to avoid an "Unknown command line option" in dmesg. This also updates ignore_param() to generate a temporary variable name so that the macro can be used more than once per file. Signed-off-by: Trammell hudson <hudson@trmm.net>