On Tue, Jun 13, 2017 at 09:33:59AM +0200, Greg Kurz wrote:
> On Mon, 12 Jun 2017 22:24:56 +0800
> David Gibson <da...@gibson.dropbear.id.au> wrote:
>
> > 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.
> >
>
> It is also passed as opaque argument to vmstate_register(), where it is
> used as a key when calling vmstate_unregister(). I could possibly pass
> (void *) i instead, but I'm not a big fan of hijacking pointer arguments
> to pass numbers.
Ah, I see your point. Creating an array, purely to generate arbitrary
pointers is also kind of ugly, though. Really the cpu_index / XICS
server number makes sense to identify the vmstate, but it looks like
vmstate_unregister() doesn't take that.
--
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
