Re: [PATCH v4 17/19] spapr: Remove last pieces of SpaprIrq
On Wed, 20 Nov 2019 16:38:37 +1100
David Gibson wrote:
[...]
> >
> > 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.
>
The idea with "max cap index" isn't that all the lower indexed caps are
present but rather higher indexed caps are absent. Maybe rename it to
something like "ignore higher cap index" or any better naming you can
think of ?
> >
> > > > > 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.
>
I mean that if we didn't check the XICS and XIVE capabilities, we
would still fail migration when it is really needed, ie. migrating
from ic-mode=xics to ic-mode=xive or the other way round. This
would it make it possible to migrate anything to ic-mode=dual though
but I don't think this is a problem since it doesn't break anything.
> > > 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.
>
pgprlj2bam7DG.pgp
Description: OpenPGP digital signature
Re: [PATCH v4 17/19] spapr: Remove last pieces of SpaprIrq
On Mon, Oct 14, 2019 at 11:15:16AM +0200, Greg Kurz wrote:
> On Sat, 12 Oct 2019 11:00:41 +1100
> David Gibson 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 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 wrote:
> > > > >
> > > > > > On Thu, 10 Oct 2019 13:02:09 +1100
> > > > > > David Gibson 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 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
> > > > > >
> > > > >
> > > > > 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
Re: [PATCH v4 17/19] spapr: Remove last pieces of SpaprIrq
On Sat, 12 Oct 2019 11:00:41 +1100
David Gibson 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 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 wrote:
> > > >
> > > > > On Thu, 10 Oct 2019 13:02:09 +1100
> > > > > David Gibson 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 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
> > > > >
> > > >
> > > > 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.
Re: [PATCH v4 17/19] spapr: Remove last pieces of SpaprIrq
On Fri, Oct 11, 2019 at 08:13:33AM +0200, Greg Kurz wrote:
> On Fri, 11 Oct 2019 16:07:58 +1100
> David Gibson 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 wrote:
> > >
> > > > On Thu, 10 Oct 2019 13:02:09 +1100
> > > > David Gibson 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 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
> > > >
> > >
> > > 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
Re: [PATCH v4 17/19] spapr: Remove last pieces of SpaprIrq
On Fri, 11 Oct 2019 08:13:33 +0200
Greg Kurz wrote:
> On Fri, 11 Oct 2019 16:07:58 +1100
> David Gibson 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 wrote:
> > >
> > > > On Thu, 10 Oct 2019 13:02:09 +1100
> > > > David Gibson 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 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
> > > >
> > >
> > > 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
Re: [PATCH v4 17/19] spapr: Remove last pieces of SpaprIrq
On Fri, 11 Oct 2019 16:07:58 +1100
David Gibson 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 wrote:
> >
> > > On Thu, 10 Oct 2019 13:02:09 +1100
> > > David Gibson 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 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
> > >
> >
> > 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 &
Re: [PATCH v4 17/19] spapr: Remove last pieces of SpaprIrq
On Thu, Oct 10, 2019 at 10:33:04PM +0200, Greg Kurz wrote:
> On Thu, 10 Oct 2019 08:29:58 +0200
> Greg Kurz wrote:
>
> > On Thu, 10 Oct 2019 13:02:09 +1100
> > David Gibson 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 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
> >
>
> 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 17/19] spapr: Remove last pieces of SpaprIrq
On Thu, 10 Oct 2019 08:29:58 +0200
Greg Kurz wrote:
> On Thu, 10 Oct 2019 13:02:09 +1100
> David Gibson 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 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
>
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;
}
Maybe we shouldn't check capabilities that we know the source
isn't supposed to send, eg. by having a smc->max_cap ?
> > >
> > > > Signed-off-by: David Gibson
> > > > ---
> > > > hw/ppc/spapr.c | 40 ++
> > > > hw/ppc/spapr_caps.c| 64 +
> > > > hw/ppc/spapr_hcall.c | 7 ++--
> > > > hw/ppc/spapr_irq.c | 84 ++
> > > > include/hw/ppc/spapr.h | 10 +++--
> > > > include/hw/ppc/spapr_irq.h | 10 -
> > > > 6 files changed, 103 insertions(+), 112 deletions(-)
> > > >
> > > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > > > index e1ff03152e..bf9fdb1693 100644
> > > > --- a/hw/ppc/spapr.c
> > > > +++ b/hw/ppc/spapr.c
> > > > @@ -1072,12 +1072,13 @@ static void
> > > > spapr_dt_ov5_platform_support(SpaprMachineState *spapr, void *fdt,
> > > > 26, 0x40, /* Radix options: GTSE == yes. */
> > > > };
> > > >
> > > > -if (spapr->irq->xics && spapr->irq->xive) {
> > > > +if (spapr_get_cap(spapr, SPAPR_CAP_XICS)
> > > > +&& spapr_get_cap(spapr, SPAPR_CAP_XIVE)) {
> > > > val[1] = SPAPR_OV5_XIVE_BOTH;
> > > > -} else if (spapr->irq->xive) {
> > > > +} else if (spapr_get_cap(spapr, SPAPR_CAP_XIVE)) {
> > > > val[1] = SPAPR_OV5_XIVE_EXPLOIT;
> > > > } else {
> > > > -assert(spapr->irq->xics);
> > > > +assert(spapr_get_cap(spapr, SPAPR_CAP_XICS));
> > > > val[1] = SPAPR_OV5_XIVE_LEGACY;
> > > > }
> > > >
> > > > @@ -2075,6 +2076,8 @@ static const VMStateDescription vm
Re: [PATCH v4 17/19] spapr: Remove last pieces of SpaprIrq
On Thu, 10 Oct 2019 13:02:09 +1100
David Gibson 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 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
> >
> > > Signed-off-by: David Gibson
> > > ---
> > > hw/ppc/spapr.c | 40 ++
> > > hw/ppc/spapr_caps.c| 64 +
> > > hw/ppc/spapr_hcall.c | 7 ++--
> > > hw/ppc/spapr_irq.c | 84 ++
> > > include/hw/ppc/spapr.h | 10 +++--
> > > include/hw/ppc/spapr_irq.h | 10 -
> > > 6 files changed, 103 insertions(+), 112 deletions(-)
> > >
> > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > > index e1ff03152e..bf9fdb1693 100644
> > > --- a/hw/ppc/spapr.c
> > > +++ b/hw/ppc/spapr.c
> > > @@ -1072,12 +1072,13 @@ static void
> > > spapr_dt_ov5_platform_support(SpaprMachineState *spapr, void *fdt,
> > > 26, 0x40, /* Radix options: GTSE == yes. */
> > > };
> > >
> > > -if (spapr->irq->xics && spapr->irq->xive) {
> > > +if (spapr_get_cap(spapr, SPAPR_CAP_XICS)
> > > +&& spapr_get_cap(spapr, SPAPR_CAP_XIVE)) {
> > > val[1] = SPAPR_OV5_XIVE_BOTH;
> > > -} else if (spapr->irq->xive) {
> > > +} else if (spapr_get_cap(spapr, SPAPR_CAP_XIVE)) {
> > > val[1] = SPAPR_OV5_XIVE_EXPLOIT;
> > > } else {
> > > -assert(spapr->irq->xics);
> > > +assert(spapr_get_cap(spapr, SPAPR_CAP_XICS));
> > > val[1] = SPAPR_OV5_XIVE_LEGACY;
> > > }
> > >
> > > @@ -2075,6 +2076,8 @@ static const VMStateDescription vmstate_spapr = {
> > > &vmstate_spapr_dtb,
> > > &vmstate_spapr_cap_large_decr,
> > > &vmstate_spapr_cap_ccf_assist,
> > > +&vmstate_spapr_cap_xics,
> > > +&vmstate_spapr_cap_xive,
> > > NULL
> > > }
> > > };
> > > @@ -2775,7 +2778,7 @@ static void spapr_machine_init(MachineState
> > > *machine)
> > > spapr_ovec_set(spapr->ov5, OV5_DRMEM_V2);
> > >
> > > /* advertise XIVE on POWER9 machines */
> > > -if (spapr->irq->xive) {
> > > +if (spapr_get_cap(spapr, SPAPR_CAP_XIVE)) {
> > > spapr_ovec_set(spapr->ov5, OV5_XIVE_EXPLOIT);
> > > }
> > >
> > > @@ -3242,14 +3245,18 @@ static void spapr_set_vsmt(Object *obj, Visitor
> > > *v, const char *name,
> > > static char *spapr_get_ic_mode(Object *obj, Error **errp)
> > > {
> > > SpaprMachineState *spapr = SPAPR_MACHINE(obj);
> > > +SpaprMachineClass *smc = SPAPR_MACHINE_GET_CLASS(spapr);
> > >
> > > -if (spapr->irq == &spapr_irq_xics_legacy) {
> > > +if (smc->legacy_irq_allocation) {
> > > return g_strdup("legacy");
> > > -} else if (spapr->irq == &spapr_irq_xics) {
> > > +} else if (spapr_get_cap(spapr, SPAPR_CAP_XICS)
> > > + && !spapr_get_cap(spapr, SPAPR_CAP_XIVE)) {
> > > return g_strdup("xics");
> > > -} else if (spapr->irq == &spapr_irq_xive) {
> > > +} else if (!spapr_get_cap(spapr, SPAPR_CAP_XICS)
> > > + && spapr_get_cap(spapr, SPAPR_CAP_XIVE)) {
> > > return g_strdup("xive");
> > > -} else if (spapr->irq == &spapr_irq_dual) {
> > > +} else if (spapr_get_cap(spapr, SPAPR_CAP_XICS)
> > > + && spapr_get_cap(spapr, SPAPR_CAP_XIVE)) {
> > > return g_strdup("dual");
> > > }
> > > g_assert_not_reached();
> > > @@ -3266,11 +3273,14 @@ static void spapr_set_ic_mode(Object *obj, const
> > > char *value, Error **errp)
> > >
> > > /* The legacy IRQ backend can not be set */
> > > if (strcmp(value, "xics"
Re: [PATCH v4 17/19] spapr: Remove last pieces of SpaprIrq
On Wed, Oct 09, 2019 at 06:44:53PM +0200, Cédric Le Goater wrote:
> On 09/10/2019 08:08, David Gibson 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.
> >
> > Signed-off-by: David Gibson
>
> Nice. That is what you had in mind :) Let's make sure we use your
> cap framework for the next major change, power10.
Yeah, adding a cap-xive2 should be pretty straightforward.
> ic-mode should
> be deprecated one day I suppose.
I guess. I'm not in a great rush, since the compat code for it is
pretty simple and localized.
> Reviewed-by: Cédric Le Goater
>
> C.
>
>
> > ---
> > hw/ppc/spapr.c | 40 ++
> > hw/ppc/spapr_caps.c| 64 +
> > hw/ppc/spapr_hcall.c | 7 ++--
> > hw/ppc/spapr_irq.c | 84 ++
> > include/hw/ppc/spapr.h | 10 +++--
> > include/hw/ppc/spapr_irq.h | 10 -
> > 6 files changed, 103 insertions(+), 112 deletions(-)
> >
> > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > index e1ff03152e..bf9fdb1693 100644
> > --- a/hw/ppc/spapr.c
> > +++ b/hw/ppc/spapr.c
> > @@ -1072,12 +1072,13 @@ static void
> > spapr_dt_ov5_platform_support(SpaprMachineState *spapr, void *fdt,
> > 26, 0x40, /* Radix options: GTSE == yes. */
> > };
> >
> > -if (spapr->irq->xics && spapr->irq->xive) {
> > +if (spapr_get_cap(spapr, SPAPR_CAP_XICS)
> > +&& spapr_get_cap(spapr, SPAPR_CAP_XIVE)) {
> > val[1] = SPAPR_OV5_XIVE_BOTH;
> > -} else if (spapr->irq->xive) {
> > +} else if (spapr_get_cap(spapr, SPAPR_CAP_XIVE)) {
> > val[1] = SPAPR_OV5_XIVE_EXPLOIT;
> > } else {
> > -assert(spapr->irq->xics);
> > +assert(spapr_get_cap(spapr, SPAPR_CAP_XICS));
> > val[1] = SPAPR_OV5_XIVE_LEGACY;
> > }
> >
> > @@ -2075,6 +2076,8 @@ static const VMStateDescription vmstate_spapr = {
> > &vmstate_spapr_dtb,
> > &vmstate_spapr_cap_large_decr,
> > &vmstate_spapr_cap_ccf_assist,
> > +&vmstate_spapr_cap_xics,
> > +&vmstate_spapr_cap_xive,
> > NULL
> > }
> > };
> > @@ -2775,7 +2778,7 @@ static void spapr_machine_init(MachineState *machine)
> > spapr_ovec_set(spapr->ov5, OV5_DRMEM_V2);
> >
> > /* advertise XIVE on POWER9 machines */
> > -if (spapr->irq->xive) {
> > +if (spapr_get_cap(spapr, SPAPR_CAP_XIVE)) {
> > spapr_ovec_set(spapr->ov5, OV5_XIVE_EXPLOIT);
> > }
> >
> > @@ -3242,14 +3245,18 @@ static void spapr_set_vsmt(Object *obj, Visitor *v,
> > const char *name,
> > static char *spapr_get_ic_mode(Object *obj, Error **errp)
> > {
> > SpaprMachineState *spapr = SPAPR_MACHINE(obj);
> > +SpaprMachineClass *smc = SPAPR_MACHINE_GET_CLASS(spapr);
> >
> > -if (spapr->irq == &spapr_irq_xics_legacy) {
> > +if (smc->legacy_irq_allocation) {
> > return g_strdup("legacy");
> > -} else if (spapr->irq == &spapr_irq_xics) {
> > +} else if (spapr_get_cap(spapr, SPAPR_CAP_XICS)
> > + && !spapr_get_cap(spapr, SPAPR_CAP_XIVE)) {
> > return g_strdup("xics");
> > -} else if (spapr->irq == &spapr_irq_xive) {
> > +} else if (!spapr_get_cap(spapr, SPAPR_CAP_XICS)
> > + && spapr_get_cap(spapr, SPAPR_CAP_XIVE)) {
> > return g_strdup("xive");
> > -} else if (spapr->irq == &spapr_irq_dual) {
> > +} else if (spapr_get_cap(spapr, SPAPR_CAP_XICS)
> > + && spapr_get_cap(spapr, SPAPR_CAP_XIVE)) {
> > return g_strdup("dual");
> > }
> > g_assert_not_reached();
> > @@ -3266,11 +3273,14 @@ static void spapr_set_ic_mode(Object *obj, const
> > char *value, Error **errp)
> >
> > /* The legacy IRQ backend can not be set */
> > if (strcmp(value, "xics") == 0) {
> > -spapr->irq = &spapr_irq_xics;
> > +object_property_set_bool(obj, true, "cap-xics", errp);
> > +object_property_set_bool(obj, false, "cap-xive", errp);
> > } else if (strcmp(value, "xive") == 0) {
> > -spapr->irq = &spapr_irq_xive;
> > +object_property_set_bool(obj, false, "cap-xics", errp);
> > +object_property_set_bool(obj, true, "cap-xive", errp);
> > } else if (strcmp(value, "dual") == 0) {
> > -spapr->irq = &spapr_irq_dual;
> > +object_property_set_bool(obj, true, "cap-xics", errp);
> > +object_property_set_bool(obj, true, "cap-xive", errp);
> > } else {
> > error_setg(errp, "Bad value for \"ic-mode\" property");
> > }
> > @@ -3309,7 +3319,6 @@ static void spapr_set_host_serial(Object *obj, const
> > char *value, Error **errp)
> > static void spapr_instance_init(Object *obj)
> > {
> > SpaprMachineStat
Re: [PATCH v4 17/19] spapr: Remove last pieces of SpaprIrq
On Wed, Oct 09, 2019 at 07:02:15PM +0200, Greg Kurz wrote:
> On Wed, 9 Oct 2019 17:08:16 +1100
> David Gibson 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.
>
> > Signed-off-by: David Gibson
> > ---
> > hw/ppc/spapr.c | 40 ++
> > hw/ppc/spapr_caps.c| 64 +
> > hw/ppc/spapr_hcall.c | 7 ++--
> > hw/ppc/spapr_irq.c | 84 ++
> > include/hw/ppc/spapr.h | 10 +++--
> > include/hw/ppc/spapr_irq.h | 10 -
> > 6 files changed, 103 insertions(+), 112 deletions(-)
> >
> > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > index e1ff03152e..bf9fdb1693 100644
> > --- a/hw/ppc/spapr.c
> > +++ b/hw/ppc/spapr.c
> > @@ -1072,12 +1072,13 @@ static void
> > spapr_dt_ov5_platform_support(SpaprMachineState *spapr, void *fdt,
> > 26, 0x40, /* Radix options: GTSE == yes. */
> > };
> >
> > -if (spapr->irq->xics && spapr->irq->xive) {
> > +if (spapr_get_cap(spapr, SPAPR_CAP_XICS)
> > +&& spapr_get_cap(spapr, SPAPR_CAP_XIVE)) {
> > val[1] = SPAPR_OV5_XIVE_BOTH;
> > -} else if (spapr->irq->xive) {
> > +} else if (spapr_get_cap(spapr, SPAPR_CAP_XIVE)) {
> > val[1] = SPAPR_OV5_XIVE_EXPLOIT;
> > } else {
> > -assert(spapr->irq->xics);
> > +assert(spapr_get_cap(spapr, SPAPR_CAP_XICS));
> > val[1] = SPAPR_OV5_XIVE_LEGACY;
> > }
> >
> > @@ -2075,6 +2076,8 @@ static const VMStateDescription vmstate_spapr = {
> > &vmstate_spapr_dtb,
> > &vmstate_spapr_cap_large_decr,
> > &vmstate_spapr_cap_ccf_assist,
> > +&vmstate_spapr_cap_xics,
> > +&vmstate_spapr_cap_xive,
> > NULL
> > }
> > };
> > @@ -2775,7 +2778,7 @@ static void spapr_machine_init(MachineState *machine)
> > spapr_ovec_set(spapr->ov5, OV5_DRMEM_V2);
> >
> > /* advertise XIVE on POWER9 machines */
> > -if (spapr->irq->xive) {
> > +if (spapr_get_cap(spapr, SPAPR_CAP_XIVE)) {
> > spapr_ovec_set(spapr->ov5, OV5_XIVE_EXPLOIT);
> > }
> >
> > @@ -3242,14 +3245,18 @@ static void spapr_set_vsmt(Object *obj, Visitor *v,
> > const char *name,
> > static char *spapr_get_ic_mode(Object *obj, Error **errp)
> > {
> > SpaprMachineState *spapr = SPAPR_MACHINE(obj);
> > +SpaprMachineClass *smc = SPAPR_MACHINE_GET_CLASS(spapr);
> >
> > -if (spapr->irq == &spapr_irq_xics_legacy) {
> > +if (smc->legacy_irq_allocation) {
> > return g_strdup("legacy");
> > -} else if (spapr->irq == &spapr_irq_xics) {
> > +} else if (spapr_get_cap(spapr, SPAPR_CAP_XICS)
> > + && !spapr_get_cap(spapr, SPAPR_CAP_XIVE)) {
> > return g_strdup("xics");
> > -} else if (spapr->irq == &spapr_irq_xive) {
> > +} else if (!spapr_get_cap(spapr, SPAPR_CAP_XICS)
> > + && spapr_get_cap(spapr, SPAPR_CAP_XIVE)) {
> > return g_strdup("xive");
> > -} else if (spapr->irq == &spapr_irq_dual) {
> > +} else if (spapr_get_cap(spapr, SPAPR_CAP_XICS)
> > + && spapr_get_cap(spapr, SPAPR_CAP_XIVE)) {
> > return g_strdup("dual");
> > }
> > g_assert_not_reached();
> > @@ -3266,11 +3273,14 @@ static void spapr_set_ic_mode(Object *obj, const
> > char *value, Error **errp)
> >
> > /* The legacy IRQ backend can not be set */
> > if (strcmp(value, "xics") == 0) {
> > -spapr->irq = &spapr_irq_xics;
> > +object_property_set_bool(obj, true, "cap-xics", errp);
> > +object_property_set_bool(obj, false, "cap-xive", errp);
> > } else if (strcmp(value, "xive") == 0) {
> > -spapr->irq = &spapr_irq_xive;
> > +object_property_set_bool(obj,
Re: [PATCH v4 17/19] spapr: Remove last pieces of SpaprIrq
On Wed, 9 Oct 2019 17:08:16 +1100
David Gibson 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 ?
> Signed-off-by: David Gibson
> ---
> hw/ppc/spapr.c | 40 ++
> hw/ppc/spapr_caps.c| 64 +
> hw/ppc/spapr_hcall.c | 7 ++--
> hw/ppc/spapr_irq.c | 84 ++
> include/hw/ppc/spapr.h | 10 +++--
> include/hw/ppc/spapr_irq.h | 10 -
> 6 files changed, 103 insertions(+), 112 deletions(-)
>
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index e1ff03152e..bf9fdb1693 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -1072,12 +1072,13 @@ static void
> spapr_dt_ov5_platform_support(SpaprMachineState *spapr, void *fdt,
> 26, 0x40, /* Radix options: GTSE == yes. */
> };
>
> -if (spapr->irq->xics && spapr->irq->xive) {
> +if (spapr_get_cap(spapr, SPAPR_CAP_XICS)
> +&& spapr_get_cap(spapr, SPAPR_CAP_XIVE)) {
> val[1] = SPAPR_OV5_XIVE_BOTH;
> -} else if (spapr->irq->xive) {
> +} else if (spapr_get_cap(spapr, SPAPR_CAP_XIVE)) {
> val[1] = SPAPR_OV5_XIVE_EXPLOIT;
> } else {
> -assert(spapr->irq->xics);
> +assert(spapr_get_cap(spapr, SPAPR_CAP_XICS));
> val[1] = SPAPR_OV5_XIVE_LEGACY;
> }
>
> @@ -2075,6 +2076,8 @@ static const VMStateDescription vmstate_spapr = {
> &vmstate_spapr_dtb,
> &vmstate_spapr_cap_large_decr,
> &vmstate_spapr_cap_ccf_assist,
> +&vmstate_spapr_cap_xics,
> +&vmstate_spapr_cap_xive,
> NULL
> }
> };
> @@ -2775,7 +2778,7 @@ static void spapr_machine_init(MachineState *machine)
> spapr_ovec_set(spapr->ov5, OV5_DRMEM_V2);
>
> /* advertise XIVE on POWER9 machines */
> -if (spapr->irq->xive) {
> +if (spapr_get_cap(spapr, SPAPR_CAP_XIVE)) {
> spapr_ovec_set(spapr->ov5, OV5_XIVE_EXPLOIT);
> }
>
> @@ -3242,14 +3245,18 @@ static void spapr_set_vsmt(Object *obj, Visitor *v,
> const char *name,
> static char *spapr_get_ic_mode(Object *obj, Error **errp)
> {
> SpaprMachineState *spapr = SPAPR_MACHINE(obj);
> +SpaprMachineClass *smc = SPAPR_MACHINE_GET_CLASS(spapr);
>
> -if (spapr->irq == &spapr_irq_xics_legacy) {
> +if (smc->legacy_irq_allocation) {
> return g_strdup("legacy");
> -} else if (spapr->irq == &spapr_irq_xics) {
> +} else if (spapr_get_cap(spapr, SPAPR_CAP_XICS)
> + && !spapr_get_cap(spapr, SPAPR_CAP_XIVE)) {
> return g_strdup("xics");
> -} else if (spapr->irq == &spapr_irq_xive) {
> +} else if (!spapr_get_cap(spapr, SPAPR_CAP_XICS)
> + && spapr_get_cap(spapr, SPAPR_CAP_XIVE)) {
> return g_strdup("xive");
> -} else if (spapr->irq == &spapr_irq_dual) {
> +} else if (spapr_get_cap(spapr, SPAPR_CAP_XICS)
> + && spapr_get_cap(spapr, SPAPR_CAP_XIVE)) {
> return g_strdup("dual");
> }
> g_assert_not_reached();
> @@ -3266,11 +3273,14 @@ static void spapr_set_ic_mode(Object *obj, const char
> *value, Error **errp)
>
> /* The legacy IRQ backend can not be set */
> if (strcmp(value, "xics") == 0) {
> -spapr->irq = &spapr_irq_xics;
> +object_property_set_bool(obj, true, "cap-xics", errp);
> +object_property_set_bool(obj, false, "cap-xive", errp);
> } else if (strcmp(value, "xive") == 0) {
> -spapr->irq = &spapr_irq_xive;
> +object_property_set_bool(obj, false, "cap-xics", errp);
> +object_property_set_bool(obj, true, "cap-xive", errp);
> } else if (strcmp(value, "dual") == 0) {
> -spapr->irq = &spapr_irq_dual;
> +object_property_set_bool(obj, true, "cap-xics", errp);
> +object_property_set_bool(obj, true, "cap-xive", errp);
> } else {
> error_setg(errp, "Bad value for \"ic-mode\" property");
> }
> @@ -3309,7 +3319,6 @@ static void spapr_set_host_serial(Object *obj, const
> char *value, Error **errp)
> static void spapr_instance_init(Object *obj)
> {
> SpaprMachineState *spapr = SPAPR_MACHINE(obj);
> -SpaprMachineCl
Re: [PATCH v4 17/19] spapr: Remove last pieces of SpaprIrq
On 09/10/2019 08:08, David Gibson 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.
>
> Signed-off-by: David Gibson
Nice. That is what you had in mind :) Let's make sure we use your
cap framework for the next major change, power10. ic-mode should
be deprecated one day I suppose.
Reviewed-by: Cédric Le Goater
C.
> ---
> hw/ppc/spapr.c | 40 ++
> hw/ppc/spapr_caps.c| 64 +
> hw/ppc/spapr_hcall.c | 7 ++--
> hw/ppc/spapr_irq.c | 84 ++
> include/hw/ppc/spapr.h | 10 +++--
> include/hw/ppc/spapr_irq.h | 10 -
> 6 files changed, 103 insertions(+), 112 deletions(-)
>
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index e1ff03152e..bf9fdb1693 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -1072,12 +1072,13 @@ static void
> spapr_dt_ov5_platform_support(SpaprMachineState *spapr, void *fdt,
> 26, 0x40, /* Radix options: GTSE == yes. */
> };
>
> -if (spapr->irq->xics && spapr->irq->xive) {
> +if (spapr_get_cap(spapr, SPAPR_CAP_XICS)
> +&& spapr_get_cap(spapr, SPAPR_CAP_XIVE)) {
> val[1] = SPAPR_OV5_XIVE_BOTH;
> -} else if (spapr->irq->xive) {
> +} else if (spapr_get_cap(spapr, SPAPR_CAP_XIVE)) {
> val[1] = SPAPR_OV5_XIVE_EXPLOIT;
> } else {
> -assert(spapr->irq->xics);
> +assert(spapr_get_cap(spapr, SPAPR_CAP_XICS));
> val[1] = SPAPR_OV5_XIVE_LEGACY;
> }
>
> @@ -2075,6 +2076,8 @@ static const VMStateDescription vmstate_spapr = {
> &vmstate_spapr_dtb,
> &vmstate_spapr_cap_large_decr,
> &vmstate_spapr_cap_ccf_assist,
> +&vmstate_spapr_cap_xics,
> +&vmstate_spapr_cap_xive,
> NULL
> }
> };
> @@ -2775,7 +2778,7 @@ static void spapr_machine_init(MachineState *machine)
> spapr_ovec_set(spapr->ov5, OV5_DRMEM_V2);
>
> /* advertise XIVE on POWER9 machines */
> -if (spapr->irq->xive) {
> +if (spapr_get_cap(spapr, SPAPR_CAP_XIVE)) {
> spapr_ovec_set(spapr->ov5, OV5_XIVE_EXPLOIT);
> }
>
> @@ -3242,14 +3245,18 @@ static void spapr_set_vsmt(Object *obj, Visitor *v,
> const char *name,
> static char *spapr_get_ic_mode(Object *obj, Error **errp)
> {
> SpaprMachineState *spapr = SPAPR_MACHINE(obj);
> +SpaprMachineClass *smc = SPAPR_MACHINE_GET_CLASS(spapr);
>
> -if (spapr->irq == &spapr_irq_xics_legacy) {
> +if (smc->legacy_irq_allocation) {
> return g_strdup("legacy");
> -} else if (spapr->irq == &spapr_irq_xics) {
> +} else if (spapr_get_cap(spapr, SPAPR_CAP_XICS)
> + && !spapr_get_cap(spapr, SPAPR_CAP_XIVE)) {
> return g_strdup("xics");
> -} else if (spapr->irq == &spapr_irq_xive) {
> +} else if (!spapr_get_cap(spapr, SPAPR_CAP_XICS)
> + && spapr_get_cap(spapr, SPAPR_CAP_XIVE)) {
> return g_strdup("xive");
> -} else if (spapr->irq == &spapr_irq_dual) {
> +} else if (spapr_get_cap(spapr, SPAPR_CAP_XICS)
> + && spapr_get_cap(spapr, SPAPR_CAP_XIVE)) {
> return g_strdup("dual");
> }
> g_assert_not_reached();
> @@ -3266,11 +3273,14 @@ static void spapr_set_ic_mode(Object *obj, const char
> *value, Error **errp)
>
> /* The legacy IRQ backend can not be set */
> if (strcmp(value, "xics") == 0) {
> -spapr->irq = &spapr_irq_xics;
> +object_property_set_bool(obj, true, "cap-xics", errp);
> +object_property_set_bool(obj, false, "cap-xive", errp);
> } else if (strcmp(value, "xive") == 0) {
> -spapr->irq = &spapr_irq_xive;
> +object_property_set_bool(obj, false, "cap-xics", errp);
> +object_property_set_bool(obj, true, "cap-xive", errp);
> } else if (strcmp(value, "dual") == 0) {
> -spapr->irq = &spapr_irq_dual;
> +object_property_set_bool(obj, true, "cap-xics", errp);
> +object_property_set_bool(obj, true, "cap-xive", errp);
> } else {
> error_setg(errp, "Bad value for \"ic-mode\" property");
> }
> @@ -3309,7 +3319,6 @@ static void spapr_set_host_serial(Object *obj, const
> char *value, Error **errp)
> static void spapr_instance_init(Object *obj)
> {
> SpaprMachineState *spapr = SPAPR_MACHINE(obj);
> -SpaprMachineClass *smc = SPAPR_MACHINE_GET_CLASS(spapr);
>
> spapr->htab_fd = -1;
> spapr->use_hotplug_event_source = true;
> @@ -3345,7 +3354,6 @@ static void spapr_instance_init(Object *obj)
> spapr_get_msix_emulation, NULL, NULL);
>
> /* The machine class defines the default interrupt controller mode */
> -spapr->irq = smc->irq;
> object_property_add_s
