[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v4 17/19] spapr: Remove last pieces of SpaprIrq
From: |
Greg Kurz |
Subject: |
Re: [PATCH v4 17/19] spapr: Remove last pieces of SpaprIrq |
Date: |
Fri, 11 Oct 2019 08:13:33 +0200 |
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;
}
pgpxHsIJ9QdSo.pgp
Description: OpenPGP digital signature
- Re: [PATCH v4 13/19] spapr, xics, xive: Move SpaprIrq::reset hook logic into activate/deactivate, (continued)
- [PATCH v4 17/19] spapr: Remove last pieces of SpaprIrq, David Gibson, 2019/10/09
- Re: [PATCH v4 17/19] spapr: Remove last pieces of SpaprIrq, Cédric Le Goater, 2019/10/09
- Re: [PATCH v4 17/19] spapr: Remove last pieces of SpaprIrq, Greg Kurz, 2019/10/09
- Re: [PATCH v4 17/19] spapr: Remove last pieces of SpaprIrq, David Gibson, 2019/10/09
- Re: [PATCH v4 17/19] spapr: Remove last pieces of SpaprIrq, Greg Kurz, 2019/10/10
- Re: [PATCH v4 17/19] spapr: Remove last pieces of SpaprIrq, Greg Kurz, 2019/10/10
- Re: [PATCH v4 17/19] spapr: Remove last pieces of SpaprIrq, David Gibson, 2019/10/11
- Re: [PATCH v4 17/19] spapr: Remove last pieces of SpaprIrq,
Greg Kurz <=
- Re: [PATCH v4 17/19] spapr: Remove last pieces of SpaprIrq, Greg Kurz, 2019/10/11
- Re: [PATCH v4 17/19] spapr: Remove last pieces of SpaprIrq, David Gibson, 2019/10/11
- Re: [PATCH v4 17/19] spapr: Remove last pieces of SpaprIrq, Greg Kurz, 2019/10/14
Re: [PATCH v4 00/19] spapr: IRQ subsystem cleanup, David Gibson, 2019/10/09
Re: [PATCH v4 00/19] spapr: IRQ subsystem cleanup, Greg Kurz, 2019/10/16