| Message ID | 20240819142953.415817-1-frediano.ziglio@cloud.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v4] Avoid crash calling PrintErrMesg from efi_multiboot2 | expand |
On Mon, Aug 19, 2024 at 3:30 PM Frediano Ziglio <frediano.ziglio@cloud.com> wrote: > > Although code is compiled with -fpic option data is not position > independent. This causes data pointer to become invalid if > code is not relocated properly which is what happens for > efi_multiboot2 which is called by multiboot entry code. > > Code tested adding > PrintErrMesg(L"Test message", EFI_BUFFER_TOO_SMALL); > in efi_multiboot2 before calling efi_arch_edd (this function > can potentially call PrintErrMesg). > > Before the patch (XenServer installation on Qemu, xen replaced > with vanilla xen.gz): > Booting `XenServer (Serial)'Booting `XenServer (Serial)' > Test message: !!!! X64 Exception Type - 0E(#PF - Page-Fault) CPU Apic ID - 00000000 !!!! > ExceptionData - 0000000000000000 I:0 R:0 U:0 W:0 P:0 PK:0 SS:0 SGX:0 > RIP - 000000007DC29E46, CS - 0000000000000038, RFLAGS - 0000000000210246 > RAX - 0000000000000000, RCX - 0000000000000050, RDX - 0000000000000000 > RBX - 000000007DAB4558, RSP - 000000007EFA1200, RBP - 0000000000000000 > RSI - FFFF82D040467A88, RDI - 0000000000000000 > R8 - 000000007EFA1238, R9 - 000000007EFA1230, R10 - 0000000000000000 > R11 - 000000007CF42665, R12 - FFFF82D040467A88, R13 - 000000007EFA1228 > R14 - 000000007EFA1225, R15 - 000000007DAB45A8 > DS - 0000000000000030, ES - 0000000000000030, FS - 0000000000000030 > GS - 0000000000000030, SS - 0000000000000030 > CR0 - 0000000080010033, CR2 - FFFF82D040467A88, CR3 - 000000007EC01000 > CR4 - 0000000000000668, CR8 - 0000000000000000 > DR0 - 0000000000000000, DR1 - 0000000000000000, DR2 - 0000000000000000 > DR3 - 0000000000000000, DR6 - 00000000FFFF0FF0, DR7 - 0000000000000400 > GDTR - 000000007E9E2000 0000000000000047, LDTR - 0000000000000000 > IDTR - 000000007E4E5018 0000000000000FFF, TR - 0000000000000000 > FXSAVE_STATE - 000000007EFA0E60 > !!!! Find image based on IP(0x7DC29E46) (No PDB) (ImageBase=000000007DC28000, EntryPoint=000000007DC2B917) !!!! > > After the patch: > Booting `XenServer (Serial)'Booting `XenServer (Serial)' > Test message: Buffer too small > BdsDxe: loading Boot0000 "UiApp" from Fv(7CB8BDC9-F8EB-4F34-AAEA-3EE4AF6516A1)/FvFile(462CAA21-7614-4503-836E-8AB6F4662331) > BdsDxe: starting Boot0000 "UiApp" from Fv(7CB8BDC9-F8EB-4F34-AAEA-3EE4AF6516A1)/FvFile(462CAA21-7614-4503-836E-8AB6F4662331) > > Fixes: 9180f5365524 ("x86: add multiboot2 protocol support for EFI platforms") > Signed-off-by: Frediano Ziglio <frediano.ziglio@cloud.com> > --- > xen/common/efi/boot.c | 46 ++++++++++++++++++++++++++++++------------- > 1 file changed, 32 insertions(+), 14 deletions(-) > --- > Changes since v1: > - added "Fixes:" tag; > - fixed cast style change. > > Changes since v2: > - wrap long line. > > Changes since v3: > - fixed "Fixes:" tag. > > diff --git a/xen/common/efi/boot.c b/xen/common/efi/boot.c > index efbec00af9..fdbe75005c 100644 > --- 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; > > @@ -308,7 +325,8 @@ static void __init PrintErrMesg(const CHAR16 *mesg, EFI_STATUS ErrCode) > PrintErr(L": "); > > if( (ErrIdx < ARRAY_SIZE(ErrCodeToStr)) && ErrCodeToStr[ErrIdx] ) > - mesg = ErrCodeToStr[ErrIdx]; > + mesg = (const CHAR16 *)((const void *)&ErrorStrings + > + ErrCodeToStr[ErrIdx]); > else > { > PrintErr(L"ErrCode: "); Any update on this? Frediano
On Thu, Aug 29, 2024 at 10:03 AM Frediano Ziglio <frediano.ziglio@cloud.com> wrote: > On Mon, Aug 19, 2024 at 3:30 PM Frediano Ziglio > <frediano.ziglio@cloud.com> wrote: > > > > Although code is compiled with -fpic option data is not position > > independent. This causes data pointer to become invalid if > > code is not relocated properly which is what happens for > > efi_multiboot2 which is called by multiboot entry code. > > > > Code tested adding > > PrintErrMesg(L"Test message", EFI_BUFFER_TOO_SMALL); > > in efi_multiboot2 before calling efi_arch_edd (this function > > can potentially call PrintErrMesg). > > > > Before the patch (XenServer installation on Qemu, xen replaced > > with vanilla xen.gz): > > Booting `XenServer (Serial)'Booting `XenServer (Serial)' > > Test message: !!!! X64 Exception Type - 0E(#PF - Page-Fault) CPU Apic > ID - 00000000 !!!! > > ExceptionData - 0000000000000000 I:0 R:0 U:0 W:0 P:0 PK:0 SS:0 SGX:0 > > RIP - 000000007DC29E46, CS - 0000000000000038, RFLAGS - > 0000000000210246 > > RAX - 0000000000000000, RCX - 0000000000000050, RDX - 0000000000000000 > > RBX - 000000007DAB4558, RSP - 000000007EFA1200, RBP - 0000000000000000 > > RSI - FFFF82D040467A88, RDI - 0000000000000000 > > R8 - 000000007EFA1238, R9 - 000000007EFA1230, R10 - 0000000000000000 > > R11 - 000000007CF42665, R12 - FFFF82D040467A88, R13 - 000000007EFA1228 > > R14 - 000000007EFA1225, R15 - 000000007DAB45A8 > > DS - 0000000000000030, ES - 0000000000000030, FS - 0000000000000030 > > GS - 0000000000000030, SS - 0000000000000030 > > CR0 - 0000000080010033, CR2 - FFFF82D040467A88, CR3 - 000000007EC01000 > > CR4 - 0000000000000668, CR8 - 0000000000000000 > > DR0 - 0000000000000000, DR1 - 0000000000000000, DR2 - 0000000000000000 > > DR3 - 0000000000000000, DR6 - 00000000FFFF0FF0, DR7 - 0000000000000400 > > GDTR - 000000007E9E2000 0000000000000047, LDTR - 0000000000000000 > > IDTR - 000000007E4E5018 0000000000000FFF, TR - 0000000000000000 > > FXSAVE_STATE - 000000007EFA0E60 > > !!!! Find image based on IP(0x7DC29E46) (No PDB) > (ImageBase=000000007DC28000, EntryPoint=000000007DC2B917) !!!! > > > > After the patch: > > Booting `XenServer (Serial)'Booting `XenServer (Serial)' > > Test message: Buffer too small > > BdsDxe: loading Boot0000 "UiApp" from > Fv(7CB8BDC9-F8EB-4F34-AAEA-3EE4AF6516A1)/FvFile(462CAA21-7614-4503-836E-8AB6F4662331) > > BdsDxe: starting Boot0000 "UiApp" from > Fv(7CB8BDC9-F8EB-4F34-AAEA-3EE4AF6516A1)/FvFile(462CAA21-7614-4503-836E-8AB6F4662331) > > > > Fixes: 9180f5365524 ("x86: add multiboot2 protocol support for EFI > platforms") > > Signed-off-by: Frediano Ziglio <frediano.ziglio@cloud.com> > > --- > > xen/common/efi/boot.c | 46 ++++++++++++++++++++++++++++++------------- > > 1 file changed, 32 insertions(+), 14 deletions(-) > > --- > > Changes since v1: > > - added "Fixes:" tag; > > - fixed cast style change. > > > > Changes since v2: > > - wrap long line. > > > > Changes since v3: > > - fixed "Fixes:" tag. > > > > diff --git a/xen/common/efi/boot.c b/xen/common/efi/boot.c > > index efbec00af9..fdbe75005c 100644 > > --- 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; > > > > @@ -308,7 +325,8 @@ static void __init PrintErrMesg(const CHAR16 *mesg, > EFI_STATUS ErrCode) > > PrintErr(L": "); > > > > if( (ErrIdx < ARRAY_SIZE(ErrCodeToStr)) && ErrCodeToStr[ErrIdx] ) > > - mesg = ErrCodeToStr[ErrIdx]; > > + mesg = (const CHAR16 *)((const void *)&ErrorStrings + > > + ErrCodeToStr[ErrIdx]); > > else > > { > > PrintErr(L"ErrCode: "); > > Any update on this? > > ping The issue still apply, checked that I addressed all comments. Frediano
On Mon, Aug 19, 2024 at 03:29:52PM +0100, Frediano Ziglio wrote: > Although code is compiled with -fpic option data is not position > independent. This causes data pointer to become invalid if > code is not relocated properly which is what happens for > efi_multiboot2 which is called by multiboot entry code. > > Code tested adding > PrintErrMesg(L"Test message", EFI_BUFFER_TOO_SMALL); > in efi_multiboot2 before calling efi_arch_edd (this function > can potentially call PrintErrMesg). > > Before the patch (XenServer installation on Qemu, xen replaced > with vanilla xen.gz): > Booting `XenServer (Serial)'Booting `XenServer (Serial)' > Test message: !!!! X64 Exception Type - 0E(#PF - Page-Fault) CPU Apic ID - 00000000 !!!! > ExceptionData - 0000000000000000 I:0 R:0 U:0 W:0 P:0 PK:0 SS:0 SGX:0 > RIP - 000000007DC29E46, CS - 0000000000000038, RFLAGS - 0000000000210246 > RAX - 0000000000000000, RCX - 0000000000000050, RDX - 0000000000000000 > RBX - 000000007DAB4558, RSP - 000000007EFA1200, RBP - 0000000000000000 > RSI - FFFF82D040467A88, RDI - 0000000000000000 > R8 - 000000007EFA1238, R9 - 000000007EFA1230, R10 - 0000000000000000 > R11 - 000000007CF42665, R12 - FFFF82D040467A88, R13 - 000000007EFA1228 > R14 - 000000007EFA1225, R15 - 000000007DAB45A8 > DS - 0000000000000030, ES - 0000000000000030, FS - 0000000000000030 > GS - 0000000000000030, SS - 0000000000000030 > CR0 - 0000000080010033, CR2 - FFFF82D040467A88, CR3 - 000000007EC01000 > CR4 - 0000000000000668, CR8 - 0000000000000000 > DR0 - 0000000000000000, DR1 - 0000000000000000, DR2 - 0000000000000000 > DR3 - 0000000000000000, DR6 - 00000000FFFF0FF0, DR7 - 0000000000000400 > GDTR - 000000007E9E2000 0000000000000047, LDTR - 0000000000000000 > IDTR - 000000007E4E5018 0000000000000FFF, TR - 0000000000000000 > FXSAVE_STATE - 000000007EFA0E60 > !!!! Find image based on IP(0x7DC29E46) (No PDB) (ImageBase=000000007DC28000, EntryPoint=000000007DC2B917) !!!! > > After the patch: > Booting `XenServer (Serial)'Booting `XenServer (Serial)' > Test message: Buffer too small > BdsDxe: loading Boot0000 "UiApp" from Fv(7CB8BDC9-F8EB-4F34-AAEA-3EE4AF6516A1)/FvFile(462CAA21-7614-4503-836E-8AB6F4662331) > BdsDxe: starting Boot0000 "UiApp" from Fv(7CB8BDC9-F8EB-4F34-AAEA-3EE4AF6516A1)/FvFile(462CAA21-7614-4503-836E-8AB6F4662331) > > Fixes: 9180f5365524 ("x86: add multiboot2 protocol support for EFI platforms") > Signed-off-by: Frediano Ziglio <frediano.ziglio@cloud.com> I was hoping it would fix also an issue with xen.efi as the crash is pretty similar (https://github.com/QubesOS/qubes-issues/issues/8206#issuecomment-2366835136), but it seems to be something different. Anyway, Acked-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com> > --- > xen/common/efi/boot.c | 46 ++++++++++++++++++++++++++++++------------- > 1 file changed, 32 insertions(+), 14 deletions(-) > --- > Changes since v1: > - added "Fixes:" tag; > - fixed cast style change. > > Changes since v2: > - wrap long line. > > Changes since v3: > - fixed "Fixes:" tag. > > diff --git a/xen/common/efi/boot.c b/xen/common/efi/boot.c > index efbec00af9..fdbe75005c 100644 > --- 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; > > @@ -308,7 +325,8 @@ static void __init PrintErrMesg(const CHAR16 *mesg, EFI_STATUS ErrCode) > PrintErr(L": "); > > if( (ErrIdx < ARRAY_SIZE(ErrCodeToStr)) && ErrCodeToStr[ErrIdx] ) > - mesg = ErrCodeToStr[ErrIdx]; > + mesg = (const CHAR16 *)((const void *)&ErrorStrings + > + ErrCodeToStr[ErrIdx]); > else > { > PrintErr(L"ErrCode: "); > -- > 2.46.0 > >
On 26/09/2024 2:26 pm, Marek Marczykowski-Górecki wrote: > On Mon, Aug 19, 2024 at 03:29:52PM +0100, Frediano Ziglio wrote: >> Although code is compiled with -fpic option data is not position >> independent. This causes data pointer to become invalid if >> code is not relocated properly which is what happens for >> efi_multiboot2 which is called by multiboot entry code. >> >> Code tested adding >> PrintErrMesg(L"Test message", EFI_BUFFER_TOO_SMALL); >> in efi_multiboot2 before calling efi_arch_edd (this function >> can potentially call PrintErrMesg). >> >> Before the patch (XenServer installation on Qemu, xen replaced >> with vanilla xen.gz): >> Booting `XenServer (Serial)'Booting `XenServer (Serial)' >> Test message: !!!! X64 Exception Type - 0E(#PF - Page-Fault) CPU Apic ID - 00000000 !!!! >> ExceptionData - 0000000000000000 I:0 R:0 U:0 W:0 P:0 PK:0 SS:0 SGX:0 >> RIP - 000000007DC29E46, CS - 0000000000000038, RFLAGS - 0000000000210246 >> RAX - 0000000000000000, RCX - 0000000000000050, RDX - 0000000000000000 >> RBX - 000000007DAB4558, RSP - 000000007EFA1200, RBP - 0000000000000000 >> RSI - FFFF82D040467A88, RDI - 0000000000000000 >> R8 - 000000007EFA1238, R9 - 000000007EFA1230, R10 - 0000000000000000 >> R11 - 000000007CF42665, R12 - FFFF82D040467A88, R13 - 000000007EFA1228 >> R14 - 000000007EFA1225, R15 - 000000007DAB45A8 >> DS - 0000000000000030, ES - 0000000000000030, FS - 0000000000000030 >> GS - 0000000000000030, SS - 0000000000000030 >> CR0 - 0000000080010033, CR2 - FFFF82D040467A88, CR3 - 000000007EC01000 >> CR4 - 0000000000000668, CR8 - 0000000000000000 >> DR0 - 0000000000000000, DR1 - 0000000000000000, DR2 - 0000000000000000 >> DR3 - 0000000000000000, DR6 - 00000000FFFF0FF0, DR7 - 0000000000000400 >> GDTR - 000000007E9E2000 0000000000000047, LDTR - 0000000000000000 >> IDTR - 000000007E4E5018 0000000000000FFF, TR - 0000000000000000 >> FXSAVE_STATE - 000000007EFA0E60 >> !!!! Find image based on IP(0x7DC29E46) (No PDB) (ImageBase=000000007DC28000, EntryPoint=000000007DC2B917) !!!! >> >> After the patch: >> Booting `XenServer (Serial)'Booting `XenServer (Serial)' >> Test message: Buffer too small >> BdsDxe: loading Boot0000 "UiApp" from Fv(7CB8BDC9-F8EB-4F34-AAEA-3EE4AF6516A1)/FvFile(462CAA21-7614-4503-836E-8AB6F4662331) >> BdsDxe: starting Boot0000 "UiApp" from Fv(7CB8BDC9-F8EB-4F34-AAEA-3EE4AF6516A1)/FvFile(462CAA21-7614-4503-836E-8AB6F4662331) >> >> Fixes: 9180f5365524 ("x86: add multiboot2 protocol support for EFI platforms") >> Signed-off-by: Frediano Ziglio <frediano.ziglio@cloud.com> > I was hoping it would fix also an issue with xen.efi as the crash is > pretty similar > (https://github.com/QubesOS/qubes-issues/issues/8206#issuecomment-2366835136), > but it seems to be something different. > > Anyway, > > Acked-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com> While I hate to drag this on further, there's a very relevant observation in https://lore.kernel.org/xen-devel/20240925150059.3955569-31-ardb+git@google.com/T/#u which was posted yesterday. Exactly the same is true of the early MB2 code calling into regular EFI code, and I wonder if it's causing the other issue too. I suspect following that pattern will be rather more robust. Thoughts? ~Andrew
On 26.09.2024 16:11, Andrew Cooper wrote: > On 26/09/2024 2:26 pm, Marek Marczykowski-Górecki wrote: >> On Mon, Aug 19, 2024 at 03:29:52PM +0100, Frediano Ziglio wrote: >>> Although code is compiled with -fpic option data is not position >>> independent. This causes data pointer to become invalid if >>> code is not relocated properly which is what happens for >>> efi_multiboot2 which is called by multiboot entry code. >>> >>> Code tested adding >>> PrintErrMesg(L"Test message", EFI_BUFFER_TOO_SMALL); >>> in efi_multiboot2 before calling efi_arch_edd (this function >>> can potentially call PrintErrMesg). >>> >>> Before the patch (XenServer installation on Qemu, xen replaced >>> with vanilla xen.gz): >>> Booting `XenServer (Serial)'Booting `XenServer (Serial)' >>> Test message: !!!! X64 Exception Type - 0E(#PF - Page-Fault) CPU Apic ID - 00000000 !!!! >>> ExceptionData - 0000000000000000 I:0 R:0 U:0 W:0 P:0 PK:0 SS:0 SGX:0 >>> RIP - 000000007DC29E46, CS - 0000000000000038, RFLAGS - 0000000000210246 >>> RAX - 0000000000000000, RCX - 0000000000000050, RDX - 0000000000000000 >>> RBX - 000000007DAB4558, RSP - 000000007EFA1200, RBP - 0000000000000000 >>> RSI - FFFF82D040467A88, RDI - 0000000000000000 >>> R8 - 000000007EFA1238, R9 - 000000007EFA1230, R10 - 0000000000000000 >>> R11 - 000000007CF42665, R12 - FFFF82D040467A88, R13 - 000000007EFA1228 >>> R14 - 000000007EFA1225, R15 - 000000007DAB45A8 >>> DS - 0000000000000030, ES - 0000000000000030, FS - 0000000000000030 >>> GS - 0000000000000030, SS - 0000000000000030 >>> CR0 - 0000000080010033, CR2 - FFFF82D040467A88, CR3 - 000000007EC01000 >>> CR4 - 0000000000000668, CR8 - 0000000000000000 >>> DR0 - 0000000000000000, DR1 - 0000000000000000, DR2 - 0000000000000000 >>> DR3 - 0000000000000000, DR6 - 00000000FFFF0FF0, DR7 - 0000000000000400 >>> GDTR - 000000007E9E2000 0000000000000047, LDTR - 0000000000000000 >>> IDTR - 000000007E4E5018 0000000000000FFF, TR - 0000000000000000 >>> FXSAVE_STATE - 000000007EFA0E60 >>> !!!! Find image based on IP(0x7DC29E46) (No PDB) (ImageBase=000000007DC28000, EntryPoint=000000007DC2B917) !!!! >>> >>> After the patch: >>> Booting `XenServer (Serial)'Booting `XenServer (Serial)' >>> Test message: Buffer too small >>> BdsDxe: loading Boot0000 "UiApp" from Fv(7CB8BDC9-F8EB-4F34-AAEA-3EE4AF6516A1)/FvFile(462CAA21-7614-4503-836E-8AB6F4662331) >>> BdsDxe: starting Boot0000 "UiApp" from Fv(7CB8BDC9-F8EB-4F34-AAEA-3EE4AF6516A1)/FvFile(462CAA21-7614-4503-836E-8AB6F4662331) >>> >>> Fixes: 9180f5365524 ("x86: add multiboot2 protocol support for EFI platforms") >>> Signed-off-by: Frediano Ziglio <frediano.ziglio@cloud.com> >> I was hoping it would fix also an issue with xen.efi as the crash is >> pretty similar >> (https://github.com/QubesOS/qubes-issues/issues/8206#issuecomment-2366835136), >> but it seems to be something different. >> >> Anyway, >> >> Acked-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com> > > While I hate to drag this on further, there's a very relevant observation in > > https://lore.kernel.org/xen-devel/20240925150059.3955569-31-ardb+git@google.com/T/#u > > which was posted yesterday. Exactly the same is true of the early MB2 > code calling into regular EFI code, and I wonder if it's causing the > other issue too. > > I suspect following that pattern will be rather more robust. Thoughts? That builds upon there being a secondary mapping, which isn't the case here I'm afraid. Jan
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. Jan
On Tue, Nov 5, 2024 at 2:52 PM Jan Beulich <jbeulich@suse.com> 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. > > Jan 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? Frediano
On 14.11.2024 15:02, Frediano Ziglio wrote: > On Tue, Nov 5, 2024 at 2:52 PM Jan Beulich <jbeulich@suse.com> 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. > > 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. Jan
On Thu, Nov 14, 2024 at 3:04 PM Jan Beulich <jbeulich@suse.com> wrote: > > On 14.11.2024 15:02, Frediano Ziglio wrote: > > On Tue, Nov 5, 2024 at 2:52 PM Jan Beulich <jbeulich@suse.com> 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. > > > > 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. > > Jan What alternative are you talking about? Was it something discussed elsewhere? Frediano
On 15.11.2024 11:27, Frediano Ziglio wrote: > On Thu, Nov 14, 2024 at 3:04 PM Jan Beulich <jbeulich@suse.com> wrote: >> >> On 14.11.2024 15:02, Frediano Ziglio wrote: >>> On Tue, Nov 5, 2024 at 2:52 PM Jan Beulich <jbeulich@suse.com> 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
diff --git a/xen/common/efi/boot.c b/xen/common/efi/boot.c index efbec00af9..fdbe75005c 100644 --- 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; @@ -308,7 +325,8 @@ static void __init PrintErrMesg(const CHAR16 *mesg, EFI_STATUS ErrCode) PrintErr(L": "); if( (ErrIdx < ARRAY_SIZE(ErrCodeToStr)) && ErrCodeToStr[ErrIdx] ) - mesg = ErrCodeToStr[ErrIdx]; + mesg = (const CHAR16 *)((const void *)&ErrorStrings + + ErrCodeToStr[ErrIdx]); else { PrintErr(L"ErrCode: ");
Although code is compiled with -fpic option data is not position independent. This causes data pointer to become invalid if code is not relocated properly which is what happens for efi_multiboot2 which is called by multiboot entry code. Code tested adding PrintErrMesg(L"Test message", EFI_BUFFER_TOO_SMALL); in efi_multiboot2 before calling efi_arch_edd (this function can potentially call PrintErrMesg). Before the patch (XenServer installation on Qemu, xen replaced with vanilla xen.gz): Booting `XenServer (Serial)'Booting `XenServer (Serial)' Test message: !!!! X64 Exception Type - 0E(#PF - Page-Fault) CPU Apic ID - 00000000 !!!! ExceptionData - 0000000000000000 I:0 R:0 U:0 W:0 P:0 PK:0 SS:0 SGX:0 RIP - 000000007DC29E46, CS - 0000000000000038, RFLAGS - 0000000000210246 RAX - 0000000000000000, RCX - 0000000000000050, RDX - 0000000000000000 RBX - 000000007DAB4558, RSP - 000000007EFA1200, RBP - 0000000000000000 RSI - FFFF82D040467A88, RDI - 0000000000000000 R8 - 000000007EFA1238, R9 - 000000007EFA1230, R10 - 0000000000000000 R11 - 000000007CF42665, R12 - FFFF82D040467A88, R13 - 000000007EFA1228 R14 - 000000007EFA1225, R15 - 000000007DAB45A8 DS - 0000000000000030, ES - 0000000000000030, FS - 0000000000000030 GS - 0000000000000030, SS - 0000000000000030 CR0 - 0000000080010033, CR2 - FFFF82D040467A88, CR3 - 000000007EC01000 CR4 - 0000000000000668, CR8 - 0000000000000000 DR0 - 0000000000000000, DR1 - 0000000000000000, DR2 - 0000000000000000 DR3 - 0000000000000000, DR6 - 00000000FFFF0FF0, DR7 - 0000000000000400 GDTR - 000000007E9E2000 0000000000000047, LDTR - 0000000000000000 IDTR - 000000007E4E5018 0000000000000FFF, TR - 0000000000000000 FXSAVE_STATE - 000000007EFA0E60 !!!! Find image based on IP(0x7DC29E46) (No PDB) (ImageBase=000000007DC28000, EntryPoint=000000007DC2B917) !!!! After the patch: Booting `XenServer (Serial)'Booting `XenServer (Serial)' Test message: Buffer too small BdsDxe: loading Boot0000 "UiApp" from Fv(7CB8BDC9-F8EB-4F34-AAEA-3EE4AF6516A1)/FvFile(462CAA21-7614-4503-836E-8AB6F4662331) BdsDxe: starting Boot0000 "UiApp" from Fv(7CB8BDC9-F8EB-4F34-AAEA-3EE4AF6516A1)/FvFile(462CAA21-7614-4503-836E-8AB6F4662331) Fixes: 9180f5365524 ("x86: add multiboot2 protocol support for EFI platforms") Signed-off-by: Frediano Ziglio <frediano.ziglio@cloud.com> --- xen/common/efi/boot.c | 46 ++++++++++++++++++++++++++++++------------- 1 file changed, 32 insertions(+), 14 deletions(-) --- Changes since v1: - added "Fixes:" tag; - fixed cast style change. Changes since v2: - wrap long line. Changes since v3: - fixed "Fixes:" tag.