| Message ID | 1445364840-7056-1-git-send-email-lersek@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 20/10/2015 20:14, Laszlo Ersek wrote: > Commit 4d00636e97b7 ("ich9: Add the lpc chip", Nov 14 2012) added the > ich9_apm_ctrl_changed() ioport write callback function such that it would > inject the SMI, in response to a write to the APM_CNT register, on the > first CPU, invariably. > > Since this register is used by guest code to trigger an SMI synchronously, > the interrupt should be injected on the VCPU that is performing the write. > > apm_ioport_writeb() is the .write callback of the "apm_ops" > MemoryRegionOps [hw/isa/apm.c]; it is parametrized to call > ich9_apm_ctrl_changed() by ich9_lpc_init() [hw/isa/lpc_ich9.c], via > apm_init(). Therefore this change affects no other board. > > ich9_generate_smi() is an unrelated function that is called by the TCO > watchdog; a watchdog is likely in its right to (asynchronously) inject > interrupts on the first CPU only. > > This patch allows the combined edk2/OVMF SMM driver stack to work with > multiple VCPUs on TCG, using both qemu-system-i386 and qemu-system-x86_64. > > Cc: Paolo Bonzini <pbonzini@redhat.com> > Cc: Gerd Hoffmann <kraxel@redhat.com> > Cc: Jordan Justen <jordan.l.justen@intel.com> > Cc: Michael Kinney <michael.d.kinney@intel.com> > Cc: "Michael S. Tsirkin" <mst@redhat.com> > Signed-off-by: Laszlo Ersek <lersek@redhat.com> > --- > hw/isa/lpc_ich9.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/hw/isa/lpc_ich9.c b/hw/isa/lpc_ich9.c > index 360699f..1ffc803 100644 > --- a/hw/isa/lpc_ich9.c > +++ b/hw/isa/lpc_ich9.c > @@ -394,7 +394,7 @@ static void ich9_apm_ctrl_changed(uint32_t val, void *arg) > > /* SMI_EN = PMBASE + 30. SMI control and enable register */ > if (lpc->pm.smi_en & ICH9_PMIO_SMI_EN_APMC_EN) { > - cpu_interrupt(first_cpu, CPU_INTERRUPT_SMI); > + cpu_interrupt(current_cpu, CPU_INTERRUPT_SMI); > } > } > > Reviewed-by: Paolo Bonzini <pbonzini@redhat.com> The same issue happens on PIIX4. I can fix it as the change is a bit more involved (it goes through a qemu_irq). Paolo
On Wed, Oct 21, 2015 at 11:49:13AM +0200, Paolo Bonzini wrote: > > > On 20/10/2015 20:14, Laszlo Ersek wrote: > > Commit 4d00636e97b7 ("ich9: Add the lpc chip", Nov 14 2012) added the > > ich9_apm_ctrl_changed() ioport write callback function such that it would > > inject the SMI, in response to a write to the APM_CNT register, on the > > first CPU, invariably. > > > > Since this register is used by guest code to trigger an SMI synchronously, > > the interrupt should be injected on the VCPU that is performing the write. > > > > apm_ioport_writeb() is the .write callback of the "apm_ops" > > MemoryRegionOps [hw/isa/apm.c]; it is parametrized to call > > ich9_apm_ctrl_changed() by ich9_lpc_init() [hw/isa/lpc_ich9.c], via > > apm_init(). Therefore this change affects no other board. > > > > ich9_generate_smi() is an unrelated function that is called by the TCO > > watchdog; a watchdog is likely in its right to (asynchronously) inject > > interrupts on the first CPU only. > > > > This patch allows the combined edk2/OVMF SMM driver stack to work with > > multiple VCPUs on TCG, using both qemu-system-i386 and qemu-system-x86_64. > > > > Cc: Paolo Bonzini <pbonzini@redhat.com> > > Cc: Gerd Hoffmann <kraxel@redhat.com> > > Cc: Jordan Justen <jordan.l.justen@intel.com> > > Cc: Michael Kinney <michael.d.kinney@intel.com> > > Cc: "Michael S. Tsirkin" <mst@redhat.com> > > Signed-off-by: Laszlo Ersek <lersek@redhat.com> > > --- > > hw/isa/lpc_ich9.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/hw/isa/lpc_ich9.c b/hw/isa/lpc_ich9.c > > index 360699f..1ffc803 100644 > > --- a/hw/isa/lpc_ich9.c > > +++ b/hw/isa/lpc_ich9.c > > @@ -394,7 +394,7 @@ static void ich9_apm_ctrl_changed(uint32_t val, void *arg) > > > > /* SMI_EN = PMBASE + 30. SMI control and enable register */ > > if (lpc->pm.smi_en & ICH9_PMIO_SMI_EN_APMC_EN) { > > - cpu_interrupt(first_cpu, CPU_INTERRUPT_SMI); > > + cpu_interrupt(current_cpu, CPU_INTERRUPT_SMI); > > } > > } > > > > > > Reviewed-by: Paolo Bonzini <pbonzini@redhat.com> Sorry, only saw this now, and it's already on its way upstream. > The same issue happens on PIIX4. I can fix it as the change is a bit > more involved (it goes through a qemu_irq). > > Paolo Thanks!
On 2015-10-20 11:14:00, Laszlo Ersek wrote: > Commit 4d00636e97b7 ("ich9: Add the lpc chip", Nov 14 2012) added the > ich9_apm_ctrl_changed() ioport write callback function such that it would > inject the SMI, in response to a write to the APM_CNT register, on the > first CPU, invariably. > > Since this register is used by guest code to trigger an SMI synchronously, > the interrupt should be injected on the VCPU that is performing the write. Why not send an SMI to *all* processors, like the real chipsets do? -Jordan > apm_ioport_writeb() is the .write callback of the "apm_ops" > MemoryRegionOps [hw/isa/apm.c]; it is parametrized to call > ich9_apm_ctrl_changed() by ich9_lpc_init() [hw/isa/lpc_ich9.c], via > apm_init(). Therefore this change affects no other board. > > ich9_generate_smi() is an unrelated function that is called by the TCO > watchdog; a watchdog is likely in its right to (asynchronously) inject > interrupts on the first CPU only. > > This patch allows the combined edk2/OVMF SMM driver stack to work with > multiple VCPUs on TCG, using both qemu-system-i386 and qemu-system-x86_64. > > Cc: Paolo Bonzini <pbonzini@redhat.com> > Cc: Gerd Hoffmann <kraxel@redhat.com> > Cc: Jordan Justen <jordan.l.justen@intel.com> > Cc: Michael Kinney <michael.d.kinney@intel.com> > Cc: "Michael S. Tsirkin" <mst@redhat.com> > Signed-off-by: Laszlo Ersek <lersek@redhat.com> > --- > hw/isa/lpc_ich9.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/hw/isa/lpc_ich9.c b/hw/isa/lpc_ich9.c > index 360699f..1ffc803 100644 > --- a/hw/isa/lpc_ich9.c > +++ b/hw/isa/lpc_ich9.c > @@ -394,7 +394,7 @@ static void ich9_apm_ctrl_changed(uint32_t val, void *arg) > > /* SMI_EN = PMBASE + 30. SMI control and enable register */ > if (lpc->pm.smi_en & ICH9_PMIO_SMI_EN_APMC_EN) { > - cpu_interrupt(first_cpu, CPU_INTERRUPT_SMI); > + cpu_interrupt(current_cpu, CPU_INTERRUPT_SMI); > } > } > > -- > 1.8.3.1 >
On 21/10/2015 20:36, Jordan Justen wrote: > On 2015-10-20 11:14:00, Laszlo Ersek wrote: > > Commit 4d00636e97b7 ("ich9: Add the lpc chip", Nov 14 2012) added the > > ich9_apm_ctrl_changed() ioport write callback function such that it would > > inject the SMI, in response to a write to the APM_CNT register, on the > > first CPU, invariably. > > > > Since this register is used by guest code to trigger an SMI synchronously, > > the interrupt should be injected on the VCPU that is performing the write. > > Why not send an SMI to *all* processors, like the real chipsets do? That's much less scalable, and more important I would have to check that SeaBIOS can handle that correctly. It probably doesn't, as it doesn't relocate SMBASEs. Paolo
On 10/22/15 10:40, Paolo Bonzini wrote: > > > On 21/10/2015 20:36, Jordan Justen wrote: >> On 2015-10-20 11:14:00, Laszlo Ersek wrote: >>> Commit 4d00636e97b7 ("ich9: Add the lpc chip", Nov 14 2012) added the >>> ich9_apm_ctrl_changed() ioport write callback function such that it would >>> inject the SMI, in response to a write to the APM_CNT register, on the >>> first CPU, invariably. >>> >>> Since this register is used by guest code to trigger an SMI synchronously, >>> the interrupt should be injected on the VCPU that is performing the write. >> >> Why not send an SMI to *all* processors, like the real chipsets do? > > That's much less scalable, and more important I would have to check that > SeaBIOS can handle that correctly. It probably doesn't, as it doesn't > relocate SMBASEs. We could invent a magic value for APM_STS (not used by SeaBIOS) that would decide between "all" and "current". It would be an ugly hack, yes, but this is a virtual platform. :) Theoretically, the Trigger() function in OVMF can take a value for APM_STS from the caller -- this is specified even on the protocol level --, but the only caller, the SMM core, doesn't fill in that optional parameter (the pointer to the APM_STS value is NULL): MdeModulePkg/Core/PiSmmCore/PiSmmIpl.c: Status = mSmmControl2->Trigger (mSmmControl2, NULL, NULL, FALSE, 0); So in OVMF's implementation of Trigger(), we could replace IoWrite8 (ICH9_APM_STS, DataPort == NULL ? 0 : *DataPort); with IoWrite8 (ICH9_APM_STS, DataPort == NULL ? MAGIC : *DataPort); and then in QEMU the cpu_interrupt() call in question could be wrapped in a loop for all CPUs. (Or maybe we already have a helper function for that.) ... With the "relaxed" method configured in OVMF, the above change would make no difference as long as the BSP executes the firmware -- which is guaranteed before ExitBootServices() --, but it still makes a difference if later a runtime service is called by an AP. In that case the AP must drag in the BSP, and that takes very long (1 second loop). We can decrease that loop length of course, but how much? 100ms? 10ms? Anyway, just an idea. Thanks Laszlo > > Paolo >
On 22/10/2015 11:50, Laszlo Ersek wrote: > ... With the "relaxed" method configured in OVMF, the above change would > make no difference as long as the BSP executes the firmware -- which is > guaranteed before ExitBootServices() --, but it still makes a difference > if later a runtime service is called by an AP. In that case the AP must > drag in the BSP, and that takes very long (1 second loop). We can > decrease that loop length of course, but how much? 100ms? 10ms? Timeouts are evil. In virtual machines there's no way to bound the timeout. Things such as SMIs on the host (!) can introduce latency. So the best timeout for OVMF is an infinite timeout. :) Perhaps we can introduce another PCD to remove the first timeout and start immediately with the SMI IPIs? Or a PCD to make the SMI handler send an SMI too all-excluding-self upon entry, since we cannot do that from Trigger() after ExitBootServices(). Paolo
On Thu, Oct 22, 2015 at 10:40:08AM +0200, Paolo Bonzini wrote: > On 21/10/2015 20:36, Jordan Justen wrote: > > On 2015-10-20 11:14:00, Laszlo Ersek wrote: > > > Commit 4d00636e97b7 ("ich9: Add the lpc chip", Nov 14 2012) added the > > > ich9_apm_ctrl_changed() ioport write callback function such that it would > > > inject the SMI, in response to a write to the APM_CNT register, on the > > > first CPU, invariably. > > > > > > Since this register is used by guest code to trigger an SMI synchronously, > > > the interrupt should be injected on the VCPU that is performing the write. > > > > Why not send an SMI to *all* processors, like the real chipsets do? > > That's much less scalable, and more important I would have to check that > SeaBIOS can handle that correctly. It probably doesn't, as it doesn't > relocate SMBASEs. SeaBIOS is only expecting its SMI handler to be called once in response to a synchronous SMI. We can change SeaBIOS to fix that. SeaBIOS does relocate the smbase from 0x30000 to 0xa0000 during its init phase (by creating a synchronous SMI on the BSP and then setting the smbase register to 0xa0000 in the smi handler). -Kevin
On 22/10/2015 20:04, Kevin O'Connor wrote: > On Thu, Oct 22, 2015 at 10:40:08AM +0200, Paolo Bonzini wrote: >> On 21/10/2015 20:36, Jordan Justen wrote: >>> On 2015-10-20 11:14:00, Laszlo Ersek wrote: >>>> Commit 4d00636e97b7 ("ich9: Add the lpc chip", Nov 14 2012) added the >>>> ich9_apm_ctrl_changed() ioport write callback function such that it would >>>> inject the SMI, in response to a write to the APM_CNT register, on the >>>> first CPU, invariably. >>>> >>>> Since this register is used by guest code to trigger an SMI synchronously, >>>> the interrupt should be injected on the VCPU that is performing the write. >>> >>> Why not send an SMI to *all* processors, like the real chipsets do? >> >> That's much less scalable, and more important I would have to check that >> SeaBIOS can handle that correctly. It probably doesn't, as it doesn't >> relocate SMBASEs. > > SeaBIOS is only expecting its SMI handler to be called once in > response to a synchronous SMI. We can change SeaBIOS to fix that. > > SeaBIOS does relocate the smbase from 0x30000 to 0xa0000 during its > init phase (by creating a synchronous SMI on the BSP and then setting > the smbase register to 0xa0000 in the smi handler). Right; however it would also have to relocate the SMBASE on the APs (in case they were halted with cli;hlt and not INITed). It's really not worth the hassle, it's not even documented in the chipset docs whether 0xb2 sends an SMI to all processors or only the running one. Paolo
On 2015-10-22 12:46:56, Paolo Bonzini wrote: > > On 22/10/2015 20:04, Kevin O'Connor wrote: > > On Thu, Oct 22, 2015 at 10:40:08AM +0200, Paolo Bonzini wrote: > >> On 21/10/2015 20:36, Jordan Justen wrote: > >>> On 2015-10-20 11:14:00, Laszlo Ersek wrote: > >>>> Commit 4d00636e97b7 ("ich9: Add the lpc chip", Nov 14 2012) added the > >>>> ich9_apm_ctrl_changed() ioport write callback function such that it would > >>>> inject the SMI, in response to a write to the APM_CNT register, on the > >>>> first CPU, invariably. > >>>> > >>>> Since this register is used by guest code to trigger an SMI synchronously, > >>>> the interrupt should be injected on the VCPU that is performing the write. > >>> > >>> Why not send an SMI to *all* processors, like the real chipsets do? > >> > >> That's much less scalable, and more important I would have to check that > >> SeaBIOS can handle that correctly. It probably doesn't, as it doesn't > >> relocate SMBASEs. > > > > SeaBIOS is only expecting its SMI handler to be called once in > > response to a synchronous SMI. We can change SeaBIOS to fix that. > > > > SeaBIOS does relocate the smbase from 0x30000 to 0xa0000 during its > > init phase (by creating a synchronous SMI on the BSP and then setting > > the smbase register to 0xa0000 in the smi handler). > > Right; however it would also have to relocate the SMBASE on the APs (in > case they were halted with cli;hlt and not INITed). It's really not > worth the hassle, It's not worth the hassle to relocate the SMBASE of the APs? So, basically, write to 0x30000-0x38000, then send an SMI IPI to the AP and now you have the AP running in SMI and it has extra privileges? > it's not even documented in the chipset docs whether > 0xb2 sends an SMI to all processors or only the running one. My knowledge of recent chipsets is getting rusty, but in ICH ~ ICH6 the SMI# pin was wired to every processor. Any chipset based SMI would cause all processors to enter SMM. In more recent chipsets, I could speculate that maybe the SMI could be delivered over a bus instead. But, I still think the chipset SMI's would go to all processors. I did note from the ICH9 datasheet that there is still a SMI# pin. I also saw this under "Interrupt Message Format" of the ICH9 datasheet: "The ICH9’s I/O APIC can only send interrupts due to interrupts which do not include SMI, NMI or INIT. This means that in IA-32/Intel ® 64 based platforms, Front Side Bus interrupt message format delivery modes 010 (SMI/ PMI), 100 (NMI), and 101 (INIT) as indicated in this section, must not be used and is not supported. Only the hardware pin connection is supported by ICH9." This leads me to think that the ICH9 system designs continued to wire the SMI# pin to all processors. -Jordan
On 23/10/2015 06:41, Jordan Justen wrote: > On 2015-10-22 12:46:56, Paolo Bonzini wrote: >> >> On 22/10/2015 20:04, Kevin O'Connor wrote: >>> On Thu, Oct 22, 2015 at 10:40:08AM +0200, Paolo Bonzini wrote: >>>> On 21/10/2015 20:36, Jordan Justen wrote: >>>>> On 2015-10-20 11:14:00, Laszlo Ersek wrote: >>>>>> Commit 4d00636e97b7 ("ich9: Add the lpc chip", Nov 14 2012) added the >>>>>> ich9_apm_ctrl_changed() ioport write callback function such that it would >>>>>> inject the SMI, in response to a write to the APM_CNT register, on the >>>>>> first CPU, invariably. >>>>>> >>>>>> Since this register is used by guest code to trigger an SMI synchronously, >>>>>> the interrupt should be injected on the VCPU that is performing the write. >>>>> >>>>> Why not send an SMI to *all* processors, like the real chipsets do? >>>> >>>> That's much less scalable, and more important I would have to check that >>>> SeaBIOS can handle that correctly. It probably doesn't, as it doesn't >>>> relocate SMBASEs. >>> >>> SeaBIOS is only expecting its SMI handler to be called once in >>> response to a synchronous SMI. We can change SeaBIOS to fix that. >>> >>> SeaBIOS does relocate the smbase from 0x30000 to 0xa0000 during its >>> init phase (by creating a synchronous SMI on the BSP and then setting >>> the smbase register to 0xa0000 in the smi handler). >> >> Right; however it would also have to relocate the SMBASE on the APs (in >> case they were halted with cli;hlt and not INITed). It's really not >> worth the hassle, > > It's not worth the hassle to relocate the SMBASE of the APs? > So, basically, write to 0x30000-0x38000, then send an SMI IPI to the > AP and now you have the AP running in SMI and it has extra privileges? Extra privileges compared to what? Legacy BIOS does not really put anything privileged in SMRAM, while OVMF does and _hence_ relocates the SMBASE of the AP. It would have been nice to get it right from the beginning, but right now it's not worth forcing a lockstep QEMU-SeaBIOS update. Paolo
On 10/23/15 09:26, Paolo Bonzini wrote: > > > On 23/10/2015 06:41, Jordan Justen wrote: >> On 2015-10-22 12:46:56, Paolo Bonzini wrote: >>> >>> On 22/10/2015 20:04, Kevin O'Connor wrote: >>>> On Thu, Oct 22, 2015 at 10:40:08AM +0200, Paolo Bonzini wrote: >>>>> On 21/10/2015 20:36, Jordan Justen wrote: >>>>>> On 2015-10-20 11:14:00, Laszlo Ersek wrote: >>>>>>> Commit 4d00636e97b7 ("ich9: Add the lpc chip", Nov 14 2012) added the >>>>>>> ich9_apm_ctrl_changed() ioport write callback function such that it would >>>>>>> inject the SMI, in response to a write to the APM_CNT register, on the >>>>>>> first CPU, invariably. >>>>>>> >>>>>>> Since this register is used by guest code to trigger an SMI synchronously, >>>>>>> the interrupt should be injected on the VCPU that is performing the write. >>>>>> >>>>>> Why not send an SMI to *all* processors, like the real chipsets do? >>>>> >>>>> That's much less scalable, and more important I would have to check that >>>>> SeaBIOS can handle that correctly. It probably doesn't, as it doesn't >>>>> relocate SMBASEs. >>>> >>>> SeaBIOS is only expecting its SMI handler to be called once in >>>> response to a synchronous SMI. We can change SeaBIOS to fix that. >>>> >>>> SeaBIOS does relocate the smbase from 0x30000 to 0xa0000 during its >>>> init phase (by creating a synchronous SMI on the BSP and then setting >>>> the smbase register to 0xa0000 in the smi handler). >>> >>> Right; however it would also have to relocate the SMBASE on the APs (in >>> case they were halted with cli;hlt and not INITed). It's really not >>> worth the hassle, >> >> It's not worth the hassle to relocate the SMBASE of the APs? >> So, basically, write to 0x30000-0x38000, then send an SMI IPI to the >> AP and now you have the AP running in SMI and it has extra privileges? > > Extra privileges compared to what? Legacy BIOS does not really put > anything privileged in SMRAM, while OVMF does and _hence_ relocates the > SMBASE of the AP. It would have been nice to get it right from the > beginning, but right now it's not worth forcing a lockstep QEMU-SeaBIOS > update. So what are we thinking about a magic APM_STS value to trigger an SMI for all VCPUs? 0x51 ('Q') would be cool. :) Thanks Laszlo
On Fri, Oct 23, 2015 at 09:26:38AM +0200, Paolo Bonzini wrote: > On 23/10/2015 06:41, Jordan Justen wrote: > > On 2015-10-22 12:46:56, Paolo Bonzini wrote: > >> On 22/10/2015 20:04, Kevin O'Connor wrote: > >>> On Thu, Oct 22, 2015 at 10:40:08AM +0200, Paolo Bonzini wrote: > >>>> On 21/10/2015 20:36, Jordan Justen wrote: > >>>>> On 2015-10-20 11:14:00, Laszlo Ersek wrote: > >>>>>> Commit 4d00636e97b7 ("ich9: Add the lpc chip", Nov 14 2012) added the > >>>>>> ich9_apm_ctrl_changed() ioport write callback function such that it would > >>>>>> inject the SMI, in response to a write to the APM_CNT register, on the > >>>>>> first CPU, invariably. > >>>>>> > >>>>>> Since this register is used by guest code to trigger an SMI synchronously, > >>>>>> the interrupt should be injected on the VCPU that is performing the write. > >>>>> > >>>>> Why not send an SMI to *all* processors, like the real chipsets do? > >>>> > >>>> That's much less scalable, and more important I would have to check that > >>>> SeaBIOS can handle that correctly. It probably doesn't, as it doesn't > >>>> relocate SMBASEs. > >>> > >>> SeaBIOS is only expecting its SMI handler to be called once in > >>> response to a synchronous SMI. We can change SeaBIOS to fix that. > >>> > >>> SeaBIOS does relocate the smbase from 0x30000 to 0xa0000 during its > >>> init phase (by creating a synchronous SMI on the BSP and then setting > >>> the smbase register to 0xa0000 in the smi handler). > >> > >> Right; however it would also have to relocate the SMBASE on the APs (in > >> case they were halted with cli;hlt and not INITed). It's really not > >> worth the hassle, > > > > It's not worth the hassle to relocate the SMBASE of the APs? > > So, basically, write to 0x30000-0x38000, then send an SMI IPI to the > > AP and now you have the AP running in SMI and it has extra privileges? > > Extra privileges compared to what? Legacy BIOS does not really put > anything privileged in SMRAM, while OVMF does and _hence_ relocates the > SMBASE of the AP. It would have been nice to get it right from the > beginning, but right now it's not worth forcing a lockstep QEMU-SeaBIOS > update. We could add code to SeaBIOS now that protects against multiple SMI handlers running, and then at some future date QEMU could be updated. I'll defer to your judgment if that makes sense. BTW, how does OVMF handle SMIs on multiple processors? Does it setup a unique SMBASE for each cpu, or does it inspect the apic id (or something similar) in the smi handler to determine which cpu should handle the event? -Kevin
On 23/10/2015 18:54, Kevin O'Connor wrote: >> > >> > Extra privileges compared to what? Legacy BIOS does not really put >> > anything privileged in SMRAM, while OVMF does and _hence_ relocates the >> > SMBASE of the AP. It would have been nice to get it right from the >> > beginning, but right now it's not worth forcing a lockstep QEMU-SeaBIOS >> > update. > We could add code to SeaBIOS now that protects against multiple SMI > handlers running, and then at some future date QEMU could be updated. > I'll defer to your judgment if that makes sense. > > BTW, how does OVMF handle SMIs on multiple processors? Does it setup > a unique SMBASE for each cpu, or does it inspect the apic id (or > something similar) in the smi handler to determine which cpu should > handle the event? It does the former. Paolo
On 2015-10-23 05:53:17, Laszlo Ersek wrote: > On 10/23/15 09:26, Paolo Bonzini wrote: > > > > On 23/10/2015 06:41, Jordan Justen wrote: > >> On 2015-10-22 12:46:56, Paolo Bonzini wrote: > >>> > >>> On 22/10/2015 20:04, Kevin O'Connor wrote: > >>>> On Thu, Oct 22, 2015 at 10:40:08AM +0200, Paolo Bonzini wrote: > >>>>> On 21/10/2015 20:36, Jordan Justen wrote: > >>>>>> On 2015-10-20 11:14:00, Laszlo Ersek wrote: > >>>>>>> Commit 4d00636e97b7 ("ich9: Add the lpc chip", Nov 14 2012) added the > >>>>>>> ich9_apm_ctrl_changed() ioport write callback function such that it would > >>>>>>> inject the SMI, in response to a write to the APM_CNT register, on the > >>>>>>> first CPU, invariably. > >>>>>>> > >>>>>>> Since this register is used by guest code to trigger an SMI synchronously, > >>>>>>> the interrupt should be injected on the VCPU that is performing the write. > >>>>>> > >>>>>> Why not send an SMI to *all* processors, like the real chipsets do? > >>>>> > >>>>> That's much less scalable, and more important I would have to check that > >>>>> SeaBIOS can handle that correctly. It probably doesn't, as it doesn't > >>>>> relocate SMBASEs. > >>>> > >>>> SeaBIOS is only expecting its SMI handler to be called once in > >>>> response to a synchronous SMI. We can change SeaBIOS to fix that. > >>>> > >>>> SeaBIOS does relocate the smbase from 0x30000 to 0xa0000 during its > >>>> init phase (by creating a synchronous SMI on the BSP and then setting > >>>> the smbase register to 0xa0000 in the smi handler). > >>> > >>> Right; however it would also have to relocate the SMBASE on the APs (in > >>> case they were halted with cli;hlt and not INITed). It's really not > >>> worth the hassle, > >> > >> It's not worth the hassle to relocate the SMBASE of the APs? > >> So, basically, write to 0x30000-0x38000, then send an SMI IPI to the > >> AP and now you have the AP running in SMI and it has extra privileges? > > > > Extra privileges compared to what? Legacy BIOS does not really put > > anything privileged in SMRAM, Why does seabios even bother relocating the BSP's SMBASE if it doesn't relocate the SMBASE for the APs? > > while OVMF does and _hence_ relocates the > > SMBASE of the AP. It would have been nice to get it right from the > > beginning, but right now it's not worth forcing a lockstep QEMU-SeaBIOS > > update. > > So what are we thinking about a magic APM_STS value to trigger an SMI > for all VCPUs? 0x51 ('Q') would be cool. :) This seems like a further deviation from the actual hardware. I understand that QEMU draws a line about strict hardware emulation, but I just wanted to point out the discrepancy. So, the trouble with changing QEMU to better emulate the hardware is that seabios can't handle multiple processors entering SMM? I think if SMM does anything interesting, then it is basically a requirement for all processors to enter SMM together. If not, you have an OS running that just lost one its processors. Maybe the OS will decide it try to restart the processor to regain control. Maybe the OS will try to talk to some chipset hardware in a way that will conflict with whatever the processor in SMM is doing (or vice-versa). -Jordan
On 23/10/2015 20:20, Jordan Justen wrote: >>>> It's not worth the hassle to relocate the SMBASE of the APs? >>>> So, basically, write to 0x30000-0x38000, then send an SMI IPI to the >>>> AP and now you have the AP running in SMI and it has extra privileges? >>> >>> Extra privileges compared to what? Legacy BIOS does not really put >>> anything privileged in SMRAM, > > Why does seabios even bother relocating the BSP's SMBASE if it doesn't > relocate the SMBASE for the APs? It uses SMM to run INT 13h in 32-bit real mode, basically. It's for MS-DOS usage only, so the APs don't matter. >> So what are we thinking about a magic APM_STS value to trigger an SMI >> for all VCPUs? 0x51 ('Q') would be cool. :) > > This seems like a further deviation from the actual hardware. I > understand that QEMU draws a line about strict hardware emulation, but > I just wanted to point out the discrepancy. Yeah, I am also a bit doubtful about that. > So, the trouble with changing QEMU to better emulate the hardware is > that seabios can't handle multiple processors entering SMM? Yes. Paolo
On 10/23/15 20:20, Jordan Justen wrote: > On 2015-10-23 05:53:17, Laszlo Ersek wrote: >> On 10/23/15 09:26, Paolo Bonzini wrote: >>> >>> On 23/10/2015 06:41, Jordan Justen wrote: >>>> On 2015-10-22 12:46:56, Paolo Bonzini wrote: >>>>> >>>>> On 22/10/2015 20:04, Kevin O'Connor wrote: >>>>>> On Thu, Oct 22, 2015 at 10:40:08AM +0200, Paolo Bonzini wrote: >>>>>>> On 21/10/2015 20:36, Jordan Justen wrote: >>>>>>>> On 2015-10-20 11:14:00, Laszlo Ersek wrote: >>>>>>>>> Commit 4d00636e97b7 ("ich9: Add the lpc chip", Nov 14 2012) added the >>>>>>>>> ich9_apm_ctrl_changed() ioport write callback function such that it would >>>>>>>>> inject the SMI, in response to a write to the APM_CNT register, on the >>>>>>>>> first CPU, invariably. >>>>>>>>> >>>>>>>>> Since this register is used by guest code to trigger an SMI synchronously, >>>>>>>>> the interrupt should be injected on the VCPU that is performing the write. >>>>>>>> >>>>>>>> Why not send an SMI to *all* processors, like the real chipsets do? >>>>>>> >>>>>>> That's much less scalable, and more important I would have to check that >>>>>>> SeaBIOS can handle that correctly. It probably doesn't, as it doesn't >>>>>>> relocate SMBASEs. >>>>>> >>>>>> SeaBIOS is only expecting its SMI handler to be called once in >>>>>> response to a synchronous SMI. We can change SeaBIOS to fix that. >>>>>> >>>>>> SeaBIOS does relocate the smbase from 0x30000 to 0xa0000 during its >>>>>> init phase (by creating a synchronous SMI on the BSP and then setting >>>>>> the smbase register to 0xa0000 in the smi handler). >>>>> >>>>> Right; however it would also have to relocate the SMBASE on the APs (in >>>>> case they were halted with cli;hlt and not INITed). It's really not >>>>> worth the hassle, >>>> >>>> It's not worth the hassle to relocate the SMBASE of the APs? >>>> So, basically, write to 0x30000-0x38000, then send an SMI IPI to the >>>> AP and now you have the AP running in SMI and it has extra privileges? >>> >>> Extra privileges compared to what? Legacy BIOS does not really put >>> anything privileged in SMRAM, > > Why does seabios even bother relocating the BSP's SMBASE if it doesn't > relocate the SMBASE for the APs? > >>> while OVMF does and _hence_ relocates the >>> SMBASE of the AP. It would have been nice to get it right from the >>> beginning, but right now it's not worth forcing a lockstep QEMU-SeaBIOS >>> update. >> >> So what are we thinking about a magic APM_STS value to trigger an SMI >> for all VCPUs? 0x51 ('Q') would be cool. :) > > This seems like a further deviation from the actual hardware. I > understand that QEMU draws a line about strict hardware emulation, but > I just wanted to point out the discrepancy. I'm completely okay if we control this behavior with another method, for example another -machine option, like "-machine smi=current" vs. "-machine smi=all". I needed something to work with, and the question should be discussed on the list, so I think the patch was good for that at least. BTW I have some incredible news to report, but that will take a separate email. :) Thanks Laszlo > > So, the trouble with changing QEMU to better emulate the hardware is > that seabios can't handle multiple processors entering SMM? > > I think if SMM does anything interesting, then it is basically a > requirement for all processors to enter SMM together. If not, you have > an OS running that just lost one its processors. Maybe the OS will > decide it try to restart the processor to regain control. Maybe the OS > will try to talk to some chipset hardware in a way that will conflict > with whatever the processor in SMM is doing (or vice-versa). > > -Jordan >
diff --git a/hw/isa/lpc_ich9.c b/hw/isa/lpc_ich9.c index 360699f..1ffc803 100644 --- a/hw/isa/lpc_ich9.c +++ b/hw/isa/lpc_ich9.c @@ -394,7 +394,7 @@ static void ich9_apm_ctrl_changed(uint32_t val, void *arg) /* SMI_EN = PMBASE + 30. SMI control and enable register */ if (lpc->pm.smi_en & ICH9_PMIO_SMI_EN_APMC_EN) { - cpu_interrupt(first_cpu, CPU_INTERRUPT_SMI); + cpu_interrupt(current_cpu, CPU_INTERRUPT_SMI); } }
Commit 4d00636e97b7 ("ich9: Add the lpc chip", Nov 14 2012) added the ich9_apm_ctrl_changed() ioport write callback function such that it would inject the SMI, in response to a write to the APM_CNT register, on the first CPU, invariably. Since this register is used by guest code to trigger an SMI synchronously, the interrupt should be injected on the VCPU that is performing the write. apm_ioport_writeb() is the .write callback of the "apm_ops" MemoryRegionOps [hw/isa/apm.c]; it is parametrized to call ich9_apm_ctrl_changed() by ich9_lpc_init() [hw/isa/lpc_ich9.c], via apm_init(). Therefore this change affects no other board. ich9_generate_smi() is an unrelated function that is called by the TCO watchdog; a watchdog is likely in its right to (asynchronously) inject interrupts on the first CPU only. This patch allows the combined edk2/OVMF SMM driver stack to work with multiple VCPUs on TCG, using both qemu-system-i386 and qemu-system-x86_64. Cc: Paolo Bonzini <pbonzini@redhat.com> Cc: Gerd Hoffmann <kraxel@redhat.com> Cc: Jordan Justen <jordan.l.justen@intel.com> Cc: Michael Kinney <michael.d.kinney@intel.com> Cc: "Michael S. Tsirkin" <mst@redhat.com> Signed-off-by: Laszlo Ersek <lersek@redhat.com> --- hw/isa/lpc_ich9.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)