[Qemu-devel] [PATCH v2 0/2] spapr: disable hotplugging without OS

[Qemu-devel] [PATCH v2 0/2] spapr: disable hotplugging without OS

[Laurent Vivier]

If the OS is not started, QEMU sends an event to the OS
that is lost and cannot be recovered. An unplug is not
able to restore QEMU in a coherent state.
So, while the OS is not started, disable CPU and memory hotplug.
We guess the OS is started if the CAS has been negotiated.

This series also revert previous fix which was not really fixing
the hotplug problem when the OS is not running.

v2:
- fix indent
- remove useless local_err
- allow hotplug if the VM is not started (pre-launch or incoming state)
- remove vector 6, instead just mark the end of CAS negotiation

Laurent Vivier (2):
  spapr: disable hotplugging without OS
  Revert "spapr: fix memory hot-unplugging"

 hw/ppc/spapr.c             | 51 +++++++++++++++++++++++++++++++++++++++++++---
 hw/ppc/spapr_drc.c         | 20 +++---------------
 hw/ppc/spapr_hcall.c       |  1 +
 include/hw/ppc/spapr.h     |  2 ++
 include/hw/ppc/spapr_drc.h |  1 -
 5 files changed, 54 insertions(+), 21 deletions(-)

-- 
2.9.4

[Qemu-devel] [PATCH v2 1/2] spapr: disable hotplugging without OS

[Laurent Vivier]

If the OS is not started, QEMU sends an event to the OS
that is lost and cannot be recovered. An unplug is not
able to restore QEMU in a coherent state.
So, while the OS is not started, disable CPU and memory hotplug.
We guess the OS is started if the CAS has been negotiated.

Signed-off-by: Laurent Vivier <lvivier@redhat.com>
---
 hw/ppc/spapr.c         | 51 +++++++++++++++++++++++++++++++++++++++++++++++---
 hw/ppc/spapr_hcall.c   |  1 +
 include/hw/ppc/spapr.h |  2 ++
 3 files changed, 51 insertions(+), 3 deletions(-)

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 91b4057..4c979d5 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -1308,6 +1308,7 @@ static void ppc_spapr_reset(void)
 {
     MachineState *machine = MACHINE(qdev_get_machine());
     sPAPRMachineState *spapr = SPAPR_MACHINE(machine);
+    sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(machine);
     PowerPCCPU *first_ppc_cpu;
     uint32_t rtas_limit;
     hwaddr rtas_addr, fdt_addr;
@@ -1373,6 +1374,7 @@ static void ppc_spapr_reset(void)
     first_ppc_cpu->env.nip = SPAPR_ENTRY_POINT;
 
     spapr->cas_reboot = false;
+    spapr->cas_completed = smc->cas_completed_default;
 }
 
 static void spapr_create_nvram(sPAPRMachineState *spapr)
@@ -1528,6 +1530,27 @@ static const VMStateDescription vmstate_spapr_patb_entry = {
     },
 };
 
+static bool spapr_cas_completed_needed(void *opaque)
+{
+    sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(opaque);
+
+    /* we need to migrate cas_completed only if it is
+     * not set by default
+     */
+    return !smc->cas_completed_default;
+}
+
+static const VMStateDescription vmstate_spapr_cas_completed = {
+    .name = "spapr_cas_completed",
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .needed = spapr_cas_completed_needed,
+    .fields = (VMStateField[]) {
+        VMSTATE_BOOL(cas_completed, sPAPRMachineState),
+        VMSTATE_END_OF_LIST()
+    },
+};
+
 static const VMStateDescription vmstate_spapr = {
     .name = "spapr",
     .version_id = 3,
@@ -1546,6 +1569,7 @@ static const VMStateDescription vmstate_spapr = {
     .subsections = (const VMStateDescription*[]) {
         &vmstate_spapr_ov5_cas,
         &vmstate_spapr_patb_entry,
+        &vmstate_spapr_cas_completed,
         NULL
     }
 };
@@ -2599,6 +2623,7 @@ out:
 static void spapr_memory_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
                                   Error **errp)
 {
+    sPAPRMachineState *ms = SPAPR_MACHINE(hotplug_dev);
     PCDIMMDevice *dimm = PC_DIMM(dev);
     PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(dimm);
     MemoryRegion *mr = ddc->get_memory_region(dimm);
@@ -2617,6 +2642,14 @@ static void spapr_memory_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
                    "Use 'memory-backend-file' with correct mem-path.");
         return;
     }
