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
