[PATCH] x86/pv-shim: Avoid a command line parameter warning for CONFIG_PV_SHIM_EXCLUSIVE builds

[PATCH] x86/pv-shim: Avoid a command line parameter warning for CONFIG_PV_SHIM_EXCLUSIVE builds

[Andrew Cooper]

When booting a guest using "pvshim=1" in the VM configuration file, libxl's
default command line is "pv-shim console=xen,pv".

With a CONFIG_PV_SHIM_EXCLUSIVE hypervisor, the boolean_param() is compiled
out, resulting in a command line parsing warning:

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

Avoid the warning by including a second boolean_param() for "pv-shim" which
writes into a unused variable.

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>
---
 xen/arch/x86/pv/shim.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/xen/arch/x86/pv/shim.c b/xen/arch/x86/pv/shim.c
index 636a9d6..c45885a 100644
--- a/xen/arch/x86/pv/shim.c
+++ b/xen/arch/x86/pv/shim.c
@@ -40,7 +40,11 @@
 #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. */
+static bool _discard;
+boolean_param("pv-shim", _discard);
+#else
 bool pv_shim;
 boolean_param("pv-shim", pv_shim);
 #endif
-- 
2.1.4


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

Re: [PATCH] x86/pv-shim: Avoid a command line parameter warning for CONFIG_PV_SHIM_EXCLUSIVE builds

[Jan Beulich]

>>> On 29.01.19 at 20:07, <andrew.cooper3@citrix.com> wrote:
> --- a/xen/arch/x86/pv/shim.c
> +++ b/xen/arch/x86/pv/shim.c
> @@ -40,7 +40,11 @@
>  #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. */
> +static bool _discard;
> +boolean_param("pv-shim", _discard);
> +#else
>  bool pv_shim;
>  boolean_param("pv-shim", pv_shim);
>  #endif

It would end up being less extra code if you did

#ifdef CONFIG_PV_SHIM_EXCLUSIVE
/* Tolerate "pv-shim" being passed to a CONFIG_PV_SHIM_EXCLUSIVE hypervisor. */
static bool __initdata pv_shim;
#else
bool pv_shim;
#endif
boolean_param("pv-shim", pv_shim);

But at the very least the __initdata wants adding. Preferably in the
suggested form
Acked-by: Jan Beulich <jbeulich@suse.com>

Jan



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

Re: [PATCH] x86/pv-shim: Avoid a command line parameter warning for CONFIG_PV_SHIM_EXCLUSIVE builds

[Andrew Cooper]

On 30/01/2019 09:57, Jan Beulich wrote:
>>>> On 29.01.19 at 20:07, <andrew.cooper3@citrix.com> wrote:
>> --- a/xen/arch/x86/pv/shim.c
>> +++ b/xen/arch/x86/pv/shim.c
>> @@ -40,7 +40,11 @@
>>  #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. */
>> +static bool _discard;
>> +boolean_param("pv-shim", _discard);
>> +#else
>>  bool pv_shim;
>>  boolean_param("pv-shim", pv_shim);
>>  #endif
> It would end up being less extra code if you did
>
> #ifdef CONFIG_PV_SHIM_EXCLUSIVE
> /* Tolerate "pv-shim" being passed to a CONFIG_PV_SHIM_EXCLUSIVE hypervisor. */
> static bool __initdata pv_shim;
> #else
> bool pv_shim;
> #endif
> boolean_param("pv-shim", pv_shim);

Sadly not.  In the EXCLUSIVE case, pv_shim is defined to be 1, and then
you've got an object named with just a number.  (I tried this approach
first.)

I can't think of any cleaner solution.

~Andrew

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

Re: [PATCH] x86/pv-shim: Avoid a command line parameter warning for CONFIG_PV_SHIM_EXCLUSIVE builds

[Jan Beulich]

