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

[Laszlo Ersek]

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.

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