Re: [Qemu-devel] [PATCH] target/i386: enable A20 automatically in system management mode

[Paolo Bonzini <pbonzini@redhat.com>]

On 11/05/2017 16:53, Kevin O'Connor wrote:
> On Thu, May 11, 2017 at 01:35:28PM +0200, Paolo Bonzini wrote:
>> Ignore env->a20_mask when running in system management mode.
> 
> Thanks Paolo.  I don't think this patch will help SeaBIOS though.  The
> SeaBIOS SMM handler doesn't do much - it doesn't even access ram above
> 1MiB.  See SeaBIOS' code in src/fw/smm.c:handle_smi().
> 
> Instead, the SeaBIOS code does a cpu state backup/restore to switch
> into 32bit mode.  I thought the A20 state would be part of that cpu
> backup/restore.  However, looking at the Intel SDM docs now, it's not
> really clear to me how the processor "inhibits" A20 when in SMM mode -
> does it save/restore that state on SMI/RSM or does it have special
> logic to ignore A20 while in SMM mode?

There isn't any documented place for A20 in the state save map (I checked
AMD's BIOS/Kernel Developer Guide which is pretty comprehensive), so I
think the latter is more plausible.  What I'm doing in this patch is
ignoring A20 while in SMM mode.

Then you would have to add an A20 save/restore in handle_smi; since 
CALL32SMM_ENTERID should not nest, I think you can just do this:

diff --git a/src/fw/smm.c b/src/fw/smm.c
index 95f6ba7..711dae3 100644
--- a/src/fw/smm.c
+++ b/src/fw/smm.c
@@ -54,7 +54,8 @@ struct smm_layout {
     struct smm_state backup2;
     u8 stack[0x7c00];
     u64 codeentry;
-    u8 pad_8008[0x7df8];
+    u8 a20;
+    u8 pad_8009[0x7df7];
     struct smm_state cpu;
 };
 
@@ -102,10 +103,13 @@ handle_smi(u16 cs)
                 memcpy(&smm->cpu, &smm->backup1, sizeof(smm->cpu));
                 memcpy(&smm->cpu.i32.eax, regs, sizeof(regs));
                 smm->cpu.i32.eip = regs[3];
+                // Enable a20 and backup its previous state
+                smm->a20 = set_a20(1);
             } else if (smm->cpu.i32.ecx == CALL32SMM_RETURNID) {
                 dprintf(9, "smm cpu ret %x esp=%x\n", regs[3], regs[4]);
                 memcpy(&smm->cpu, &smm->backup2, sizeof(smm->cpu));
                 memcpy(&smm->cpu.i32.eax, regs, sizeof(regs));
+                set_a20(smm->a20);
                 smm->cpu.i32.eip = regs[3];
             }
         } else if (rev == SMM_REV_I64) {


Paolo