Re: [PATCH v6] Avoid crash calling PrintErrMesg from efi_multiboot2

[Jan Beulich <jbeulich@xxxxxxxx>]

On 19.02.2025 17:34, Frediano Ziglio wrote:
> On Mon, Feb 17, 2025 at 4:56 PM Jan Beulich <jbeulich@xxxxxxxx> wrote:
>> On 17.02.2025 17:52, Frediano Ziglio wrote:
>>> On Mon, Feb 17, 2025 at 4:41 PM Andrew Cooper <andrew.cooper3@xxxxxxxxxx> 
>>> wrote:
>>>> On 17/02/2025 4:31 pm, Jan Beulich wrote:
>>>>> On 17.02.2025 17:26, Frediano Ziglio wrote:
>>>>>> --- a/xen/common/efi/efi-common.mk
>>>>>> +++ b/xen/common/efi/efi-common.mk
>>>>>> @@ -2,6 +2,7 @@ EFIOBJ-y := boot.init.o pe.init.o ebmalloc.o runtime.o
>>>>>>  EFIOBJ-$(CONFIG_COMPAT) += compat.o
>>>>>>
>>>>>>  CFLAGS-y += -fshort-wchar
>>>>>> +CFLAGS-y += -fno-jump-tables
>>>>>>  CFLAGS-y += -iquote $(srctree)/common/efi
>>>>>>  CFLAGS-y += -iquote $(srcdir)
>>>>> Do source files other than boot.c really need this? Do any other files 
>>>>> outside
>>>>> of efi/ maybe also need this (iirc this point was made along with the v5 
>>>>> comment
>>>>> you got)?
>>>>
>>>> Every TU reachable from efi_multiboot2() needs this, because all of
>>>> those have logic executed at an incorrect virtual address.
>>>
>>> I assume all the files in efi directory will deal with EFI boot so
>>> it's good to have the option enabled for the files inside the
>>> directory. The only exception seems runtime.c file.
>>
>> And compat.c, being a wrapper around runtime.c.
>>
>>> About external dependencies not sure how to detect them (beside
>>> looking at all symbols).
>>
>> Which raises the question whether we don't need to turn on that option
>> globally, without (as we have it now) any condition.
> 
> I would avoid adding a potential option that could affect performances
> so badly at the moment.
> These changes are pretty contained.
> I would merge this patch and check any external dependencies as a follow up.

Well. It's a judgement call to the maintainers. If I were them, I'd demand
that Andrew's remark be addressed, one way or another.

Jan