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

[Kevin O'Connor]

On Thu, May 11, 2017 at 05:32:47PM +0200, Paolo Bonzini wrote:
> 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.

Okay.

> 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:

Yes, that should be fine.

> --- 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;
>  };

In order to avoid mixing code and data in the same cache line we could
do this instead:

 struct smm_layout {
     struct smm_state backup1;
     struct smm_state backup2;
-    u8 stack[0x7c00];
+    u32 backup_a20;
+    u8 stack[0x8000 - sizeof(struct smm_state)*2 - sizeof(u32)];
     u64 codeentry;
     u8 pad_8008[0x7df8];
     struct smm_state cpu;

Thanks,
-Kevin