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

[Peter Xu <peterx@redhat.com>]

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

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,

-- 
Peter Xu