>>> On 30.01.19 at 11:01, <andrew.cooper3@citrix.com> wrote:
> On 30/01/2019 09:57, Jan Beulich wrote:
>>>>> On 29.01.19 at 20:07, <andrew.cooper3@citrix.com> wrote:
>>> --- a/xen/arch/x86/pv/shim.c
>>> +++ b/xen/arch/x86/pv/shim.c
>>> @@ -40,7 +40,11 @@
>>>  #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. */
>>> +static bool _discard;
>>> +boolean_param("pv-shim", _discard);
>>> +#else
>>>  bool pv_shim;
>>>  boolean_param("pv-shim", pv_shim);
>>>  #endif
>> It would end up being less extra code if you did
>>
>> #ifdef CONFIG_PV_SHIM_EXCLUSIVE
>> /* Tolerate "pv-shim" being passed to a CONFIG_PV_SHIM_EXCLUSIVE hypervisor. */
>> static bool __initdata pv_shim;
>> #else
>> bool pv_shim;
>> #endif
>> boolean_param("pv-shim", pv_shim);
> 
> Sadly not.  In the EXCLUSIVE case, pv_shim is defined to be 1, and then
> you've got an object named with just a number.  (I tried this approach
> first.)

Oh, that's unfortunate in this particular case. In which case I
don't have any better suggestion either. One that you and others
perhaps wouldn't like would be

custom_param("pv-shim", NULL);

with parse_params() suitably adjusted to avoid the call in that
case.

Jan



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

Re: [PATCH] x86/pv-shim: Avoid a command line parameter warning for CONFIG_PV_SHIM_EXCLUSIVE builds

[Andrew Cooper]

On 30/01/2019 10:01, Andrew Cooper wrote:
> On 30/01/2019 09:57, Jan Beulich wrote:
>>>>> On 29.01.19 at 20:07, <andrew.cooper3@citrix.com> wrote:
>>> --- a/xen/arch/x86/pv/shim.c
>>> +++ b/xen/arch/x86/pv/shim.c
>>> @@ -40,7 +40,11 @@
>>>  #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. */
>>> +static bool _discard;
>>> +boolean_param("pv-shim", _discard);
>>> +#else
>>>  bool pv_shim;
>>>  boolean_param("pv-shim", pv_shim);
>>>  #endif
>> It would end up being less extra code if you did
>>
>> #ifdef CONFIG_PV_SHIM_EXCLUSIVE
>> /* Tolerate "pv-shim" being passed to a CONFIG_PV_SHIM_EXCLUSIVE hypervisor. */
>> static bool __initdata pv_shim;
>> #else
>> bool pv_shim;
>> #endif
>> boolean_param("pv-shim", pv_shim);
> Sadly not.  In the EXCLUSIVE case, pv_shim is defined to be 1, and then
> you've got an object named with just a number.  (I tried this approach
> first.)
>
> I can't think of any cleaner solution.

Actually, I could move this to the bottom of the file, and just undef
pv_shim.

~Andrew

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

Re: [PATCH] x86/pv-shim: Avoid a command line parameter warning for CONFIG_PV_SHIM_EXCLUSIVE builds

[Jan Beulich]

>>> On 30.01.19 at 11:07, <andrew.cooper3@citrix.com> wrote:
> On 30/01/2019 10:01, Andrew Cooper wrote:
>> On 30/01/2019 09:57, Jan Beulich wrote:
>>>>>> On 29.01.19 at 20:07, <andrew.cooper3@citrix.com> wrote:
>>>> --- a/xen/arch/x86/pv/shim.c
>>>> +++ b/xen/arch/x86/pv/shim.c
>>>> @@ -40,7 +40,11 @@
>>>>  #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. */
>>>> +static bool _discard;
>>>> +boolean_param("pv-shim", _discard);
>>>> +#else
>>>>  bool pv_shim;
>>>>  boolean_param("pv-shim", pv_shim);
>>>>  #endif
>>> It would end up being less extra code if you did
>>>
>>> #ifdef CONFIG_PV_SHIM_EXCLUSIVE
>>> /* Tolerate "pv-shim" being passed to a CONFIG_PV_SHIM_EXCLUSIVE hypervisor. */
>>> static bool __initdata pv_shim;
>>> #else
>>> bool pv_shim;
>>> #endif
>>> boolean_param("pv-shim", pv_shim);
>> Sadly not.  In the EXCLUSIVE case, pv_shim is defined to be 1, and then
>> you've got an object named with just a number.  (I tried this approach
>> first.)
>>
>> I can't think of any cleaner solution.
> 
> Actually, I could move this to the bottom of the file, and just undef
> pv_shim.

