Re: [Qemu-devel] [PATCH v3 06/13] kvm: let kvm use AccelState.global_props

[Peter Xu]

On Tue, Jun 20, 2017 at 11:07:34AM -0300, Eduardo Habkost wrote:
> On Tue, Jun 20, 2017 at 09:55:03PM +0800, Peter Xu wrote:
> > On Mon, Jun 19, 2017 at 01:14:03PM -0300, Eduardo Habkost wrote:

[...]

> > > This is where things get tricky and fragile: the translation from
> > > -cpu to global properties is done very late inside machine init
> > > today, but we should be able to do that much earlier, once we
> > > refactor the -cpu parsing code.
> > > 
> > > Hence my suggestion is to not touch x86_cpu_change_kvm_default()
> > > and just move the other properties (everything in
> > > kvm_default_props except svm, x2apic, and kvm-pv-eoi) to a static
> > > AccelClass::global_props field.
> > 
> > Yes it's fragile and complicated.
> > 
> > How about this:
> > 
> > I introduce AccelClass::global_props, only use it in Xen but nowhere
> > else? After all, what I really want to do is just let migration codes
> > start to use "-global" properties and compatibility fields. And if
> > there is still no good idea to ideally solve this x86 cpu property
> > issue, I would prefer to keep it (it'll also be simpler for me).
> 
> Sounds good to me.

Thanks for the confirmation. Let me cook another simpler series then.

> 
> > 
> > Another thing worries me a bit is that I may make things more
> > confusing if I separate this list into two (then we'll have part of
> > the properties in accel code, and the rest ones still in cpu.c).
> > 
> > (then I can also avoid using hard code in accel.c/kvm.c as well, which
> >  is something I really want to stop from doing. Maybe there can be
> >  some better idea, but I cannot really figure it out now...)
> > 
> > I'll just hold here to see whether you like above idea before moving
> > on to further comments.  Thanks,
> 
> 
> Agreed.
> 
> When I suggested using accel-provided global properties to
> replace kvm_default_props, I forgot x86_cpu_change_kvm_default()
> existed, and it makes things much more complex.
> 
> I really really want to make the existing x2apic/svm/kvm-pv-eoi
> compat stuff be based on static lists of properties.  If we make
> them dynamically built at runtime, we still can't introspect them
> and it won't be worth the extra complexity.
> 
> I believe we can still find a solution to represent the
> x2apic/svm/kvm-pv-eoi rules using static lists, but this
> shouldn't block the migration work you are doing.

One thing I think of is:

Let MachineClass::compat_props be MachCompatProp (rather than
GlobalProperty), then define it as:

struct MachCompatProp {
    GlobalProperty prop;
    bool (*prop_valid)();
};

(We may pass MigrationState *ms into prop_valid(), but for current
 requirements we may not need that, since what we need is basically
 tcg_enabled(), kvm_enabled(), or kvm_irqchip_is_split() checks)

Then this property will only be delivered to the global_props list
only if its prop_valid() check pass.  Thanks,

-- 
Peter Xu