[Qemu-devel] [PATCH RFC] PCMachineState: introduce acpi_build_enabled field

[Qemu-devel] [PATCH RFC] PCMachineState: introduce acpi_build_enabled field

[Wei Liu]

Introduce this field to control whether ACPI build is enabled by a
particular machine or accelerator.

It defaults to true so that PC machine has ACPI build by default. Xen
accelerator will disable it because Xen is in charge of building ACPI
tables for the guest.

Signed-off-by: Wei Liu <wei.liu2@citrix.com>
---
Cc: Igor Mammedov <imammedo@redhat.com>
Cc: Eduardo Habkost <ehabkost@redhat.com>
Cc: Anthony PERARD <anthony.perard@citrix.com>
Cc: Stefano Stabellini <sstabellini@kernel.org>
Cc: Sander Eikelenboom <linux@eikelenboom.it>

Tested a backport version which only involves trivial code movement. It
worked with both -m xenfv and -m pc,accel=xen.

Sander, if you want the backported patch please let me know.
---
 hw/i386/acpi-build.c |  2 +-
 hw/i386/pc.c         | 19 +++++++++++++++++++
 include/hw/i386/pc.h |  3 +++
 xen-common.c         |  6 ++++++
 4 files changed, 29 insertions(+), 1 deletion(-)

diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index 93be96f..a5cd2fd 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -2954,7 +2954,7 @@ void acpi_setup(void)
         return;
     }
 
-    if (!pcmc->has_acpi_build) {
+    if (!pcmc->has_acpi_build || !pcms->acpi_build_enabled) {
         ACPI_BUILD_DPRINTF("ACPI build disabled. Bailing out.\n");
         return;
     }
diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index f56ea0f..3e7982f 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -2143,6 +2143,20 @@ static bool pc_machine_get_nvdimm(Object *obj, Error **errp)
     return pcms->acpi_nvdimm_state.is_enabled;
 }
 
+static bool pc_machine_get_acpi_build(Object *obj, Error **errp)
+{
+    PCMachineState *pcms = PC_MACHINE(obj);
+
+    return pcms->acpi_build_enabled;
+}
+
+static void pc_machine_set_acpi_build(Object *obj, bool value, Error **errp)
+{
+    PCMachineState *pcms = PC_MACHINE(obj);
+
+    pcms->acpi_build_enabled = value;
+}
+
 static void pc_machine_set_nvdimm(Object *obj, bool value, Error **errp)
 {
     PCMachineState *pcms = PC_MACHINE(obj);
@@ -2159,6 +2173,8 @@ static void pc_machine_initfn(Object *obj)
     pcms->vmport = ON_OFF_AUTO_AUTO;
     /* nvdimm is disabled on default. */
     pcms->acpi_nvdimm_state.is_enabled = false;
+    /* acpi build is enabled by default. */
+    pcms->acpi_build_enabled = true;
 }
 
 static void pc_machine_reset(void)
@@ -2319,6 +2335,9 @@ static void pc_machine_class_init(ObjectClass *oc, void *data)
 
     object_class_property_add_bool(oc, PC_MACHINE_NVDIMM,
         pc_machine_get_nvdimm, pc_machine_set_nvdimm, &error_abort);
