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

["Michael S. Tsirkin" <mst@redhat.com>]

On Wed, Nov 16, 2016 at 06:37:30PM +0100, Laszlo Ersek wrote:
> On 11/16/16 13:47, 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.
> 
> Okay, but this still means that the PCD has to become dynamic, and we
> must set the PCD earlier (likely in PlatformPei) based on something.
> 
> I guess that's what the next paragraph is about:
> 
> > - 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.
> 
> Please explain in more detail. If I write to 0xB2 (by invoking the
> Trigger() method or somehow else), then on old QEMU's that will raise a
> sync / unicast SMI. The SMI handler in edk2 will run, but no request
> parameters will have been set up by OVMF, so the SMI handler will do...
> no clue what. I don't think this is a good idea.
> 
> My preference is fw_cfg ATM. It provides a prove, flexible and
> extensible interface (it's easy to add new files for future features).
> If we expect more knobs in the area, I can modify my proposal to use
> "etc/smi/broadcast", so we can add "etc/smi/XXXX" later.
> 
> Do you have any specific arguments against fw_cfg? As I suggested in my
> previous email, with fw_cfg I can implement the change in OVMF such that
> the default behavior wouldn't change -- the default delivery would
> remain relaxed, and the broadcast wouldn't be requested, unless the
> fw_cfg file told OVMF otherwise.

Only thing is, I think it's a good idea in the future to be able
to build OVMF without legacy QEMU support. E.g. there are all
people that want to speed up boot.
Add some ifdefs in code for that?
And add comments to document which version needs these hacks.



> > 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.
> 
> Are we talking control/command (0xB2) or scratch/data (0xB3) register
> values? My patches currently use the scratch/data register to provide
> the hint to QEMU; that register is less likely to interfere with
> anything the SMM core in edk2 does. I seem to recall that SmmSwDispatch
> uses command/control values to distinguish the called functions. Should
> we keep the broadcast / unicast decision separate from the
> control/command value ?
> 
> Thanks
> Laszlo
> 
> > 
> > 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);
> >>>>>> +        }
> >>>>>>      }
> >>>>>>  }
> >>>>>>  
> >>>>>>
> >>
> >>