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