diff mbox

target/i386: enable A20 automatically in system management mode

Message ID 618febcf-af6d-5fc6-0274-4f64c53f9763@redhat.com
State New
Headers show

Commit Message

Paolo Bonzini May 11, 2017, 3:32 p.m. UTC 
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:



Paolo

Comments

Kevin O'Connor May 11, 2017, 4:34 p.m. UTC | #1 
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
Xu, Anthony May 11, 2017, 11:55 p.m. UTC | #2 
Hi Paolo,

In KVM mode, seems A20 is ignored.
Do you see any potential issue here?


Anthony 


> -----Original Message-----
> From: Kevin O'Connor [mailto:kevin@koconnor.net]
> Sent: Thursday, May 11, 2017 9:35 AM
> To: Paolo Bonzini <pbonzini@redhat.com>
> Cc: qemu-devel@nongnu.org; Xu, Anthony <anthony.xu@intel.com>
> Subject: Re: [PATCH] target/i386: enable A20 automatically in system
> management mode
> 
> 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
Paolo Bonzini May 12, 2017, 12:16 p.m. UTC | #3 
On 12/05/2017 01:55, Xu, Anthony wrote:
> Hi Paolo,
> 
> In KVM mode, seems A20 is ignored.
> Do you see any potential issue here?

No; recent processors don't have A20 at all.

Paolo

> 
> Anthony 
> 
> 
>> -----Original Message-----
>> From: Kevin O'Connor [mailto:kevin@koconnor.net]
>> Sent: Thursday, May 11, 2017 9:35 AM
>> To: Paolo Bonzini <pbonzini@redhat.com>
>> Cc: qemu-devel@nongnu.org; Xu, Anthony <anthony.xu@intel.com>
>> Subject: Re: [PATCH] target/i386: enable A20 automatically in system
>> management mode
>>
>> 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
Xu, Anthony May 12, 2017, 6:55 p.m. UTC | #4 
> On 12/05/2017 01:55, Xu, Anthony wrote:

> > Hi Paolo,

> >

> > In KVM mode, seems A20 is ignored.

> > Do you see any potential issue here?

> 

> No; recent processors don't have A20 at all.


I mean A20 in guest, not A20 in host.  
Guest is running on old platform, it tries to control A20  through port 92 like what SeaBios does.
QEMU/KVM does handle port 92 access to set correct env->a20_mask,
but QEMU/KVM ignores A20 status when handling guest memory access.

Since QEMU/KVM works well with SeaBios, does that imply SeaBios doesn't generate address
larger than 0x100000 in real mode?

If that's the case,  QEMU/TCG should work with SeaBios even with ignoring A20.

During SeaBios boot, there are >350 port 92 access, if we don't need to handle A20, 
we can make A20 configurable in Seabios, It may reduce SeaBios boot time.


Anthony
Paolo Bonzini May 12, 2017, 7:16 p.m. UTC | #5 
On 12/05/2017 20:55, Xu, Anthony wrote:
>  
>> On 12/05/2017 01:55, Xu, Anthony wrote:
>>> Hi Paolo,
>>>
>>> In KVM mode, seems A20 is ignored.
>>> Do you see any potential issue here?
>>
>> No; recent processors don't have A20 at all.
> 
> I mean A20 in guest, not A20 in host.  
> Guest is running on old platform, it tries to control A20  through port 92 like what SeaBios does.
> QEMU/KVM does handle port 92 access to set correct env->a20_mask,
> but QEMU/KVM ignores A20 status when handling guest memory access.
> 
> Since QEMU/KVM works well with SeaBios, does that imply SeaBios doesn't generate address
> larger than 0x100000 in real mode?

No, only the guest's OS or software (not the firmware) might require
A20.  But really anything that ran with MS-DOS 5.0 HIMEM.SYS or newer
(which used DOS=HIGH to relocate DOS into the HMA, I think) does not
need it.

> If that's the case,  QEMU/TCG should work with SeaBios even with ignoring A20.
> 
> During SeaBios boot, there are >350 port 92 access, if we don't need to handle A20, 
> we can make A20 configurable in Seabios, It may reduce SeaBios boot time.

Yes, that's a good idea.

Paolo
Kevin O'Connor May 12, 2017, 7:38 p.m. UTC | #6 
On Fri, May 12, 2017 at 09:16:31PM +0200, Paolo Bonzini wrote:
> On 12/05/2017 20:55, Xu, Anthony wrote:
> > If that's the case,  QEMU/TCG should work with SeaBios even with ignoring A20.
> > 
> > During SeaBios boot, there are >350 port 92 access, if we don't need to handle A20, 
> > we can make A20 configurable in Seabios, It may reduce SeaBios boot time.
> 
> Yes, that's a good idea.

SeaBIOS defaults to enabling A20 and it's a rare beast that disables
it.  One could change x86.h:set_a20 and romlayout.S:transition32 to
only issue the outb() if the inb() indicates a change is needed.  That
would likely eliminate half the accesses.

I'd be surprised if it would impact the overall boot time though.
SeaBIOS only touches the port on a cpu mode switch and I would have
thought that was heavier than an IO port access.  Maybe that is skewed
on KVM though.

-Kevin
diff mbox

Patch

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) {