Re: [Qemu-devel] [PATCH v2 3/6] migration: move global_state.optional out

[Peter Xu]

On Fri, Jun 16, 2017 at 11:34:32AM -0300, Eduardo Habkost wrote:
> On Fri, Jun 16, 2017 at 03:58:20PM +0800, Peter Xu wrote:
> > On Tue, Jun 13, 2017 at 08:16:35AM -0300, Eduardo Habkost wrote:

[...]

> > Hi, Eduardo,
> > 
> > I'm working on providing a AccelState.global_props, then let KVM/TCG
> > to use it to handle X86_CPU properties there.  However, I stuck at a
> > point where TCG/KVM cannot know what is the macro "TYPE_X86_CPU"
> > (since it's only there on X86).  The change is something like this (it
> > cannot be applied directly to master since it's only one patch among
> > my series, it is used to show what I am doing and the problem):
> 
> Unfortunately TYPE_X86_CPU macro is arch-specific because it may
> be expanded to "x86_64-cpu" or "i386-cpu" depending on the QEMU
> target.  So it actually represents two different classes, because
> the target/i386/cpu.c code is compiled twice (once for i386 and
> once for x86_64).
> 
> In this case, you will need two entries on the tcg default
> property list:
> 
>   x86_64-cpu.vme=off
>   i386-cpu.vme=off
> 
> Now, we could hardcode the class names, or provide TYPE_I386_CPU
> and TYPE_X86_64_CPU macros from a header available to generic
> code[1].  I don't know what's the best solution.
> 
> [1] i.e. not cpu.h. I was going to suggest including
>     "target/i386/cpu-qom.h", but I'm not sure if
>     it can be safely included by generic code.

Thanks. I'll choose to hard code it for now (with some comments).

> 
> > 
> > --8<--
> > diff --git a/accel.c b/accel.c
> > index 82b318c..db503b6 100644
> > --- a/accel.c
> > +++ b/accel.c
> > @@ -34,13 +34,34 @@
> >  #include "sysemu/qtest.h"
> >  #include "hw/xen/xen.h"
> >  #include "qom/object.h"
> > +#if TARGET_I386
> > +#include "target/i386/cpu-qom.h"
> > +#endif
> > 
> >  int tcg_tb_size;
> >  static bool tcg_allowed = true;
> > 
> > +static AccelPropValue x86_tcg_default_props[] = {
> > +    { "vme", "off" },
> > +    { NULL, NULL },
> 
> You need a "driver" (type) field too.  I suggest using struct
> GlobalProperty like we do on MachineClass.

Yeah, I did it ...

> 
> > +};
> > +
> > +static void tcg_register_accel_props(AccelState *accel)
> > +{
> > +#if TARGET_I386
> > +    AccelPropValue *entry;
> > +
> > +    for (entry = x86_tcg_default_props; entry->prop; entry++) {
> > +        global_property_list_register(accel->global_props, TYPE_X86_CPU,
> > +                                      entry->prop, entry->value, false);

... here, since the driver field is the same (we'll just apply this
property list to both "i386-cpu" and "x86_64-cpu").

> > +    }
> > +#endif
> > +}
> > +

[...]

> > What above patch did is something like "moving x86_cpu properties from
> > x86 CPU codes into tcg" (I am using tcg as example, kvm is more
> > complex but has similar issue).
> > 
> > Here the general problem is that, there are some properties to be
> > applied when both conditions match:
> > 
> > - target is X86 (so using X86 cpus)
> > - accel is tcg
> > 
> > In the old code, it works since in x86 cpu.c codes we can use this:
> > 
> >   if (tcg_enabled()) {
> >       x86_cpu_apply_props(cpu, tcg_default_props);
> >   }
> > 
> > to know "whether accel is TCG". However I cannot do similar thing in
> > TCG code to know "whether target is x86". (I was trying to use
> > TARGET_I386 but it was poisoned, of course...)
> > 
> > Any thoughts? (Markus/Paolo/others?)
> 
> You don't need to make the property conditional on the target, if
> you just use the right class name on the "driver' field.

Yeah, as long as I can use the hard coded "i386-cpu"/"x86_64-cpu" in
accelerator codes, then I'll be fine.  Thanks again!

-- 
Peter Xu