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

[Greg Kurz]

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 ?

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

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

> 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() ?

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

pgpueBPUKau6E.pgp
Description: OpenPGP digital signature