+    if (dev->hotplugged) {
+        if (!runstate_check(RUN_STATE_PRELAUNCH) &&
+            !runstate_check(RUN_STATE_INMIGRATE) &&
+            !ms->cas_completed) {
+            error_setg(errp, "Memory hotplug not supported without OS");
+            return;
+        }
+    }
 }
 
 struct sPAPRDIMMState {
@@ -2915,6 +2948,7 @@ static void spapr_core_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
                                 Error **errp)
 {
     MachineState *machine = MACHINE(OBJECT(hotplug_dev));
+    sPAPRMachineState *ms = SPAPR_MACHINE(machine);
     MachineClass *mc = MACHINE_GET_CLASS(hotplug_dev);
     Error *local_err = NULL;
     CPUCore *cc = CPU_CORE(dev);
@@ -2923,9 +2957,18 @@ static void spapr_core_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
     CPUArchId *core_slot;
     int index;
 
-    if (dev->hotplugged && !mc->has_hotpluggable_cpus) {
-        error_setg(&local_err, "CPU hotplug not supported for this machine");
-        goto out;
+    if (dev->hotplugged) {
+        if (!mc->has_hotpluggable_cpus) {
+            error_setg(&local_err,
+                       "CPU hotplug not supported for this machine");
+            goto out;
+        }
+        if (!runstate_check(RUN_STATE_PRELAUNCH) &&
+            !runstate_check(RUN_STATE_INMIGRATE) &&
+            !ms->cas_completed) {
+            error_setg(&local_err, "CPU hotplug not supported without OS");
+            goto out;
+        }
     }
 
     if (strcmp(base_core_type, type)) {
@@ -3358,9 +3401,11 @@ static void spapr_machine_2_9_instance_options(MachineState *machine)
 
 static void spapr_machine_2_9_class_options(MachineClass *mc)
 {
+    sPAPRMachineClass *smc = SPAPR_MACHINE_CLASS(mc);
     spapr_machine_2_10_class_options(mc);
     SET_MACHINE_COMPAT(mc, SPAPR_COMPAT_2_9);
     mc->numa_auto_assign_ram = numa_legacy_auto_assign_ram;
+    smc->cas_completed_default = true;
 }
 
 DEFINE_SPAPR_MACHINE(2_9, "2.9", false);
diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
index aa1ffea..d561a00 100644
--- a/hw/ppc/spapr_hcall.c
+++ b/hw/ppc/spapr_hcall.c
@@ -1185,6 +1185,7 @@ static target_ulong h_client_architecture_support(PowerPCCPU *cpu,
         }
     }
 
+    spapr->cas_completed = true;
     return H_SUCCESS;
 }
 
diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
index f973b02..f5835db 100644
--- a/include/hw/ppc/spapr.h
+++ b/include/hw/ppc/spapr.h
@@ -57,6 +57,7 @@ struct sPAPRMachineClass {
                           uint64_t *buid, hwaddr *pio, 
                           hwaddr *mmio32, hwaddr *mmio64,
                           unsigned n_dma, uint32_t *liobns, Error **errp);
+    bool cas_completed_default;
 };
 
 /**
@@ -90,6 +91,7 @@ struct sPAPRMachineState {
     sPAPROptionVector *ov5_cas;     /* negotiated (via CAS) option vectors */
     bool cas_reboot;
     bool cas_legacy_guest_workaround;
+    bool cas_completed;
 
     Notifier epow_notifier;
     QTAILQ_HEAD(, sPAPREventLogEntry) pending_events;
-- 
2.9.4

[Qemu-devel] [PATCH v2 2/2] Revert "spapr: fix memory hot-unplugging"

[Laurent Vivier]

This reverts commit fe6824d12642b005c69123ecf8631f9b13553f8b.

Conflicts hw/ppc/spapr_drc.c, because get_index() has been renamed
spapr_get_index().

This didn't fix the problem. Once the hotplug has been started
some memory is allocated and some structures are allocated.
We don't free it when we ignore the unplug, and we can't because
they can be in use by the kernel.

Signed-off-by: Laurent Vivier <lvivier@redhat.com>
---
 hw/ppc/spapr_drc.c         | 20 +++-----------------
 include/hw/ppc/spapr_drc.h |  1 -
 2 files changed, 3 insertions(+), 18 deletions(-)

diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
index 39e7f30..7605977 100644
--- a/hw/ppc/spapr_drc.c
+++ b/hw/ppc/spapr_drc.c
@@ -140,17 +140,6 @@ static uint32_t set_allocation_state(sPAPRDRConnector *drc,
         if (!drc->dev) {
             return RTAS_OUT_NO_SUCH_INDICATOR;
         }
-        if (drc->awaiting_release && drc->awaiting_allocation) {
-            /* kernel is acknowledging a previous hotplug event
-             * while we are already removing it.
-             * it's safe to ignore awaiting_allocation here since we know the
-             * situation is predicated on the guest either already having done
-             * so (boot-time hotplug), or never being able to acquire in the
-             * first place (hotplug followed by immediate unplug).
-             */
-            drc->awaiting_allocation_skippable = true;
-            return RTAS_OUT_NO_SUCH_INDICATOR;
-        }
     }
 
     if (spapr_drc_type(drc) != SPAPR_DR_CONNECTOR_TYPE_PCI) {
@@ -401,11 +390,9 @@ static void detach(sPAPRDRConnector *drc, DeviceState *d, Error **errp)
     }
 
     if (drc->awaiting_allocation) {
-        if (!drc->awaiting_allocation_skippable) {
-            drc->awaiting_release = true;
-            trace_spapr_drc_awaiting_allocation(spapr_drc_index(drc));
-            return;
-        }
+        drc->awaiting_release = true;
+        trace_spapr_drc_awaiting_allocation(spapr_drc_index(drc));
+        return;
     }
 
     drc->indicator_state = SPAPR_DR_INDICATOR_STATE_INACTIVE;
@@ -428,7 +415,6 @@ static void detach(sPAPRDRConnector *drc, DeviceState *d, Error **errp)
     }
 
     drc->awaiting_release = false;
-    drc->awaiting_allocation_skippable = false;
     g_free(drc->fdt);
     drc->fdt = NULL;
     drc->fdt_start_offset = 0;
diff --git a/include/hw/ppc/spapr_drc.h b/include/hw/ppc/spapr_drc.h
index c88e1be..84b58f0 100644
--- a/include/hw/ppc/spapr_drc.h
+++ b/include/hw/ppc/spapr_drc.h
@@ -200,7 +200,6 @@ typedef struct sPAPRDRConnector {
     bool awaiting_release;
     bool signalled;
     bool awaiting_allocation;
-    bool awaiting_allocation_skippable;
 
     /* device pointer, via link property */
     DeviceState *dev;
-- 
2.9.4

Re: [Qemu-devel] [Qemu-ppc] [PATCH v2 0/2] spapr: disable hotplugging without OS

[Daniel Henrique Barboza]

On 06/08/2017 02:27 PM, Laurent Vivier wrote:
> If the OS is not started, QEMU sends an event to the OS
> that is lost and cannot be recovered. An unplug is not
> able to restore QEMU in a coherent state.
> So, while the OS is not started, disable CPU and memory hotplug.
> We guess the OS is started if the CAS has been negotiated.
>
> This series also revert previous fix which was not really fixing
> the hotplug problem when the OS is not running.
>
> v2:
> - fix indent
> - remove useless local_err
> - allow hotplug if the VM is not started (pre-launch or incoming state)
> - remove vector 6, instead just mark the end of CAS negotiation
>
> Laurent Vivier (2):
>    spapr: disable hotplugging without OS
>    Revert "spapr: fix memory hot-unplugging"
>
>   hw/ppc/spapr.c             | 51 +++++++++++++++++++++++++++++++++++++++++++---
>   hw/ppc/spapr_drc.c         | 20 +++---------------
>   hw/ppc/spapr_hcall.c       |  1 +
>   include/hw/ppc/spapr.h     |  2 ++
>   include/hw/ppc/spapr_drc.h |  1 -
>   5 files changed, 54 insertions(+), 21 deletions(-)
>
Tested-by: Daniel Barboza <danielhb@linux.vnet.ibm.com>

This is curious. I was having a little look into hotplug/unplug and DRC 
states yesterday
while taking a look at the latest David's cleanup. I was trying to find 
out a way to tune
spapr_drc_needed() in a way that we don't accidentally migrate more DRCs 
than necessary
and at the same time handle that scenario of libvirt migration after 
hotplug (libvirt hotplugs
the device on target instead of adding it in the command line).

I would like to migrate the DRCs if and only if:

1 - a device was hotplugged
2 - a device is undergoing hotplug/unplug

(2) is easy and is already taken care of. But (1) is tricky because, 
before this patch, if a migration
occurs *before* CAS we don't need to migrate the DRC - the object will 
go through the state
changes after the migration. With this series this scenario is not going 
to happen, then
we can happily add a

if (dev->hotplugged) return true;

in spapr_drc_needed and fix the libvirt migration scenario again.



Daniel

Re: [Qemu-devel] [PATCH v2 2/2] Revert "spapr: fix memory hot-unplugging"

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 3401 bytes --]

On Thu, Jun 08, 2017 at 07:27:43PM +0200, Laurent Vivier wrote:
> This reverts commit fe6824d12642b005c69123ecf8631f9b13553f8b.
> 
> Conflicts hw/ppc/spapr_drc.c, because get_index() has been renamed
> spapr_get_index().
> 
> This didn't fix the problem. Once the hotplug has been started
> some memory is allocated and some structures are allocated.
> We don't free it when we ignore the unplug, and we can't because
> they can be in use by the kernel.
> 
> Signed-off-by: Laurent Vivier <lvivier@redhat.com>

