Re: [SeaBIOS] [PATCH v3] Update EFI_COMPATIBILITY16_TABLE to match 0.98 spec update
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 smime.p7s Description: S/MIME cryptographic signature ___ SeaBIOS mailing list SeaBIOS@seabios.org http://www.seabios.org/mailman/listinfo/seabios
Re: [SeaBIOS] [PATCH v3] Update EFI_COMPATIBILITY16_TABLE to match 0.98 spec update
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 0xC to
0xF 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 = 0x1,
+#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 + 0x1 - 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
smime.p7s
Description: S/MIME cryptographic signature
___
SeaBIOS mailing list
SeaBIOS@seabios.org
http://www.seabios.org/mailman/listinfo/seabios
Re: [SeaBIOS] [PATCH v3] Update EFI_COMPATIBILITY16_TABLE to match 0.98 spec update
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 0xC to
0xF 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 = 0x1,
+#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 + 0x1 - 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
___
SeaBIOS mailing list
SeaBIOS@seabios.org
http://www.seabios.org/mailman/listinfo/seabios
Re: [SeaBIOS] [PATCH v3] Update EFI_COMPATIBILITY16_TABLE to match 0.98 spec update
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-0xf 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 smime.p7s Description: S/MIME cryptographic signature ___ SeaBIOS mailing list SeaBIOS@seabios.org http://www.seabios.org/mailman/listinfo/seabios
Re: [SeaBIOS] [PATCH v3] Update EFI_COMPATIBILITY16_TABLE to match 0.98 spec update
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-0xf 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-0xf 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 ___ SeaBIOS mailing list SeaBIOS@seabios.org http://www.seabios.org/mailman/listinfo/seabios
Re: [SeaBIOS] [PATCH v3] Update EFI_COMPATIBILITY16_TABLE to match 0.98 spec update
On Wed, 2014-05-21 at 09:52 -0400, Kevin O'Connor wrote: Why is it wrong to declare memory at 0xef000-0xf 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 WoodhouseOpen Source Technology Centre david.woodho...@intel.com Intel Corporation smime.p7s Description: S/MIME cryptographic signature ___ SeaBIOS mailing list SeaBIOS@seabios.org http://www.seabios.org/mailman/listinfo/seabios
Re: [SeaBIOS] [PATCH v3] Update EFI_COMPATIBILITY16_TABLE to match 0.98 spec update
On Wed, 2014-05-21 at 09:52 -0400, Kevin O'Connor wrote: Why is it wrong to declare memory at 0xef000-0xf 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-0xf region when it actually issued a Legacy16Boot request. Prior to that, the entire 0xc-0x10 region was remaining locked. I'll fix my EDK2 patch accordingly. -- David WoodhouseOpen Source Technology Centre david.woodho...@intel.com Intel Corporation smime.p7s Description: S/MIME cryptographic signature ___ SeaBIOS mailing list SeaBIOS@seabios.org http://www.seabios.org/mailman/listinfo/seabios
