Re: [Qemu-devel] [PATCH v2] hw/isa/lpc_ich9: inject SMI on all VCPUs if APM_STS == 'Q'

[Paolo Bonzini]

> If the consensus is that the patch is a QEMU bugfix (as opposed to a
> feature) and that it is eligible for the currently supported upstream
> stable branches, that's the best, no doubt.

The currently supported upstream stable branches is just 2.7. :)

I'm okay with bending the rules and including it in 2.8, but it's
worrisome that you also needed to go back from relaxed to traditional
delivery, meaning that old QEMU + new OVMF will take ages to boot.

If this is the case, I still think this needs some kind of discovery
mechanism, unless OVMF can just say "things were too broken, stop
supporting SMM on QEMUs older than 2.8".

For example:

- OVMF should keep on using 0x00 (no broadcast) if the relaxed AP
setting is used for the PCD; this would be backwards compatibility mode.

- we could have another magic 0xB2 value, which is implemented directly
in QEMU and sets 0xB3 to a magic value.  Then OVMF can invoke it
after SMBASE relocation and SMM IPL (so as not to crash on old QEMUs)
to detect the new feature.  It can fail to start if using traditional
AP and the new feature is not there.

By the way, in case OVMF needs to use SmmSwDispatch in the future, I
would make QEMU use broadcast behavior for all values in the 0x10-0xff
range, or something like that.

Paolo

> For reference, the OVMF documentation recommends QEMU 2.5+ for SMM. The
> SMM enablement in libvirt enforces QEMU 2.4+. (Libvirt is actually
> correct; when I was writing the OVMF docs, I must have misunderstood the
> requirements and needlessly required 2.5+; 2.4+ should have been fine.)
> 
> Which means the fix should be backported as far as stable-2.4.
> 
> Should we proceed with that? CC'ing Mike Roth and the stable list.
> 
> Thanks!
> Laszlo
> 
> > 
> > 
> >>>
> >>> Paolo
> >>>
> >>>> ---
> >>>>  hw/isa/lpc_ich9.c | 12 +++++++++++-
> >>>>  1 file changed, 11 insertions(+), 1 deletion(-)
> >>>>
> >>>> diff --git a/hw/isa/lpc_ich9.c b/hw/isa/lpc_ich9.c
> >>>> index 10d1ee8b9310..f2fe644fdaa4 100644
> >>>> --- a/hw/isa/lpc_ich9.c
> >>>> +++ b/hw/isa/lpc_ich9.c
> >>>> @@ -372,6 +372,8 @@ void ich9_lpc_pm_init(PCIDevice *lpc_pci, bool
> >>>> smm_enabled)
> >>>>  
> >>>>  /* APM */
> >>>>  
> >>>> +#define QEMU_ICH9_APM_STS_BROADCAST_SMI 'Q'
> >>>> +
> >>>>  static void ich9_apm_ctrl_changed(uint32_t val, void *arg)
> >>>>  {
> >>>>      ICH9LPCState *lpc = arg;
> >>>> @@ -386,7 +388,15 @@ 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(current_cpu, CPU_INTERRUPT_SMI);
> >>>> +        if (lpc->apm.apms == QEMU_ICH9_APM_STS_BROADCAST_SMI) {
> >>>> +            CPUState *cs;
> >>>> +
> >>>> +            CPU_FOREACH(cs) {
> >>>> +                cpu_interrupt(cs, CPU_INTERRUPT_SMI);
> >>>> +            }
> >>>> +        } else {
> >>>> +            cpu_interrupt(current_cpu, CPU_INTERRUPT_SMI);
> >>>> +        }
> >>>>      }
> >>>>  }
> >>>>  
> >>>>
> 
>