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

[Laszlo Ersek]

On 11/16/16 14:18, Michael S. Tsirkin wrote:
> 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?

A feature bitmap is not a bad idea; I can modify my proposal to say,
'"etc/smi/features" is a little-endian uint64_t feature bitmap, where
bit #0 is the availability of broadcast SMIs. Request it by writing 'Q'
to STS before triggering an SMI via writing CNT'.

Another example where we use a feature bitmap is fw_cfg itself (the DMA
capability is signaled by bit 1).

However, feature *negotiation* is overkill, in my opinion.

> 
>> 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?

It seems so, yes -- as I understand it, the software-initiated SMI on
bare metal Q35 is meant to be broadcast unconditionally, but we had
diverged from that in our Q35 implementation, historically. SeaBIOS came
to rely on the unicast nature of QEMU's SMI (AIUI) and now we have to
invent a way to select the non-historical broadcast.

(

BTW, I foresee further Frankensteinization of Q35, as the maximum amount
of SMRAM (TSEG) it provides, by spec, is 8MB, and that might not be
enough for a very large VCPU count.

(The SMM stack was originally tested against 255 VCPUs, yes, but the
VCPU max continues to grow, plus edk2 developers keep adding SMM
features that require more SMRAM -- sometimes more SMRAM even per CPU.)

We have one unused bit pattern left in the TSEG_SZ bit field of the
ESMRAMC register, namely binary 11, which stands for "reserved". We
might want to commandeer that down the line, and associate a really
large SMRAM / TSEG size with it -- 128MB or 256MB, for example. Or, we
could use it to signal some other way for TSEG size configuration.

The TSEG is carved out of the end of the <4GB RAM, so larger TSEGs than
8MB should fit, as long as the guest is started with enough memory.

Anyway, I digress...

)

Thanks
Laszlo

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