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

[Eduardo Habkost <ehabkost@redhat.com>]

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