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: > > On Tue, Jun 13, 2017 at 11:41:02AM +0800, Peter Xu wrote: > > > On Mon, Jun 12, 2017 at 04:18:06PM +0800, Peter Xu wrote: > > > > On Fri, Jun 09, 2017 at 10:40:10AM -0300, Eduardo Habkost wrote: > > > > > On Fri, Jun 09, 2017 at 11:48:59AM +0800, Peter Xu wrote: > > > > > > Put it into MigrationState then we can use the properties to specify > > > > > > whether to enable storing global state. > > > > > > > > > > > > Removing global_state_set_optional() since now we can use > > > > > > HW_COMPAT_2_3 > > > > > > for x86/power in general, and the register_compat_prop() for > > > > > > xen_init(). > > > > > > > > > > > > Signed-off-by: Peter Xu <pet...@redhat.com> > > [...] > > > > > > diff --git a/hw/xen/xen-common.c b/hw/xen/xen-common.c > > > > > > index 0bed577..8240d50 100644 > > > > > > --- a/hw/xen/xen-common.c > > > > > > +++ b/hw/xen/xen-common.c > > > > > > @@ -138,7 +138,13 @@ static int xen_init(MachineState *ms) > > > > > > } > > > > > > qemu_add_vm_change_state_handler(xen_change_state_handler, > > > > > > NULL); > > > > > > > > > > > > - global_state_set_optional(); > > > > > > + /* > > > > > > + * TODO: make sure global MigrationState has not yet been > > > > > > created > > > > > > + * (otherwise the compat trick won't work). For now we are in > > > > > > + * configure_accelerator() so we are mostly good. Better to > > > > > > + * confirm that in the future. > > > > > > + */ > > > > > > > > > > So, this makes accelerator initialization very sensitive to > > > > > ordering. > > > > > > > > > > > + register_compat_prop("migration", "store-global-state", "off"); > > > > > > > > > > It's already hard to track down machine-type stuff that is > > > > > initialized at machine->init() time but it's not introspectable, > > > > > let's not do the same mistake with accelerators. > > > > > > > > > > Can't this be solved by a AcceleratorClass::global_props field, > > > > > so we are sure there's a single place in the code where globals > > > > > are registered? (Currently, they are all registered by the few > > > > > lines around the machine_register_compat_props() call in main()). > > > > > > > > > > I think I see other use cases for a new > > > > > AcceleratorClass::global_props field. e.g.: replacing > > > > > kvm_default_props and tcg_default_props in target/i386/cpu.c. > > > > > > > > Hmm, this sounds nice. Then we can also get rid of the TODO above. > > > > > > > > Thanks for your suggestion and review! I'll see whether I can writeup > > > > something as RFC for this. > > > > > > After a second thought, I see these requirements are slightly > > > different. > > > > > > For xen_init(), AccelClass::global_props may help since that's mainly > > > properties to be setup for TYPE_MIGRATION (let's assume it's okay that > > > MigrationState can be a QDev). > > > > > > For kvm_default_props and tcg_default_props, AccelClass::global_props > > > may not help since they are setting up properties for TYPE_CPU, and > > > TYPE_CPU cannot really use global property features yet. :( > > > > TYPE_CPU can use global properties, for sure. TYPE_CPU is a > > TYPE_DEVICE subclass, and -cpu is implemented internally using > > global properties. > > 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. > > --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. > +}; > + > +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); > + } > +#endif > +} > + > static int tcg_init(MachineState *ms) > { > tcg_exec_init(tcg_tb_size * 1024 * 1024); > + tcg_register_accel_props(ms->accelerator); > return 0; > } > > diff --git a/target/i386/cpu.c b/target/i386/cpu.c > index 5214fba..89f67b0 100644 > --- a/target/i386/cpu.c > +++ b/target/i386/cpu.c > @@ -1480,17 +1480,6 @@ static X86CPUDefinition builtin_x86_defs[] = { > }, > }; > > -typedef struct PropValue { > - const char *prop, *value; > -} PropValue; > - > -/* TCG-specific defaults that override all CPU models when using TCG > - */ > -static PropValue tcg_default_props[] = { > - { "vme", "off" }, > - { NULL, NULL }, > -}; > - > static uint32_t x86_cpu_get_supported_feature_word(FeatureWord w, > bool migratable_only); > > @@ -2262,18 +2251,6 @@ static void x86_cpu_report_filtered_features(X86CPU > *cpu) > } > } > > -static void x86_cpu_apply_props(X86CPU *cpu, PropValue *props) > -{ > - PropValue *pv; > - for (pv = props; pv->prop; pv++) { > - if (!pv->value) { > - continue; > - } > - object_property_parse(OBJECT(cpu), pv->value, pv->prop, > - &error_abort); > - } > -} > - > /* Load data from X86CPUDefinition into a X86CPU object > */ > static void x86_cpu_load_def(X86CPU *cpu, X86CPUDefinition *def, Error > **errp) > @@ -2300,11 +2277,6 @@ static void x86_cpu_load_def(X86CPU *cpu, > X86CPUDefinition *def, Error **errp) > env->features[w] = def->features[w]; > } > > - /* Special cases not set in the X86CPUDefinition structs: */ > - if (tcg_enabled()) { > - x86_cpu_apply_props(cpu, tcg_default_props); > - } > - > env->features[FEAT_1_ECX] |= CPUID_EXT_HYPERVISOR; > > /* sysenter isn't supported in compatibility mode on AMD, > -->8-- > > 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. -- Eduardo