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
