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

[Laszlo Ersek]

On 11/16/16 21:38, Michael S. Tsirkin wrote:
> On Wed, Nov 16, 2016 at 01:04:00PM -0500, Paolo Bonzini wrote:
>>> 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.
>>
>> It should hopefully do nothing.  A spurious SMI (such as the one caused
>> by the write to 0xB2) should not crash OVMF.
>>
>> SMBASE relocation uses IPIs, so my hope was to use the
>> SmmCpuFeaturesSmmRelocationComplete hook.
>>
>>> 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.
>>
>> Did you know there are 16 entries only for fw_cfg files? :)  And we're
>> using already 20 in the worst case:
>>
>>     genroms/linuxboot.bin
>>     genroms/kvmvapic.bin
>>     NVDIMM_DSM_MEM_FILE
>>     "etc/smbios/smbios-tables"
>>     "etc/smbios/smbios-anchor"
>>     "etc/acpi/tables"
>>     "etc/table-loader"
>>     ACPI_BUILD_TPMLOG_FILE
>>     ACPI_BUILD_RSDP_FILE
>>     "etc/e820"
>>     "etc/msr_feature_control"
>>     "etc/reserved-memory-end"
>>     "etc/pvpanic-port"
>>     "etc/boot-menu-wait"
>>     "bootsplash.jpg"
>>     "etc/boot-fail-wait"
>>     "etc/igd-opregion"
>>     "etc/igd-bdsm-size"
>>     "etc/extra-pci-roots"
>>     "bootorder"
>>
>> Therefore, so close to the release I'm a bit worried about doing
>> changes to fw_cfg or adding more fw_cfg files.
> 
> Indeed. Is an unconditional thing so bad?
> What would be the observed behaviour with new OVMF on old QEMU?

The SMM stack would expect broadcast SMIs but only unicast SMIs would
occur. This would
- introduce very long delays in the handling on each SMI as the
synchronization algorithms time out and then force individual APs into
SMM by LAPIC writes,
- expose obscure synchronization bugs in the SMM stack, especially
during S3 resume.

The directed / unicast SMI model is less tested in edk2 and a number of
super-obscure corner cases remain.

Thanks
Laszlo

> Note you need to migrate during boot to notice this.
> 
>> Though we just got
>> rid of one file for the number of CPUs, so I guess we might not care.
>>
>>> 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.
>>
>> Sorry I confused the two registers.  0xb3 is more or less unused as far
>> as I can see indeed.
>>
>> Paolo