Ah, yes. Fine with me (i.e. feel free to keep my ack then).

Jan



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

Re: [PATCH] x86/pv-shim: Avoid a command line parameter warning for CONFIG_PV_SHIM_EXCLUSIVE builds

[Juergen Gross]

On 30/01/2019 15:09, Juergen Gross wrote:
> On 30/01/2019 11:06, Jan Beulich wrote:
>>>>> On 30.01.19 at 11:01, <andrew.cooper3@citrix.com> wrote:
>>> On 30/01/2019 09:57, Jan Beulich wrote:
>>>>>>> On 29.01.19 at 20:07, <andrew.cooper3@citrix.com> wrote:
>>>>> --- a/xen/arch/x86/pv/shim.c
>>>>> +++ b/xen/arch/x86/pv/shim.c
>>>>> @@ -40,7 +40,11 @@
>>>>>  #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. */
>>>>> +static bool _discard;
>>>>> +boolean_param("pv-shim", _discard);
>>>>> +#else
>>>>>  bool pv_shim;
>>>>>  boolean_param("pv-shim", pv_shim);
>>>>>  #endif
>>>> It would end up being less extra code if you did
>>>>
>>>> #ifdef CONFIG_PV_SHIM_EXCLUSIVE
>>>> /* Tolerate "pv-shim" being passed to a CONFIG_PV_SHIM_EXCLUSIVE hypervisor. */
>>>> static bool __initdata pv_shim;
>>>> #else
>>>> bool pv_shim;
>>>> #endif
>>>> boolean_param("pv-shim", pv_shim);
>>>
>>> Sadly not.  In the EXCLUSIVE case, pv_shim is defined to be 1, and then
>>> you've got an object named with just a number.  (I tried this approach
>>> first.)
>>
>> Oh, that's unfortunate in this particular case. In which case I
>> don't have any better suggestion either. One that you and others
>> perhaps wouldn't like would be
>>
>> custom_param("pv-shim", NULL);
>>
>> with parse_params() suitably adjusted to avoid the call in that
>> case.
> 
> I'd rather add something like ignore_param("pv-shim") instead (with the
> new type OPT_IGNORE).

BTW, in case this is the selected solution we might want to add

ignore_param("placeholder");

as well. Some old grub scripts seem to add "placeholder" as the first
boot parameter to mitigate a grub error (eating first parameter).


Juergen

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

Re: [PATCH] x86/pv-shim: Avoid a command line parameter warning for CONFIG_PV_SHIM_EXCLUSIVE builds

[Juergen Gross]

On 30/01/2019 11:06, Jan Beulich wrote:
>>>> On 30.01.19 at 11:01, <andrew.cooper3@citrix.com> wrote:
>> On 30/01/2019 09:57, Jan Beulich wrote:
>>>>>> On 29.01.19 at 20:07, <andrew.cooper3@citrix.com> wrote:
>>>> --- a/xen/arch/x86/pv/shim.c
>>>> +++ b/xen/arch/x86/pv/shim.c
>>>> @@ -40,7 +40,11 @@
>>>>  #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. */
>>>> +static bool _discard;
>>>> +boolean_param("pv-shim", _discard);
>>>> +#else
>>>>  bool pv_shim;
>>>>  boolean_param("pv-shim", pv_shim);
>>>>  #endif
>>> It would end up being less extra code if you did
>>>
>>> #ifdef CONFIG_PV_SHIM_EXCLUSIVE
>>> /* Tolerate "pv-shim" being passed to a CONFIG_PV_SHIM_EXCLUSIVE hypervisor. */
>>> static bool __initdata pv_shim;
>>> #else
>>> bool pv_shim;
>>> #endif
>>> boolean_param("pv-shim", pv_shim);
>>
>> Sadly not.  In the EXCLUSIVE case, pv_shim is defined to be 1, and then
>> you've got an object named with just a number.  (I tried this approach
>> first.)
> 
> Oh, that's unfortunate in this particular case. In which case I
> don't have any better suggestion either. One that you and others
> perhaps wouldn't like would be
> 
> custom_param("pv-shim", NULL);
> 
> with parse_params() suitably adjusted to avoid the call in that
> case.

I'd rather add something like ignore_param("pv-shim") instead (with the
new type OPT_IGNORE).


Juergen

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