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

[Greg Kurz]

On Wed, 20 Nov 2019 16:38:37 +1100
David Gibson <address@hidden> 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.
>

pgp_qRr9UE1J8.pgp
Description: OpenPGP digital signature