Heh.  I've just been looking at awaiting_allocation_skippable and
trying - but failing - to work out what it accomplishes.  Applied to
ppc-for-2.10.

> ---
>  hw/ppc/spapr_drc.c         | 20 +++-----------------
>  include/hw/ppc/spapr_drc.h |  1 -
>  2 files changed, 3 insertions(+), 18 deletions(-)
> 
> diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
> index 39e7f30..7605977 100644
> --- a/hw/ppc/spapr_drc.c
> +++ b/hw/ppc/spapr_drc.c
> @@ -140,17 +140,6 @@ static uint32_t set_allocation_state(sPAPRDRConnector *drc,
>          if (!drc->dev) {
>              return RTAS_OUT_NO_SUCH_INDICATOR;
>          }
> -        if (drc->awaiting_release && drc->awaiting_allocation) {
> -            /* kernel is acknowledging a previous hotplug event
> -             * while we are already removing it.
> -             * it's safe to ignore awaiting_allocation here since we know the
> -             * situation is predicated on the guest either already having done
> -             * so (boot-time hotplug), or never being able to acquire in the
> -             * first place (hotplug followed by immediate unplug).
> -             */
> -            drc->awaiting_allocation_skippable = true;
> -            return RTAS_OUT_NO_SUCH_INDICATOR;
> -        }
>      }
>  
>      if (spapr_drc_type(drc) != SPAPR_DR_CONNECTOR_TYPE_PCI) {
> @@ -401,11 +390,9 @@ static void detach(sPAPRDRConnector *drc, DeviceState *d, Error **errp)
>      }
>  
>      if (drc->awaiting_allocation) {
> -        if (!drc->awaiting_allocation_skippable) {
> -            drc->awaiting_release = true;
> -            trace_spapr_drc_awaiting_allocation(spapr_drc_index(drc));
> -            return;
> -        }
> +        drc->awaiting_release = true;
> +        trace_spapr_drc_awaiting_allocation(spapr_drc_index(drc));
> +        return;
>      }
>  
>      drc->indicator_state = SPAPR_DR_INDICATOR_STATE_INACTIVE;
> @@ -428,7 +415,6 @@ static void detach(sPAPRDRConnector *drc, DeviceState *d, Error **errp)
>      }
>  
>      drc->awaiting_release = false;
> -    drc->awaiting_allocation_skippable = false;
>      g_free(drc->fdt);
>      drc->fdt = NULL;
>      drc->fdt_start_offset = 0;
> diff --git a/include/hw/ppc/spapr_drc.h b/include/hw/ppc/spapr_drc.h
> index c88e1be..84b58f0 100644
> --- a/include/hw/ppc/spapr_drc.h
> +++ b/include/hw/ppc/spapr_drc.h
> @@ -200,7 +200,6 @@ typedef struct sPAPRDRConnector {
>      bool awaiting_release;
>      bool signalled;
>      bool awaiting_allocation;
> -    bool awaiting_allocation_skippable;
>  
>      /* device pointer, via link property */
>      DeviceState *dev;

-- 
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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [Qemu-devel] [PATCH v2 1/2] spapr: disable hotplugging without OS

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 7080 bytes --]

On Thu, Jun 08, 2017 at 07:27:42PM +0200, Laurent Vivier wrote:
> If the OS is not started, QEMU sends an event to the OS
> that is lost and cannot be recovered. An unplug is not
> able to restore QEMU in a coherent state.
> So, while the OS is not started, disable CPU and memory hotplug.
> We guess the OS is started if the CAS has been negotiated.
> 
> Signed-off-by: Laurent Vivier <lvivier@redhat.com>

It seems a pain to introduce a whole new (migrated) variable just to
check this.  Could we instead tweak the allocation of spapr->ov5_cas,
so it is NULL until CAS is completed?

