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

[Michael S. Tsirkin]

On Wed, Nov 16, 2016 at 07:47:42AM -0500, Paolo Bonzini wrote:
> 
> > 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.

If we keep collecting these magic values, should architect it
and do a host/guest bitmap like virtio does?

> 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

It bothers me with all these ideas is that it's PV.
Unavoidable?

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