[PATCH v2] Update EFI_COMPATIBILITY16_TABLE to match 0.98 spec update
We expect to use the space between the top of option ROMs and the bottom of our own BIOS code as a stack. OVMF was previously marking the whole region from 0xC0000 to 0xFFFFF read-only before invoking our Legacy16Boot method. Read-only stack considered harmful. Version 0.98 of the CSM spec adds the UmaAddress and UmaSize fields, which allow the CSM to specify a memory region that needs to be writable. There exists CONFIG_MALLOC_UPPERMEMORY which we could turn off to use the 9-segment, but that isn't particularly useful for the CSM case either because that memory isn't ours to play with until the final Legacy16Boot call. There's a LowPmmMemory given to use by UEFI to play with, but that's right in the *middle* of low memory and using that for persistent allocations would be painful. So just require CONFIG_MALLOC_UPPERMEMORY when building a CSM. Signed-off-by: David Woodhouse <David.Woodhouse@intel.com> --- v2: Require CONFIG_MALLOC_UPPERMEMORY. Default UmaAddress to &zonelow_base. src/Kconfig | 2 +- src/fw/csm.c | 6 +++++- src/std/LegacyBios.h | 20 ++++++++++++++++++++ 3 files changed, 26 insertions(+), 2 deletions(-) diff --git a/src/Kconfig b/src/Kconfig index a42ab2d..093075c 100644 --- a/src/Kconfig +++ b/src/Kconfig @@ -114,7 +114,7 @@ endchoice the BIOS in 16bit protected mode. config MALLOC_UPPERMEMORY - bool "Allocate memory that needs to be in first Meg above 0xc0000" + bool "Allocate memory that needs to be in first Meg above 0xc0000" if !CSM default y help Use the "Upper Memory Block" area (0xc0000-0xf0000) for diff --git a/src/fw/csm.c b/src/fw/csm.c index dfb0d12..4e4b688 100644 --- a/src/fw/csm.c +++ b/src/fw/csm.c @@ -34,6 +34,8 @@ EFI_COMPATIBILITY16_TABLE csm_compat_table VARFSEG __aligned(16) = { .Compatibility16CallOffset = 0 /* Filled in by checkrom.py */, .OemIdStringPointer = (u32)"SeaBIOS", .AcpiRsdPtrPointer = (u32)&csm_rsdp, + .UmaAddress = (u32)&zonelow_base, + .UmaSize = 0x10000, }; EFI_TO_COMPATIBILITY16_INIT_TABLE *csm_init_table; @@ -46,9 +48,11 @@ extern void __csm_return(struct bregs *regs) __noreturn; static void csm_return(struct bregs *regs) { - dprintf(3, "handle_csm returning AX=%04x\n", regs->ax); + u32 top = rom_get_max(); PICMask = pic_irqmask_read(); + csm_compat_table.UmaAddress = top; + csm_compat_table.UmaSize = 0xf0000 - top; __csm_return(regs); } diff --git a/src/std/LegacyBios.h b/src/std/LegacyBios.h index cf0c3c5..5170c37 100644 --- a/src/std/LegacyBios.h +++ b/src/std/LegacyBios.h @@ -228,6 +228,26 @@ typedef struct { /// Maximum PCI bus number assigned. /// UINT8 LastPciBus; + + /// + /// Start address of UMB RAM + /// + UINT32 UmaAddress; + + /// + /// Size of UMB RAM + /// + UINT32 UmaSize; + + /// + /// Start address of persistent allocation in high (>1MiB) memory + /// + UINT32 HiPermanentMemoryAddress; + + /// + /// Size of persistent allocation in high (>1MiB) memory + /// + UINT32 HiPermanentMemorySize; } EFI_COMPATIBILITY16_TABLE; /// -- 1.8.3.1 -- David Woodhouse Open Source Technology Centre David.Woodhouse@intel.com Intel Corporation
On Fri, Dec 06, 2013 at 11:56:22AM +0000, David Woodhouse wrote:
We expect to use the space between the top of option ROMs and the bottom of our own BIOS code as a stack. OVMF was previously marking the whole region from 0xC0000 to 0xFFFFF read-only before invoking our Legacy16Boot method. Read-only stack considered harmful.
Version 0.98 of the CSM spec adds the UmaAddress and UmaSize fields, which allow the CSM to specify a memory region that needs to be writable.
There exists CONFIG_MALLOC_UPPERMEMORY which we could turn off to use the 9-segment, but that isn't particularly useful for the CSM case either because that memory isn't ours to play with until the final Legacy16Boot call. There's a LowPmmMemory given to use by UEFI to play with, but that's right in the *middle* of low memory and using that for persistent allocations would be painful. So just require CONFIG_MALLOC_UPPERMEMORY when building a CSM.
Hi David, Are you still looking at this? If I recall correctly, you were going to run a test without CONFIG_MALLOC_UPPERMEMORY set. -Kevin
On Fri, 2014-01-10 at 12:49 -0500, Kevin O'Connor wrote:
On Fri, Dec 06, 2013 at 11:56:22AM +0000, David Woodhouse wrote:
We expect to use the space between the top of option ROMs and the bottom of our own BIOS code as a stack. OVMF was previously marking the whole region from 0xC0000 to 0xFFFFF read-only before invoking our Legacy16Boot method. Read-only stack considered harmful.
Version 0.98 of the CSM spec adds the UmaAddress and UmaSize fields, which allow the CSM to specify a memory region that needs to be writable.
There exists CONFIG_MALLOC_UPPERMEMORY which we could turn off to use the 9-segment, but that isn't particularly useful for the CSM case either because that memory isn't ours to play with until the final Legacy16Boot call. There's a LowPmmMemory given to use by UEFI to play with, but that's right in the *middle* of low memory and using that for persistent allocations would be painful. So just require CONFIG_MALLOC_UPPERMEMORY when building a CSM.
Hi David,
Are you still looking at this? If I recall correctly, you were going to run a test without CONFIG_MALLOC_UPPERMEMORY set.
Apologies for the sporadic nature of my attention to this. It's back at the top of my pile again for a while. I've submitted the corresponding patch to EDK2 and it should hopefully be accepted now that the spec is published. As I'm testing on Quark today, I tried reverting the EDK2-side patch and the SeaBIOS patch, and disabling CONFIG_MALLOC_UPPERMEMORY. It booted FreeDOS and worked fine. Then I re-enabled CONFIG_MALLOC_UPPERMEMORY and it still worked, which wasn't expected :) I'll have to retest with OVMF under Qemu (with KVM). -- dwmw2
On Fri, 2014-01-10 at 12:49 -0500, Kevin O'Connor wrote:
On Fri, Dec 06, 2013 at 11:56:22AM +0000, David Woodhouse wrote:
We expect to use the space between the top of option ROMs and the bottom of our own BIOS code as a stack. OVMF was previously marking the whole region from 0xC0000 to 0xFFFFF read-only before invoking our Legacy16Boot method. Read-only stack considered harmful.
Version 0.98 of the CSM spec adds the UmaAddress and UmaSize fields, which allow the CSM to specify a memory region that needs to be writable.
There exists CONFIG_MALLOC_UPPERMEMORY which we could turn off to use the 9-segment, but that isn't particularly useful for the CSM case either because that memory isn't ours to play with until the final Legacy16Boot call. There's a LowPmmMemory given to use by UEFI to play with, but that's right in the *middle* of low memory and using that for persistent allocations would be painful. So just require CONFIG_MALLOC_UPPERMEMORY when building a CSM.
Hi David,
Are you still looking at this? If I recall correctly, you were going to run a test without CONFIG_MALLOC_UPPERMEMORY set.
That appears to make it boot OK, although I'm still not sure it's *correct* to be using the 9-segment from the CSM before we've actually been told to *boot*. Maybe we just get lucky. Do you want a version of the patch which doesn't add the dependency on CONFIG_MALLOC_UPPERMEMORY? -- dwmw2
On Mon, May 19, 2014 at 09:12:28PM +0100, David Woodhouse wrote:
On Fri, 2014-01-10 at 12:49 -0500, Kevin O'Connor wrote:
On Fri, Dec 06, 2013 at 11:56:22AM +0000, David Woodhouse wrote:
We expect to use the space between the top of option ROMs and the bottom of our own BIOS code as a stack. OVMF was previously marking the whole region from 0xC0000 to 0xFFFFF read-only before invoking our Legacy16Boot method. Read-only stack considered harmful.
Version 0.98 of the CSM spec adds the UmaAddress and UmaSize fields, which allow the CSM to specify a memory region that needs to be writable.
There exists CONFIG_MALLOC_UPPERMEMORY which we could turn off to use the 9-segment, but that isn't particularly useful for the CSM case either because that memory isn't ours to play with until the final Legacy16Boot call. There's a LowPmmMemory given to use by UEFI to play with, but that's right in the *middle* of low memory and using that for persistent allocations would be painful. So just require CONFIG_MALLOC_UPPERMEMORY when building a CSM.
Hi David,
Are you still looking at this? If I recall correctly, you were going to run a test without CONFIG_MALLOC_UPPERMEMORY set.
That appears to make it boot OK, although I'm still not sure it's *correct* to be using the 9-segment from the CSM before we've actually been told to *boot*. Maybe we just get lucky.
Do you want a version of the patch which doesn't add the dependency on CONFIG_MALLOC_UPPERMEMORY?
Yes, thanks. We're about to make the next release, but we can put this in for the next release. -Kevin
From d478ac82045a27f82f44ea9ce65f642197fe6078 Mon Sep 17 00:00:00 2001 From: David Woodhouse <David.Woodhouse@intel.com> Date: Fri, 25 Jan 2013 19:39:07 -0600 Subject: [PATCH] Update EFI_COMPATIBILITY16_TABLE to match 0.98 spec update Unless CONFIG_MALLOC_UPPERMEMORY is turned off, we expect to use the space between the top of option ROMs and the bottom of our own BIOS code as a stack. OVMF was previously marking the whole region from 0xC0000 to 0xFFFFF read-only before invoking our Legacy16Boot method. Read-only stack considered harmful. Version 0.98 of the CSM spec adds the UmaAddress and UmaSize fields, which allow the CSM to specify a memory region that needs to be writable, so provide that information. Signed-off-by: David Woodhouse <David.Woodhouse@intel.com> --- With your incremental patch from 2013-12-02 at 12:46 -0500 merged. However, this doesn't work if I have both CONFIG_MALLOC_UPPERMEMORY *and* CONFIG_EXTRA_STACK enabled. Symptoms are the same as the original problem which both CONFIG_MALLOC_UPPERMEMORY and the CSM spec update set out to solve: it works only with KVM enabled, because we're writing to regions which are supposed to be marked as ROM. Last I see is: handle_csm returning AX=0000 UmaAddress ef000, size 1000 InstallProtocolInterface: DB9A1E3D-45CB-4ABB-853B-E5387FDB2E2D 6950C30 tqemu: fatal: Trying to execute code outside RAM or ROM at 0x00000000000a0000 src/fw/csm.c | 9 +++++++++ src/std/LegacyBios.h | 20 ++++++++++++++++++++ 2 files changed, 29 insertions(+) diff --git a/src/fw/csm.c b/src/fw/csm.c index a44ed26..1f85087 100644 --- a/src/fw/csm.c +++ b/src/fw/csm.c @@ -34,6 +34,10 @@ EFI_COMPATIBILITY16_TABLE csm_compat_table VARFSEG __aligned(16) = { .Compatibility16CallOffset = 0 /* Filled in by checkrom.py */, .OemIdStringPointer = (u32)"SeaBIOS", .AcpiRsdPtrPointer = (u32)&csm_rsdp, +#if CONFIG_MALLOC_UPPERMEMORY + .UmaAddress = (u32)&zonelow_base, + .UmaSize = 0x10000, +#endif }; EFI_TO_COMPATIBILITY16_INIT_TABLE *csm_init_table; @@ -49,6 +53,11 @@ csm_return(struct bregs *regs) dprintf(3, "handle_csm returning AX=%04x\n", regs->ax); PICMask = pic_irqmask_read(); + if (CONFIG_MALLOC_UPPERMEMORY) { + u32 top = rom_get_max(); + csm_compat_table.UmaAddress = top; + csm_compat_table.UmaSize = (u32)zonelow_base + 0x10000 - top; + } __csm_return(regs); } diff --git a/src/std/LegacyBios.h b/src/std/LegacyBios.h index cf0c3c5..5170c37 100644 --- a/src/std/LegacyBios.h +++ b/src/std/LegacyBios.h @@ -228,6 +228,26 @@ typedef struct { /// Maximum PCI bus number assigned. /// UINT8 LastPciBus; + + /// + /// Start address of UMB RAM + /// + UINT32 UmaAddress; + + /// + /// Size of UMB RAM + /// + UINT32 UmaSize; + + /// + /// Start address of persistent allocation in high (>1MiB) memory + /// + UINT32 HiPermanentMemoryAddress; + + /// + /// Size of persistent allocation in high (>1MiB) memory + /// + UINT32 HiPermanentMemorySize; } EFI_COMPATIBILITY16_TABLE; /// -- 1.9.0 -- dwmw2
On Tue, 2014-05-20 at 14:22 +0100, David Woodhouse wrote:
However, this doesn't work if I have both CONFIG_MALLOC_UPPERMEMORY *and* CONFIG_EXTRA_STACK enabled.
Hm, this appears to be because rom_get_max() is returning 0xef000, causing us to ask UEFI to leave only the range 0xef000-0xf0000 writeable. And that doesn't work quite so nicely when we use the extra stack which in my case is at 0xef520. Is rom_get_max() not what I should be using for this? -- dwmw2
On Wed, May 21, 2014 at 11:15:44AM +0100, David Woodhouse wrote:
On Tue, 2014-05-20 at 14:22 +0100, David Woodhouse wrote:
However, this doesn't work if I have both CONFIG_MALLOC_UPPERMEMORY *and* CONFIG_EXTRA_STACK enabled.
Did you mean CONFIG_ENTRY_EXTRASTACK? I don't see how CONFIG_ENTRY_EXTRASTACK would have an impact, as we allocate and use the same extra stack regardless of that config setting.
Hm, this appears to be because rom_get_max() is returning 0xef000, causing us to ask UEFI to leave only the range 0xef000-0xf0000 writeable. And that doesn't work quite so nicely when we use the extra stack which in my case is at 0xef520.
Why is it wrong to declare memory at 0xef000-0xf0000 and have a stack at 0xef520-0xefd20?
Is rom_get_max() not what I should be using for this?
That is the right function (when CONFIG_MALLOC_UPPERMEMORY). It's what fw/shadow.c:make_bios_readonly_intel() uses. -Kevin
On Wed, 2014-05-21 at 09:52 -0400, Kevin O'Connor wrote:
Why is it wrong to declare memory at 0xef000-0xf0000 and have a stack at 0xef520-0xefd20?
Er, it's not. I'm stupid. But still it didn't work and it was almost certainly because it's trying to write to read-only memory, given the symptoms and the fact that it doesn't fail when KVM is enabled. I'll run it in qemu with insane levels of tracing, and see if I can work out precisely where it goes wrong. -- David Woodhouse Open Source Technology Centre David.Woodhouse@intel.com Intel Corporation
On Wed, 2014-05-21 at 09:52 -0400, Kevin O'Connor wrote:
Why is it wrong to declare memory at 0xef000-0xf0000 and have a stack at 0xef520-0xefd20?
I found the problem and it was on the UEFI side, not with the SeaBIOS patch. UEFI was only unlocking the requested 0xef000-0xf0000 region when it actually issued a Legacy16Boot request. Prior to that, the entire 0xc0000-0x100000 region was remaining locked. I'll fix my EDK2 patch accordingly. -- David Woodhouse Open Source Technology Centre David.Woodhouse@intel.com Intel Corporation
On Tue, May 20, 2014 at 02:22:16PM +0100, David Woodhouse wrote:
Unless CONFIG_MALLOC_UPPERMEMORY is turned off, we expect to use the space between the top of option ROMs and the bottom of our own BIOS code as a stack. OVMF was previously marking the whole region from 0xC0000 to 0xFFFFF read-only before invoking our Legacy16Boot method. Read-only stack considered harmful.
Version 0.98 of the CSM spec adds the UmaAddress and UmaSize fields, which allow the CSM to specify a memory region that needs to be writable, so provide that information. [...] --- a/src/fw/csm.c +++ b/src/fw/csm.c @@ -34,6 +34,10 @@ EFI_COMPATIBILITY16_TABLE csm_compat_table VARFSEG __aligned(16) = { .Compatibility16CallOffset = 0 /* Filled in by checkrom.py */, .OemIdStringPointer = (u32)"SeaBIOS", .AcpiRsdPtrPointer = (u32)&csm_rsdp, +#if CONFIG_MALLOC_UPPERMEMORY + .UmaAddress = (u32)&zonelow_base, + .UmaSize = 0x10000, +#endif
Is there an advantage to setting this at compile time vs only setting these fields during runtime?
@@ -49,6 +53,11 @@ csm_return(struct bregs *regs) dprintf(3, "handle_csm returning AX=%04x\n", regs->ax);
PICMask = pic_irqmask_read(); + if (CONFIG_MALLOC_UPPERMEMORY) { + u32 top = rom_get_max(); + csm_compat_table.UmaAddress = top; + csm_compat_table.UmaSize = (u32)zonelow_base + 0x10000 - top; + }
Would this work instead? u32 rommax = rom_get_max(); extern u8 final_readonly_start[]; csm_compat_table.UmaAddress = rommax; csm_compat_table.UmaSize = (u32)final_readonly_start - rommax; This should result in the same values as your patch when CONFIG_MALLOC_UPPERMEMORY. For !CONFIG_MALLOC_UPPERMEMORY it will result in UmaAddress==final_readonly_start and UmaSize==0. -Kevin
On Wed, 2014-05-28 at 09:58 -0400, Kevin O'Connor wrote:
On Tue, May 20, 2014 at 02:22:16PM +0100, David Woodhouse wrote:
Unless CONFIG_MALLOC_UPPERMEMORY is turned off, we expect to use the space between the top of option ROMs and the bottom of our own BIOS code as a stack. OVMF was previously marking the whole region from 0xC0000 to 0xFFFFF read-only before invoking our Legacy16Boot method. Read-only stack considered harmful.
Version 0.98 of the CSM spec adds the UmaAddress and UmaSize fields, which allow the CSM to specify a memory region that needs to be writable, so provide that information. [...] --- a/src/fw/csm.c +++ b/src/fw/csm.c @@ -34,6 +34,10 @@ EFI_COMPATIBILITY16_TABLE csm_compat_table VARFSEG __aligned(16) = { .Compatibility16CallOffset = 0 /* Filled in by checkrom.py */, .OemIdStringPointer = (u32)"SeaBIOS", .AcpiRsdPtrPointer = (u32)&csm_rsdp, +#if CONFIG_MALLOC_UPPERMEMORY + .UmaAddress = (u32)&zonelow_base, + .UmaSize = 0x10000, +#endif
Is there an advantage to setting this at compile time vs only setting these fields during runtime?
No, I don't believe so. We are invoked with *everything* writeable the first time, so we don't have to have this filled in.
@@ -49,6 +53,11 @@ csm_return(struct bregs *regs) dprintf(3, "handle_csm returning AX=%04x\n", regs->ax);
PICMask = pic_irqmask_read(); + if (CONFIG_MALLOC_UPPERMEMORY) { + u32 top = rom_get_max(); + csm_compat_table.UmaAddress = top; + csm_compat_table.UmaSize = (u32)zonelow_base + 0x10000 - top; + }
Would this work instead?
u32 rommax = rom_get_max(); extern u8 final_readonly_start[]; csm_compat_table.UmaAddress = rommax; csm_compat_table.UmaSize = (u32)final_readonly_start - rommax;
This should result in the same values as your patch when CONFIG_MALLOC_UPPERMEMORY. For !CONFIG_MALLOC_UPPERMEMORY it will result in UmaAddress==final_readonly_start and UmaSize==0.
Looks sane; I'll test it. Perhaps when I get home next week. -- dwmw2
On Wed, 2014-05-28 at 09:58 -0400, Kevin O'Connor wrote:
Is there an advantage to setting this at compile time vs only setting these fields during runtime?
No.
Would this work instead?
u32 rommax = rom_get_max(); extern u8 final_readonly_start[]; csm_compat_table.UmaAddress = rommax; csm_compat_table.UmaSize = (u32)final_readonly_start - rommax;
Yes. New patch to follow... -- dwmw2
Unless CONFIG_MALLOC_UPPERMEMORY is turned off, we expect to use the space between the top of option ROMs and the bottom of our own BIOS code as a stack. OVMF was previously marking the whole region from 0xC0000 to 0xFFFFF read-only before invoking our Legacy16Boot method. Read-only stack considered harmful. Version 0.98 of the CSM spec adds the UmaAddress and UmaSize fields which allow the CSM to specify a memory region that needs to be writeable, so provide that information. Signed-off-by: David Woodhouse <David.Woodhouse@intel.com> --- src/fw/csm.c | 6 ++++++ src/std/LegacyBios.h | 20 ++++++++++++++++++++ 2 files changed, 26 insertions(+) diff --git a/src/fw/csm.c b/src/fw/csm.c index a44ed26..7cdb398 100644 --- a/src/fw/csm.c +++ b/src/fw/csm.c @@ -46,8 +46,14 @@ extern void __csm_return(struct bregs *regs) __noreturn; static void csm_return(struct bregs *regs) { + u32 rommax = rom_get_max(); + extern u8 final_readonly_start[]; + dprintf(3, "handle_csm returning AX=%04x\n", regs->ax); + csm_compat_table.UmaAddress = rommax; + csm_compat_table.UmaSize = (u32)final_readonly_start - rommax; + PICMask = pic_irqmask_read(); __csm_return(regs); } diff --git a/src/std/LegacyBios.h b/src/std/LegacyBios.h index cf0c3c5..5170c37 100644 --- a/src/std/LegacyBios.h +++ b/src/std/LegacyBios.h @@ -228,6 +228,26 @@ typedef struct { /// Maximum PCI bus number assigned. /// UINT8 LastPciBus; + + /// + /// Start address of UMB RAM + /// + UINT32 UmaAddress; + + /// + /// Size of UMB RAM + /// + UINT32 UmaSize; + + /// + /// Start address of persistent allocation in high (>1MiB) memory + /// + UINT32 HiPermanentMemoryAddress; + + /// + /// Size of persistent allocation in high (>1MiB) memory + /// + UINT32 HiPermanentMemorySize; } EFI_COMPATIBILITY16_TABLE; /// -- 1.9.0 -- dwmw2
On Mon, Jun 02, 2014 at 02:00:14PM +0100, David Woodhouse wrote:
Unless CONFIG_MALLOC_UPPERMEMORY is turned off, we expect to use the space between the top of option ROMs and the bottom of our own BIOS code as a stack. OVMF was previously marking the whole region from 0xC0000 to 0xFFFFF read-only before invoking our Legacy16Boot method. Read-only stack considered harmful.
Version 0.98 of the CSM spec adds the UmaAddress and UmaSize fields which allow the CSM to specify a memory region that needs to be writeable, so provide that information.
Thanks. I committed this patch. -Kevin
participants (2)
-
David Woodhouse -
Kevin O'Connor