> ---
>  hw/ppc/spapr.c         | 51 +++++++++++++++++++++++++++++++++++++++++++++++---
>  hw/ppc/spapr_hcall.c   |  1 +
>  include/hw/ppc/spapr.h |  2 ++
>  3 files changed, 51 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 91b4057..4c979d5 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -1308,6 +1308,7 @@ static void ppc_spapr_reset(void)
>  {
>      MachineState *machine = MACHINE(qdev_get_machine());
>      sPAPRMachineState *spapr = SPAPR_MACHINE(machine);
> +    sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(machine);
>      PowerPCCPU *first_ppc_cpu;
>      uint32_t rtas_limit;
>      hwaddr rtas_addr, fdt_addr;
> @@ -1373,6 +1374,7 @@ static void ppc_spapr_reset(void)
>      first_ppc_cpu->env.nip = SPAPR_ENTRY_POINT;
>  
>      spapr->cas_reboot = false;
> +    spapr->cas_completed = smc->cas_completed_default;

I also dislike the cas_completed_default thing.  If
cas_completed_default != false, it means that we will have
cas_completed == true when we have not, in fact, completed CAS.  I see
why you're doing this for migration compat, but that seems like a
recipe for confusion in the future.

>  }
>  
>  static void spapr_create_nvram(sPAPRMachineState *spapr)
> @@ -1528,6 +1530,27 @@ static const VMStateDescription vmstate_spapr_patb_entry = {
>      },
>  };
>  
> +static bool spapr_cas_completed_needed(void *opaque)
> +{
> +    sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(opaque);
> +
> +    /* we need to migrate cas_completed only if it is
> +     * not set by default
> +     */
> +    return !smc->cas_completed_default;
> +}
> +
> +static const VMStateDescription vmstate_spapr_cas_completed = {
> +    .name = "spapr_cas_completed",
> +    .version_id = 1,
> +    .minimum_version_id = 1,
> +    .needed = spapr_cas_completed_needed,
> +    .fields = (VMStateField[]) {
> +        VMSTATE_BOOL(cas_completed, sPAPRMachineState),
> +        VMSTATE_END_OF_LIST()
> +    },
> +};
> +
>  static const VMStateDescription vmstate_spapr = {
>      .name = "spapr",
>      .version_id = 3,
> @@ -1546,6 +1569,7 @@ static const VMStateDescription vmstate_spapr = {
>      .subsections = (const VMStateDescription*[]) {
>          &vmstate_spapr_ov5_cas,
>          &vmstate_spapr_patb_entry,
> +        &vmstate_spapr_cas_completed,
>          NULL
>      }
>  };
> @@ -2599,6 +2623,7 @@ out:
>  static void spapr_memory_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
>                                    Error **errp)
>  {
> +    sPAPRMachineState *ms = SPAPR_MACHINE(hotplug_dev);
>      PCDIMMDevice *dimm = PC_DIMM(dev);
>      PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(dimm);
>      MemoryRegion *mr = ddc->get_memory_region(dimm);
> @@ -2617,6 +2642,14 @@ static void spapr_memory_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
>                     "Use 'memory-backend-file' with correct mem-path.");
>          return;
>      }
> +    if (dev->hotplugged) {
> +        if (!runstate_check(RUN_STATE_PRELAUNCH) &&
> +            !runstate_check(RUN_STATE_INMIGRATE) &&
> +            !ms->cas_completed) {
> +            error_setg(errp, "Memory hotplug not supported without OS");
> +            return;
> +        }
> +    }
>  }
>  
>  struct sPAPRDIMMState {
> @@ -2915,6 +2948,7 @@ static void spapr_core_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
>                                  Error **errp)
>  {
>      MachineState *machine = MACHINE(OBJECT(hotplug_dev));
> +    sPAPRMachineState *ms = SPAPR_MACHINE(machine);
>      MachineClass *mc = MACHINE_GET_CLASS(hotplug_dev);
>      Error *local_err = NULL;
>      CPUCore *cc = CPU_CORE(dev);
> @@ -2923,9 +2957,18 @@ static void spapr_core_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
>      CPUArchId *core_slot;
>      int index;
>  
> -    if (dev->hotplugged && !mc->has_hotpluggable_cpus) {
> -        error_setg(&local_err, "CPU hotplug not supported for this machine");
> -        goto out;
> +    if (dev->hotplugged) {
> +        if (!mc->has_hotpluggable_cpus) {
> +            error_setg(&local_err,
> +                       "CPU hotplug not supported for this machine");
> +            goto out;
> +        }
> +        if (!runstate_check(RUN_STATE_PRELAUNCH) &&
> +            !runstate_check(RUN_STATE_INMIGRATE) &&
> +            !ms->cas_completed) {
> +            error_setg(&local_err, "CPU hotplug not supported without OS");
> +            goto out;
> +        }
>      }
>  
>      if (strcmp(base_core_type, type)) {
> @@ -3358,9 +3401,11 @@ static void spapr_machine_2_9_instance_options(MachineState *machine)
>  
>  static void spapr_machine_2_9_class_options(MachineClass *mc)
>  {
> +    sPAPRMachineClass *smc = SPAPR_MACHINE_CLASS(mc);
>      spapr_machine_2_10_class_options(mc);
>      SET_MACHINE_COMPAT(mc, SPAPR_COMPAT_2_9);
>      mc->numa_auto_assign_ram = numa_legacy_auto_assign_ram;
> +    smc->cas_completed_default = true;
>  }
>  
>  DEFINE_SPAPR_MACHINE(2_9, "2.9", false);
> diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
> index aa1ffea..d561a00 100644
> --- a/hw/ppc/spapr_hcall.c
> +++ b/hw/ppc/spapr_hcall.c
> @@ -1185,6 +1185,7 @@ static target_ulong h_client_architecture_support(PowerPCCPU *cpu,
>          }
>      }
>  
> +    spapr->cas_completed = true;
>      return H_SUCCESS;
>  }
>  
> diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
> index f973b02..f5835db 100644
> --- a/include/hw/ppc/spapr.h
> +++ b/include/hw/ppc/spapr.h
> @@ -57,6 +57,7 @@ struct sPAPRMachineClass {
>                            uint64_t *buid, hwaddr *pio, 
>                            hwaddr *mmio32, hwaddr *mmio64,
>                            unsigned n_dma, uint32_t *liobns, Error **errp);
> +    bool cas_completed_default;
>  };
>  
>  /**
> @@ -90,6 +91,7 @@ struct sPAPRMachineState {
>      sPAPROptionVector *ov5_cas;     /* negotiated (via CAS) option vectors */
>      bool cas_reboot;
>      bool cas_legacy_guest_workaround;
> +    bool cas_completed;
>  
>      Notifier epow_notifier;
>      QTAILQ_HEAD(, sPAPREventLogEntry) pending_events;

