[PATCH v2 for-4.12] xen/cmdline: Work around some specific command line warnings

[PATCH v2 for-4.12] xen/cmdline: Work around some specific command line warnings

[Andrew Cooper]

Xen will warn when an unknown parameter is found in the command line.  e.g.

  (d8) [ 1556.334664] (XEN) parameter "pv-shim" unknown!

One case where this goes wrong is a workaround for an old grub bug, which
resulted in "placeholder" being prepended to the command line.

Another case is when booting a CONFIG_PV_SHIM_EXCLUSIVE build, in which the
parsing for the "pv-shim" parameter is discarded.

Introduce ignore_param() and OPT_IGNORE to cope with known cases, where
warning the user is the wrong course of action to take.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Wei Liu <wei.liu2@citrix.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Juergen Gross <jgross@suse.com>

v2:
 * Rewrite from scratch, following Juergen's suggestion

An implementation detail of ignore_param() is that it can only be used once
per translation unit, which is fine for now.  Two options to fix this are to
tokenise __LINE__ in (requires some extreme preprocessor magic to make work,
as ## inhibits expansion, and may cause livepatching issues), or to retain the
_val parameter and require callers to just pass in a unique string.

Thoughts welcome.
---
 xen/arch/x86/pv/shim.c | 5 ++++-
 xen/arch/x86/setup.c   | 6 ++++++
 xen/common/kernel.c    | 2 ++
 xen/include/xen/init.h | 8 +++++++-
 4 files changed, 19 insertions(+), 2 deletions(-)

diff --git a/xen/arch/x86/pv/shim.c b/xen/arch/x86/pv/shim.c
index 636a9d6..324ca27 100644
--- a/xen/arch/x86/pv/shim.c
+++ b/xen/arch/x86/pv/shim.c
@@ -40,7 +40,10 @@
 #undef virt_to_mfn
 #define virt_to_mfn(va) _mfn(__virt_to_mfn(va))
 
-#ifndef CONFIG_PV_SHIM_EXCLUSIVE
+#ifdef CONFIG_PV_SHIM_EXCLUSIVE
+/* Tolerate "pv-shim" being passed to a CONFIG_PV_SHIM_EXCLUSIVE hypervisor. */
+ignore_param("pv-shim");
+#else
 bool pv_shim;
 boolean_param("pv-shim", pv_shim);
 #endif
diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
index 06eb483..92da060 100644
--- a/xen/arch/x86/setup.c
+++ b/xen/arch/x86/setup.c
@@ -639,6 +639,12 @@ static void __init noreturn reinit_bsp_stack(void)
     reset_stack_and_jump(init_done);
 }
 
+/*
+ * Some scripts add "placeholder" to work around a grub error where it ate the
+ * first parameter.
+ */
+ignore_param("placeholder");
+
 static bool __init loader_is_grub2(const char *loader_name)
 {
     /* GRUB1="GNU GRUB 0.xx"; GRUB2="GRUB 1.xx" */
diff --git a/xen/common/kernel.c b/xen/common/kernel.c
index 053c31d..6125754 100644
--- a/xen/common/kernel.c
+++ b/xen/common/kernel.c
@@ -162,6 +162,8 @@ static int parse_params(const char *cmdline, const struct kernel_param *start,
                 }
                 rctmp = param->par.func(optval);
                 break;
+            case OPT_IGNORE:
+                break;
             default:
                 BUG();
                 break;
diff --git a/xen/include/xen/init.h b/xen/include/xen/init.h
index c6b453a..9384914 100644
--- a/xen/include/xen/init.h
+++ b/xen/include/xen/init.h
@@ -81,7 +81,8 @@ struct kernel_param {
         OPT_UINT,
         OPT_BOOL,
         OPT_SIZE,
-        OPT_CUSTOM
+        OPT_CUSTOM,
+        OPT_IGNORE,
     } type;
     unsigned int len;
     union {
@@ -136,6 +137,11 @@ extern const struct kernel_param __param_start[], __param_end[];
           .type = OPT_STR, \
           .len = sizeof(_var), \
           .par.var = &_var }
+#define ignore_param(_name)                 \
+    __setup_str __setup_str_ign[] = _name;  \
+    __kparam __setup_ign =                  \
+        { .name = __setup_str_ign,          \
+          .type = OPT_IGNORE }
 
 #define __rtparam         __param(__dataparam)
 
-- 
2.1.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v2 for-4.12] xen/cmdline: Work around some specific command line warnings

[Juergen Gross]

