Message ID | 20170513000141.GA542@morn.lan |
---|---|
State | New |
Headers | show |
> -----Original Message----- > From: Kevin O'Connor [mailto:kevin@koconnor.net] > Sent: Friday, May 12, 2017 5:02 PM > To: Xu, Anthony <anthony.xu@intel.com> > Cc: Paolo Bonzini <pbonzini@redhat.com>; qemu-devel@nongnu.org > Subject: Re: [PATCH] target/i386: enable A20 automatically in system > management mode > > On Fri, May 12, 2017 at 11:19:00PM +0000, Xu, Anthony wrote: > > > 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. > > > > The 350 port 92 access is for write operation only. > > If include the inb(), it would be 700, and every time it actually has a change > > To be precise, It is about 175 switches from 32 bit to 16 bit, then back to 32 > bit. > > call16 is called 175 times during Seabios boot without any option rom, > > It would be more if some option roms are included. > > > > > > I think A20 is disabled by default in SeaBios. > > I don't know why you think that. One can check with: > > --- a/src/stacks.c > +++ b/src/stacks.c > @@ -99,6 +99,8 @@ call32_post(void) > if (cr0_caching) > cr0_mask(CR0_CD|CR0_NW, cr0_caching); > } > + if (!get_a20()) > + dprintf(1, "a20=0\n"); > > // Restore cmos index register > outb(GET_LOW(Call16Data.cmosindex), PORT_CMOS_INDEX); > > With the above I only see a handful of cases where SeaBIOS has to > restore a20 to a disabled state. I think it is related to accel and platform, the result I gave before is for q35 tcg, With the above change, I got below data Platform accel count of restoring A20 to 0 Q35 kvm 96 Q35 tcg 271 PC kvm 3 PC tcg 3 A lot of A20 restoring happen when SeaBIOS scans AHCI links. > > The handful I do see are due to cases where yield() is called prior to > option rom initialization. Those handful are eliminated for me with > the following fix: > > --- a/src/stacks.c > +++ b/src/stacks.c > @@ -496,6 +496,7 @@ void > thread_setup(void) > { > CanInterrupt = 1; > + call16_override(1); > if (! CONFIG_THREADS) > return; > ThreadControl = romfile_loadint("etc/threads", 1); But I still see a lot of PORT_A20 accesses in QEMU as I expected > > What OS / bootloader are you running? /x86_64-softmmu/qemu-system-x86_64 -bios /home/root/git/seabios/out/bios.bin -smp 1 -machine q35,accel=tcg -m 1G -drive format=raw,file=/home/root/images/centos7.2.img,if=ide,index=0 -nographic -nodefaults -serial stdio -monitor pty -Anthony
On Sat, May 13, 2017 at 01:24:30AM +0000, Xu, Anthony wrote: > I think it is related to accel and platform, the result I gave before is for q35 tcg, > > With the above change, I got below data > > Platform accel count of restoring A20 to 0 > Q35 kvm 96 > Q35 tcg 271 > PC kvm 3 > PC tcg 3 Okay, thanks. I think the number of a20 switches is due to differences in option rom execution interacting with the fact that some mode switches were occurring before SeaBIOS set call16_override(). > But I still see a lot of PORT_A20 accesses in QEMU as I expected Yes, but it should be possible to significantly reduce the number of outb() calls by limiting them to when A20 changes. This should also be useful to reduce the number of outb() calls needed to disable NMIs. I sent a patch series to the seabios mailing list to demonstrate the idea. -Kevin
> On Sat, May 13, 2017 at 01:24:30AM +0000, Xu, Anthony wrote: > > I think it is related to accel and platform, the result I gave before is for q35 > tcg, > > > > With the above change, I got below data > > > > Platform accel count of restoring A20 to 0 > > Q35 kvm 96 > > Q35 tcg 271 > > PC kvm 3 > > PC tcg 3 > > Okay, thanks. I think the number of a20 switches is due to > differences in option rom execution interacting with the fact that > some mode switches were occurring before SeaBIOS set > call16_override(). > > > But I still see a lot of PORT_A20 accesses in QEMU as I expected > > Yes, but it should be possible to significantly reduce the number of > outb() calls by limiting them to when A20 changes. This should also > be useful to reduce the number of outb() calls needed to disable NMIs. > I sent a patch series to the seabios mailing list to demonstrate the > idea. If both TCG and KVM work by ignoring A20, why not remove all PORT_A20 access in SeaBios when CONFIG_DISABLE_A20 is not defined? Do you see any impact? -Anthony
On Tue, May 16, 2017 at 08:00:28PM +0000, Xu, Anthony wrote: > > On Sat, May 13, 2017 at 01:24:30AM +0000, Xu, Anthony wrote: > > > I think it is related to accel and platform, the result I gave before is for q35 > > tcg, > > > > > > With the above change, I got below data > > > > > > Platform accel count of restoring A20 to 0 > > > Q35 kvm 96 > > > Q35 tcg 271 > > > PC kvm 3 > > > PC tcg 3 > > > > Okay, thanks. I think the number of a20 switches is due to > > differences in option rom execution interacting with the fact that > > some mode switches were occurring before SeaBIOS set > > call16_override(). > > > > > But I still see a lot of PORT_A20 accesses in QEMU as I expected > > > > Yes, but it should be possible to significantly reduce the number of > > outb() calls by limiting them to when A20 changes. This should also > > be useful to reduce the number of outb() calls needed to disable NMIs. > > I sent a patch series to the seabios mailing list to demonstrate the > > idea. > > If both TCG and KVM work by ignoring A20, why not remove all PORT_A20 > access in SeaBios when CONFIG_DISABLE_A20 is not defined? > Do you see any impact? The SeaBIOS CONFIG_DISABLE_A20 build option does not mean "disable support for A20"; it means "start the initial operating system bootloader with A20 disabled". CONFIG_DISABLE_A20=y is a pessimization, not an optimization. As for adding a new SeaBIOS build option to compile out support for A20 - that seems like a very small optimization that would risk memory corruption and hard to diagnose crashes. SeaBIOS runs natively on real hardware (with coreboot and as a CSM on UEFI) as well as on QEMU/KVM. -Kevin
> > > > With the above change, I got below data > > > > > > > > Platform accel count of restoring A20 to 0 > > > > Q35 kvm 96 > > > > Q35 tcg 271 > > > > PC kvm 3 > > > > PC tcg 3 > > > > > > Okay, thanks. I think the number of a20 switches is due to > > > differences in option rom execution interacting with the fact that > > > some mode switches were occurring before SeaBIOS set > > > call16_override(). > > > > > > > But I still see a lot of PORT_A20 accesses in QEMU as I expected > > > > > > Yes, but it should be possible to significantly reduce the number of > > > outb() calls by limiting them to when A20 changes. This should also > > > be useful to reduce the number of outb() calls needed to disable NMIs. > > > I sent a patch series to the seabios mailing list to demonstrate the > > > idea. > > > > If both TCG and KVM work by ignoring A20, why not remove all PORT_A20 > > access in SeaBios when CONFIG_DISABLE_A20 is not defined? > > Do you see any impact? > > The SeaBIOS CONFIG_DISABLE_A20 build option does not mean "disable > support for A20"; it means "start the initial operating system > bootloader with A20 disabled". CONFIG_DISABLE_A20=y is a > pessimization, not an optimization. Make sense, Thanks for explanation, > > As for adding a new SeaBIOS build option to compile out support for > A20 - that seems like a very small optimization that would risk memory > corruption and hard to diagnose crashes. SeaBIOS runs natively on > real hardware (with coreboot and as a CSM on UEFI) as well as on > QEMU/KVM. I heard new platform doesn't support A20. What's the hardware SeaBIOS runs natively on needs A20 support? It is just a build option, we can disable A20 support for QEMU/KVM and enable A20 support for real hardware. Any concerns here? BTW QEMU/KVM ignores A20 even SeaBIOS supports A20. Or, we can add some logic in SeaBIOS to check if the platform supports A20, if it doesn't support A20, SeaBIOS won't access PORT_A20 anymore. The check logic is like, write 0x55 to 0x00:0xeff0 (or other unused address) disable A20 write 0xaa to 0xffff:0xf000 read from 0x00:0xeff0, If the return value is 0x55, A20 is not supported by this platform. Anthony
On 16/05/2017 23:42, Kevin O'Connor wrote: > > As for adding a new SeaBIOS build option to compile out support for > A20 - that seems like a very small optimization that would risk memory > corruption and hard to diagnose crashes. SeaBIOS runs natively on > real hardware (with coreboot and as a CSM on UEFI) as well as on > QEMU/KVM. Haswell (2013) no longer has A20 support (and hypervisors never had it, not just KVM). Thanks, Paolo
--- a/src/stacks.c +++ b/src/stacks.c @@ -99,6 +99,8 @@ call32_post(void) if (cr0_caching) cr0_mask(CR0_CD|CR0_NW, cr0_caching); } + if (!get_a20()) + dprintf(1, "a20=0\n"); // Restore cmos index register outb(GET_LOW(Call16Data.cmosindex), PORT_CMOS_INDEX); With the above I only see a handful of cases where SeaBIOS has to restore a20 to a disabled state. The handful I do see are due to cases where yield() is called prior to option rom initialization. Those handful are eliminated for me with the following fix: --- a/src/stacks.c +++ b/src/stacks.c @@ -496,6 +496,7 @@ void thread_setup(void) { CanInterrupt = 1; + call16_override(1); if (! CONFIG_THREADS) return; ThreadControl = romfile_loadint("etc/threads", 1);