-- 
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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [Qemu-devel] [Qemu-ppc] [PATCH v2 0/2] spapr: disable hotplugging without OS

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 2683 bytes --]

On Thu, Jun 08, 2017 at 03:35:58PM -0300, Daniel Henrique Barboza wrote:
> 
> 
> On 06/08/2017 02:27 PM, Laurent Vivier wrote:
> > If the OS is not started, QEMU sends an event to the OS
> > that is lost and cannot be recovered. An unplug is not
> > able to restore QEMU in a coherent state.
> > So, while the OS is not started, disable CPU and memory hotplug.
> > We guess the OS is started if the CAS has been negotiated.
> > 
> > This series also revert previous fix which was not really fixing
> > the hotplug problem when the OS is not running.
> > 
> > v2:
> > - fix indent
> > - remove useless local_err
> > - allow hotplug if the VM is not started (pre-launch or incoming state)
> > - remove vector 6, instead just mark the end of CAS negotiation
> > 
> > Laurent Vivier (2):
> >    spapr: disable hotplugging without OS
> >    Revert "spapr: fix memory hot-unplugging"
> > 
> >   hw/ppc/spapr.c             | 51 +++++++++++++++++++++++++++++++++++++++++++---
> >   hw/ppc/spapr_drc.c         | 20 +++---------------
> >   hw/ppc/spapr_hcall.c       |  1 +
> >   include/hw/ppc/spapr.h     |  2 ++
> >   include/hw/ppc/spapr_drc.h |  1 -
> >   5 files changed, 54 insertions(+), 21 deletions(-)
> > 
> Tested-by: Daniel Barboza <danielhb@linux.vnet.ibm.com>
> 
> This is curious. I was having a little look into hotplug/unplug and DRC
> states yesterday
> while taking a look at the latest David's cleanup. I was trying to find out
> a way to tune
> spapr_drc_needed() in a way that we don't accidentally migrate more DRCs
> than necessary
> and at the same time handle that scenario of libvirt migration after hotplug
> (libvirt hotplugs
> the device on target instead of adding it in the command line).
> 
> I would like to migrate the DRCs if and only if:
> 
> 1 - a device was hotplugged
> 2 - a device is undergoing hotplug/unplug
> 
> (2) is easy and is already taken care of. But (1) is tricky because, before
> this patch, if a migration
> occurs *before* CAS we don't need to migrate the DRC - the object will go
> through the state
> changes after the migration. With this series this scenario is not going to
> happen, then
> we can happily add a
> 
> if (dev->hotplugged) return true;
> 
> in spapr_drc_needed and fix the libvirt migration scenario again.

As mentioned in another thread, I'm looking at explicitly resetting
all DRC states during the CAS process.  I think that will simplify
these cases.

-- 
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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [Qemu-devel] [PATCH v2 1/2] spapr: disable hotplugging without OS

[Laurent Vivier]

On 12/06/2017 16:37, David Gibson wrote:
> On Thu, Jun 08, 2017 at 07:27:42PM +0200, Laurent Vivier wrote:
>> If the OS is not started, QEMU sends an event to the OS
>> that is lost and cannot be recovered. An unplug is not
>> able to restore QEMU in a coherent state.
>> So, while the OS is not started, disable CPU and memory hotplug.
>> We guess the OS is started if the CAS has been negotiated.
>>
>> Signed-off-by: Laurent Vivier <lvivier@redhat.com>
> 
> It seems a pain to introduce a whole new (migrated) variable just to
> check this.  Could we instead tweak the allocation of spapr->ov5_cas,
> so it is NULL until CAS is completed?

