diff mbox

pc: fail memory hot-plug/unplug with -no-acpi and Q35 machine type

Message ID 20171222015120.31730-1-haozhong.zhang@intel.com (mailing list archive)
State New, archived
Headers show

Commit Message

Haozhong Zhang Dec. 22, 2017, 1:51 a.m. UTC
When -no-acpi option is used with Q35 machine type, no guest ACPI is
built, but the ACPI device is still created, so only checking the
presence of ACPI device before memory plug/unplug is not enough in
such cases. Check whether ACPI is disabled globally in addition and
fail memory plug/unplug if it's disabled.

Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com>
---
 hw/i386/pc.c | 18 ++++++++++++++----
 1 file changed, 14 insertions(+), 4 deletions(-)

Comments

Igor Mammedov Dec. 22, 2017, 3:17 p.m. UTC | #1
On Fri, 22 Dec 2017 09:51:20 +0800
Haozhong Zhang <haozhong.zhang@intel.com> wrote:

> When -no-acpi option is used with Q35 machine type, no guest ACPI is
> built, but the ACPI device is still created, so only checking the
> presence of ACPI device before memory plug/unplug is not enough in
> such cases. Check whether ACPI is disabled globally in addition and
> fail memory plug/unplug if it's disabled.
> 
> Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com>
we do not expose dimms in E820, so there is no point in allowing them
being added (hot or cold-plug) without acpi enabled as guest
won't be able to know where they are located.

Reviewed-by: Igor Mammedov <imammedo@redhat.com>

> ---
>  hw/i386/pc.c | 18 ++++++++++++++----
>  1 file changed, 14 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index 3fcf318a95..55686bf5d8 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -1695,9 +1695,14 @@ static void pc_dimm_plug(HotplugHandler *hotplug_dev,
>          align = memory_region_get_alignment(mr);
>      }
>  
> -    if (!pcms->acpi_dev) {
> +    /*
> +     * When -no-acpi is used with Q35 machine type, no ACPI is built,
> +     * but pcms->acpi_dev is still created. Check !acpi_enabled in
> +     * addition to cover this case.
> +     */
> +    if (!pcms->acpi_dev || !acpi_enabled) {
>          error_setg(&local_err,
> -                   "memory hotplug is not enabled: missing acpi device");
> +                   "memory hotplug is not enabled: missing acpi device or acpi disabled");
>          goto out;
>      }
>  
> @@ -1729,9 +1734,14 @@ static void pc_dimm_unplug_request(HotplugHandler *hotplug_dev,
>      Error *local_err = NULL;
>      PCMachineState *pcms = PC_MACHINE(hotplug_dev);
>  
> -    if (!pcms->acpi_dev) {
> +    /*
> +     * When -no-acpi is used with Q35 machine type, no ACPI is built,
> +     * but pcms->acpi_dev is still created. Check !acpi_enabled in
> +     * addition to cover this case.
> +     */
> +    if (!pcms->acpi_dev || !acpi_enabled) {
>          error_setg(&local_err,
> -                   "memory hotplug is not enabled: missing acpi device");
> +                   "memory hotplug is not enabled: missing acpi device or acpi disabled");
>          goto out;
>      }
>
Paolo Bonzini Dec. 22, 2017, 4:12 p.m. UTC | #2
On 22/12/2017 02:51, Haozhong Zhang wrote:
> When -no-acpi option is used with Q35 machine type, no guest ACPI is
> built, but the ACPI device is still created, so only checking the
> presence of ACPI device before memory plug/unplug is not enough in
> such cases. Check whether ACPI is disabled globally in addition and
> fail memory plug/unplug if it's disabled.
> 
> Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com>
> ---
>  hw/i386/pc.c | 18 ++++++++++++++----
>  1 file changed, 14 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index 3fcf318a95..55686bf5d8 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -1695,9 +1695,14 @@ static void pc_dimm_plug(HotplugHandler *hotplug_dev,
>          align = memory_region_get_alignment(mr);
>      }
>  
> -    if (!pcms->acpi_dev) {
> +    /*
> +     * When -no-acpi is used with Q35 machine type, no ACPI is built,
> +     * but pcms->acpi_dev is still created. Check !acpi_enabled in
> +     * addition to cover this case.
> +     */
> +    if (!pcms->acpi_dev || !acpi_enabled) {
>          error_setg(&local_err,
> -                   "memory hotplug is not enabled: missing acpi device");
> +                   "memory hotplug is not enabled: missing acpi device or acpi disabled");
>          goto out;
>      }
>  
> @@ -1729,9 +1734,14 @@ static void pc_dimm_unplug_request(HotplugHandler *hotplug_dev,
>      Error *local_err = NULL;
>      PCMachineState *pcms = PC_MACHINE(hotplug_dev);
>  
> -    if (!pcms->acpi_dev) {
> +    /*
> +     * When -no-acpi is used with Q35 machine type, no ACPI is built,
> +     * but pcms->acpi_dev is still created. Check !acpi_enabled in
> +     * addition to cover this case.
> +     */
> +    if (!pcms->acpi_dev || !acpi_enabled) {
>          error_setg(&local_err,
> -                   "memory hotplug is not enabled: missing acpi device");
> +                   "memory hotplug is not enabled: missing acpi device or acpi disabled");
>          goto out;
>      }
>  
> 

Queued, thanks.

Paolo
diff mbox

Patch

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 3fcf318a95..55686bf5d8 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -1695,9 +1695,14 @@  static void pc_dimm_plug(HotplugHandler *hotplug_dev,
         align = memory_region_get_alignment(mr);
     }
 
-    if (!pcms->acpi_dev) {
+    /*
+     * When -no-acpi is used with Q35 machine type, no ACPI is built,
+     * but pcms->acpi_dev is still created. Check !acpi_enabled in
+     * addition to cover this case.
+     */
+    if (!pcms->acpi_dev || !acpi_enabled) {
         error_setg(&local_err,
-                   "memory hotplug is not enabled: missing acpi device");
+                   "memory hotplug is not enabled: missing acpi device or acpi disabled");
         goto out;
     }
 
@@ -1729,9 +1734,14 @@  static void pc_dimm_unplug_request(HotplugHandler *hotplug_dev,
     Error *local_err = NULL;
     PCMachineState *pcms = PC_MACHINE(hotplug_dev);
 
-    if (!pcms->acpi_dev) {
+    /*
+     * When -no-acpi is used with Q35 machine type, no ACPI is built,
+     * but pcms->acpi_dev is still created. Check !acpi_enabled in
+     * addition to cover this case.
+     */
+    if (!pcms->acpi_dev || !acpi_enabled) {
         error_setg(&local_err,
-                   "memory hotplug is not enabled: missing acpi device");
+                   "memory hotplug is not enabled: missing acpi device or acpi disabled");
         goto out;
     }