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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 15.11.2024 11:27, Frediano Ziglio wrote:
> On Thu, Nov 14, 2024 at 3:04 PM Jan Beulich <jbeulich@xxxxxxxx> wrote:
>>
>> On 14.11.2024 15:02, Frediano Ziglio wrote:
>>> On Tue, Nov 5, 2024 at 2:52 PM Jan Beulich <jbeulich@xxxxxxxx> wrote:
>>>>
>>>> On 19.08.2024 16:29, Frediano Ziglio wrote:
>>>>> --- a/xen/common/efi/boot.c
>>>>> +++ b/xen/common/efi/boot.c
>>>>> @@ -287,19 +287,36 @@ static bool __init match_guid(const EFI_GUID 
>>>>> *guid1, const EFI_GUID *guid2)
>>>>>  /* generic routine for printing error messages */
>>>>>  static void __init PrintErrMesg(const CHAR16 *mesg, EFI_STATUS ErrCode)
>>>>>  {
>>>>> -    static const CHAR16* const ErrCodeToStr[] __initconstrel = {
>>>>> -        [~EFI_ERROR_MASK & EFI_NOT_FOUND]           = L"Not found",
>>>>> -        [~EFI_ERROR_MASK & EFI_NO_MEDIA]            = L"The device has 
>>>>> no media",
>>>>> -        [~EFI_ERROR_MASK & EFI_MEDIA_CHANGED]       = L"Media changed",
>>>>> -        [~EFI_ERROR_MASK & EFI_DEVICE_ERROR]        = L"Device error",
>>>>> -        [~EFI_ERROR_MASK & EFI_VOLUME_CORRUPTED]    = L"Volume 
>>>>> corrupted",
>>>>> -        [~EFI_ERROR_MASK & EFI_ACCESS_DENIED]       = L"Access denied",
>>>>> -        [~EFI_ERROR_MASK & EFI_OUT_OF_RESOURCES]    = L"Out of 
>>>>> resources",
>>>>> -        [~EFI_ERROR_MASK & EFI_VOLUME_FULL]         = L"Volume is full",
>>>>> -        [~EFI_ERROR_MASK & EFI_SECURITY_VIOLATION]  = L"Security 
>>>>> violation",
>>>>> -        [~EFI_ERROR_MASK & EFI_CRC_ERROR]           = L"CRC error",
>>>>> -        [~EFI_ERROR_MASK & EFI_COMPROMISED_DATA]    = L"Compromised 
>>>>> data",
>>>>> -        [~EFI_ERROR_MASK & EFI_BUFFER_TOO_SMALL]    = L"Buffer too 
>>>>> small",
>>>>> +#define ERROR_MESSAGE_LIST \
>>>>> +    ERROR_MESSAGE(EFI_NOT_FOUND, "Not found") \
>>>>> +    ERROR_MESSAGE(EFI_NO_MEDIA, "The device has no media") \
>>>>> +    ERROR_MESSAGE(EFI_MEDIA_CHANGED, "Media changed") \
>>>>> +    ERROR_MESSAGE(EFI_DEVICE_ERROR, "Device error") \
>>>>> +    ERROR_MESSAGE(EFI_VOLUME_CORRUPTED, "Volume corrupted") \
>>>>> +    ERROR_MESSAGE(EFI_ACCESS_DENIED, "Access denied") \
>>>>> +    ERROR_MESSAGE(EFI_OUT_OF_RESOURCES, "Out of resources") \
>>>>> +    ERROR_MESSAGE(EFI_VOLUME_FULL, "Volume is full") \
>>>>> +    ERROR_MESSAGE(EFI_SECURITY_VIOLATION, "Security violation") \
>>>>> +    ERROR_MESSAGE(EFI_CRC_ERROR, "CRC error") \
>>>>> +    ERROR_MESSAGE(EFI_COMPROMISED_DATA, "Compromised data") \
>>>>> +    ERROR_MESSAGE(EFI_BUFFER_TOO_SMALL, "Buffer too small")
>>>>> +
>>>>> +    static const struct ErrorStrings {
>>>>> +        CHAR16 start;
>>>>> +#undef ERROR_MESSAGE
>>>>> +#define ERROR_MESSAGE(code, str) CHAR16 msg_ ## code[sizeof(str)];
>>>>> +        ERROR_MESSAGE_LIST
>>>>> +    } ErrorStrings __initconst = {
>>>>> +        0
>>>>> +#undef ERROR_MESSAGE
>>>>> +#define ERROR_MESSAGE(code, str) , L ## str
>>>>> +        ERROR_MESSAGE_LIST
>>>>> +    };
>>>>> +    static const uint16_t ErrCodeToStr[] __initconst = {
>>>>> +#undef ERROR_MESSAGE
>>>>> +#define ERROR_MESSAGE(code, str) \
>>>>> +        [~EFI_ERROR_MASK & code] = offsetof(struct ErrorStrings, msg_ ## 
>>>>> code),
>>>>> +        ERROR_MESSAGE_LIST
>>>>>      };
>>>>>      EFI_STATUS ErrIdx = ErrCode & ~EFI_ERROR_MASK;
>>>>>
>>>>
>>>> A while ago Andrew and I discussed this, and I was apparently wrongly 
>>>> expecting
>>>> him to come back here, as (iirc; no record of this that I could find in 
>>>> the mail
>>>> archives, so I'm sorry if my recollection is wrong) he was the one to 
>>>> object. We
>>>> concluded that it wants at least considering to undo the respective part of
>>>> 00d5d5ce23e6, finding a different solution to the Clang issue there.

This is ...

>>> I thought this patch was already applied.
>>> I didn't remember any clang issue.
>>> As far as I know, this was delayed by an issue that turned out to be 
>>> different.
>>> So, any reason why not to merge the original patch?
>>
>> Afaict the alternative would result in tidier code, and hence might indeed be
>> preferable. But since the reason I didn't long commit the patch is Andrew
>> wanting it to not be committed, it'll need to be him to chime in here. Even
>> if only to indicate that I'm misremembering.
> 
> What alternative are you talking about? Was it something discussed elsewhere?

... the alternative I'm talking about.

Jan