I think it's a good idea to use ov5_cas, but we need to modify some
functions to manage the NULL pointer (spapr_ovec_test(),
spapr_ovec_populate_dt()), and I have some issues to manage the NULL
pointer in migration:

- with the previous releases, if it is NULL, we don't want to migrate it
because previous releases are not able to manage a NULL pointer, so we
don't migrate it (spapr_ov5_cas_needed() should be false if ov5_cas is
NULL) letting it to its default value (initialized but empty) in this
case on the destination,

- with the current version, if it is not NULL, we to want migrate it,
but the destination guest crashes because the pointer on the destination
is NULL and there is no memory the receive the data.

I think the problem is we can't migrate ov5_cas if it is not initialized
on the destination side[0]. Perhaps I've missed something but it seems a
NULL pointer can't be migrated and thus cannot be used as a state marker.

Any idea?

Thanks,
Laurent

[0] Perhaps we could use a VMSTATE_XXX() with a VMS_ALLOC flag instead
of VMSTATE_STRUCT_POINTER_V() to allocate the memory on the destination?

Re: [Qemu-devel] [PATCH v2 1/2] spapr: disable hotplugging without OS

[Laurent Vivier]

On 13/06/2017 22:18, Laurent Vivier wrote:
> On 12/06/2017 16:37, David Gibson wrote:
>> On Thu, Jun 08, 2017 at 07:27:42PM +0200, Laurent Vivier wrote:
>>> If the OS is not started, QEMU sends an event to the OS
>>> that is lost and cannot be recovered. An unplug is not
>>> able to restore QEMU in a coherent state.
>>> So, while the OS is not started, disable CPU and memory hotplug.
>>> We guess the OS is started if the CAS has been negotiated.
>>>
>>> Signed-off-by: Laurent Vivier <lvivier@redhat.com>
>>
>> It seems a pain to introduce a whole new (migrated) variable just to
>> check this.  Could we instead tweak the allocation of spapr->ov5_cas,
>> so it is NULL until CAS is completed?
> 
> I think it's a good idea to use ov5_cas, but we need to modify some
> functions to manage the NULL pointer (spapr_ovec_test(),
> spapr_ovec_populate_dt()), and I have some issues to manage the NULL
> pointer in migration:
> 
> - with the previous releases, if it is NULL, we don't want to migrate it
> because previous releases are not able to manage a NULL pointer, so we
> don't migrate it (spapr_ov5_cas_needed() should be false if ov5_cas is
> NULL) letting it to its default value (initialized but empty) in this
> case on the destination,
> 
> - with the current version, if it is not NULL, we to want migrate it,
> but the destination guest crashes because the pointer on the destination
> is NULL and there is no memory the receive the data.
> 
> I think the problem is we can't migrate ov5_cas if it is not initialized
> on the destination side[0]. Perhaps I've missed something but it seems a
> NULL pointer can't be migrated and thus cannot be used as a state marker.
> 
> Any idea?
> 
> Thanks,
> Laurent
> 
> [0] Perhaps we could use a VMSTATE_XXX() with a VMS_ALLOC flag instead
> of VMSTATE_STRUCT_POINTER_V() to allocate the memory on the destination?
> 

