[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v4 17/19] spapr: Remove last pieces of SpaprIrq
From: |
David Gibson |
Subject: |
Re: [PATCH v4 17/19] spapr: Remove last pieces of SpaprIrq |
Date: |
Fri, 11 Oct 2019 16:07:58 +1100 |
User-agent: |
Mutt/1.12.1 (2019-06-15) |
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 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
signature.asc
Description: PGP 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 <=
- 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, 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