On 30/01/2019 16:55, Andrew Cooper wrote:
> Xen will warn when an unknown parameter is found in the command line.  e.g.
> 
>   (d8) [ 1556.334664] (XEN) parameter "pv-shim" unknown!
> 
> One case where this goes wrong is a workaround for an old grub bug, which
> resulted in "placeholder" being prepended to the command line.
> 
> Another case is when booting a CONFIG_PV_SHIM_EXCLUSIVE build, in which the
> parsing for the "pv-shim" parameter is discarded.
> 
> Introduce ignore_param() and OPT_IGNORE to cope with known cases, where
> warning the user is the wrong course of action to take.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> ---
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Wei Liu <wei.liu2@citrix.com>
> CC: Roger Pau Monné <roger.pau@citrix.com>
> CC: Juergen Gross <jgross@suse.com>
> 
> v2:
>  * Rewrite from scratch, following Juergen's suggestion
> 
> An implementation detail of ignore_param() is that it can only be used once
> per translation unit, which is fine for now.  Two options to fix this are to
> tokenise __LINE__ in (requires some extreme preprocessor magic to make work,
> as ## inhibits expansion, and may cause livepatching issues), or to retain the
> _val parameter and require callers to just pass in a unique string.

Or make the unique string an optional parameter via:

#define ignore_param(_name, uniq...)                \
    __setup_str __setup_str_ign ## uniq[] = _name;  \
    __kparam __setup_ign ## uniq =                  \
        { .name = __setup_str_ign ## uniq,          \
          .type = OPT_IGNORE }

With or without that change:

Reviewed-by: Juergen Gross <jgross@suse.com>
Release-acked-by: Juergen Gross <jgross@suse.com>


Juergen

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v2 for-4.12] xen/cmdline: Work around some specific command line warnings

[Jan Beulich]

>>> On 30.01.19 at 16:55, <andrew.cooper3@citrix.com> wrote:
> Xen will warn when an unknown parameter is found in the command line.  e.g.
> 
>   (d8) [ 1556.334664] (XEN) parameter "pv-shim" unknown!
> 
> One case where this goes wrong is a workaround for an old grub bug, which
> resulted in "placeholder" being prepended to the command line.
> 
> Another case is when booting a CONFIG_PV_SHIM_EXCLUSIVE build, in which the
> parsing for the "pv-shim" parameter is discarded.
> 
> Introduce ignore_param() and OPT_IGNORE to cope with known cases, where
> warning the user is the wrong course of action to take.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Reviewed-by: Jan Beulich <jbeulich@suse.com>

> An implementation detail of ignore_param() is that it can only be used once
> per translation unit, which is fine for now.  Two options to fix this are to
> tokenise __LINE__ in (requires some extreme preprocessor magic to make work,
> as ## inhibits expansion, and may cause livepatching issues), or to retain the
> _val parameter and require callers to just pass in a unique string.

How about requiring the quotes to be omitted from the argument
passed to the macro and using that string to paste onto the
variable names? This would require following an earlier suggestion
of mine in that we (a) treat '-' and '_' equally when parsing
command line options (cmdline_strcmp()) and (b) the main parser
to use that function. But as you say, nothing that needs solving
right away.

> @@ -136,6 +137,11 @@ extern const struct kernel_param __param_start[], __param_end[];
>            .type = OPT_STR, \
>            .len = sizeof(_var), \
>            .par.var = &_var }
> +#define ignore_param(_name)                 \
> +    __setup_str __setup_str_ign[] = _name;  \
> +    __kparam __setup_ign =                  \
> +        { .name = __setup_str_ign,          \
> +          .type = OPT_IGNORE }

I don't suppose that I could talk you into dropping the leading
underscore from the macro parameter name, and into
reducing the double leading underscores of the variables to
single ones? I realize this won't fit the other macros, but
eventually we will want to get rid of all those name space
violations.

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v2 for-4.12] xen/cmdline: Work around some specific command line warnings

[Andrew Cooper]

