| Message ID | 20170616075820.GE30118@pxdev.xzpeter.org |
|---|---|
| State | New |
| Headers | show |
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 <peterx@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.
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!
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 }, +}; + +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,