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

[Eduardo Habkost]

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:
> > On Mon, Jun 19, 2017 at 08:49:41PM +0800, Peter Xu wrote:
> > > Let KVM be the first user of the new AccelState.global_props field.
> > > Basically kvm accel only contains accel props for TYPE_X86_CPUs but not
> > > anything else yet.
> > > 
> > > There will be a change on how these global properties are applied for
> > > TYPE_X86_CPU devices. The general workflow of the global property stuff
> > > for TYPE_X86_CPU devices can be simplied as following (this is a example
> > > routine of KVM that contains both old/new workflow, similar thing apply
> > > to TCG, but even simpler):
> > > 
> > >    - HW_COMPAT/type_init() magic played before even entering main() [1]
> > 
> > What do you mean by this?  HW_COMPAT_* is used only in
> > MachineClass::compat_props[4], and type_init() magic is triggered
> > by module_call_init(MODULE_INIT_QOM) in main().
> 
> Sorry. It should be the thing you mentioned.
> 
> > 
> > >    - main() in vl.c
> > >      - configure_accelerator()
> > >        - AccelClass.init_machine() [2]
> > >          - kvm_init() (for KVM accel)
> > >      - register global properties
> > >        - accel_register_compat_props(): register accel compat props [3]
> > >        - machine_register_compat_props(): register machine compat
> > >          props (here we'll apply all the HW_COMPAT magic into
> > >          global_props) [4]
> > >      - machine init()
> > >        - cpu init() [5]
> > >        - ...
> > > 
> > > Before this patch, the code setup TYPE_X86_CPU properties at [4] by
> > > keeping the kvm_default_props list and apply them directly to the device
> > > using x86_cpu_apply_props().
> > > 
> > > After this patch, the code setup the same properties in the sequence of
> > > [1]->[2]->[3][4]->[5]:
> > > 
> > >   - At [1] we setup machine global properties.  Note: there will be
> > >     properties that with value==NULL but it's okay - when it was applied
> > >     to global list, it'll be used to remove an entry at step [4], it
> > >     works just like the old kvm_default_props, but we just unified it to
> > >     a single place, which is the global_props list.
> > > 
> > >   - At [2] we setup accel global properties.
> > 
> > Why isn't AccelClass::global_props set up at class_init(), just
> > like we do on MachineClass::compat_props?
> 
> (explained in other thread: there is a property we only need to
>  register when split irqchip is used)
> 
> > 
> > > 
> > >   - At [3]/[4] we move machine/accel properties into global property
> > >     list. One thing to mention is that, we do [3] before [4], since we
> > >     have some cross-relation properties, e.g., property that is required
> > >     when both PC=2.1 && ACCEL=kvm happen. For now, all this kind of
> > >     properties are still in machine compat properties.
> > > 
> > >   - At [5] when TYPE_X86_CPU creates, it applies the global property from
> > >     the global property list, which is now a merged list of three: accel
> > >     property list, machine property list, and user specified "-global"
> > >     properties.
> > 
> > On which category above would the x86_cpu_change_kvm_default()
> > calls in pc_piix.c would be?  We would need to ensure they
> > override the globals registered by the accel code, but they must
> > not override the user-provided global properties (including
> > -global and -cpu options).
> 
> Oh I didn't realize this difference. x86_cpu_change_kvm_default() is
> called in [5]. It indeed breaks this rule.
> 
> > 
> > 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.

> 
> 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.

-- 
Eduardo