Re: [PATCH v2 3/5] x86/efi: Simplify efi_arch_handle_cmdline()

2023-11-22 Thread Andrew Cooper 
On 22/11/2023 9:27 am, Jan Beulich wrote:
> On 21.11.2023 21:15, Andrew Cooper wrote:
>> -Wwrite-strings is unhappy with assigning "xen" to a mutable pointer, but all
>> this work is useless; it's making a memory allocation just to prepend the
>> image name which cmdline_cook() intentionally strips back out.
>>
>> Simply forgo the work and identify EFI_LOADER as one of the loaders which
>> doesn't prepend the image name.
>>
>> No functional change.
>>
>> Signed-off-by: Andrew Cooper 
> Reviewed-by: Jan Beulich 

Thanks.

> with one nit:
>
>> --- a/xen/arch/x86/setup.c
>> +++ b/xen/arch/x86/setup.c
>> @@ -850,8 +850,11 @@ static const char *__init cmdline_cook(const char *p, 
>> const char *loader_name)
>>  while ( *p == ' ' )
>>  p++;
>>  
>> -/* GRUB2 and PVH don't not include image name as first item on command 
>> line. */
>> -if ( xen_guest || loader_is_grub2(loader_name) )
>> +/*
>> + * PVH, our EFI loader, and GRUB2 don't not include image name as first
> Just "don't", or "do not"? (I realize it was this way before, but perhaps a
> good chance to tidy.)

Will fix.  I completely missed that while rewording it.

~Andrew

Re: [PATCH v2 3/5] x86/efi: Simplify efi_arch_handle_cmdline()

2023-11-22 Thread Jan Beulich 
On 21.11.2023 21:15, Andrew Cooper wrote:
> -Wwrite-strings is unhappy with assigning "xen" to a mutable pointer, but all
> this work is useless; it's making a memory allocation just to prepend the
> image name which cmdline_cook() intentionally strips back out.
> 
> Simply forgo the work and identify EFI_LOADER as one of the loaders which
> doesn't prepend the image name.
> 
> No functional change.
> 
> Signed-off-by: Andrew Cooper 

Reviewed-by: Jan Beulich 
with one nit:

> --- a/xen/arch/x86/setup.c
> +++ b/xen/arch/x86/setup.c
> @@ -850,8 +850,11 @@ static const char *__init cmdline_cook(const char *p, 
> const char *loader_name)
>  while ( *p == ' ' )
>  p++;
>  
> -/* GRUB2 and PVH don't not include image name as first item on command 
> line. */
> -if ( xen_guest || loader_is_grub2(loader_name) )
> +/*
> + * PVH, our EFI loader, and GRUB2 don't not include image name as first

Just "don't", or "do not"? (I realize it was this way before, but perhaps a
good chance to tidy.)

Jan

> + * item on command line.
> + */
> +if ( xen_guest || efi_enabled(EFI_LOADER) || 
> loader_is_grub2(loader_name) )
>  return p;
>  
>  /* Strip image name plus whitespace. */

[PATCH v2 3/5] x86/efi: Simplify efi_arch_handle_cmdline()

2023-11-21 Thread Andrew Cooper 
-Wwrite-strings is unhappy with assigning "xen" to a mutable pointer, but all
this work is useless; it's making a memory allocation just to prepend the
image name which cmdline_cook() intentionally strips back out.

Simply forgo the work and identify EFI_LOADER as one of the loaders which
doesn't prepend the image name.

No functional change.

Signed-off-by: Andrew Cooper 
---
CC: Jan Beulich 
CC: Roger Pau Monné 
CC: Wei Liu 
CC: Stefano Stabellini 
CC: Julien Grall 
CC: Volodymyr Babchuk 
CC: Bertrand Marquis 
CC: Roberto Bagnara 
CC: Nicola Vetrini 

v2:
 * Brand new.

I can't find anything which cares about the image name in the slightest.  This
logic is from bf6501a62e80 ("x86-64: EFI boot code") when EFI was introduced,
at whcih point GRUB2 was the only excluded loader in cmdline_cook().
---
 xen/arch/x86/efi/efi-boot.h | 10 +-
 xen/arch/x86/setup.c|  7 +--
 2 files changed, 6 insertions(+), 11 deletions(-)

diff --git a/xen/arch/x86/efi/efi-boot.h b/xen/arch/x86/efi/efi-boot.h
index eebc54180bf7..1a2a2dd83c9b 100644
--- a/xen/arch/x86/efi/efi-boot.h
+++ b/xen/arch/x86/efi/efi-boot.h
@@ -309,6 +309,7 @@ static void __init efi_arch_handle_cmdline(CHAR16 
*image_name,
 {
 union string name;
 
+/* NB place_string() prepends, so call in reverse order. */
 if ( cmdline_options )
 {
 name.w = cmdline_options;
@@ -317,15 +318,6 @@ static void __init efi_arch_handle_cmdline(CHAR16 
*image_name,
 }
 if ( cfgfile_options )
 place_string(, cfgfile_options);
-/* Insert image name last, as it gets prefixed to the other options. */
-if ( image_name )
-{
-name.w = image_name;
-w2s();
-}
-else
-name.s = "xen";
-place_string(, name.s);
 
 if ( mbi.cmdline )
 mbi.flags |= MBI_CMDLINE;
diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
index b171a8f4cf84..200520392481 100644
--- a/xen/arch/x86/setup.c
+++ b/xen/arch/x86/setup.c
@@ -850,8 +850,11 @@ static const char *__init cmdline_cook(const char *p, 
const char *loader_name)
 while ( *p == ' ' )
 p++;
 
-/* GRUB2 and PVH don't not include image name as first item on command 
line. */
-if ( xen_guest || loader_is_grub2(loader_name) )
+/*
+ * PVH, our EFI loader, and GRUB2 don't not include image name as first
+ * item on command line.
+ */
+if ( xen_guest || efi_enabled(EFI_LOADER) || loader_is_grub2(loader_name) )
 return p;
 
 /* Strip image name plus whitespace. */
-- 
2.30.2