Hi Paolo, In KVM mode, seems A20 is ignored. Do you see any potential issue here?
Anthony > -----Original Message----- > From: Kevin O'Connor [mailto:ke...@koconnor.net] > Sent: Thursday, May 11, 2017 9:35 AM > To: Paolo Bonzini <pbonz...@redhat.com> > Cc: qemu-devel@nongnu.org; Xu, Anthony <anthony...@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
