Re: [PATCH v4 17/19] spapr: Remove last pieces of SpaprIrq

From: David Gibson <david@gibson.dropbear.id.au>
To: Greg Kurz <groug@kaod.org>
Cc: "Jason Wang" <jasowang@redhat.com>,
	"Riku Voipio" <riku.voipio@iki.fi>,
	qemu-devel@nongnu.org, qemu-ppc@nongnu.org, clg@kaod.org,
	"Paolo Bonzini" <pbonzini@redhat.com>,
	"Marc-André Lureau" <marcandre.lureau@redhat.com>,
	philmd@redhat.com
Subject: Re: [PATCH v4 17/19] spapr: Remove last pieces of SpaprIrq
Date: Sat, 12 Oct 2019 11:00:41 +1100	[thread overview]
Message-ID: <20191012000041.GE4080@umbus.fritz.box> (raw)
In-Reply-To: <20191011081333.7e483b95@bahia.lan>

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

On Fri, Oct 11, 2019 at 08:13:33AM +0200, Greg Kurz wrote:
> On Fri, 11 Oct 2019 16:07:58 +1100
> David Gibson <david@gibson.dropbear.id.au> wrote:
> 
> > On Thu, Oct 10, 2019 at 10:33:04PM +0200, Greg Kurz wrote:
> > > On Thu, 10 Oct 2019 08:29:58 +0200
> > > Greg Kurz <groug@kaod.org> wrote:
> > > 
> > > > On Thu, 10 Oct 2019 13:02:09 +1100
> > > > David Gibson <david@gibson.dropbear.id.au> wrote:
> > > > 
> > > > > On Wed, Oct 09, 2019 at 07:02:15PM +0200, Greg Kurz wrote:
> > > > > > On Wed,  9 Oct 2019 17:08:16 +1100
> > > > > > David Gibson <david@gibson.dropbear.id.au> wrote:
> > > > > > 
> > > > > > > The only thing remaining in this structure are the flags to allow either
> > > > > > > XICS or XIVE to be present.  These actually make more sense as spapr
> > > > > > > capabilities - that way they can take advantage of the existing
> > > > > > > infrastructure to sanity check capability states across migration and so
> > > > > > > forth.
> > > > > > > 
> > > > > > 
> > > > > > The user can now choose the interrupt controller mode either through
> > > > > > ic-mode or through cap-xics/cap-xive. I guess it doesn't break anything
> > > > > > to expose another API to do the same thing but it raises some questions.
> > > > > > 
> > > > > > We should at least document somewhere that ic-mode is an alias to these
> > > > > > caps, and maybe state which is the preferred method (I personally vote
> > > > > > for the caps).
> > > > > > 
> > > > > > Also, we must keep ic-mode for the moment to stay compatible with the
> > > > > > existing pseries-4.0 and pseries-4.1 machine types, but will we
> > > > > > keep ic-mode forever ? If no, maybe start by not allowing it for
> > > > > > pseries-4.2 ?
> > > > > 
> > > > > I'm actually inclined to keep it for now, maybe even leave it as the
> > > > > suggested way to configure this.  The caps are nice from an internal
> > > > > organization point of view, but ic-mode is arguably a more user
> > > > > friendly way of configuring it.  The conversion of one to the other is
> > > > > straightforward, isolated ans small, so I'm not especially bothered by
> > > > > keeping it around.
> > > > > 
> > > > 
> > > > Fair enough.
> > > > 
> > > > Reviewed-by: Greg Kurz <groug@kaod.org>
> > > > 
> > > 
> > > But unfortunately this still requires care :-\
> > > 
> > > qemu-system-ppc64: cap-xive higher level (1) in incoming stream than on destination (0)
> > > qemu-system-ppc64: error while loading state for instance 0x0 of device 'spapr'
> > > qemu-system-ppc64: load of migration failed: Invalid argument
> > > 
> > > or
> > > 
> > > qemu-system-ppc64: cap-xics higher level (1) in incoming stream than on destination (0)
> > > qemu-system-ppc64: error while loading state for instance 0x0 of device 'spapr'
> > > qemu-system-ppc64: load of migration failed: Invalid argument
> > > 
> > > when migrating from QEMU 4.1 with ic-mode=xics and ic-mode=xive respectively.
> > > 
> > > This happens because the existing pseries-4.1 machine type doesn't send the
> > > new caps and the logic in spapr_caps_post_migration() wrongly assumes that
> > > the source has both caps set:
> > > 
> > >     srccaps = default_caps_with_cpu(spapr, MACHINE(spapr)->cpu_type);
> > >     for (i = 0; i < SPAPR_CAP_NUM; i++) {
> > >         /* If not default value then assume came in with the migration */
> > >         if (spapr->mig.caps[i] != spapr->def.caps[i]) {
> > > 
> > > spapr->mig.caps[SPAPR_CAP_XICS] = 0
> > > spapr->mig.caps[SPAPR_CAP_XIVE] = 0
> > > 
> > >             srccaps.caps[i] = spapr->mig.caps[i];
> > > 
> > > srcaps.caps[SPAPR_CAP_XICS] = 1
> > > srcaps.caps[SPAPR_CAP_XIVE] = 1
> > > 
> > >         }
> > >     }
> > > 
> > > and breaks
> > > 
> > >     for (i = 0; i < SPAPR_CAP_NUM; i++) {
> > >         SpaprCapabilityInfo *info = &capability_table[i];
> > > 
> > >         if (srccaps.caps[i] > dstcaps.caps[i]) {
> > > 
> > > srcaps.caps[SPAPR_CAP_XICS] = 0 when ic-mode=xive
> > > srcaps.caps[SPAPR_CAP_XIVE] = 0 when ic-mode=xics
> > > 
> > >             error_report("cap-%s higher level (%d) in incoming stream than on destination (%d)",
> > >                          info->name, srccaps.caps[i], dstcaps.caps[i]);
> > >             ok = false;
> > >         }
> > 
> > Ah.. right.  I thought there would be problems with backwards
> > migration, but I didn't think of this problem even with forward
> > migration.
> > 
> > > Maybe we shouldn't check capabilities that we know the source
> > > isn't supposed to send, eg. by having a smc->max_cap ?
> > 
> > Uh.. I'm not really sure what exactly you're suggesting here.
> > 
> 
> I'm suggesting to have a per-machine version smc->max_cap that
> contains the highest supported cap index, to be used instead of
> SPAPR_CAP_NUM in this functions, ie.
> 
> for (i = 0; i <= smc->max_cap; i++) {
>     ...
> }
> 
> where we would have
> 
> smc->max_cap = SPAPR_CAP_CCF_ASSIST for pseries-4.1
> 
> and
> 
> smc->max_cap = SPAPR_CAP_XIVE for psereis-4.2

Oh, I see, a max cap index.  I think that sounds fragile if we ever
deprecate any caps, and it also might be problematic for downstream
where we've sometimes selectively backported caps.

> > I think what we need here is a custom migrate_needed function, like we
> > already have for cap_hpt_maxpagesize, to exclude it from the migration
> > stream for machine versions before 4.2.
> > 
> 
> No, VMState needed() hooks are for outgoing migration only.

Ah, yeah, right.  Essentially the problem is that in the absence of
caps, the new qemu assumes they're in the default state, but if an old
source had ic-mode set, then they effectively aren't.  Or looked at
another way, it's now trying to check that the ends match w.r.t. intc
selection, but doesn't have enough information supplied by old sources
to do so correctly.

Ugh, that's a bit trickier to work around.

> bool vmstate_save_needed(const VMStateDescription *vmsd, void *opaque)
> {
>     if (vmsd->needed && !vmsd->needed(opaque)) {
>         /* optional section not needed */
>         return false;
>     }
>     return true;
> }

-- 
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: 833 bytes --]