Re: [Qemu-devel] [PATCH v5 wave 2 3/3] hw/isa/lpc_ich9: negotiate SMI broadcast on pc-q35-2.9+ machine types

[Laszlo Ersek]

On 01/12/17 13:40, Eduardo Habkost wrote:
> On Wed, Jan 11, 2017 at 06:35:28PM +0100, Laszlo Ersek wrote:
>> Cc: "Michael S. Tsirkin" <address@hidden>
>> Cc: Eduardo Habkost <address@hidden>
>> Cc: Gerd Hoffmann <address@hidden>
>> Cc: Igor Mammedov <address@hidden>
>> Cc: Paolo Bonzini <address@hidden>
>> Signed-off-by: Laszlo Ersek <address@hidden>
>> ---
>>
>> Notes:
>>     v5:
>>     - replace the v4 patch "hw/i386/pc_q35: advertise broadcast SMI if VCPU
>>       hotplug is turned off" with a simple device property and compat
>>       setting [Igor]
>>
>>  include/hw/i386/pc.h | 5 +++++
>>  hw/isa/lpc_ich9.c    | 2 ++
>>  2 files changed, 7 insertions(+)
>>
>> diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
>> index 230e9e70c504..fb8ca7c907f6 100644
>> --- a/include/hw/i386/pc.h
>> +++ b/include/hw/i386/pc.h
>> @@ -376,6 +376,11 @@ bool e820_get_entry(int, uint32_t, uint64_t *, uint64_t 
>> *);
>>  
>>  #define PC_COMPAT_2_8 \
>>      HW_COMPAT_2_8 \
>> +    {\
>> +        .driver   = "ICH9-LPC",\
>> +        .property = "smi_broadcast",\
>> +        .value    = "off",\
>> +    },\
>>  
>>  
>>  #define PC_COMPAT_2_7 \
>> diff --git a/hw/isa/lpc_ich9.c b/hw/isa/lpc_ich9.c
>> index ced6f803a4f2..27fae5144744 100644
>> --- a/hw/isa/lpc_ich9.c
>> +++ b/hw/isa/lpc_ich9.c
>> @@ -776,6 +776,8 @@ static const VMStateDescription vmstate_ich9_lpc = {
>>  
>>  static Property ich9_lpc_properties[] = {
>>      DEFINE_PROP_BOOL("noreboot", ICH9LPCState, pin_strap.spkr_hi, true),
>> +    DEFINE_PROP_BIT64("smi_broadcast", ICH9LPCState, smi_host_features,
>> +                      ICH9_LPC_SMI_F_BROADCAST_BIT, true),
> 
> Please use hyphens instead of underscores on QOM property names.

Will do, thank you.

> 
> Also, if this is not supposed to be configured directly by the
> user, please use a "x-" prefix.

Hmm, for "normal use", the user is not supposed to mess with it,
correct. However, at least for testing OVMF's behavior when the
negotiation is available vs. unavailable, the flag is useful on the
command line too (although the same effect can be achieved with
selecting pc-q35-2.8 vs. pc-q35-2.9). What does this imply?...

Okay, I think I convinced myself that this should be called
x-smi-broadcast. Ultimately I don't think libvirt should ever try to set
this property, for example.

> With that fixed:
> 
> Reviewed-by: Eduardo Habkost <address@hidden>
> 

Many thanks!
Laszlo