This is what I've tried but migration crashes if the OS is started on
source guest (ov5_cas != NULL, because on destination guest ov5_cas ==
NULL and the guest doesn't allocate the memory on migration). I think my
v2 looks cleaner.

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index b2951d7..742cbe7 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -1343,7 +1343,7 @@ static void ppc_spapr_reset(void)
      * negotiated options and start from scratch */
     if (!spapr->cas_reboot) {
         spapr_ovec_cleanup(spapr->ov5_cas);
-        spapr->ov5_cas = spapr_ovec_new();
+        spapr->ov5_cas = NULL;
     }

     fdt = spapr_build_fdt(spapr, rtas_addr, spapr->rtas_size);
@@ -1457,6 +1457,10 @@ static bool spapr_ov5_cas_needed(void *opaque)
     sPAPROptionVector *ov5_removed = spapr_ovec_new();
     bool cas_needed;

+    if (spapr->ov5_cas == NULL) {
+        return false;
+    }
+
     /* Prior to the introduction of sPAPROptionVector, we had two option
      * vectors we dealt with: OV5_FORM1_AFFINITY, and OV5_DRCONF_MEMORY.
      * Both of these options encode machine topology into the device-tree
@@ -2105,7 +2109,7 @@ static void ppc_spapr_init(MachineState *machine)

     /* Set up containers for ibm,client-set-architecture negotiated
options */
     spapr->ov5 = spapr_ovec_new();
-    spapr->ov5_cas = spapr_ovec_new();
+    spapr->ov5_cas = NULL;

     if (smc->dr_lmb_enabled) {
         spapr_ovec_set(spapr->ov5, OV5_DRCONF_MEMORY);
@@ -2604,6 +2608,7 @@ out:
 static void spapr_memory_pre_plug(HotplugHandler *hotplug_dev,
DeviceState *dev,
                                   Error **errp)
 {
+    sPAPRMachineState *ms = SPAPR_MACHINE(hotplug_dev);
     PCDIMMDevice *dimm = PC_DIMM(dev);
     PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(dimm);
     MemoryRegion *mr = ddc->get_memory_region(dimm);
@@ -2616,6 +2621,15 @@ static void spapr_memory_pre_plug(HotplugHandler
*hotplug_dev, DeviceState *dev,
         return;
     }

+    if (dev->hotplugged) {
+        if (!runstate_check(RUN_STATE_PRELAUNCH) &&
+            !runstate_check(RUN_STATE_INMIGRATE) &&
+            ms->ov5_cas == NULL) {
+            error_setg(errp, "Memory hotplug not supported without OS");
+            return;
+        }
+    }
+
     mem_dev = object_property_get_str(OBJECT(dimm),
PC_DIMM_MEMDEV_PROP, NULL);
     if (mem_dev && !kvmppc_is_mem_backend_page_size_ok(mem_dev)) {
         error_setg(errp, "Memory backend has bad page size. "
@@ -2919,6 +2933,7 @@ static void spapr_core_pre_plug(HotplugHandler
*hotplug_dev, DeviceState *dev,
                                 Error **errp)
 {
     MachineState *machine = MACHINE(OBJECT(hotplug_dev));
+    sPAPRMachineState *ms = SPAPR_MACHINE(machine);
     MachineClass *mc = MACHINE_GET_CLASS(hotplug_dev);
     Error *local_err = NULL;
     CPUCore *cc = CPU_CORE(dev);
@@ -2927,9 +2942,18 @@ static void spapr_core_pre_plug(HotplugHandler
*hotplug_dev, DeviceState *dev,
     CPUArchId *core_slot;
     int index;

-    if (dev->hotplugged && !mc->has_hotpluggable_cpus) {
-        error_setg(&local_err, "CPU hotplug not supported for this
machine");
-        goto out;
+    if (dev->hotplugged) {
+        if (!mc->has_hotpluggable_cpus) {
+            error_setg(&local_err,
+                       "CPU hotplug not supported for this machine");
+            goto out;
+        }
+        if (!runstate_check(RUN_STATE_PRELAUNCH) &&
+            !runstate_check(RUN_STATE_INMIGRATE) &&
+            ms->ov5_cas == NULL) {
+            error_setg(&local_err, "CPU hotplug not supported without OS");
+            goto out;
+        }
     }

     if (strcmp(base_core_type, type)) {
diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
index aa1ffea..fa25a34 100644
--- a/hw/ppc/spapr_hcall.c
+++ b/hw/ppc/spapr_hcall.c
@@ -1133,6 +1133,10 @@ static target_ulong
h_client_architecture_support(PowerPCCPU *cpu,
     guest_radix = spapr_ovec_test(ov5_guest, OV5_MMU_RADIX_300);
     spapr_ovec_clear(ov5_guest, OV5_MMU_RADIX_300);

+    if (spapr->ov5_cas == NULL) {
+        spapr->ov5_cas = spapr_ovec_new();
+    }
+
     /* NOTE: there are actually a number of ov5 bits where input from the
      * guest is always zero, and the platform/QEMU enables them
independently
      * of guest input. To model these properly we'd want some sort of mask,
diff --git a/hw/ppc/spapr_ovec.c b/hw/ppc/spapr_ovec.c
index 41df4c3..5f0c2d9 100644
--- a/hw/ppc/spapr_ovec.c
+++ b/hw/ppc/spapr_ovec.c
@@ -128,9 +128,12 @@ void spapr_ovec_clear(sPAPROptionVector *ov, long
bitnr)

 bool spapr_ovec_test(sPAPROptionVector *ov, long bitnr)
 {
-    g_assert(ov);
     g_assert_cmpint(bitnr, <, OV_MAXBITS);

+    if (ov == NULL) {
+        return false;
+    }
+
     return test_bit(bitnr, ov->bitmap) ? true : false;
 }

@@ -217,7 +220,10 @@ int spapr_ovec_populate_dt(void *fdt, int fdt_offset,
     unsigned long lastbit;
     int i;

-    g_assert(ov);
+    if (ov == NULL) {
+        vec[0] = 0;
+        return fdt_setprop(fdt, fdt_offset, name, vec, 2);
+    }