+
+    object_class_property_add_bool(oc, PC_MACHINE_ACPI_BUILD,
+        pc_machine_get_acpi_build, pc_machine_set_acpi_build, &error_abort);
 }
 
 static const TypeInfo pc_machine_info = {
diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
index 17fff80..ec8cd0c 100644
--- a/include/hw/i386/pc.h
+++ b/include/hw/i386/pc.h
@@ -63,6 +63,8 @@ struct PCMachineState {
 
     AcpiNVDIMMState acpi_nvdimm_state;
 
+    bool acpi_build_enabled;
+
     /* RAM information (sizes, addresses, configuration): */
     ram_addr_t below_4g_mem_size, above_4g_mem_size;
 
@@ -87,6 +89,7 @@ struct PCMachineState {
 #define PC_MACHINE_VMPORT           "vmport"
 #define PC_MACHINE_SMM              "smm"
 #define PC_MACHINE_NVDIMM           "nvdimm"
+#define PC_MACHINE_ACPI_BUILD       "acpi-build"
 
 /**
  * PCMachineClass:
diff --git a/xen-common.c b/xen-common.c
index e641ad1..b1858d7 100644
--- a/xen-common.c
+++ b/xen-common.c
@@ -9,6 +9,7 @@
  */
 
 #include "qemu/osdep.h"
+#include "hw/i386/pc.h"
 #include "hw/xen/xen_backend.h"
 #include "qmp-commands.h"
 #include "sysemu/char.h"
@@ -114,6 +115,11 @@ static void xen_change_state_handler(void *opaque, int running,
 
 static int xen_init(MachineState *ms)
 {
+    PCMachineState *pcms = PC_MACHINE(ms);
+
+    /* Disable ACPI build because Xen handles it */
+    pcms->acpi_build_enabled = false;
+
     xen_xc = xc_interface_open(0, 0, 0);
     if (xen_xc == NULL) {
         xen_be_printf(NULL, 0, "can't open xen interface\n");
-- 
2.1.4

Re: [Qemu-devel] [PATCH RFC] PCMachineState: introduce acpi_build_enabled field

[Eduardo Habkost]

On Tue, Nov 01, 2016 at 04:43:11PM +0000, Wei Liu wrote:
> Introduce this field to control whether ACPI build is enabled by a
> particular machine or accelerator.
> 
> It defaults to true so that PC machine has ACPI build by default. Xen
> accelerator will disable it because Xen is in charge of building ACPI
> tables for the guest.
> 
> Signed-off-by: Wei Liu <wei.liu2@citrix.com>
> ---
> Cc: Igor Mammedov <imammedo@redhat.com>
> Cc: Eduardo Habkost <ehabkost@redhat.com>
> Cc: Anthony PERARD <anthony.perard@citrix.com>
> Cc: Stefano Stabellini <sstabellini@kernel.org>
> Cc: Sander Eikelenboom <linux@eikelenboom.it>
> 
> Tested a backport version which only involves trivial code movement. It
> worked with both -m xenfv and -m pc,accel=xen.
> 
> Sander, if you want the backported patch please let me know.
> ---
>  hw/i386/acpi-build.c |  2 +-
>  hw/i386/pc.c         | 19 +++++++++++++++++++
>  include/hw/i386/pc.h |  3 +++
>  xen-common.c         |  6 ++++++
>  4 files changed, 29 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index 93be96f..a5cd2fd 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -2954,7 +2954,7 @@ void acpi_setup(void)
>          return;
>      }
>  
> -    if (!pcmc->has_acpi_build) {
> +    if (!pcmc->has_acpi_build || !pcms->acpi_build_enabled) {
>          ACPI_BUILD_DPRINTF("ACPI build disabled. Bailing out.\n");
>          return;
>      }
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index f56ea0f..3e7982f 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -2143,6 +2143,20 @@ static bool pc_machine_get_nvdimm(Object *obj, Error **errp)
>      return pcms->acpi_nvdimm_state.is_enabled;
>  }
>  
> +static bool pc_machine_get_acpi_build(Object *obj, Error **errp)
> +{
> +    PCMachineState *pcms = PC_MACHINE(obj);
> +
> +    return pcms->acpi_build_enabled;
> +}
> +
> +static void pc_machine_set_acpi_build(Object *obj, bool value, Error **errp)
> +{
> +    PCMachineState *pcms = PC_MACHINE(obj);
> +
> +    pcms->acpi_build_enabled = value;
> +}
> +
>  static void pc_machine_set_nvdimm(Object *obj, bool value, Error **errp)
>  {
>      PCMachineState *pcms = PC_MACHINE(obj);
> @@ -2159,6 +2173,8 @@ static void pc_machine_initfn(Object *obj)
>      pcms->vmport = ON_OFF_AUTO_AUTO;
>      /* nvdimm is disabled on default. */
>      pcms->acpi_nvdimm_state.is_enabled = false;
> +    /* acpi build is enabled by default. */
> +    pcms->acpi_build_enabled = true;

If you set:
  pcms->acpi_build_enabled = PC_MACHINE_GET_CLASS(pcms)->has_acpi_build;
the default value will be more consistent with the actual results
(where pc-1.6 and older don't have ACPI build). Then you would
probably be able to remove the pcmc->has_acpi_build check from
acpi_setup() and only check pcms->acpi_build_enabled.

>  }
>  
>  static void pc_machine_reset(void)
> @@ -2319,6 +2335,9 @@ static void pc_machine_class_init(ObjectClass *oc, void *data)
>  
>      object_class_property_add_bool(oc, PC_MACHINE_NVDIMM,
>          pc_machine_get_nvdimm, pc_machine_set_nvdimm, &error_abort);
> +
> +    object_class_property_add_bool(oc, PC_MACHINE_ACPI_BUILD,
> +        pc_machine_get_acpi_build, pc_machine_set_acpi_build, &error_abort);
>  }
>  
>  static const TypeInfo pc_machine_info = {
> diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
> index 17fff80..ec8cd0c 100644
> --- a/include/hw/i386/pc.h
> +++ b/include/hw/i386/pc.h
> @@ -63,6 +63,8 @@ struct PCMachineState {
>  
>      AcpiNVDIMMState acpi_nvdimm_state;
>  
> +    bool acpi_build_enabled;
> +
>      /* RAM information (sizes, addresses, configuration): */
>      ram_addr_t below_4g_mem_size, above_4g_mem_size;
>  
> @@ -87,6 +89,7 @@ struct PCMachineState {
>  #define PC_MACHINE_VMPORT           "vmport"
>  #define PC_MACHINE_SMM              "smm"
>  #define PC_MACHINE_NVDIMM           "nvdimm"
> +#define PC_MACHINE_ACPI_BUILD       "acpi-build"
>  
>  /**
>   * PCMachineClass:
> diff --git a/xen-common.c b/xen-common.c
> index e641ad1..b1858d7 100644
> --- a/xen-common.c
> +++ b/xen-common.c
> @@ -9,6 +9,7 @@
>   */
>  
>  #include "qemu/osdep.h"
> +#include "hw/i386/pc.h"
>  #include "hw/xen/xen_backend.h"
>  #include "qmp-commands.h"
>  #include "sysemu/char.h"
> @@ -114,6 +115,11 @@ static void xen_change_state_handler(void *opaque, int running,
>  
>  static int xen_init(MachineState *ms)
>  {
> +    PCMachineState *pcms = PC_MACHINE(ms);
> +
> +    /* Disable ACPI build because Xen handles it */
> +    pcms->acpi_build_enabled = false;
> +
>      xen_xc = xc_interface_open(0, 0, 0);
>      if (xen_xc == NULL) {
>          xen_be_printf(NULL, 0, "can't open xen interface\n");
> -- 
> 2.1.4
> 

-- 
Eduardo

Re: [Qemu-devel] [PATCH RFC] PCMachineState: introduce acpi_build_enabled field

[Wei Liu]

On Tue, Nov 01, 2016 at 02:48:27PM -0200, Eduardo Habkost wrote:
[...]
> >  static void pc_machine_set_nvdimm(Object *obj, bool value, Error **errp)
> >  {
> >      PCMachineState *pcms = PC_MACHINE(obj);
> > @@ -2159,6 +2173,8 @@ static void pc_machine_initfn(Object *obj)
> >      pcms->vmport = ON_OFF_AUTO_AUTO;
> >      /* nvdimm is disabled on default. */
> >      pcms->acpi_nvdimm_state.is_enabled = false;
> > +    /* acpi build is enabled by default. */
> > +    pcms->acpi_build_enabled = true;
> 
> If you set:
>   pcms->acpi_build_enabled = PC_MACHINE_GET_CLASS(pcms)->has_acpi_build;
> the default value will be more consistent with the actual results
> (where pc-1.6 and older don't have ACPI build). Then you would
> probably be able to remove the pcmc->has_acpi_build check from
> acpi_setup() and only check pcms->acpi_build_enabled.
> 

Thank you for your good advice.

I take it that you're ok with the name of the field and the code in
general? If so I will drop RFC tag in my next submission.

Wei.

Re: [Qemu-devel] [PATCH RFC] PCMachineState: introduce acpi_build_enabled field

[Eduardo Habkost]

On Tue, Nov 01, 2016 at 04:53:17PM +0000, Wei Liu wrote:
> On Tue, Nov 01, 2016 at 02:48:27PM -0200, Eduardo Habkost wrote:
> [...]
> > >  static void pc_machine_set_nvdimm(Object *obj, bool value, Error **errp)
> > >  {
> > >      PCMachineState *pcms = PC_MACHINE(obj);
> > > @@ -2159,6 +2173,8 @@ static void pc_machine_initfn(Object *obj)
> > >      pcms->vmport = ON_OFF_AUTO_AUTO;
> > >      /* nvdimm is disabled on default. */
> > >      pcms->acpi_nvdimm_state.is_enabled = false;
> > > +    /* acpi build is enabled by default. */
> > > +    pcms->acpi_build_enabled = true;
> > 
> > If you set:
> >   pcms->acpi_build_enabled = PC_MACHINE_GET_CLASS(pcms)->has_acpi_build;
> > the default value will be more consistent with the actual results
> > (where pc-1.6 and older don't have ACPI build). Then you would
> > probably be able to remove the pcmc->has_acpi_build check from
> > acpi_setup() and only check pcms->acpi_build_enabled.
> > 
> 
> Thank you for your good advice.
> 
> I take it that you're ok with the name of the field and the code in
> general? If so I will drop RFC tag in my next submission.

The rest looks good to me, except that I don't see a reason to
add a "acpi-build" property if it's not used for anything (all
code is using the struct field directly).

-- 
Eduardo

Re: [Qemu-devel] [PATCH RFC] PCMachineState: introduce acpi_build_enabled field

[Wei Liu]

On Tue, Nov 01, 2016 at 02:59:25PM -0200, Eduardo Habkost wrote:
> On Tue, Nov 01, 2016 at 04:53:17PM +0000, Wei Liu wrote:
> > On Tue, Nov 01, 2016 at 02:48:27PM -0200, Eduardo Habkost wrote:
> > [...]
> > > >  static void pc_machine_set_nvdimm(Object *obj, bool value, Error **errp)
> > > >  {
> > > >      PCMachineState *pcms = PC_MACHINE(obj);
> > > > @@ -2159,6 +2173,8 @@ static void pc_machine_initfn(Object *obj)
> > > >      pcms->vmport = ON_OFF_AUTO_AUTO;
> > > >      /* nvdimm is disabled on default. */
> > > >      pcms->acpi_nvdimm_state.is_enabled = false;
> > > > +    /* acpi build is enabled by default. */
> > > > +    pcms->acpi_build_enabled = true;
> > > 
> > > If you set:
> > >   pcms->acpi_build_enabled = PC_MACHINE_GET_CLASS(pcms)->has_acpi_build;
> > > the default value will be more consistent with the actual results
> > > (where pc-1.6 and older don't have ACPI build). Then you would
> > > probably be able to remove the pcmc->has_acpi_build check from
> > > acpi_setup() and only check pcms->acpi_build_enabled.
> > > 
> > 
> > Thank you for your good advice.
> > 
> > I take it that you're ok with the name of the field and the code in
> > general? If so I will drop RFC tag in my next submission.
> 
> The rest looks good to me, except that I don't see a reason to
> add a "acpi-build" property if it's not used for anything (all
> code is using the struct field directly).
> 

OK. I'm fine with deleting that. I was just following existing examples.

Wei.

> -- 
> Eduardo

Re: [Qemu-devel] [PATCH RFC] PCMachineState: introduce acpi_build_enabled field

[Eduardo Habkost]

On Tue, Nov 01, 2016 at 04:43:11PM +0000, Wei Liu wrote:
[...]
> @@ -114,6 +115,11 @@ static void xen_change_state_handler(void *opaque, int running,
>  
>  static int xen_init(MachineState *ms)
>  {
> +    PCMachineState *pcms = PC_MACHINE(ms);
> +
> +    /* Disable ACPI build because Xen handles it */
> +    pcms->acpi_build_enabled = false;

I just noticed that I don't see any code to disable ACPI build in
the case of "-machine pc,accel=xen". I suggest a xen_enabled()
check in pc_init1() and pc_q35_init().

-- 
Eduardo

Re: [Qemu-devel] [PATCH RFC] PCMachineState: introduce acpi_build_enabled field

[Wei Liu]

On Tue, Nov 01, 2016 at 03:02:31PM -0200, Eduardo Habkost wrote:
> On Tue, Nov 01, 2016 at 04:43:11PM +0000, Wei Liu wrote:
> [...]
> > @@ -114,6 +115,11 @@ static void xen_change_state_handler(void *opaque, int running,
> >  
> >  static int xen_init(MachineState *ms)
> >  {
> > +    PCMachineState *pcms = PC_MACHINE(ms);
> > +
> > +    /* Disable ACPI build because Xen handles it */
> > +    pcms->acpi_build_enabled = false;
> 
> I just noticed that I don't see any code to disable ACPI build in
> the case of "-machine pc,accel=xen". I suggest a xen_enabled()

Hmm... I think this code snippet does exactly that -- xen_init is the
initialization function for Xen accelerator. So this covers -m xenfv
(because it sets accelerator to xen) and -m whatever,accel=xen.

Did I miss anything?

Wei.

Re: [Qemu-devel] [PATCH RFC] PCMachineState: introduce acpi_build_enabled field

[Eduardo Habkost]

On Tue, Nov 01, 2016 at 05:12:37PM +0000, Wei Liu wrote:
> On Tue, Nov 01, 2016 at 03:02:31PM -0200, Eduardo Habkost wrote:
> > On Tue, Nov 01, 2016 at 04:43:11PM +0000, Wei Liu wrote:
> > [...]
> > > @@ -114,6 +115,11 @@ static void xen_change_state_handler(void *opaque, int running,
> > >  
> > >  static int xen_init(MachineState *ms)
> > >  {
> > > +    PCMachineState *pcms = PC_MACHINE(ms);
> > > +
> > > +    /* Disable ACPI build because Xen handles it */
> > > +    pcms->acpi_build_enabled = false;
> > 
> > I just noticed that I don't see any code to disable ACPI build in
> > the case of "-machine pc,accel=xen". I suggest a xen_enabled()
> 
> Hmm... I think this code snippet does exactly that -- xen_init is the
> initialization function for Xen accelerator. So this covers -m xenfv
> (because it sets accelerator to xen) and -m whatever,accel=xen.
> 
> Did I miss anything?

Nevermind, I confused it with pc_xen_hvm_init() (which is
specific for xenfv). The patch looks correct.

-- 
Eduardo