On Wed, Jan 28, 2015 at 11:03:19AM -0500, Konrad Rzeszutek Wilk wrote:
> On Wed, Jan 28, 2015 at 08:40:44AM +0000, Jan Beulich wrote:
> > >>> On 27.01.15 at 19:20, <konrad.w...@oracle.com> wrote:
> > > On Tue, Jan 27, 2015 at 04:17:05PM +0000, Jan Beulich wrote:
> > >> Again - apart from mapping the range, did you also make sure it
> > >> didn't get passed to the allocator (and hence couldn't have got
> > >> overwritten)?
> > >
> > > Yes, see patch:
> >
> > Oh, sorry, I must have not looked closely enough.
> >
> > > Also see attached of the code with what Linux sees and what Xen sees
> > > (Linux first).
> >
> > Indeed this
> >
> > 8b: 44 38 2d c2 10 00 00 cmp %r13b,0x10c2(%rip) #
> > 0x115 [01 01 00 00 00 00 00 00][00 01 00 00 00 00 00 00
> > 92: 75 12 jne 0xa6
> >
> > causes the code in question to be skipped under Linux.
> >
> > > I am thinking that the firmware is under the assumption
> > > that if SetVirtualAddressMap is not called then you MUST be still
> > > before ExitBootServices has been called. Going to verify that by
> > > implementing an GetNextVariableName before calling ExitBootServices)
> >
> > Not sure how exactly you envision to do this, but I'm having a hard
> > time seeing how this would prove anything, in particular because
> > calling runtime services functions prior to exiting boot services must
> > be possible anyway.
>
> And it works - one of the patches implements an routine to call the
> dreaded function before ExitBootServices and it works nicely (ie - I
> see:
>
> Xen 4.6-unstable (c/s Tue Jan 27 14:05:36 2015 -0500 git:332c049-dirty) EFI
> loader
> Using configuration file 'xen.cfg'
> vmlinuz-3.19.0-rc4+: 0x00000000cf81a000-0x00000000cfda5f90
> initramfs-3.19.0-rc4+.img: 0x00000000cb522000-0x00000000cd7b04d3
> GetNextVariableName:
> 0x8be4df610x93ca0x11d20xaa0xd0x00xe00x980x30x2b0x8c:SimpleBootFlag 0x0
> 0x5e724c0c0x5c030x45430xbc0xb60xc10xe20x3d0xe20x410x36:TpmSaveState 0x0
> 0x6403753b0xabde0x4da20xaa0x110x690x830xef0x2a0x7a0x69:TpmAcpiData 0x0
> 0x8be4df610x93ca0x11d20xaa0xd0x00xe00x980x30x2b0x8c:SetupMode 0x0
> 0x8be4df610x93ca0x11d20xaa0xd0x00xe00x980x30x2b0x8c:SecureBoot 0x0
> 0xe5bbf7be0x24170x499b0x970xdb0x390xf40x890x630x910xbc:BuildDate 0x0
> Could not get next variable
>
>
> It is just that making that call after ExitBootServices does not
> work. But on Linux the extra mix is that we also call 'SetVirtualAddressMap'
> which we do not do.
> >
> > And iirc you had already tried calling the function prior to doing much
> > else (namely, prior to loading Dom0), and it still crashed? Did you
>
> Yes, it did crash. That is - we call it _after_ calling ExitBootServices.
>
> > investigate when the memory type of that region changes (in an
> > earlier mail you said dmem from the EFI shell reported it as Boot
> > Services, albeit it's not fully clear what that tagging is supposed to
> > be telling us)?
>
> It did not help. Having the mapping of BootServicesData&Code did not
> help with the problem. However having disabled the call to
> ExitBootServices the mapping of BootServicesData&Code did help - as we
> were able to make the proper calls.
>
> In short, I think the firmware has a bug where it makes the assumption
> that if SetVirtualAddressMap then BootServices should _not_ be used
> - otherwise it will use it.
>
> I am not really sure of what the work-around should be in Xen except
> making SetVirtualAddressMap work..
And the patches I sent were missing one. Here are all of them.
> >
> > Jan
> >
>From 60f7b43349c2ed961459a1902165a41799d58c90 Mon Sep 17 00:00:00 2001
From: Konrad Rzeszutek Wilk <konrad.w...@oracle.com>
Date: Sun, 25 Jan 2015 16:50:58 -0500
Subject: [PATCH 1/5] EFI: Call GetNextVariableName during normal init.
Along with some extra debugging code to help diagnose
the issue.
Signed-off-by: Konrad Rzeszutek Wilk <konrad.w...@oracle.com>
---
xen/arch/x86/efi/stub.c | 1 +
xen/arch/x86/setup.c | 7 +++
xen/common/efi/boot.c | 3 +
xen/common/efi/runtime.c | 148 ++++++++++++++++++++++++++++++++++++++++++++++-
xen/include/xen/efi.h | 1 +
5 files changed, 159 insertions(+), 1 deletion(-)
diff --git a/xen/arch/x86/efi/stub.c b/xen/arch/x86/efi/stub.c
index b8f49f8..cefd18d 100644
--- a/xen/arch/x86/efi/stub.c
+++ b/xen/arch/x86/efi/stub.c
@@ -21,6 +21,7 @@ unsigned long efi_get_time(void)
return 0;
}
+long efi_debug(void) { return -ENOSYS; }
void efi_halt_system(void) { }
void efi_reset_system(bool_t warm) { }
diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
index 39f2a4d..f82e3c0 100644
--- a/xen/arch/x86/setup.c
+++ b/xen/arch/x86/setup.c
@@ -1316,6 +1316,13 @@ void __init noreturn __start_xen(unsigned long mbi_p)
console_init_postirq();
+ if ( efi_enabled )
+ {
+ long ret = efi_debug();
+ if ( ret )
+ printk("efi_debug return code: %lx\n", ret);
+ }
+
system_state = SYS_STATE_smp_boot;
do_presmp_initcalls();
diff --git a/xen/common/efi/boot.c b/xen/common/efi/boot.c
index ac6881e..c04e0a2 100644
--- a/xen/common/efi/boot.c
+++ b/xen/common/efi/boot.c
@@ -1163,7 +1163,10 @@ void __init efi_init_memory(void)
desc->Type, desc->Attribute);
if ( !efi_rs_enable || !(desc->Attribute & EFI_MEMORY_RUNTIME) )
+ {
+ printk(XENLOG_INFO " .. skipped!\n");
continue;
+ }
desc->VirtualStart = INVALID_VIRTUAL_ADDRESS;
diff --git a/xen/common/efi/runtime.c b/xen/common/efi/runtime.c
index c840e08..ef6b1df 100644
--- a/xen/common/efi/runtime.c
+++ b/xen/common/efi/runtime.c
@@ -5,7 +5,8 @@
#include <xen/guest_access.h>
#include <xen/irq.h>
#include <xen/time.h>
-
+#include <xen/delay.h>
+#include <xen/softirq.h>
DEFINE_XEN_GUEST_HANDLE(CHAR16);
#ifndef COMPAT
@@ -128,6 +129,151 @@ unsigned long efi_get_time(void)
time.Hour, time.Minute, time.Second);
}
+static void _delay(void)
+{
+ unsigned int i;
+
+ printk("Delay of 3 seconds: ");
+ for (i = 0; i < 3; i++)
+ {
+ unsigned int j;
+ printk("..%d", (3 - i));
+ for (j = 0; j < 100; j++)
+ {
+ process_pending_softirqs();
+ mdelay(10);
+ }
+ }
+ printk("\n");
+}
+#define DUMP_LEN 1024
+static void __init _dumpcode(unsigned long s, unsigned long e)
+{
+ unsigned long idx, e_idx;
+ unsigned long cr3;
+ unsigned int i;
+ char *code;
+
+ if ( s > e )
+ return;
+
+ idx = s;
+
+ code = xzalloc_bytes(DUMP_LEN);
+ if ( !code )
+ return;
+
+ printk("%lx -> %lx\nCode:", s, e);
+ do {
+ e_idx = idx + DUMP_LEN - 1;
+ if ( e_idx > e )
+ e_idx = e;
+
+ process_pending_softirqs();
+
+ memset(code, 0, DUMP_LEN);
+
+ cr3 = efi_rs_enter();
+ memcpy(code, (void *)idx, e_idx - idx);
+ efi_rs_leave(cr3);
+
+ for ( i = 0; i < e_idx - idx ;i++)
+ {
+ if ( i & 0xFF )
+ process_pending_softirqs();
+ printk(" %02x", (unsigned short)code[i] & 0xFF);
+ }
+ printk("\n");
+ idx = e_idx + 1;
+ } while ( idx < e );
+ xfree(code);
+ printk("\n");
+ _delay();
+}
+
+long __init efi_debug(void)
+{
+ unsigned long cr3 = efi_rs_enter();
+ union {
+ CHAR16 *str;
+ unsigned char *raw;
+ } name;
+ char *p;
+ UINTN size = 1024;
+ struct xenpf_efi_guid guid;
+ EFI_STATUS status;
+ unsigned int i, idx;
+ unsigned int rev;
+ unsigned long get;
+
+ if ( !cr3 )
+ return -EOPNOTSUPP;
+ efi_rs_leave(cr3);
+
+ name.raw = xzalloc_bytes(size);
+ if ( !name.raw )
+ return -ENOMEM;
+
+ p = xzalloc_bytes(size);
+ if ( !p )
+ {
+ xfree(name.raw);
+ return -ENOMEM;
+ }
+
+ printk("EFI v");
+
+ cr3 = efi_rs_enter();
+ rev = efi_rs->Hdr.Revision;
+ efi_rs_leave(cr3);
+
+ printk("%d.%d", rev >> 16, rev & 0xF);
+
+ cr3 = efi_rs_enter();
+ get = (unsigned long)efi_rs->GetNextVariableName;
+ efi_rs_leave(cr3);
+
+ printk(", GetNextVariableName: %lx\n", get);
+
+ _dumpcode(get, get+1024);
+
+ idx = 1;
+ do {
+ printk("%4d:", idx++);
+
+ cr3 = efi_rs_enter();
+ if ( !cr3 )
+ break;
+
+ size = 1024;
+ status = efi_rs->GetNextVariableName(&size, name.str, (void *)&guid);
+ efi_rs_leave(cr3);
+
+ if ( !EFI_ERROR(status) )
+ {
+ unsigned int j;
+
+ printk("%04x-%02x-%02x-", guid.data1, guid.data2, guid.data3);
+ for ( i = 0; i < 8; i++)
+ printk("-%02x", guid.data4[i]);
+
+ for ( i = 0, j = 0; i < size && j < size / sizeof(CHAR16); i++,
j++)
+ p[j] = name.str[i] & 0xFF;
+
+ p[j] = '\0';
+ printk(": %s [%lx]\n", p, status);
+ } else
+ printk("EFI_ERROR: %lx\n", status);
+ } while ( !EFI_ERROR(status) );
+
+ xfree(p);
+ xfree(name.raw);
+
+ if ( EFI_ERROR(EFI_NOT_FOUND) )
+ return 0;
+
+ return status;
+}
void efi_halt_system(void)
{
EFI_STATUS status;
diff --git a/xen/include/xen/efi.h b/xen/include/xen/efi.h
index 5e02724..4e9ffbf 100644
--- a/xen/include/xen/efi.h
+++ b/xen/include/xen/efi.h
@@ -30,6 +30,7 @@ struct compat_pf_efi_runtime_call;
void efi_init_memory(void);
paddr_t efi_rs_page_table(void);
unsigned long efi_get_time(void);
+long efi_debug(void);
void efi_halt_system(void);
void efi_reset_system(bool_t warm);
#ifndef COMPAT
--
2.1.0
>From 49aab3d6e623c907164475403ba495ede442e6e5 Mon Sep 17 00:00:00 2001
From: Konrad Rzeszutek Wilk <konrad.w...@oracle.com>
Date: Tue, 27 Jan 2015 15:32:10 -0500
Subject: [PATCH 2/5] EFI: Map also BootServicesData and BootServicesCode
Signed-off-by: Konrad Rzeszutek Wilk <konrad.w...@oracle.com>
---
xen/arch/x86/efi/efi-boot.h | 2 --
xen/common/efi/boot.c | 27 ++++++++++++++++++++++++---
2 files changed, 24 insertions(+), 5 deletions(-)
diff --git a/xen/arch/x86/efi/efi-boot.h b/xen/arch/x86/efi/efi-boot.h
index 3a3b4fe..c3bdb8d 100644
--- a/xen/arch/x86/efi/efi-boot.h
+++ b/xen/arch/x86/efi/efi-boot.h
@@ -152,8 +152,6 @@ static void __init
efi_arch_process_memory_map(EFI_SYSTEM_TABLE *SystemTable,
type = E820_RESERVED;
break;
case EfiConventionalMemory:
- case EfiBootServicesCode:
- case EfiBootServicesData:
if ( !trampoline_phys && desc->PhysicalStart + len <= 0x100000 &&
len >= cfg.size && desc->PhysicalStart + len > cfg.addr )
cfg.addr = (desc->PhysicalStart + len - cfg.size) & PAGE_MASK;
diff --git a/xen/common/efi/boot.c b/xen/common/efi/boot.c
index c04e0a2..b0bbc4b 100644
--- a/xen/common/efi/boot.c
+++ b/xen/common/efi/boot.c
@@ -1156,13 +1156,23 @@ void __init efi_init_memory(void)
u64 len = desc->NumberOfPages << EFI_PAGE_SHIFT;
unsigned long smfn, emfn;
unsigned int prot = PAGE_HYPERVISOR;
+ unsigned int skip = 1;
printk(XENLOG_INFO " %013" PRIx64 "-%013" PRIx64
" type=%u attr=%016" PRIx64 "\n",
desc->PhysicalStart, desc->PhysicalStart + len - 1,
desc->Type, desc->Attribute);
- if ( !efi_rs_enable || !(desc->Attribute & EFI_MEMORY_RUNTIME) )
+ if ( desc->Attribute & EFI_MEMORY_RUNTIME )
+ skip = 0;
+
+ if ( desc->Type == 4 && desc->Attribute != 0 )
+ skip = 0;
+
+ if ( desc->Type == 3 && desc->Attribute != 0 )
+ skip = 0;
+
+ if ( !efi_rs_enable || skip )
{
printk(XENLOG_INFO " .. skipped!\n");
continue;
@@ -1246,18 +1256,29 @@ void __init efi_init_memory(void)
copy_mapping(0, max_page, ram_range_valid);
+ printk(XENLOG_INFO "Copying..\n");
/* Insert non-RAM runtime mappings inside the direct map. */
for ( i = 0; i < efi_memmap_size; i += efi_mdesc_size )
{
const EFI_MEMORY_DESCRIPTOR *desc = efi_memmap + i;
- if ( (desc->Attribute & EFI_MEMORY_RUNTIME) &&
+ if ( ((desc->Attribute & EFI_MEMORY_RUNTIME) ||
+ (desc->Type == 3 && desc->Attribute != 0 ) ||
+ (desc->Type == 4 && desc->Attribute != 0 )) &&
desc->VirtualStart != INVALID_VIRTUAL_ADDRESS &&
- desc->VirtualStart != desc->PhysicalStart )
+ desc->VirtualStart != desc->PhysicalStart ) {
+
+ printk(XENLOG_INFO " %013" PRIx64 "-%013" PRIx64
+ " type=%u attr=%016" PRIx64 "\n",
+ PFN_DOWN(desc->PhysicalStart),
+ PFN_UP(desc->PhysicalStart + (desc->NumberOfPages <<
EFI_PAGE_SHIFT)),
+ desc->Type, desc->Attribute);
+
copy_mapping(PFN_DOWN(desc->PhysicalStart),
PFN_UP(desc->PhysicalStart +
(desc->NumberOfPages << EFI_PAGE_SHIFT)),
rt_range_valid);
+ }
}
/* Insert non-RAM runtime mappings outside of the direct map. */
--
2.1.0
>From ffb09e4938021ed47bc7097585dd19a10f2e09de Mon Sep 17 00:00:00 2001
From: Konrad Rzeszutek Wilk <konrad.w...@oracle.com>
Date: Tue, 27 Jan 2015 14:04:30 -0500
Subject: [PATCH 3/5] EFI/early: Implement GetNextVariableName and /query and
/noexitboot parameters
In the early EFI boot we will enumerate up to five EFI variables
to make sure it works.
The /query will just enumerate them and then quit. Helps in
troubleshooting and redirecting the output to a file (xen.efi /query > q).
The /noexitboot will inhibit Xen from calling ExitBootServices.
This allows on Lenovo ThinkPad x230 to use GetNextVariableName
in 1-1 mapping mode.
Signed-off-by: Konrad Rzeszutek Wilk <konrad.w...@oracle.com>
---
xen/common/efi/boot.c | 83 ++++++++++++++++++++++++++++++++++++++++++++++++++-
1 file changed, 82 insertions(+), 1 deletion(-)
diff --git a/xen/common/efi/boot.c b/xen/common/efi/boot.c
index b0bbc4b..4ee8f68 100644
--- a/xen/common/efi/boot.c
+++ b/xen/common/efi/boot.c
@@ -673,6 +673,74 @@ static void __init setup_efi_pci(void)
efi_bs->FreePool(handles);
}
+static int __init efi_getnextvariable(bool_t query_only)
+{
+ EFI_GUID guid;
+ UINTN i, size, idx;
+ CHAR16 *name;
+ EFI_STATUS status;
+
+
+ status = efi_bs->AllocatePool(EfiLoaderData, 1024, (void *)&name);
+ if ( EFI_ERROR(status) )
+ return status;
+
+ guid.Data1 = 0;
+ guid.Data2 = 0;
+ guid.Data3 = 0;
+ for ( i = 0; i < 8; i++ )
+ guid.Data4[i] = 0;
+
+ for ( i = 0 ; i < 1024 / sizeof (CHAR16); i++ )
+ name[i] = 0;
+
+ PrintStr(L"GetNextVariableName: ");
+ PrintStr(name);
+ PrintStr(newline);
+ idx = 0;
+ do {
+ size = 1024;
+ efi_rs->GetNextVariableName(&size, name, &guid);
+ if ( EFI_ERROR(status) )
+ {
+ if ( query_only )
+ {
+ efi_bs->FreePool(name);
+ PrintErrMesg(L"Failed to GetNextVariableName", status);
+ }
+ else
+ {
+ PrintStr(L"Warning: GetNextVariableName: ");
+ DisplayUint(status, 0);
+ PrintStr(newline);
+ }
+ }
+ else
+ {
+ DisplayUint(guid.Data1, 4);
+ DisplayUint(guid.Data2, 2);
+ DisplayUint(guid.Data3, 2);
+ for ( i = 0; i < 8; i++ )
+ DisplayUint(guid.Data4[i], 1);
+
+ PrintStr(L":");
+ PrintStr(name);
+ PrintStr(L" ");
+ DisplayUint(status, 0);
+ PrintStr(newline);
+ }
+ } while ( !EFI_ERROR(status) && idx++ < 5 );
+
+ efi_bs->FreePool(name);
+
+ if ( query_only )
+ return -EINVAL;
+
+ if ( EFI_ERROR(EFI_NOT_FOUND) )
+ return 0;
+
+ return status;
+}
static int __init __maybe_unused set_color(u32 mask, int bpp, u8 *pos, u8 *sz)
{
if ( bpp < 0 )
@@ -705,8 +773,10 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE
*SystemTable)
EFI_GRAPHICS_OUTPUT_MODE_INFORMATION *mode_info;
union string section = { NULL }, name;
bool_t base_video = 0, retry;
+ bool_t query_only = 0;
char *option_str;
bool_t use_cfg_file;
+ bool_t exit_boot_services = 0;
efi_ih = ImageHandle;
efi_bs = SystemTable->BootServices;
@@ -751,6 +821,10 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE
*SystemTable)
{
if ( wstrcmp(ptr + 1, L"basevideo") == 0 )
base_video = 1;
+ else if ( wstrcmp(ptr + 1, L"query") == 0 )
+ query_only = 1;
+ else if ( wstrcmp(ptr + 1, L"noexitboot") == 0 )
+ exit_boot_services = 0;
else if ( wstrncmp(ptr + 1, L"cfg=", 4) == 0 )
cfg_file_name = ptr + 5;
else if ( i + 1 < argc && wstrcmp(ptr + 1, L"cfg") == 0 )
@@ -1031,6 +1105,9 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE
*SystemTable)
PrintStr(newline);
}
+ if ( efi_getnextvariable(query_only) )
+ blexit(L"Could not get next variable");
+
efi_arch_memory_setup();
if ( gop )
@@ -1064,7 +1141,11 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE
*SystemTable)
efi_arch_pre_exit_boot();
- status = efi_bs->ExitBootServices(ImageHandle, map_key);
+ if ( exit_boot_services )
+ status = efi_bs->ExitBootServices(ImageHandle, map_key);
+ else
+ status = 0;
+
if ( status != EFI_INVALID_PARAMETER || retry )
break;
}
--
2.1.0
>From 76bee287621d22b2c9082eae6eacfea2446722c9 Mon Sep 17 00:00:00 2001
From: Konrad Rzeszutek Wilk <konrad.w...@oracle.com>
Date: Tue, 27 Jan 2015 15:35:35 -0500
Subject: [PATCH 4/5] EFI/early: Swap noexitboot to exitboot and by default
don't call ExitBootServices.
Signed-off-by: Konrad Rzeszutek Wilk <konrad.w...@oracle.com>
---
xen/common/efi/boot.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/xen/common/efi/boot.c b/xen/common/efi/boot.c
index 4ee8f68..d9aabd3 100644
--- a/xen/common/efi/boot.c
+++ b/xen/common/efi/boot.c
@@ -823,8 +823,8 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE
*SystemTable)
base_video = 1;
else if ( wstrcmp(ptr + 1, L"query") == 0 )
query_only = 1;
- else if ( wstrcmp(ptr + 1, L"noexitboot") == 0 )
- exit_boot_services = 0;
+ else if ( wstrcmp(ptr + 1, L"exitboot") == 0 )
+ exit_boot_services = 1;
else if ( wstrncmp(ptr + 1, L"cfg=", 4) == 0 )
cfg_file_name = ptr + 5;
else if ( i + 1 < argc && wstrcmp(ptr + 1, L"cfg") == 0 )
@@ -834,6 +834,8 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE
*SystemTable)
{
PrintStr(L"Xen EFI Loader options:\r\n");
PrintStr(L"-basevideo retain current video mode\r\n");
+ PrintStr(L"-exitboot call ExitBootServices\r\n");
+ PrintStr(L"-query call GetNextVariableName for up to
five times\r\n");
PrintStr(L"-cfg=<file> specify configuration file\r\n");
PrintStr(L"-help, -? display this help\r\n");
blexit(NULL);
--
2.1.0
>From 9f7469cd3b8a89594b16f2e39886948987bfa2ae Mon Sep 17 00:00:00 2001
From: Konrad Rzeszutek Wilk <konrad.w...@oracle.com>
Date: Tue, 27 Jan 2015 16:09:51 -0500
Subject: [PATCH 5/5] EFI: Dump 0xcfda270 and the other address
Signed-off-by: Konrad Rzeszutek Wilk <konrad.w...@oracle.com>
---
xen/common/efi/runtime.c | 7 ++++++-
1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/xen/common/efi/runtime.c b/xen/common/efi/runtime.c
index ef6b1df..79e8072 100644
--- a/xen/common/efi/runtime.c
+++ b/xen/common/efi/runtime.c
@@ -236,7 +236,12 @@ long __init efi_debug(void)
printk(", GetNextVariableName: %lx\n", get);
_dumpcode(get, get+1024);
-
+ get = 0xcfdba270;
+ _dumpcode(get, get+8);
+ _dumpcode(get + 0x18, get + 0x18 + 8);
+ _dumpcode(get + 0x20, get + 0x20 + 8);
+ get = 0xcfdc3048;
+ _dumpcode(get, get+1024);
idx = 1;
do {
printk("%4d:", idx++);
--
2.1.0
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
