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: ");
--
2.46.0On 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
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
>
>
--
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab
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 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
When entering via MB2+EFI, the early EFI code hasn't been relocated down to
it's load address. As a consequence, efi_multboot2() is still expecting to
run at high address.
To set this up, we need Xen's high mappings, while also retaining the EFI
physical-mode mappings in the low half. Introduce a new efi_l4_bootmap[] for
the purpose.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Frediano Ziglio <frediano.ziglio@cloud.com>
CC: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
CC: Daniel Smith <dpsmith@apertussolutions.com>
Very RFC, compile tested only.
---
xen/arch/x86/boot/head.S | 69 +++++++++++++++++++++++++++++++++++++-
xen/arch/x86/boot/x86_64.S | 4 +++
2 files changed, 72 insertions(+), 1 deletion(-)
diff --git a/xen/arch/x86/boot/head.S b/xen/arch/x86/boot/head.S
index e0901ee40044..ef07f30d13da 100644
--- a/xen/arch/x86/boot/head.S
+++ b/xen/arch/x86/boot/head.S
@@ -344,6 +344,66 @@ __efi64_mb2_start:
lea .Lmb2_no_ih(%rip),%r15
jz x86_32_switch
+ push %rax
+ push %rcx
+ push %rdx
+ push %rsi
+ push %rdi
+
+ /* Merge lower half of EFI pagtables with upper half of Xen pagetables */
+ mov %cr3, %rsi
+ lea efi_l4_bootmap(%rip), %rdi
+ mov $L4_PAGETABLE_ENTRIES / 2, %ecx
+ rep movsq
+ lea ((L4_PAGETABLE_ENTRIES / 2) * 8) + idle_pg_table(%rip), %rsi
+ mov $L4_PAGETABLE_ENTRIES / 2, %ecx
+ rep movsq
+
+ /* Switch to merged pagetables */
+ lea efi_l4_bootmap(%rip), %rax
+ mov %rax, %cr3
+
+ lea __image_base__(%rip), %esi
+
+ /* Map Xen into the higher mappings using 2M superpages. */
+ lea _PAGE_PSE + PAGE_HYPERVISOR_RWX + sym_esi(_start), %eax
+ mov $sym_offs(_start), %ecx /* %eax = PTE to write ^ */
+ mov $sym_offs(_end - 1), %edx
+ shr $L2_PAGETABLE_SHIFT, %ecx /* %ecx = First slot to write */
+ shr $L2_PAGETABLE_SHIFT, %edx /* %edx = Final slot to write */
+
+1: mov %eax, sym_offs(l2_xenmap)(%esi, %ecx, 8)
+ add $1, %ecx
+ add $1 << L2_PAGETABLE_SHIFT, %eax
+
+ cmp %edx, %ecx
+ jbe 1b
+
+ /*
+ * Map Xen into the directmap (needed for early-boot pagetable
+ * handling/walking), and identity map Xen into bootmap (needed for
+ * the transition into long mode), using 2M superpages.
+ */
+ lea sym_esi(_start), %ecx
+ lea -1 + sym_esi(_end), %edx
+ lea _PAGE_PSE + PAGE_HYPERVISOR_RWX(%ecx), %eax /* PTE to write. */
+ shr $L2_PAGETABLE_SHIFT, %ecx /* First slot to write. */
+ shr $L2_PAGETABLE_SHIFT, %edx /* Final slot to write. */
+
+1: mov %eax, sym_offs(l2_bootmap) (%esi, %ecx, 8)
+ mov %eax, sym_offs(l2_directmap)(%esi, %ecx, 8)
+ add $1, %ecx
+ add $1 << L2_PAGETABLE_SHIFT, %eax
+
+ cmp %edx, %ecx
+ jbe 1b
+
+ pop %rdi
+ pop %rsi
+ pop %rdx
+ pop %rcx
+ pop %rax
+
/* Save Multiboot2 magic on the stack. */
push %rax
@@ -354,8 +414,15 @@ __efi64_mb2_start:
* efi_multiboot2() is called according to System V AMD64 ABI:
* - IN: %rdi - EFI ImageHandle, %rsi - EFI SystemTable,
* %rdx - MB2 cmdline
+ *
+ * Call via the high mappings
*/
- call efi_multiboot2
+ lea __image_base__(%rip), %r10
+ lea efi_multiboot2(%rip), %rax
+ sub %r10, %rax
+ mov $__XEN_VIRT_START, %r10
+ addq %r10, %rax
+ call *%rax
/* Just pop an item from the stack. */
pop %rax
diff --git a/xen/arch/x86/boot/x86_64.S b/xen/arch/x86/boot/x86_64.S
index 04bb62ae8680..93938d0b03f8 100644
--- a/xen/arch/x86/boot/x86_64.S
+++ b/xen/arch/x86/boot/x86_64.S
@@ -206,3 +206,7 @@ GLOBAL(l2_bootmap)
GLOBAL(l3_bootmap)
.fill L3_PAGETABLE_ENTRIES, 8, 0
.size l3_bootmap, . - l3_bootmap
+
+efi_l4_bootmap:
+ .fill L4_PAGETABLE_ENTRIES, 8, 0
+ .size efi_l4_bootmap, . - efi_l4_bootmap
base-commit: 457052167b4dbcda59e06300039302479cc1debf
--
2.39.5
On Mon, Sep 30, 2024 at 1:06 PM Andrew Cooper <andrew.cooper3@citrix.com> wrote: > > When entering via MB2+EFI, the early EFI code hasn't been relocated down to > it's load address. As a consequence, efi_multboot2() is still expecting to > run at high address. > > To set this up, we need Xen's high mappings, while also retaining the EFI > physical-mode mappings in the low half. Introduce a new efi_l4_bootmap[] for > the purpose. > > Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> > --- > CC: Jan Beulich <JBeulich@suse.com> > CC: Roger Pau Monné <roger.pau@citrix.com> > CC: Frediano Ziglio <frediano.ziglio@cloud.com> > CC: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com> > CC: Daniel Smith <dpsmith@apertussolutions.com> > > Very RFC, compile tested only. Tested, it crashes. But not the point of a RFC. > --- > xen/arch/x86/boot/head.S | 69 +++++++++++++++++++++++++++++++++++++- > xen/arch/x86/boot/x86_64.S | 4 +++ > 2 files changed, 72 insertions(+), 1 deletion(-) > > diff --git a/xen/arch/x86/boot/head.S b/xen/arch/x86/boot/head.S > index e0901ee40044..ef07f30d13da 100644 > --- a/xen/arch/x86/boot/head.S > +++ b/xen/arch/x86/boot/head.S > @@ -344,6 +344,66 @@ __efi64_mb2_start: > lea .Lmb2_no_ih(%rip),%r15 > jz x86_32_switch > > + push %rax > + push %rcx > + push %rdx > + push %rsi > + push %rdi > + > + /* Merge lower half of EFI pagtables with upper half of Xen pagetables */ > + mov %cr3, %rsi > + lea efi_l4_bootmap(%rip), %rdi > + mov $L4_PAGETABLE_ENTRIES / 2, %ecx > + rep movsq > + lea ((L4_PAGETABLE_ENTRIES / 2) * 8) + idle_pg_table(%rip), %rsi > + mov $L4_PAGETABLE_ENTRIES / 2, %ecx > + rep movsq > + > + /* Switch to merged pagetables */ > + lea efi_l4_bootmap(%rip), %rax > + mov %rax, %cr3 > + I agree with Jan, we are presuming firmware to behave in some way we can't guarantee. For instance, it could assume that pages after %cr3 are leafs of the first. Or for some reason restore %cr3 at some point, making your code crash. This could be alleviated with a trampoline setting back %cr3 before calling EFI code but looks like quite some work to do. Ignoring those, it could work. > + lea __image_base__(%rip), %esi > + > + /* Map Xen into the higher mappings using 2M superpages. */ > + lea _PAGE_PSE + PAGE_HYPERVISOR_RWX + sym_esi(_start), %eax > + mov $sym_offs(_start), %ecx /* %eax = PTE to write ^ */ > + mov $sym_offs(_end - 1), %edx > + shr $L2_PAGETABLE_SHIFT, %ecx /* %ecx = First slot to write */ > + shr $L2_PAGETABLE_SHIFT, %edx /* %edx = Final slot to write */ > + > +1: mov %eax, sym_offs(l2_xenmap)(%esi, %ecx, 8) > + add $1, %ecx > + add $1 << L2_PAGETABLE_SHIFT, %eax > + > + cmp %edx, %ecx > + jbe 1b > + > + /* > + * Map Xen into the directmap (needed for early-boot pagetable > + * handling/walking), and identity map Xen into bootmap (needed for > + * the transition into long mode), using 2M superpages. > + */ > + lea sym_esi(_start), %ecx > + lea -1 + sym_esi(_end), %edx > + lea _PAGE_PSE + PAGE_HYPERVISOR_RWX(%ecx), %eax /* PTE to write. */ > + shr $L2_PAGETABLE_SHIFT, %ecx /* First slot to write. */ > + shr $L2_PAGETABLE_SHIFT, %edx /* Final slot to write. */ > + > +1: mov %eax, sym_offs(l2_bootmap) (%esi, %ecx, 8) > + mov %eax, sym_offs(l2_directmap)(%esi, %ecx, 8) > + add $1, %ecx > + add $1 << L2_PAGETABLE_SHIFT, %eax > + > + cmp %edx, %ecx > + jbe 1b > + I think in addition to these settings, you need to adjust the pointers already present in the half table you are copying (I suppose that's why it crashes). > + pop %rdi > + pop %rsi > + pop %rdx > + pop %rcx > + pop %rax > + > /* Save Multiboot2 magic on the stack. */ > push %rax > > @@ -354,8 +414,15 @@ __efi64_mb2_start: > * efi_multiboot2() is called according to System V AMD64 ABI: > * - IN: %rdi - EFI ImageHandle, %rsi - EFI SystemTable, > * %rdx - MB2 cmdline > + * > + * Call via the high mappings > */ > - call efi_multiboot2 > + lea __image_base__(%rip), %r10 > + lea efi_multiboot2(%rip), %rax > + sub %r10, %rax > + mov $__XEN_VIRT_START, %r10 > + addq %r10, %rax > + call *%rax > > /* Just pop an item from the stack. */ > pop %rax > diff --git a/xen/arch/x86/boot/x86_64.S b/xen/arch/x86/boot/x86_64.S > index 04bb62ae8680..93938d0b03f8 100644 > --- a/xen/arch/x86/boot/x86_64.S > +++ b/xen/arch/x86/boot/x86_64.S > @@ -206,3 +206,7 @@ GLOBAL(l2_bootmap) > GLOBAL(l3_bootmap) > .fill L3_PAGETABLE_ENTRIES, 8, 0 > .size l3_bootmap, . - l3_bootmap > + > +efi_l4_bootmap: > + .fill L4_PAGETABLE_ENTRIES, 8, 0 > + .size efi_l4_bootmap, . - efi_l4_bootmap > > base-commit: 457052167b4dbcda59e06300039302479cc1debf Another way to alleviate the "relocation issues" could be to relocate to the lower addresses, but this could end up setting some pointers that won't be relocated back to upper addresses. Another way would be to consider this code as a separate loader, making sure it's all data and code position independent (sorry, no compiler option could do, that's why I wrote that RPC script to check it). Note: not saying this patch could not work. Frediano
On 30.09.2024 14:06, Andrew Cooper wrote: > --- a/xen/arch/x86/boot/head.S > +++ b/xen/arch/x86/boot/head.S > @@ -344,6 +344,66 @@ __efi64_mb2_start: > lea .Lmb2_no_ih(%rip),%r15 > jz x86_32_switch > > + push %rax > + push %rcx > + push %rdx > + push %rsi > + push %rdi > + > + /* Merge lower half of EFI pagtables with upper half of Xen pagetables */ > + mov %cr3, %rsi > + lea efi_l4_bootmap(%rip), %rdi > + mov $L4_PAGETABLE_ENTRIES / 2, %ecx > + rep movsq > + lea ((L4_PAGETABLE_ENTRIES / 2) * 8) + idle_pg_table(%rip), %rsi > + mov $L4_PAGETABLE_ENTRIES / 2, %ecx > + rep movsq > + > + /* Switch to merged pagetables */ > + lea efi_l4_bootmap(%rip), %rax > + mov %rax, %cr3 While it may appear to work in some environments, I don't think we're allowed to replace page tables prior to calling ExitBootServices(). IOW we may not complain if somewhere this fails to work. This also builds on the assumption that no new L4 entries would be made by firmware, nor that it would put anything in the upper half of the L4 table. > + lea __image_base__(%rip), %esi > + > + /* Map Xen into the higher mappings using 2M superpages. */ > + lea _PAGE_PSE + PAGE_HYPERVISOR_RWX + sym_esi(_start), %eax I'm surprised to see you put in place new RWX mappings, when the goal supposedly is to get rid of any such. > + mov $sym_offs(_start), %ecx /* %eax = PTE to write ^ */ > + mov $sym_offs(_end - 1), %edx > + shr $L2_PAGETABLE_SHIFT, %ecx /* %ecx = First slot to write */ > + shr $L2_PAGETABLE_SHIFT, %edx /* %edx = Final slot to write */ > + > +1: mov %eax, sym_offs(l2_xenmap)(%esi, %ecx, 8) > + add $1, %ecx > + add $1 << L2_PAGETABLE_SHIFT, %eax > + > + cmp %edx, %ecx > + jbe 1b > + > + /* > + * Map Xen into the directmap (needed for early-boot pagetable > + * handling/walking), and identity map Xen into bootmap (needed for > + * the transition into long mode), using 2M superpages. > + */ > + lea sym_esi(_start), %ecx > + lea -1 + sym_esi(_end), %edx > + lea _PAGE_PSE + PAGE_HYPERVISOR_RWX(%ecx), %eax /* PTE to write. */ > + shr $L2_PAGETABLE_SHIFT, %ecx /* First slot to write. */ > + shr $L2_PAGETABLE_SHIFT, %edx /* Final slot to write. */ > + > +1: mov %eax, sym_offs(l2_bootmap) (%esi, %ecx, 8) > + mov %eax, sym_offs(l2_directmap)(%esi, %ecx, 8) > + add $1, %ecx > + add $1 << L2_PAGETABLE_SHIFT, %eax > + > + cmp %edx, %ecx > + jbe 1b > + > + pop %rdi > + pop %rsi > + pop %rdx > + pop %rcx > + pop %rax > + > /* Save Multiboot2 magic on the stack. */ > push %rax > > @@ -354,8 +414,15 @@ __efi64_mb2_start: > * efi_multiboot2() is called according to System V AMD64 ABI: > * - IN: %rdi - EFI ImageHandle, %rsi - EFI SystemTable, > * %rdx - MB2 cmdline > + * > + * Call via the high mappings > */ > - call efi_multiboot2 > + lea __image_base__(%rip), %r10 > + lea efi_multiboot2(%rip), %rax > + sub %r10, %rax > + mov $__XEN_VIRT_START, %r10 > + addq %r10, %rax > + call *%rax While with this you arrange for all %rip-relative addressing to result in using the linked-at positions, static pointers requiring base relocations will still point at the loaded-at locations. Things would get particularly interesting if the difference of two such (dissimilar) pointers would then be calculated. Jan
© 2016 - 2024 Red Hat, Inc.