| Message ID | 20170111173528.30510-4-lersek@redhat.com |
|---|---|
| State | Superseded |
| Headers | show |
On Wed, Jan 11, 2017 at 06:35:28PM +0100, Laszlo Ersek wrote: > Cc: "Michael S. Tsirkin" <mst@redhat.com> > Cc: Eduardo Habkost <ehabkost@redhat.com> > Cc: Gerd Hoffmann <kraxel@redhat.com> > Cc: Igor Mammedov <imammedo@redhat.com> > Cc: Paolo Bonzini <pbonzini@redhat.com> > Signed-off-by: Laszlo Ersek <lersek@redhat.com> > --- > > 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. Also, if this is not supposed to be configured directly by the user, please use a "x-" prefix. With that fixed: Reviewed-by: Eduardo Habkost <ehabkost@redhat.com> -- Eduardo
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" <mst@redhat.com> >> Cc: Eduardo Habkost <ehabkost@redhat.com> >> Cc: Gerd Hoffmann <kraxel@redhat.com> >> Cc: Igor Mammedov <imammedo@redhat.com> >> Cc: Paolo Bonzini <pbonzini@redhat.com> >> Signed-off-by: Laszlo Ersek <lersek@redhat.com> >> --- >> >> 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 <ehabkost@redhat.com> > Many thanks! Laszlo
On Thu, Jan 12, 2017 at 05:40:40PM +0100, Laszlo Ersek wrote: > 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" <mst@redhat.com> > >> Cc: Eduardo Habkost <ehabkost@redhat.com> > >> Cc: Gerd Hoffmann <kraxel@redhat.com> > >> Cc: Igor Mammedov <imammedo@redhat.com> > >> Cc: Paolo Bonzini <pbonzini@redhat.com> > >> Signed-off-by: Laszlo Ersek <lersek@redhat.com> > >> --- > >> > >> 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?... It will be still available for testing, the difference is that we won't guarantee it will be available in future QEMU versions. > > 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. Exactly. -- Eduardo
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), DEFINE_PROP_END_OF_LIST(), };
Cc: "Michael S. Tsirkin" <mst@redhat.com> Cc: Eduardo Habkost <ehabkost@redhat.com> Cc: Gerd Hoffmann <kraxel@redhat.com> Cc: Igor Mammedov <imammedo@redhat.com> Cc: Paolo Bonzini <pbonzini@redhat.com> Signed-off-by: Laszlo Ersek <lersek@redhat.com> --- 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(+) -- 2.9.3