Re: [Qemu-devel] [PATCH 07/11] spapr: add hotplug interrupt machine options

2016-10-16 Thread Bharata B Rao 
On Fri, Oct 14, 2016 at 01:04:37PM -0500, Michael Roth wrote:
> Quoting Bharata B Rao (2016-10-14 03:37:32)
> > On Wed, Oct 12, 2016 at 06:13:55PM -0500, Michael Roth wrote:
> > > This adds machine options of the form:
> > > 
> > >   -machine pseries,legacy-hotplug-events=true
> > >   -machine pseries,legacy-hotplug-events=false
> > > 
> > > to denote whether or not we wish to force the use of "legacy" style
> > > hotplug events, which are surfaced through EPOW interrupts instead of
> > > a dedicated interrupt source, and lack certain features necessary,
> > > mainly, for memory unplug support.
> > > 
> > > If false, QEMU will default to "legacy" style unless the guest
> > > advertises support for the newer events via
> > > ibm,client-architecture-support hcall during early boot.
> > > 
> > > For pseries-2.7 and earlier we default to true, for newer machine
> > > types we default to false.
> > > 
> > > Signed-off-by: Michael Roth 
> > > ---
> > >  hw/ppc/spapr.c  | 31 +++
> > >  include/hw/ppc/spapr.h  |  1 +
> > >  include/hw/ppc/spapr_ovec.h |  1 +
> > >  3 files changed, 33 insertions(+)
> > > 
> > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > > index f8cde92..d80a6fa 100644
> > > --- a/hw/ppc/spapr.c
> > > +++ b/hw/ppc/spapr.c
> > > @@ -1816,6 +1816,11 @@ static void ppc_spapr_init(MachineState *machine)
> > > 
> > >  spapr_ovec_set(spapr->ov5, OV5_FORM1_AFFINITY);
> > > 
> > > +/* use dedicated HP event source if guest supports it */
> > > +if (spapr->use_hotplug_event_source) {
> > > +spapr_ovec_set(spapr->ov5, OV5_HP_EVT);
> > 
> > The above comment can be confusing. Here you really mean that
> > the machine type version supports OV5_HP_EVT right ? Because
> > guest support for the same is determined during cas call later.
> 
> What trying to get it across that support would only be enabled if the
> guest indicates support for it later. What about something like:
> 
> /* advertise support for dedicated HP event source to guests */

Sounds good. Thanks.

> > 
> > Regards,
> > Bharata.

Re: [Qemu-devel] [PATCH 07/11] spapr: add hotplug interrupt machine options

2016-10-14 Thread Michael Roth 
Quoting Bharata B Rao (2016-10-14 03:37:32)
> On Wed, Oct 12, 2016 at 06:13:55PM -0500, Michael Roth wrote:
> > This adds machine options of the form:
> > 
> >   -machine pseries,legacy-hotplug-events=true
> >   -machine pseries,legacy-hotplug-events=false
> > 
> > to denote whether or not we wish to force the use of "legacy" style
> > hotplug events, which are surfaced through EPOW interrupts instead of
> > a dedicated interrupt source, and lack certain features necessary,
> > mainly, for memory unplug support.
> > 
> > If false, QEMU will default to "legacy" style unless the guest
> > advertises support for the newer events via
> > ibm,client-architecture-support hcall during early boot.
> > 
> > For pseries-2.7 and earlier we default to true, for newer machine
> > types we default to false.
> > 
> > Signed-off-by: Michael Roth 
> > ---
> >  hw/ppc/spapr.c  | 31 +++
> >  include/hw/ppc/spapr.h  |  1 +
> >  include/hw/ppc/spapr_ovec.h |  1 +
> >  3 files changed, 33 insertions(+)
> > 
> > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > index f8cde92..d80a6fa 100644
> > --- a/hw/ppc/spapr.c
> > +++ b/hw/ppc/spapr.c
> > @@ -1816,6 +1816,11 @@ static void ppc_spapr_init(MachineState *machine)
> > 
> >  spapr_ovec_set(spapr->ov5, OV5_FORM1_AFFINITY);
> > 
> > +/* use dedicated HP event source if guest supports it */
> > +if (spapr->use_hotplug_event_source) {
> > +spapr_ovec_set(spapr->ov5, OV5_HP_EVT);
> 
> The above comment can be confusing. Here you really mean that
> the machine type version supports OV5_HP_EVT right ? Because
> guest support for the same is determined during cas call later.

What trying to get it across that support would only be enabled if the
guest indicates support for it later. What about something like:

/* advertise support for dedicated HP event source to guests */

> 
> Regards,
> Bharata.

Re: [Qemu-devel] [PATCH 07/11] spapr: add hotplug interrupt machine options

2016-10-14 Thread Michael Roth 
Quoting David Gibson (2016-10-13 23:38:19)
> On Wed, Oct 12, 2016 at 06:13:55PM -0500, Michael Roth wrote:
> > This adds machine options of the form:
> > 
> >   -machine pseries,legacy-hotplug-events=true
> >   -machine pseries,legacy-hotplug-events=false
> > 
> > to denote whether or not we wish to force the use of "legacy" style
> > hotplug events, which are surfaced through EPOW interrupts instead of
> > a dedicated interrupt source, and lack certain features necessary,
> > mainly, for memory unplug support.
> > 
> > If false, QEMU will default to "legacy" style unless the guest
> > advertises support for the newer events via
> > ibm,client-architecture-support hcall during early boot.
> > 
> > For pseries-2.7 and earlier we default to true, for newer machine
> > types we default to false.
> > 
> > Signed-off-by: Michael Roth 
> 
> Hrm.. I think it would be a little clearer if you could find a wording
> such that both the internal variable and the external property have
> the same sense - i.e. get rid of the ! in the property getters /
> setters.

Ok, wasn't sure which direction was more useful from user standpoint. I
think something like modern-hotplug-events=true|false might be
reasonable though.

> 
> > ---
> >  hw/ppc/spapr.c  | 31 +++
> >  include/hw/ppc/spapr.h  |  1 +
> >  include/hw/ppc/spapr_ovec.h |  1 +
> >  3 files changed, 33 insertions(+)
> > 
> > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > index f8cde92..d80a6fa 100644
> > --- a/hw/ppc/spapr.c
> > +++ b/hw/ppc/spapr.c
> > @@ -1816,6 +1816,11 @@ static void ppc_spapr_init(MachineState *machine)
> >  
> >  spapr_ovec_set(spapr->ov5, OV5_FORM1_AFFINITY);
> >  
> > +/* use dedicated HP event source if guest supports it */
> > +if (spapr->use_hotplug_event_source) {
> > +spapr_ovec_set(spapr->ov5, OV5_HP_EVT);
> > +}
> > +
> >  /* init CPUs */
> >  if (machine->cpu_model == NULL) {
> >  machine->cpu_model = kvm_enabled() ? "host" : smc->tcg_default_cpu;
> > @@ -2172,16 +2177,39 @@ static void spapr_set_kvm_type(Object *obj, const 
> > char *value, Error **errp)
> >  spapr->kvm_type = g_strdup(value);
> >  }
> >  
> > +static bool spapr_get_legacy_hotplug_events(Object *obj, Error **errp)
> > +{
> > +sPAPRMachineState *spapr = SPAPR_MACHINE(obj);
> > +
> > +return !spapr->use_hotplug_event_source;
> > +}
> > +
> > +static void spapr_set_legacy_hotplug_events(Object *obj, bool value,
> > +Error **errp)
> > +{
> > +sPAPRMachineState *spapr = SPAPR_MACHINE(obj);
> > +
> > +spapr->use_hotplug_event_source = !value;
> > +}
> > +
> >  static void spapr_machine_initfn(Object *obj)
> >  {
> >  sPAPRMachineState *spapr = SPAPR_MACHINE(obj);
> >  
> >  spapr->htab_fd = -1;
> > +spapr->use_hotplug_event_source = true;
> >  object_property_add_str(obj, "kvm-type",
> >  spapr_get_kvm_type, spapr_set_kvm_type, NULL);
> >  object_property_set_description(obj, "kvm-type",
> >  "Specifies the KVM virtualization mode 
> > (HV, PR)",
> >  NULL);
> > +object_property_add_bool(obj, "legacy-hotplug-events",
> > +spapr_get_legacy_hotplug_events,
> > +spapr_set_legacy_hotplug_events,
> > +NULL);
> > +object_property_set_description(obj, "legacy-hotplug-events",
> > +"Use deprecated EPOW mechanism for 
> > hotplug events",
> > +NULL);
> >  }
> >  
> >  static void spapr_machine_finalizefn(Object *obj)
> > @@ -2518,6 +2546,9 @@ DEFINE_SPAPR_MACHINE(2_8, "2.8", true);
> >  
> >  static void spapr_machine_2_7_instance_options(MachineState *machine)
> >  {
> > +sPAPRMachineState *spapr = SPAPR_MACHINE(machine);
> > +
> > +spapr->use_hotplug_event_source = false;
> >  }
> >  
> >  static void spapr_machine_2_7_class_options(MachineClass *mc)
> > diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
> > index 27a3328..d1a4a14 100644
> > --- a/include/hw/ppc/spapr.h
> > +++ b/include/hw/ppc/spapr.h
> > @@ -74,6 +74,7 @@ struct sPAPRMachineState {
> >  uint32_t check_exception_irq;
> >  Notifier epow_notifier;
> >  QTAILQ_HEAD(, sPAPREventLogEntry) pending_events;
> > +bool use_hotplug_event_source;
> >  
> >  /* Migration state */
> >  int htab_save_index;
> > diff --git a/include/hw/ppc/spapr_ovec.h b/include/hw/ppc/spapr_ovec.h
> > index 47fa04c..92167c6 100644
> > --- a/include/hw/ppc/spapr_ovec.h
> > +++ b/include/hw/ppc/spapr_ovec.h
> > @@ -45,6 +45,7 @@ typedef struct sPAPROptionVector sPAPROptionVector;
> >  /* option vector 5 */
> >  #define OV5_DRCONF_MEMORY   OV_BIT(2, 2)
> >  #define OV5_FORM1_AFFINITY  OV_BIT(5, 0)
> > +#define OV5_HP_EVT  OV_BIT(6, 5)
> >  
> >  /* interfaces

Re: [Qemu-devel] [PATCH 07/11] spapr: add hotplug interrupt machine options

2016-10-14 Thread Bharata B Rao 
On Wed, Oct 12, 2016 at 06:13:55PM -0500, Michael Roth wrote:
> This adds machine options of the form:
> 
>   -machine pseries,legacy-hotplug-events=true
>   -machine pseries,legacy-hotplug-events=false
> 
> to denote whether or not we wish to force the use of "legacy" style
> hotplug events, which are surfaced through EPOW interrupts instead of
> a dedicated interrupt source, and lack certain features necessary,
> mainly, for memory unplug support.
> 
> If false, QEMU will default to "legacy" style unless the guest
> advertises support for the newer events via
> ibm,client-architecture-support hcall during early boot.
> 
> For pseries-2.7 and earlier we default to true, for newer machine
> types we default to false.
> 
> Signed-off-by: Michael Roth 
> ---
>  hw/ppc/spapr.c  | 31 +++
>  include/hw/ppc/spapr.h  |  1 +
>  include/hw/ppc/spapr_ovec.h |  1 +
>  3 files changed, 33 insertions(+)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index f8cde92..d80a6fa 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -1816,6 +1816,11 @@ static void ppc_spapr_init(MachineState *machine)
> 
>  spapr_ovec_set(spapr->ov5, OV5_FORM1_AFFINITY);
> 
> +/* use dedicated HP event source if guest supports it */
> +if (spapr->use_hotplug_event_source) {
> +spapr_ovec_set(spapr->ov5, OV5_HP_EVT);

The above comment can be confusing. Here you really mean that
the machine type version supports OV5_HP_EVT right ? Because
guest support for the same is determined during cas call later.

Regards,
Bharata.

Re: [Qemu-devel] [PATCH 07/11] spapr: add hotplug interrupt machine options

2016-10-13 Thread David Gibson 
On Wed, Oct 12, 2016 at 06:13:55PM -0500, Michael Roth wrote:
> This adds machine options of the form:
> 
>   -machine pseries,legacy-hotplug-events=true
>   -machine pseries,legacy-hotplug-events=false
> 
> to denote whether or not we wish to force the use of "legacy" style
> hotplug events, which are surfaced through EPOW interrupts instead of
> a dedicated interrupt source, and lack certain features necessary,
> mainly, for memory unplug support.
> 
> If false, QEMU will default to "legacy" style unless the guest
> advertises support for the newer events via
> ibm,client-architecture-support hcall during early boot.
> 
> For pseries-2.7 and earlier we default to true, for newer machine
> types we default to false.
> 
> Signed-off-by: Michael Roth 

Hrm.. I think it would be a little clearer if you could find a wording
such that both the internal variable and the external property have
the same sense - i.e. get rid of the ! in the property getters /
setters.

> ---
>  hw/ppc/spapr.c  | 31 +++
>  include/hw/ppc/spapr.h  |  1 +
>  include/hw/ppc/spapr_ovec.h |  1 +
>  3 files changed, 33 insertions(+)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index f8cde92..d80a6fa 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -1816,6 +1816,11 @@ static void ppc_spapr_init(MachineState *machine)
>  
>  spapr_ovec_set(spapr->ov5, OV5_FORM1_AFFINITY);
>  
> +/* use dedicated HP event source if guest supports it */
> +if (spapr->use_hotplug_event_source) {
> +spapr_ovec_set(spapr->ov5, OV5_HP_EVT);
> +}
> +
>  /* init CPUs */
>  if (machine->cpu_model == NULL) {
>  machine->cpu_model = kvm_enabled() ? "host" : smc->tcg_default_cpu;
> @@ -2172,16 +2177,39 @@ static void spapr_set_kvm_type(Object *obj, const 
> char *value, Error **errp)
>  spapr->kvm_type = g_strdup(value);
>  }
>  
> +static bool spapr_get_legacy_hotplug_events(Object *obj, Error **errp)
> +{
> +sPAPRMachineState *spapr = SPAPR_MACHINE(obj);
> +
> +return !spapr->use_hotplug_event_source;
> +}
> +
> +static void spapr_set_legacy_hotplug_events(Object *obj, bool value,
> +Error **errp)
> +{
> +sPAPRMachineState *spapr = SPAPR_MACHINE(obj);
> +
> +spapr->use_hotplug_event_source = !value;
> +}
> +
>  static void spapr_machine_initfn(Object *obj)
>  {
>  sPAPRMachineState *spapr = SPAPR_MACHINE(obj);
>  
>  spapr->htab_fd = -1;
> +spapr->use_hotplug_event_source = true;
>  object_property_add_str(obj, "kvm-type",
>  spapr_get_kvm_type, spapr_set_kvm_type, NULL);
>  object_property_set_description(obj, "kvm-type",
>  "Specifies the KVM virtualization mode 
> (HV, PR)",
>  NULL);
> +object_property_add_bool(obj, "legacy-hotplug-events",
> +spapr_get_legacy_hotplug_events,
> +spapr_set_legacy_hotplug_events,
> +NULL);
> +object_property_set_description(obj, "legacy-hotplug-events",
> +"Use deprecated EPOW mechanism for 
> hotplug events",
> +NULL);
>  }
>  
>  static void spapr_machine_finalizefn(Object *obj)
> @@ -2518,6 +2546,9 @@ DEFINE_SPAPR_MACHINE(2_8, "2.8", true);
>  
>  static void spapr_machine_2_7_instance_options(MachineState *machine)
>  {
> +sPAPRMachineState *spapr = SPAPR_MACHINE(machine);
> +
> +spapr->use_hotplug_event_source = false;
>  }
>  
>  static void spapr_machine_2_7_class_options(MachineClass *mc)
> diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
> index 27a3328..d1a4a14 100644
> --- a/include/hw/ppc/spapr.h
> +++ b/include/hw/ppc/spapr.h
> @@ -74,6 +74,7 @@ struct sPAPRMachineState {
>  uint32_t check_exception_irq;
>  Notifier epow_notifier;
>  QTAILQ_HEAD(, sPAPREventLogEntry) pending_events;
> +bool use_hotplug_event_source;
>  
>  /* Migration state */
>  int htab_save_index;
> diff --git a/include/hw/ppc/spapr_ovec.h b/include/hw/ppc/spapr_ovec.h
> index 47fa04c..92167c6 100644
> --- a/include/hw/ppc/spapr_ovec.h
> +++ b/include/hw/ppc/spapr_ovec.h
> @@ -45,6 +45,7 @@ typedef struct sPAPROptionVector sPAPROptionVector;
>  /* option vector 5 */
>  #define OV5_DRCONF_MEMORY   OV_BIT(2, 2)
>  #define OV5_FORM1_AFFINITY  OV_BIT(5, 0)
> +#define OV5_HP_EVT  OV_BIT(6, 5)
>  
>  /* interfaces */
>  sPAPROptionVector *spapr_ovec_new(void);

-- 
David Gibson| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au  | minimalist, thank you.  NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson


signature.asc
Description: PGP signature

[Qemu-devel] [PATCH 07/11] spapr: add hotplug interrupt machine options

2016-10-12 Thread Michael Roth 
This adds machine options of the form:

  -machine pseries,legacy-hotplug-events=true
  -machine pseries,legacy-hotplug-events=false

to denote whether or not we wish to force the use of "legacy" style
hotplug events, which are surfaced through EPOW interrupts instead of
a dedicated interrupt source, and lack certain features necessary,
mainly, for memory unplug support.

If false, QEMU will default to "legacy" style unless the guest
advertises support for the newer events via
ibm,client-architecture-support hcall during early boot.

For pseries-2.7 and earlier we default to true, for newer machine
types we default to false.

Signed-off-by: Michael Roth 
---
 hw/ppc/spapr.c  | 31 +++
 include/hw/ppc/spapr.h  |  1 +
 include/hw/ppc/spapr_ovec.h |  1 +
 3 files changed, 33 insertions(+)

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index f8cde92..d80a6fa 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -1816,6 +1816,11 @@ static void ppc_spapr_init(MachineState *machine)
 
 spapr_ovec_set(spapr->ov5, OV5_FORM1_AFFINITY);
 
+/* use dedicated HP event source if guest supports it */
+if (spapr->use_hotplug_event_source) {
+spapr_ovec_set(spapr->ov5, OV5_HP_EVT);
+}
+
 /* init CPUs */
 if (machine->cpu_model == NULL) {
 machine->cpu_model = kvm_enabled() ? "host" : smc->tcg_default_cpu;
@@ -2172,16 +2177,39 @@ static void spapr_set_kvm_type(Object *obj, const char 
*value, Error **errp)
 spapr->kvm_type = g_strdup(value);
 }
 
+static bool spapr_get_legacy_hotplug_events(Object *obj, Error **errp)
+{
+sPAPRMachineState *spapr = SPAPR_MACHINE(obj);
+
+return !spapr->use_hotplug_event_source;
+}
+
+static void spapr_set_legacy_hotplug_events(Object *obj, bool value,
+Error **errp)
+{
+sPAPRMachineState *spapr = SPAPR_MACHINE(obj);
+
+spapr->use_hotplug_event_source = !value;
+}
+
 static void spapr_machine_initfn(Object *obj)
 {
 sPAPRMachineState *spapr = SPAPR_MACHINE(obj);
 
 spapr->htab_fd = -1;
+spapr->use_hotplug_event_source = true;
 object_property_add_str(obj, "kvm-type",
 spapr_get_kvm_type, spapr_set_kvm_type, NULL);
 object_property_set_description(obj, "kvm-type",
 "Specifies the KVM virtualization mode 
(HV, PR)",
 NULL);
+object_property_add_bool(obj, "legacy-hotplug-events",
+spapr_get_legacy_hotplug_events,
+spapr_set_legacy_hotplug_events,
+NULL);
+object_property_set_description(obj, "legacy-hotplug-events",
+"Use deprecated EPOW mechanism for hotplug 
events",
+NULL);
 }
 
 static void spapr_machine_finalizefn(Object *obj)
@@ -2518,6 +2546,9 @@ DEFINE_SPAPR_MACHINE(2_8, "2.8", true);
 
 static void spapr_machine_2_7_instance_options(MachineState *machine)
 {
+sPAPRMachineState *spapr = SPAPR_MACHINE(machine);
+
+spapr->use_hotplug_event_source = false;
 }
 
 static void spapr_machine_2_7_class_options(MachineClass *mc)
diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
index 27a3328..d1a4a14 100644
--- a/include/hw/ppc/spapr.h
+++ b/include/hw/ppc/spapr.h
@@ -74,6 +74,7 @@ struct sPAPRMachineState {
 uint32_t check_exception_irq;
 Notifier epow_notifier;
 QTAILQ_HEAD(, sPAPREventLogEntry) pending_events;
+bool use_hotplug_event_source;
 
 /* Migration state */
 int htab_save_index;
diff --git a/include/hw/ppc/spapr_ovec.h b/include/hw/ppc/spapr_ovec.h
index 47fa04c..92167c6 100644
--- a/include/hw/ppc/spapr_ovec.h
+++ b/include/hw/ppc/spapr_ovec.h
@@ -45,6 +45,7 @@ typedef struct sPAPROptionVector sPAPROptionVector;
 /* option vector 5 */
 #define OV5_DRCONF_MEMORY   OV_BIT(2, 2)
 #define OV5_FORM1_AFFINITY  OV_BIT(5, 0)
+#define OV5_HP_EVT  OV_BIT(6, 5)
 
 /* interfaces */
 sPAPROptionVector *spapr_ovec_new(void);
-- 
1.9.1