[Qemu-devel] [PATCH] hw/isa/lpc_ich9: inject the SMI on the VCPU that is writing to APM_CNT

[Qemu-devel] [PATCH] hw/isa/lpc_ich9: inject the SMI on the VCPU that is writing to APM_CNT

[Laszlo Ersek]

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);
     }
 }
 
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH] hw/isa/lpc_ich9: inject the SMI on the VCPU that is writing to APM_CNT

[Paolo Bonzini]

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

Re: [Qemu-devel] [PATCH] hw/isa/lpc_ich9: inject the SMI on the VCPU that is writing to APM_CNT

[Michael S. Tsirkin]

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!

-- 
MST

Re: [Qemu-devel] [PATCH] hw/isa/lpc_ich9: inject the SMI on the VCPU that is writing to APM_CNT

[Jordan Justen]

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
> 

Re: [Qemu-devel] [PATCH] hw/isa/lpc_ich9: inject the SMI on the VCPU that is writing to APM_CNT

[Paolo Bonzini]

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

Re: [Qemu-devel] [PATCH] hw/isa/lpc_ich9: inject the SMI on the VCPU that is writing to APM_CNT

[Laszlo Ersek]

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
> 

Re: [Qemu-devel] [PATCH] hw/isa/lpc_ich9: inject the SMI on the VCPU that is writing to APM_CNT

[Paolo Bonzini]

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

Re: [Qemu-devel] [PATCH] hw/isa/lpc_ich9: inject the SMI on the VCPU that is writing to APM_CNT

[Kevin O'Connor]

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

Re: [Qemu-devel] [PATCH] hw/isa/lpc_ich9: inject the SMI on the VCPU that is writing to APM_CNT

[Paolo Bonzini]

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

Re: [Qemu-devel] [PATCH] hw/isa/lpc_ich9: inject the SMI on the VCPU that is writing to APM_CNT

[Jordan Justen]

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

Re: [Qemu-devel] [PATCH] hw/isa/lpc_ich9: inject the SMI on the VCPU that is writing to APM_CNT

[Paolo Bonzini]

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

Re: [Qemu-devel] [PATCH] hw/isa/lpc_ich9: inject the SMI on the VCPU that is writing to APM_CNT

[Laszlo Ersek]

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

Re: [Qemu-devel] [PATCH] hw/isa/lpc_ich9: inject the SMI on the VCPU that is writing to APM_CNT

[Jordan Justen]

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

Re: [Qemu-devel] [PATCH] hw/isa/lpc_ich9: inject the SMI on the VCPU that is writing to APM_CNT

[Paolo Bonzini]

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

Re: [Qemu-devel] [PATCH] hw/isa/lpc_ich9: inject the SMI on the VCPU that is writing to APM_CNT

[Laszlo Ersek]

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
> 

Re: [Qemu-devel] [PATCH] hw/isa/lpc_ich9: inject the SMI on the VCPU that is writing to APM_CNT

[Kevin O'Connor]

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

Re: [Qemu-devel] [PATCH] hw/isa/lpc_ich9: inject the SMI on the VCPU that is writing to APM_CNT

[Paolo Bonzini]

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