On 30/01/2019 16:31, Juergen Gross wrote:
> On 30/01/2019 16:55, Andrew Cooper wrote:
>> Xen will warn when an unknown parameter is found in the command line.  e.g.
>>
>>   (d8) [ 1556.334664] (XEN) parameter "pv-shim" unknown!
>>
>> One case where this goes wrong is a workaround for an old grub bug, which
>> resulted in "placeholder" being prepended to the command line.
>>
>> Another case is when booting a CONFIG_PV_SHIM_EXCLUSIVE build, in which the
>> parsing for the "pv-shim" parameter is discarded.
>>
>> Introduce ignore_param() and OPT_IGNORE to cope with known cases, where
>> warning the user is the wrong course of action to take.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>> ---
>> CC: Jan Beulich <JBeulich@suse.com>
>> CC: Wei Liu <wei.liu2@citrix.com>
>> CC: Roger Pau Monné <roger.pau@citrix.com>
>> CC: Juergen Gross <jgross@suse.com>
>>
>> v2:
>>  * Rewrite from scratch, following Juergen's suggestion
>>
>> An implementation detail of ignore_param() is that it can only be used once
>> per translation unit, which is fine for now.  Two options to fix this are to
>> tokenise __LINE__ in (requires some extreme preprocessor magic to make work,
>> as ## inhibits expansion, and may cause livepatching issues), or to retain the
>> _val parameter and require callers to just pass in a unique string.
> Or make the unique string an optional parameter via:
>
> #define ignore_param(_name, uniq...)                \
>     __setup_str __setup_str_ign ## uniq[] = _name;  \
>     __kparam __setup_ign ## uniq =                  \
>         { .name = __setup_str_ign ## uniq,          \
>           .type = OPT_IGNORE }
>
> With or without that change:
>
> Reviewed-by: Juergen Gross <jgross@suse.com>
> Release-acked-by: Juergen Gross <jgross@suse.com>

Actually I like this as an option.  I'll fold this in, along with Jan's
namespacing request.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v2 for-4.12] xen/cmdline: Work around some specific command line warnings

[Andrew Cooper]

On 30/01/2019 17:07, Andrew Cooper wrote:
> On 30/01/2019 16:31, Juergen Gross wrote:
>> On 30/01/2019 16:55, Andrew Cooper wrote:
>>> Xen will warn when an unknown parameter is found in the command line.  e.g.
>>>
>>>   (d8) [ 1556.334664] (XEN) parameter "pv-shim" unknown!
>>>
>>> One case where this goes wrong is a workaround for an old grub bug, which
>>> resulted in "placeholder" being prepended to the command line.
>>>
>>> Another case is when booting a CONFIG_PV_SHIM_EXCLUSIVE build, in which the
>>> parsing for the "pv-shim" parameter is discarded.
>>>
>>> Introduce ignore_param() and OPT_IGNORE to cope with known cases, where
>>> warning the user is the wrong course of action to take.
>>>
>>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>>> ---
>>> CC: Jan Beulich <JBeulich@suse.com>
>>> CC: Wei Liu <wei.liu2@citrix.com>
>>> CC: Roger Pau Monné <roger.pau@citrix.com>
>>> CC: Juergen Gross <jgross@suse.com>
>>>
>>> v2:
>>>  * Rewrite from scratch, following Juergen's suggestion
>>>
>>> An implementation detail of ignore_param() is that it can only be used once
>>> per translation unit, which is fine for now.  Two options to fix this are to
>>> tokenise __LINE__ in (requires some extreme preprocessor magic to make work,
>>> as ## inhibits expansion, and may cause livepatching issues), or to retain the
>>> _val parameter and require callers to just pass in a unique string.
>> Or make the unique string an optional parameter via:
>>
>> #define ignore_param(_name, uniq...)                \
>>     __setup_str __setup_str_ign ## uniq[] = _name;  \
>>     __kparam __setup_ign ## uniq =                  \
>>         { .name = __setup_str_ign ## uniq,          \
>>           .type = OPT_IGNORE }
>>
>> With or without that change:
>>
>> Reviewed-by: Juergen Gross <jgross@suse.com>
>> Release-acked-by: Juergen Gross <jgross@suse.com>
> Actually I like this as an option.

Sadly, it doesn't work, as you end up with:

__setup_str setup_str_ign_ ## [] = "pv-shim";

and ## is a binary operator.

I'll leave it as-was and we can figure this out in the future if necessary.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v2 for-4.12] xen/cmdline: Work around some specific command line warnings

[Andrew Cooper]

On 30/01/2019 16:38, Jan Beulich wrote:
>>>> On 30.01.19 at 16:55, <andrew.cooper3@citrix.com> wrote:
>> Xen will warn when an unknown parameter is found in the command line.  e.g.
>>
>>   (d8) [ 1556.334664] (XEN) parameter "pv-shim" unknown!
>>
>> One case where this goes wrong is a workaround for an old grub bug, which
>> resulted in "placeholder" being prepended to the command line.
>>
>> Another case is when booting a CONFIG_PV_SHIM_EXCLUSIVE build, in which the
>> parsing for the "pv-shim" parameter is discarded.
>>
>> Introduce ignore_param() and OPT_IGNORE to cope with known cases, where
>> warning the user is the wrong course of action to take.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Reviewed-by: Jan Beulich <jbeulich@suse.com>
>
>> An implementation detail of ignore_param() is that it can only be used once
>> per translation unit, which is fine for now.  Two options to fix this are to
>> tokenise __LINE__ in (requires some extreme preprocessor magic to make work,
>> as ## inhibits expansion, and may cause livepatching issues), or to retain the
>> _val parameter and require callers to just pass in a unique string.
> How about requiring the quotes to be omitted from the argument
> passed to the macro and using that string to paste onto the
> variable names? This would require following an earlier suggestion
> of mine in that we (a) treat '-' and '_' equally when parsing
> command line options (cmdline_strcmp()) and (b) the main parser
> to use that function. But as you say, nothing that needs solving
> right away.
>
>> @@ -136,6 +137,11 @@ extern const struct kernel_param __param_start[], __param_end[];
>>            .type = OPT_STR, \
>>            .len = sizeof(_var), \
>>            .par.var = &_var }
>> +#define ignore_param(_name)                 \
>> +    __setup_str __setup_str_ign[] = _name;  \
>> +    __kparam __setup_ign =                  \
>> +        { .name = __setup_str_ign,          \
>> +          .type = OPT_IGNORE }
> I don't suppose that I could talk you into dropping the leading
> underscore from the macro parameter name,

No.  That results in a failure to compile because of ".name" when
initialising the structure.

I've left it as is for consistency with the other macros.

> and into
> reducing the double leading underscores of the variables to
> single ones?

Done.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v2 for-4.12] xen/cmdline: Work around some specific command line warnings

