On Thu, Jun 08, 2017 at 11:54:10AM +0200, Greg Kurz wrote:
> On Thu, 8 Jun 2017 14:08:57 +1000
> David Gibson <da...@gibson.dropbear.id.au> wrote:
>
> > On Wed, Jun 07, 2017 at 07:17:26PM +0200, Greg Kurz wrote:
> > > Commit 5bc8d26de20c ("spapr: allocate the ICPState object from under
> > > sPAPRCPUCore") moved ICPState objects from the machine to CPU cores.
> > > This is an improvement since we no longer allocate ICPState objects
> > > that will never be used. But it has the side-effect of breaking
> > > migration of older machine types from older QEMU versions.
> > >
> > > This patch allows spapr to register dummy "icp/server" entries to vmstate.
> > > These entries use a dedicated VMStateDescription that can swallow and
> > > discard state of an incoming migration stream, and that don't send
> > > anything
> > > on outgoing migration.
> > >
> > > As for real ICPState objects, the instance_id is the cpu_index of the
> > > corresponding vCPU, which happens to be equal to the generated instance_id
> > > of older machine types.
> > >
> > > The machine can unregister/register these entries when CPUs are
> > > dynamically
> > > plugged/unplugged.
> > >
> > > This is only available for pseries-2.9 and older machines, thanks to a
> > > compat property.
> > >
> > > Signed-off-by: Greg Kurz <gr...@kaod.org>
> > > ---
> > > v3: - new logic entirely implemented in hw/ppc/spapr.c
> > > ---
> > > hw/ppc/spapr.c | 88
> > > +++++++++++++++++++++++++++++++++++++++++++++++-
> > > include/hw/ppc/spapr.h | 2 +
> > > 2 files changed, 88 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > > index 9b7ae28939a8..c15b604978f0 100644
> > > --- a/hw/ppc/spapr.c
> > > +++ b/hw/ppc/spapr.c
> > > @@ -124,9 +124,52 @@ error:
> > > return NULL;
> > > }
> > >
> > > +static bool pre_2_10_vmstate_dummy_icp_needed(void *opaque)
> > > +{
> > > + return false;
> > > +}
> >
> > Uh.. the needed function always returns false, how can that work?
> >
>
> The needed function is used for outgoing migration only:
>
> bool vmstate_save_needed(const VMStateDescription *vmsd, void *opaque)
> {
> if (vmsd->needed && !vmsd->needed(opaque)) {
> /* optional section not needed */
> return false;
> }
> return true;
> }
>
> The idea is that all ICPState objects that were created but not associated
> to a vCPU by pre-2.10 machine types don't need to be migrated at all, as
> their state hasn't changed.
>
> We don't even create these unneeded ICPState objects here, but simply
> register their ids to vmstate.
>
> > > +
> > > +static const VMStateDescription pre_2_10_vmstate_dummy_icp = {
> > > + .name = "icp/server",
> > > + .version_id = 1,
> > > + .minimum_version_id = 1,
> > > + .needed = pre_2_10_vmstate_dummy_icp_needed,
>
> Outgoing migration:
> - machine in older QEMU have unused ICPState objects (default state)
> - machine in QEMU 2.10 doesn't even have extra ICPState objects
>
> => don't send anything
>
> > > + .fields = (VMStateField[]) {
> > > + VMSTATE_UNUSED(4), /* uint32_t xirr */
> > > + VMSTATE_UNUSED(1), /* uint8_t pending_priority */
> > > + VMSTATE_UNUSED(1), /* uint8_t mfrr */
>
> Incoming migration from older QEMU: we don't have the extra ICPState objects.
>
> => accept the state and discard it
>
> > > + VMSTATE_END_OF_LIST()
> > > + },
> > > +};
> > > +
> > > +static void pre_2_10_vmstate_register_dummy_icp(sPAPRMachineState
> > > *spapr, int i)
> > > +{
> > > + bool *flag = &spapr->pre_2_10_ignore_icp[i];
> > > +
> > > + g_assert(!*flag);
> >
> > Apart from this assert(), you never seem to test the values in the
> > pre_2_10_ignore_icp() array, so it seems a bit pointless.
> >
>
> There's the opposite check in pre_2_10_vmstate_unregister_dummy_icp().
> But I agree it isn't really useful... but more because of paranoia :)
I'm all for paranoid assert()s if they can be made using data readily
to hand. Adding a data structure just for the purpose of making an
assert() later, not so much.
> > > + vmstate_register(NULL, i, &pre_2_10_vmstate_dummy_icp, flag);
> > > + *flag = true;
> > > +}
> > > +
> > > +static void pre_2_10_vmstate_unregister_dummy_icp(sPAPRMachineState
> > > *spapr,
> > > + int i)
> > > +{
> > > + bool *flag = &spapr->pre_2_10_ignore_icp[i];
> > > +
> > > + g_assert(*flag);
> > > + vmstate_unregister(NULL, &pre_2_10_vmstate_dummy_icp, flag);
> > > + *flag = false;
> > > +}
> > > +
> > > +static inline int xics_nr_servers(void)
> >
> > Maybe a different name to emphasise that this is only used for the
> > backwards compat logic.
> >
>
> It is also used to compute the "ibm,interrupt-server-ranges" DT prop.
>
> /* /interrupt controller */
> spapr_dt_xics(xics_nr_servers(), fdt, PHANDLE_XICP);
Ah, good point. Maybe rename to "max server number" or something,
since "nr_servers" isn't really accurate any more.
> > > +{
> > > + return DIV_ROUND_UP(max_cpus * kvmppc_smt_threads(), smp_threads);
> > > +}
> > > +
> > > static void xics_system_init(MachineState *machine, int nr_irqs, Error
> > > **errp)
> > > {
> > > sPAPRMachineState *spapr = SPAPR_MACHINE(machine);
> > > + sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(machine);
> > >
> > > if (kvm_enabled()) {
> > > if (machine_kernel_irqchip_allowed(machine) &&
> > > @@ -148,6 +191,15 @@ static void xics_system_init(MachineState *machine,
> > > int nr_irqs, Error **errp)
> > > return;
> > > }
> > > }
> > > +
> > > + if (smc->pre_2_10_has_unused_icps) {
> > > + int i;
> > > +
> > > + spapr->pre_2_10_ignore_icp = g_malloc(xics_nr_servers());
> > > + for (i = 0; i < xics_nr_servers(); i++) {
> > > + pre_2_10_vmstate_register_dummy_icp(spapr, i);
> >
> > This registers a dummy ICP for every slot, some of which will have
> > real cpus / icps. That doesn't seem right.
> >
>
> This is initialization, before we even have actual CPUs. We start with
> dummy ICPs for every slot, but they get replaced by real ICPs when we
> plug CPU cores...... (see below)
>
> > > + }
> > > + }
> > > }
> > >
> > > static int spapr_fixup_cpu_smt_dt(void *fdt, int offset, PowerPCCPU *cpu,
> > > @@ -976,7 +1028,6 @@ static void *spapr_build_fdt(sPAPRMachineState
> > > *spapr,
> > > void *fdt;
> > > sPAPRPHBState *phb;
> > > char *buf;
> > > - int smt = kvmppc_smt_threads();
> > >
> > > fdt = g_malloc0(FDT_MAX_SIZE);
> > > _FDT((fdt_create_empty_tree(fdt, FDT_MAX_SIZE)));
> > > @@ -1016,7 +1067,7 @@ static void *spapr_build_fdt(sPAPRMachineState
> > > *spapr,
> > > _FDT(fdt_setprop_cell(fdt, 0, "#size-cells", 2));
> > >
> > > /* /interrupt controller */
> > > - spapr_dt_xics(DIV_ROUND_UP(max_cpus * smt, smp_threads), fdt,
> > > PHANDLE_XICP);
> > > + spapr_dt_xics(xics_nr_servers(), fdt, PHANDLE_XICP);
> > >
> > > ret = spapr_populate_memory(spapr, fdt);
> > > if (ret < 0) {
> > > @@ -2800,9 +2851,24 @@ static void spapr_core_unplug(HotplugHandler
> > > *hotplug_dev, DeviceState *dev,
> > > Error **errp)
> > > {
> > > MachineState *ms = MACHINE(qdev_get_machine());
> > > + sPAPRMachineState *spapr = SPAPR_MACHINE(ms);
> > > CPUCore *cc = CPU_CORE(dev);
> > > CPUArchId *core_slot = spapr_find_cpu_slot(ms, cc->core_id, NULL);
> > >
> > > + if (spapr->pre_2_10_ignore_icp) {
> > > + sPAPRCPUCore *sc = SPAPR_CPU_CORE(OBJECT(dev));
> > > + sPAPRCPUCoreClass *scc = SPAPR_CPU_CORE_GET_CLASS(OBJECT(cc));
> > > + const char *typename = object_class_get_name(scc->cpu_class);
> > > + size_t size = object_type_get_instance_size(typename);
> > > + int i;
> > > +
> > > + for (i = 0; i < cc->nr_threads; i++) {
> > > + CPUState *cs = CPU(sc->threads + i * size);
> > > +
> > > + pre_2_10_vmstate_register_dummy_icp(spapr, cs->cpu_index);
> > > + }
> > > + }
> > > +
> > > assert(core_slot);
> > > core_slot->cpu = NULL;
> > > object_unparent(OBJECT(dev));
> > > @@ -2912,6 +2978,21 @@ static void spapr_core_plug(HotplugHandler
> > > *hotplug_dev, DeviceState *dev,
> > > }
> > > }
> > > core_slot->cpu = OBJECT(dev);
> > > +
> > > + if (spapr->pre_2_10_ignore_icp) {
> > > + sPAPRCPUCoreClass *scc = SPAPR_CPU_CORE_GET_CLASS(OBJECT(cc));
> > > + const char *typename = object_class_get_name(scc->cpu_class);
> > > + size_t size = object_type_get_instance_size(typename);
> > > + int i;
> > > +
> > > + for (i = 0; i < cc->nr_threads; i++) {
> > > + sPAPRCPUCore *sc = SPAPR_CPU_CORE(dev);
> > > + void *obj = sc->threads + i * size;
> > > +
> > > + cs = CPU(obj);
> > > + pre_2_10_vmstate_unregister_dummy_icp(spapr, cs->cpu_index);
>
> ...... here.
>
> The opposite happens in spapr_core_unplug().
>
> > > + }
> > > + }
> > > }
> > >
> > > static void spapr_core_pre_plug(HotplugHandler *hotplug_dev, DeviceState
> > > *dev,
> > > @@ -3361,9 +3442,12 @@ 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->pre_2_10_has_unused_icps = true;
> > > }
> > >
> > > DEFINE_SPAPR_MACHINE(2_9, "2.9", false);
> > > diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
> > > index f973b0284596..64382623199d 100644
> > > --- a/include/hw/ppc/spapr.h
> > > +++ b/include/hw/ppc/spapr.h
> > > @@ -53,6 +53,7 @@ struct sPAPRMachineClass {
> > > bool dr_lmb_enabled; /* enable dynamic-reconfig/hotplug of
> > > LMBs */
> > > bool use_ohci_by_default; /* use USB-OHCI instead of XHCI */
> > > const char *tcg_default_cpu; /* which (TCG) CPU to simulate by
> > > default */
> > > + bool pre_2_10_has_unused_icps;
> > > void (*phb_placement)(sPAPRMachineState *spapr, uint32_t index,
> > > uint64_t *buid, hwaddr *pio,
> > > hwaddr *mmio32, hwaddr *mmio64,
> > > @@ -90,6 +91,7 @@ struct sPAPRMachineState {
> > > sPAPROptionVector *ov5_cas; /* negotiated (via CAS) option
> > > vectors */
> > > bool cas_reboot;
> > > bool cas_legacy_guest_workaround;
> > > + bool *pre_2_10_ignore_icp;
> > >
> > > 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
signature.asc
Description: PGP signature
