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

[David Gibson]

On Mon, Oct 14, 2019 at 11:15:16AM +0200, Greg Kurz wrote:
> On Sat, 12 Oct 2019 11:00:41 +1100
> David Gibson <address@hidden> wrote:
> 
> > On Fri, Oct 11, 2019 at 08:13:33AM +0200, Greg Kurz wrote:
> > > On Fri, 11 Oct 2019 16:07:58 +1100
> > > David Gibson <address@hidden> 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 <address@hidden> wrote:
> > > > > 
> > > > > > On Thu, 10 Oct 2019 13:02:09 +1100
> > > > > > David Gibson <address@hidden> 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 <address@hidden> 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 <address@hidden>
> > > > > > 
> > > > > 
> > > > > 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, 
> 
> Hmmm... I had the impression that capability numbers would stay
> forever, even if at some point we may decide to not support some
> of them for newer machine types... Can you elaborate on a
> deprecating scenario that would break ?

Uhh... good point, I don't think that could break it.  Even if we
deprecated a capability we could still retain enough awareness of the
old number to sanity check this.

> > and it also might be problematic for downstream
> > where we've sometimes selectively backported caps.
> 
> Do you mean that capability numbers defined in spapr.h differ
> from the ones in upstream QEMU ?

No, they don't but that's actually the problem.  The point is that we
might backport some later caps without necessarily backporting all the
earlier ones - that means that the "max cap index" no longer implies
that all the lower indexed caps are present.

> 
> > > > 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.
> 
> Yes, but do we really need to perform strict checks on ic-mode in the first
> place ? I mean that migrating the state of XICS and/or XIVE entities _only_
> requires the destination to have instantiated them, ie:
> 
> SOURCE/DEST | xics | xive | dual
> ------------+------+------+-------
> xics        | ok   | fail | ok (*)
> xive        | fail | ok   | ok (*)
> dual        | fail | fail | ok
> 
> (*) missing migrated state for xics/xive means that the corresponding
>     objects will have reset state, like after CAS.

Yes... I don't really see where you're goig with that thought.

> > Ugh, that's a bit trickier to work around.
> > 
> 
> Maybe have a migrate_needed() hook like this:
> 
> static bool cap_xics_xive_migrate_needed(void *opaque)
> {
>     return !SPAPR_MACHINE_GET_CLASS(opaque)->pre_4_2_migration;
> }
> 
> and also use it in spapr_caps_post_migration() ?

Yeah, maybe.  I think we have a hack like this for one of the other
caps already.

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