[Wei Liu]

On Wed, Jan 30, 2019 at 05:16:47PM +0000, Andrew Cooper wrote:
> On 30/01/2019 17:07, Andrew Cooper wrote:
> > On 30/01/2019 16:31, Juergen Gross wrote:
> >> On 30/01/2019 16:55, Andrew Cooper wrote:
> >>> Xen will warn when an unknown parameter is found in the command line.  e.g.
> >>>
> >>>   (d8) [ 1556.334664] (XEN) parameter "pv-shim" unknown!
> >>>
> >>> One case where this goes wrong is a workaround for an old grub bug, which
> >>> resulted in "placeholder" being prepended to the command line.
> >>>
> >>> Another case is when booting a CONFIG_PV_SHIM_EXCLUSIVE build, in which the
> >>> parsing for the "pv-shim" parameter is discarded.
> >>>
> >>> Introduce ignore_param() and OPT_IGNORE to cope with known cases, where
> >>> warning the user is the wrong course of action to take.
> >>>
> >>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> >>> ---
> >>> CC: Jan Beulich <JBeulich@suse.com>
> >>> CC: Wei Liu <wei.liu2@citrix.com>
> >>> CC: Roger Pau Monné <roger.pau@citrix.com>
> >>> CC: Juergen Gross <jgross@suse.com>
> >>>
> >>> v2:
> >>>  * Rewrite from scratch, following Juergen's suggestion
> >>>
> >>> An implementation detail of ignore_param() is that it can only be used once
> >>> per translation unit, which is fine for now.  Two options to fix this are to
> >>> tokenise __LINE__ in (requires some extreme preprocessor magic to make work,
> >>> as ## inhibits expansion, and may cause livepatching issues), or to retain the
> >>> _val parameter and require callers to just pass in a unique string.
> >> Or make the unique string an optional parameter via:
> >>
> >> #define ignore_param(_name, uniq...)                \
> >>     __setup_str __setup_str_ign ## uniq[] = _name;  \
> >>     __kparam __setup_ign ## uniq =                  \
> >>         { .name = __setup_str_ign ## uniq,          \
> >>           .type = OPT_IGNORE }
> >>
> >> With or without that change:
> >>
> >> Reviewed-by: Juergen Gross <jgross@suse.com>
> >> Release-acked-by: Juergen Gross <jgross@suse.com>
> > Actually I like this as an option.
> 
> Sadly, it doesn't work, as you end up with:
> 
> __setup_str setup_str_ign_ ## [] = "pv-shim";
> 
> and ## is a binary operator.
> 
> I'll leave it as-was and we can figure this out in the future if necessary.

Sigh. This means you can't have two ignore_param in the same CU.

I was trying to use this for "flask" and "silo" and (re)discovered this
issue in the hard way.

Wei.

> 
> ~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel