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

[Greg Kurz]

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

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

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

pgpPxZ_4yqyoS.pgp
Description: OpenPGP digital signature