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

[Laszlo Ersek <lersek@redhat.com>]

On 11/17/16 18:46, Michael S. Tsirkin wrote:
> On Thu, Nov 17, 2016 at 02:16:35PM +0100, Laszlo Ersek wrote:
>> On 11/16/16 21:27, Michael S. Tsirkin wrote:
>>> On Wed, Nov 16, 2016 at 07:03:27PM +0100, Laszlo Ersek wrote:
>>>> On 11/16/16 15:05, Paolo Bonzini wrote:
>>>>>
>>>>>
>>>>> On 16/11/2016 14:18, Michael S. Tsirkin wrote:
>>>>>>> - 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?
>>>>>
>>>>> The value written in 0xB3 can certainly be a feature bitmap.  For now we
>>>>> would have for example
>>>>>
>>>>> bit 0	if set, writing 0x10-0xFF to 0xB2 results in a broadcast SMI
>>>>> bit 1-7	zero
>>>>
>>>> Doable, but:
>>>> - doesn't address how OVMF learns about the broadcast SMI availability,
>>>> - the command value OVMF currently writes is 0.
>>>>
>>>> How about this:
>>>> - etc/smi/features is the LE uint64_t bitmap proposed earlier, bit#0
>>>> stands for broadcast SMI availability
>>>> - 0xB2 is the command value (independent of 0xB3)
>>>> - 0XB3 is a guest feature bitmap (valid for the next request). SeaBIOS
>>>> reserves bit#0 already (uses values 0 and 1), so we can use the
>>>> remaining 7 bits for requesting features. Bit#1 (value 2) could be the
>>>> broadcast SMI.
>>>>
>>>> This does resemble a kind of feature negotiation, except the host cannot
>>>> signal back an error (unsupported combination of features), like
>>>> virtio-1.0 can. We can make QEMU abort in that case, or ignore the flags.
>>>>
>>>> Thanks
>>>> Laszlo
>>>
>>> I think that if you are going to do it, do it like 1.0:
>>> - same bitmap for host and guest. how about a writeable fw cfg file?
>>
>> fw_cfg files are not writeable since qemu 2.4 (see commits 023e3148567ac
>> and 6cec43e178cde).
>>
>> How about this alternative, in STS:
>> - bit 0: read and written transparently
>> - bit 1: on write:
>>          0 -- set features in bits 2-7
>>          1 -- query host features into bits 2-7
>>          on read:
>>          - after querying features:
>>            - reads back as 0 if the interface is supported
>>            - reads back as 1 if the interface is missing
>>          - after setting features:
>>            - reads back as 0 if the feature subset is valid
>>            - reads back as 1 otherwise
>> - bit 2: on write:
>>          - when setting features: request broadcast SMI
>>          - when querying features: ignored
>>          on read:
>>          - after setting features: zero
>>          - after querying features: broadcast SMI availability (1 if
>>            available)
>>
>> - bit 3-7: future features (I think 5 more features for SMI handling
>>            should suffice), working similarly to bit 2
>>
>> SeaBIOS writes values 0x00 and 0x01, and expects to find the same when
>> reading back. Bit pattern 0000_000?b  translates to "clear all
>> features", which always succeeds and results in behavior identical to
>> the current one, hence bits 1-7 read back as zero.
>>
>> OVMF:
>> - write 0x02, read back value:
>>   - if bit 1 is set, interface is missing
>>   - otherwise feature bitmap was returned in bits 2-7
>> - select requested features in bits 2-7, set bit 1 to 0, write value,
>>   read back value
>>   - if bit 1 is set, the feature subset is invalid
>>   - okay otherwise
>>
>> Thanks
>> Laszlo
> 
> 
> It's all fine, or we can make fw cfg writeable again (I posted
> a patch for that a while ago), but it's all a bit too much
> for this release.
> 
> Let's just defer it, or do you have a better idea?

I'm writing patches for the above proposal (including a document under
docs/specs/), and I plan to post them soon. They're definitely 2.9
material though -- I don't mind if I have to wait a bit even just to get
feedback on those patches :)

So, to make it formal for the patch that started this thread:

Self-NAK

Will post something better / more flexible soon, for 2.9.

(Hopefully I'll remember to put "[for-2.9]" in the subject.)

Thanks!
Laszlo

>>> - use 0XB3 bit for FEATURES_OK
>>>