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

[David Gibson <david@gibson.dropbear.id.au>]

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

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

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