Re: [PATCH 3/6] xen/efi: Make efi-boot.h compile with -Wwrite-strings

[Jan Beulich <jbeulich@xxxxxxxx>]

On 21.11.2023 20:13, Andrew Cooper wrote:
> On 21/11/2023 6:03 pm, Andrew Cooper wrote:
>> On 21/11/2023 8:40 am, Jan Beulich wrote:
>>> On 20.11.2023 23:49, Andrew Cooper wrote:
>>>> GCC complains:
>>>>
>>>>   In file included from arch/arm/efi/boot.c:700:
>>>>   arch/arm/efi/efi-boot.h: In function 'efi_arch_handle_cmdline':
>>>>   arch/arm/efi/efi-boot.h:482:16: error: assignment discards 'const' 
>>>> qualifier from pointer target type [-Werror=discarded-qualifiers]
>>>>     482 |         name.s = "xen";
>>>>         |                ^
>>>>
>>>> There's no easy option.  .rodata is really read-only, so the fact Xen 
>>>> doesn't
>>>> crash means these strings aren't written to.
>>> And the consuming sites confirm this being the case. Hence ...
>>>
>>>> Lie to the compiler using a union.
>>> ... to at least slightly limit the lying, how about ...
>>>
>>>> --- a/xen/arch/arm/efi/efi-boot.h
>>>> +++ b/xen/arch/arm/efi/efi-boot.h
>>>> @@ -479,7 +479,7 @@ static void __init efi_arch_handle_cmdline(CHAR16 
>>>> *image_name,
>>>>          w2s(&name);
>>>>      }
>>>>      else
>>>> -        name.s = "xen";
>>>> +        name.cs = "xen"; /* TODO, find a better way of doing this. */
>>>>  
>>>>      prop_len = 0;
>>>>      prop_len += snprintf(buf + prop_len,
>>> ... you also switch to using name.cs down below here and ...
>>>
>>>> --- a/xen/arch/x86/efi/efi-boot.h
>>>> +++ b/xen/arch/x86/efi/efi-boot.h
>>>> @@ -324,7 +324,8 @@ static void __init efi_arch_handle_cmdline(CHAR16 
>>>> *image_name,
>>>>          w2s(&name);
>>>>      }
>>>>      else
>>>> -        name.s = "xen";
>>>> +        name.cs = "xen"; /* TODO, find a better way of doing this. */
>>>> +
>>>>      place_string(&mbi.cmdline, name.s);
>>> ... here?
>>>
>>> An alternative would be to introduce 'char xen[4] = "xen";' in both
>>> cases, and use them instead of plain string literals.
>> They'd have to be static, or dynamically allocated or they'll end up out
>> of scope, wont they?

No, place_string() copies into the target area. The input string then
isn't further used.

>> I have to admit I find this logic very hard to follow.
>>
>> Furthermore, I note:
>>
>> mbi.boot_loader_name = (long)"EFI";
>>
>> which is exactly the kind of pointer which is liable to end up being
>> edited via kextra in the other patch.
> 
> Hang on.  place_string()'ing here is just to prepend a piece of data we
> go to other lengths to strip and ignore later in boot.
> 
> On x86 we can just delete it and make our lives simpler.  I hope the
> same is true on ARM too.

Well, yes, the prepending is just to allow uniform handling later on.
Surely this can be done differently. Again - I'll go look at v2.

Jan