| Message ID | 1366657220-776-7-git-send-email-ehabkost@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 04/22/2013 01:00 PM, Eduardo Habkost wrote: > This property will be useful for libvirt, as libvirt already has logic > based on low-level feature bits (not feature names), so it will be > really easy to convert the current libvirt logic to something using the > "feature-words" property. > > The property will have two main use cases: > - Checking host capabilities, by checking the features of the "host" > CPU model > - Checking which features are enabled on each CPU model > > Example output: > > $ ./QMP/qmp --path=/tmp/m qom-get --path=/machine/unattached/device[1] --property=feature-words If I'm not mistaken, the QMP counterpart that libvirt will use is: { "execute":"qom-get", "arguments": { "path":"/machine/unattached/device[1]", "property":"feature-words" } } > item[0].cpuid-register: EDX > item[0].cpuid-input-eax: 2147483658 > item[0].features: 0 > item[1].cpuid-register: EAX > item[1].cpuid-input-eax: 1073741825 > item[1].features: 0 > item[2].cpuid-register: EDX > item[2].cpuid-input-eax: 3221225473 > item[2].features: 0 > item[3].cpuid-register: ECX > item[3].cpuid-input-eax: 2147483649 > item[3].features: 101 > item[4].cpuid-register: EDX > item[4].cpuid-input-eax: 2147483649 > item[4].features: 563346425 > item[5].cpuid-register: EBX > item[5].cpuid-input-eax: 7 > item[5].features: 0 > item[5].cpuid-input-ecx: 0 > item[6].cpuid-register: ECX > item[6].cpuid-input-eax: 1 > item[6].features: 2155880449 > item[7].cpuid-register: EDX > item[7].cpuid-input-eax: 1 > item[7].features: 126614521 And this would then be returned as a JSON array containing struct members looking like this: > +{ 'type': 'X86CPUFeatureWordInfo', > + 'data': { 'cpuid-input-eax': 'int', > + '*cpuid-input-ecx': 'int', > + 'cpuid-register': 'X86CPURegister32', > + 'features': 'int' } } Looks reasonable (and better than what we've had in the past!), although I'll let Jiri Denemark give final say on whether it meets libvirt's needs.
On Mon, Apr 22, 2013 at 02:37:06PM -0600, Eric Blake wrote: > On 04/22/2013 01:00 PM, Eduardo Habkost wrote: > > This property will be useful for libvirt, as libvirt already has logic > > based on low-level feature bits (not feature names), so it will be > > really easy to convert the current libvirt logic to something using the > > "feature-words" property. > > > > The property will have two main use cases: > > - Checking host capabilities, by checking the features of the "host" > > CPU model > > - Checking which features are enabled on each CPU model > > > > Example output: > > > > $ ./QMP/qmp --path=/tmp/m qom-get --path=/machine/unattached/device[1] --property=feature-words > > If I'm not mistaken, the QMP counterpart that libvirt will use is: > > { "execute":"qom-get", > "arguments": { "path":"/machine/unattached/device[1]", > "property":"feature-words" } } Yes, but note that libvirt needs to look for the X86CPU objects, to find the actual path as there's no guarantee that it will always be /machine/unattached/devices[1]. Maybe the CPU hotplug work will end up offering a predictable path for the X86CPU objects, but this is not available yet. > > > item[0].cpuid-register: EDX > > item[0].cpuid-input-eax: 2147483658 > > item[0].features: 0 > > item[1].cpuid-register: EAX > > item[1].cpuid-input-eax: 1073741825 > > item[1].features: 0 > > item[2].cpuid-register: EDX > > item[2].cpuid-input-eax: 3221225473 > > item[2].features: 0 > > item[3].cpuid-register: ECX > > item[3].cpuid-input-eax: 2147483649 > > item[3].features: 101 > > item[4].cpuid-register: EDX > > item[4].cpuid-input-eax: 2147483649 > > item[4].features: 563346425 > > item[5].cpuid-register: EBX > > item[5].cpuid-input-eax: 7 > > item[5].features: 0 > > item[5].cpuid-input-ecx: 0 > > item[6].cpuid-register: ECX > > item[6].cpuid-input-eax: 1 > > item[6].features: 2155880449 > > item[7].cpuid-register: EDX > > item[7].cpuid-input-eax: 1 > > item[7].features: 126614521 > > And this would then be returned as a JSON array containing struct > members looking like this: > > > +{ 'type': 'X86CPUFeatureWordInfo', > > + 'data': { 'cpuid-input-eax': 'int', > > + '*cpuid-input-ecx': 'int', > > + 'cpuid-register': 'X86CPURegister32', > > + 'features': 'int' } } Yes. > > Looks reasonable (and better than what we've had in the past!), although > I'll let Jiri Denemark give final say on whether it meets libvirt's needs. Thanks for the feedback!
On Mon, 22 Apr 2013 16:00:17 -0300 Eduardo Habkost <ehabkost@redhat.com> wrote: > This property will be useful for libvirt, as libvirt already has logic > based on low-level feature bits (not feature names), so it will be > really easy to convert the current libvirt logic to something using the > "feature-words" property. > > The property will have two main use cases: > - Checking host capabilities, by checking the features of the "host" > CPU model > - Checking which features are enabled on each CPU model patch doesn't apply to current qom-cpu, more comments below. > > Example output: > > $ ./QMP/qmp --path=/tmp/m qom-get --path=/machine/unattached/device[1] --property=feature-words > item[0].cpuid-register: EDX > item[0].cpuid-input-eax: 2147483658 > item[0].features: 0 > item[1].cpuid-register: EAX > item[1].cpuid-input-eax: 1073741825 > item[1].features: 0 > item[2].cpuid-register: EDX > item[2].cpuid-input-eax: 3221225473 > item[2].features: 0 > item[3].cpuid-register: ECX > item[3].cpuid-input-eax: 2147483649 > item[3].features: 101 > item[4].cpuid-register: EDX > item[4].cpuid-input-eax: 2147483649 > item[4].features: 563346425 > item[5].cpuid-register: EBX > item[5].cpuid-input-eax: 7 > item[5].features: 0 > item[5].cpuid-input-ecx: 0 could item be represented as CPUID function in format used in Intel's SDM? item[5].CPUID: EAX=7h,ECX=0h item[5].EBX: 0 item[5].EAX: 0 for simplicity/uniformity ECX could be not optional, always present, and ignored when not needed. > item[6].cpuid-register: ECX > item[6].cpuid-input-eax: 1 > item[6].features: 2155880449 > item[7].cpuid-register: EDX > item[7].cpuid-input-eax: 1 > item[7].features: 126614521 > > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> > --- > Changes v1 -> v2: > * Merge the non-qapi and qapi patches, to keep series simpler > * Use the feature word array series as base, so we don't have > to set the feature word values one-by-one in the code > * Change type name of property from "x86-cpu-feature-words" to > "X86CPUFeatureWordInfo" > * Remove cpu-qapi-schema.json and simply add the type definitions > to qapi-schema.json, to keep the changes simpler > * This required compiling qapi-types.o and qapi-visit.o into > *-user as well > --- > .gitignore | 2 ++ > Makefile.objs | 7 +++++- > qapi-schema.json | 31 ++++++++++++++++++++++++ > target-i386/cpu.c | 70 +++++++++++++++++++++++++++++++++++++++++++++---------- > 4 files changed, 97 insertions(+), 13 deletions(-) > > diff --git a/.gitignore b/.gitignore > index 487813a..71408e3 100644 > --- a/.gitignore > +++ b/.gitignore > @@ -21,6 +21,8 @@ linux-headers/asm > qapi-generated > qapi-types.[ch] > qapi-visit.[ch] > +cpu-qapi-types.[ch] > +cpu-qapi-visit.[ch] still needed? > qmp-commands.h > qmp-marshal.c > qemu-doc.html > diff --git a/Makefile.objs b/Makefile.objs > index a473348..1835d37 100644 > --- a/Makefile.objs > +++ b/Makefile.objs > @@ -78,10 +78,15 @@ common-obj-$(CONFIG_SMARTCARD_NSS) += $(libcacard-y) > ###################################################################### > # qapi > > -common-obj-y += qmp-marshal.o qapi-visit.o qapi-types.o > +common-obj-y += qmp-marshal.o > common-obj-y += qmp.o hmp.o > endif > > +###################################################################### > +# some qapi visitors are used by both system and user emulation: > + > +common-obj-y += qapi-visit.o qapi-types.o > + > ####################################################################### > # Target-independent parts used in system and user emulation > common-obj-y += qemu-log.o > diff --git a/qapi-schema.json b/qapi-schema.json > index 751d3c2..424434f 100644 > --- a/qapi-schema.json > +++ b/qapi-schema.json > @@ -3505,3 +3505,34 @@ > '*asl_compiler_rev': 'uint32', > '*file': 'str', > '*data': 'str' }} > + > +# @X86CPURegister32 > +# > +# A X86 32-bit register > +# > +# Since: 1.5 > +## > +{ 'enum': 'X86CPURegister32', > + 'data': [ 'EAX', 'EBX', 'ECX', 'EDX', 'ESP', 'EBP', 'ESI', 'EDI' ] } > + > +## > +# @X86CPUFeatureWordInfo > +# > +# Information about a X86 CPU feature word > +# > +# @cpuid-input-eax: Input EAX value for CPUID instruction for that feature word > +# > +# @cpuid-input-ecx: #optional Input ECX value for CPUID instruction for that > +# feature word > +# > +# @cpuid-register: Output register containing the feature bits > +# > +# @features: value of output register, containing the feature bits > +# > +# Since: 1.5 > +## > +{ 'type': 'X86CPUFeatureWordInfo', > + 'data': { 'cpuid-input-eax': 'int', > + '*cpuid-input-ecx': 'int', > + 'cpuid-register': 'X86CPURegister32', > + 'features': 'int' } } > diff --git a/target-i386/cpu.c b/target-i386/cpu.c > index 314931e..757749c 100644 > --- a/target-i386/cpu.c > +++ b/target-i386/cpu.c > @@ -30,6 +30,8 @@ > #include "qemu/config-file.h" > #include "qapi/qmp/qerror.h" > > +#include "qapi-types.h" > +#include "qapi-visit.h" > #include "qapi/visitor.h" > #include "sysemu/arch_init.h" > > @@ -194,23 +196,34 @@ static FeatureWordInfo feature_word_info[FEATURE_WORDS] = { > }, > }; > > +typedef struct X86RegisterInfo32 { > + /* Name of register */ > + const char *name; > + /* QAPI enum value register */ > + X86CPURegister32 qapi_enum; > +} X86RegisterInfo32; > + > +#define REGISTER(reg) \ > + [R_##reg] = { .name = #reg, .qapi_enum = X86_C_P_U_REGISTER32_##reg } ^^^^ Using auto-generated names like this probably is very fragile. > +X86RegisterInfo32 x86_reg_info_32[CPU_NB_REGS32] = { > + REGISTER(EAX), > + REGISTER(ECX), > + REGISTER(EDX), > + REGISTER(EBX), > + REGISTER(ESP), > + REGISTER(EBP), > + REGISTER(ESI), > + REGISTER(EDI), > +}; > +#undef REGISTER > + > + > const char *get_register_name_32(unsigned int reg) > { > - static const char *reg_names[CPU_NB_REGS32] = { > - [R_EAX] = "EAX", > - [R_ECX] = "ECX", > - [R_EDX] = "EDX", > - [R_EBX] = "EBX", > - [R_ESP] = "ESP", > - [R_EBP] = "EBP", > - [R_ESI] = "ESI", > - [R_EDI] = "EDI", > - }; > - > if (reg > CPU_NB_REGS32) { > return NULL; > } > - return reg_names[reg]; > + return x86_reg_info_32[reg].name; > } > > /* collects per-function cpuid data > @@ -1360,6 +1373,36 @@ static void x86_cpuid_set_tsc_freq(Object *obj, Visitor *v, void *opaque, > cpu->env.tsc_khz = value / 1000; > } > > +static void x86_cpu_get_feature_words(Object *obj, Visitor *v, void *opaque, > + const char *name, Error **errp) > +{ > + X86CPU *cpu = X86_CPU(obj); > + CPUX86State *env = &cpu->env; > + FeatureWord w; > + Error *err = NULL; > + X86CPUFeatureWordInfo word_infos[FEATURE_WORDS] = { }; > + X86CPUFeatureWordInfoList list_entries[FEATURE_WORDS] = { }; > + X86CPUFeatureWordInfoList *list = NULL; > + > + for (w = 0; w < FEATURE_WORDS; w++) { > + FeatureWordInfo *wi = &feature_word_info[w]; > + X86CPUFeatureWordInfo *qwi = &word_infos[w]; > + qwi->cpuid_input_eax = wi->cpuid_eax; > + qwi->has_cpuid_input_ecx = wi->cpuid_needs_ecx; > + qwi->cpuid_input_ecx = wi->cpuid_ecx; > + qwi->cpuid_register = x86_reg_info_32[wi->cpuid_reg].qapi_enum; Is there way not to use qapi_enum at all and use name instead? > + qwi->features = env->features[w]; > + > + /* List will be in reverse order, but order shouldn't matter */ > + list_entries[w].next = list; > + list_entries[w].value = &word_infos[w]; list_entries[w].value = qwi; > + list = &list_entries[w]; > + } > + > + visit_type_X86CPUFeatureWordInfoList(v, &list, "feature-words", &err); > + error_propagate(errp, err); > +} > + > static int cpu_x86_find_by_name(x86_def_t *x86_cpu_def, const char *name) > { > x86_def_t *def; > @@ -2348,6 +2391,9 @@ static void x86_cpu_initfn(Object *obj) > object_property_add(obj, "tsc-frequency", "int", > x86_cpuid_get_tsc_freq, > x86_cpuid_set_tsc_freq, NULL, NULL, NULL); > + object_property_add(obj, "feature-words", "X86CPUFeatureWordInfo", > + x86_cpu_get_feature_words, > + NULL, NULL, NULL, NULL); > > env->cpuid_apic_id = x86_cpu_apic_id_from_index(cs->cpu_index); > > -- > 1.8.1.4 > >
On Fri, May 03, 2013 at 01:34:23PM +0200, Igor Mammedov wrote: > On Mon, 22 Apr 2013 16:00:17 -0300 > Eduardo Habkost <ehabkost@redhat.com> wrote: > > > This property will be useful for libvirt, as libvirt already has logic > > based on low-level feature bits (not feature names), so it will be > > really easy to convert the current libvirt logic to something using the > > "feature-words" property. > > > > The property will have two main use cases: > > - Checking host capabilities, by checking the features of the "host" > > CPU model > > - Checking which features are enabled on each CPU model > > patch doesn't apply to current qom-cpu, more comments below. > > > > > Example output: > > > > $ ./QMP/qmp --path=/tmp/m qom-get --path=/machine/unattached/device[1] --property=feature-words > > item[0].cpuid-register: EDX > > item[0].cpuid-input-eax: 2147483658 > > item[0].features: 0 > > item[1].cpuid-register: EAX > > item[1].cpuid-input-eax: 1073741825 > > item[1].features: 0 > > item[2].cpuid-register: EDX > > item[2].cpuid-input-eax: 3221225473 > > item[2].features: 0 > > item[3].cpuid-register: ECX > > item[3].cpuid-input-eax: 2147483649 > > item[3].features: 101 > > item[4].cpuid-register: EDX > > item[4].cpuid-input-eax: 2147483649 > > item[4].features: 563346425 > > item[5].cpuid-register: EBX > > item[5].cpuid-input-eax: 7 > > item[5].features: 0 > > item[5].cpuid-input-ecx: 0 > could item be represented as CPUID function in format used in Intel's SDM? > We could, but maybe it would make the interface harder to use and not easier? Even when two feature words are returned in the same CPUID leaf, they are independent and separate feature-words that must be checked individually by libvirt, so I believe returning one feature-word per array-item makes more sense. Having an extra item in the array would make it clear for libvirt that QEMU has a new feature-word that libvirt doesn't know about, and easier to spot than an extra field in an existing array item. > item[5].CPUID: EAX=7h,ECX=0h What would be the data type of this "CPUID" field? Are you suggesting returning a string to be parsed manually? > item[5].EBX: 0 > item[5].EAX: 0 > > for simplicity/uniformity ECX could be not optional, always present, and > ignored when not needed. Then how would we represent the fact that ECX is optional? If ECX is not important for a given CPUID leaf, I don't see what's the point of including it with a fake value. Note that this interface is not supposed to be a comprehensive "CPUID dump" with all CPUID bits returned by the CPU, but just a list of "CPU capability words" that happen to be returned inside CPUID leaves. > > > item[6].cpuid-register: ECX > > item[6].cpuid-input-eax: 1 > > item[6].features: 2155880449 > > item[7].cpuid-register: EDX > > item[7].cpuid-input-eax: 1 > > item[7].features: 126614521 > > > > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> > > --- > > Changes v1 -> v2: > > * Merge the non-qapi and qapi patches, to keep series simpler > > * Use the feature word array series as base, so we don't have > > to set the feature word values one-by-one in the code > > * Change type name of property from "x86-cpu-feature-words" to > > "X86CPUFeatureWordInfo" > > * Remove cpu-qapi-schema.json and simply add the type definitions > > to qapi-schema.json, to keep the changes simpler > > * This required compiling qapi-types.o and qapi-visit.o into > > *-user as well > > --- > > .gitignore | 2 ++ > > Makefile.objs | 7 +++++- > > qapi-schema.json | 31 ++++++++++++++++++++++++ > > target-i386/cpu.c | 70 +++++++++++++++++++++++++++++++++++++++++++++---------- > > 4 files changed, 97 insertions(+), 13 deletions(-) > > > > diff --git a/.gitignore b/.gitignore > > index 487813a..71408e3 100644 > > --- a/.gitignore > > +++ b/.gitignore > > @@ -21,6 +21,8 @@ linux-headers/asm > > qapi-generated > > qapi-types.[ch] > > qapi-visit.[ch] > > +cpu-qapi-types.[ch] > > +cpu-qapi-visit.[ch] > still needed? Not needed anymore. Thanks for spotting it. > > > qmp-commands.h > > qmp-marshal.c > > qemu-doc.html > > diff --git a/Makefile.objs b/Makefile.objs > > index a473348..1835d37 100644 > > --- a/Makefile.objs > > +++ b/Makefile.objs > > @@ -78,10 +78,15 @@ common-obj-$(CONFIG_SMARTCARD_NSS) += $(libcacard-y) > > ###################################################################### > > # qapi > > > > -common-obj-y += qmp-marshal.o qapi-visit.o qapi-types.o > > +common-obj-y += qmp-marshal.o > > common-obj-y += qmp.o hmp.o > > endif > > > > +###################################################################### > > +# some qapi visitors are used by both system and user emulation: > > + > > +common-obj-y += qapi-visit.o qapi-types.o > > + > > ####################################################################### > > # Target-independent parts used in system and user emulation > > common-obj-y += qemu-log.o > > diff --git a/qapi-schema.json b/qapi-schema.json > > index 751d3c2..424434f 100644 > > --- a/qapi-schema.json > > +++ b/qapi-schema.json > > @@ -3505,3 +3505,34 @@ > > '*asl_compiler_rev': 'uint32', > > '*file': 'str', > > '*data': 'str' }} > > + > > +# @X86CPURegister32 > > +# > > +# A X86 32-bit register > > +# > > +# Since: 1.5 > > +## > > +{ 'enum': 'X86CPURegister32', > > + 'data': [ 'EAX', 'EBX', 'ECX', 'EDX', 'ESP', 'EBP', 'ESI', 'EDI' ] } > > + > > +## > > +# @X86CPUFeatureWordInfo > > +# > > +# Information about a X86 CPU feature word > > +# > > +# @cpuid-input-eax: Input EAX value for CPUID instruction for that feature word > > +# > > +# @cpuid-input-ecx: #optional Input ECX value for CPUID instruction for that > > +# feature word > > +# > > +# @cpuid-register: Output register containing the feature bits > > +# > > +# @features: value of output register, containing the feature bits > > +# > > +# Since: 1.5 > > +## > > +{ 'type': 'X86CPUFeatureWordInfo', > > + 'data': { 'cpuid-input-eax': 'int', > > + '*cpuid-input-ecx': 'int', > > + 'cpuid-register': 'X86CPURegister32', > > + 'features': 'int' } } > > diff --git a/target-i386/cpu.c b/target-i386/cpu.c > > index 314931e..757749c 100644 > > --- a/target-i386/cpu.c > > +++ b/target-i386/cpu.c > > @@ -30,6 +30,8 @@ > > #include "qemu/config-file.h" > > #include "qapi/qmp/qerror.h" > > > > +#include "qapi-types.h" > > +#include "qapi-visit.h" > > #include "qapi/visitor.h" > > #include "sysemu/arch_init.h" > > > > @@ -194,23 +196,34 @@ static FeatureWordInfo feature_word_info[FEATURE_WORDS] = { > > }, > > }; > > > > +typedef struct X86RegisterInfo32 { > > + /* Name of register */ > > + const char *name; > > + /* QAPI enum value register */ > > + X86CPURegister32 qapi_enum; > > +} X86RegisterInfo32; > > + > > +#define REGISTER(reg) \ > > + [R_##reg] = { .name = #reg, .qapi_enum = X86_C_P_U_REGISTER32_##reg } > ^^^^ > Using auto-generated names like this probably is very fragile. AFAIK, using the auto-generated name is the only way of using qapi enums and the qapi code generator is supposed to never break this. [...] > > +static void x86_cpu_get_feature_words(Object *obj, Visitor *v, void *opaque, > > + const char *name, Error **errp) > > +{ > > + X86CPU *cpu = X86_CPU(obj); > > + CPUX86State *env = &cpu->env; > > + FeatureWord w; > > + Error *err = NULL; > > + X86CPUFeatureWordInfo word_infos[FEATURE_WORDS] = { }; > > + X86CPUFeatureWordInfoList list_entries[FEATURE_WORDS] = { }; > > + X86CPUFeatureWordInfoList *list = NULL; > > + > > + for (w = 0; w < FEATURE_WORDS; w++) { > > + FeatureWordInfo *wi = &feature_word_info[w]; > > + X86CPUFeatureWordInfo *qwi = &word_infos[w]; > > + qwi->cpuid_input_eax = wi->cpuid_eax; > > + qwi->has_cpuid_input_ecx = wi->cpuid_needs_ecx; > > + qwi->cpuid_input_ecx = wi->cpuid_ecx; > > + qwi->cpuid_register = x86_reg_info_32[wi->cpuid_reg].qapi_enum; > Is there way not to use qapi_enum at all and use name instead? Are you suggesting making the qapi interface be string-based instead of using an enum? Why? > > > + qwi->features = env->features[w]; > > + > > + /* List will be in reverse order, but order shouldn't matter */ > > + list_entries[w].next = list; > > + list_entries[w].value = &word_infos[w]; > list_entries[w].value = qwi; Thanks, I will fix this. > > > + list = &list_entries[w]; > > + } > > + > > + visit_type_X86CPUFeatureWordInfoList(v, &list, "feature-words", &err); > > + error_propagate(errp, err); > > +} > > + > > static int cpu_x86_find_by_name(x86_def_t *x86_cpu_def, const char *name) > > { > > x86_def_t *def; > > @@ -2348,6 +2391,9 @@ static void x86_cpu_initfn(Object *obj) > > object_property_add(obj, "tsc-frequency", "int", > > x86_cpuid_get_tsc_freq, > > x86_cpuid_set_tsc_freq, NULL, NULL, NULL); > > + object_property_add(obj, "feature-words", "X86CPUFeatureWordInfo", > > + x86_cpu_get_feature_words, > > + NULL, NULL, NULL, NULL); > > > > env->cpuid_apic_id = x86_cpu_apic_id_from_index(cs->cpu_index); > > > > -- > > 1.8.1.4 > > > > > > > -- > Regards, > Igor
On 05/03/2013 07:17 AM, Eduardo Habkost wrote: > > We could, but maybe it would make the interface harder to use and not > easier? > > Even when two feature words are returned in the same CPUID leaf, they > are independent and separate feature-words that must be checked > individually by libvirt, so I believe returning one feature-word per > array-item makes more sense. Having an extra item in the array would > make it clear for libvirt that QEMU has a new feature-word that libvirt > doesn't know about, and easier to spot than an extra field in an > existing array item. Firmly agree - bundling multiple features into one array item is not nice. > > >> item[5].CPUID: EAX=7h,ECX=0h > > What would be the data type of this "CPUID" field? Are you suggesting > returning a string to be parsed manually? Anything that requires parsing to break into pieces on the receiving end implies that it was not correctly represented in JSON in the first place. I'd much rather see it kept as multiple fields. >>> + for (w = 0; w < FEATURE_WORDS; w++) { >>> + FeatureWordInfo *wi = &feature_word_info[w]; >>> + X86CPUFeatureWordInfo *qwi = &word_infos[w]; >>> + qwi->cpuid_input_eax = wi->cpuid_eax; >>> + qwi->has_cpuid_input_ecx = wi->cpuid_needs_ecx; >>> + qwi->cpuid_input_ecx = wi->cpuid_ecx; >>> + qwi->cpuid_register = x86_reg_info_32[wi->cpuid_reg].qapi_enum; >> Is there way not to use qapi_enum at all and use name instead? > > Are you suggesting making the qapi interface be string-based instead of > using an enum? Why? enum-based is better than string based. That way, when we add introspection in qemu 1.6, libvirt can see what enum values to expect, instead of having an open-ended set of strings with no idea what strings will be present.
On 04/22/2013 01:00 PM, Eduardo Habkost wrote: > This property will be useful for libvirt, as libvirt already has logic > based on low-level feature bits (not feature names), so it will be > really easy to convert the current libvirt logic to something using the > "feature-words" property. > > The property will have two main use cases: > - Checking host capabilities, by checking the features of the "host" > CPU model > - Checking which features are enabled on each CPU model > > item[6].cpuid-register: ECX > item[6].cpuid-input-eax: 1 > item[6].features: 2155880449 > item[7].cpuid-register: EDX > item[7].cpuid-input-eax: 1 > item[7].features: 126614521 I'm guessing the corresponding JSON passed over QMP would look something like: [ ... { "cpuid-register": "ECX", "cpuid-input-eax": 1, "features": 2155880449 }, { "cpuid-register": "EDX", "cpuid-input-eax": 1, "features": 126614521 } ] which libvirt can reasonably parse. > > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> > --- > Changes v1 -> v2: > * Merge the non-qapi and qapi patches, to keep series simpler > * Use the feature word array series as base, so we don't have > to set the feature word values one-by-one in the code > * Change type name of property from "x86-cpu-feature-words" to > "X86CPUFeatureWordInfo" > * Remove cpu-qapi-schema.json and simply add the type definitions > to qapi-schema.json, to keep the changes simpler > * This required compiling qapi-types.o and qapi-visit.o into > *-user as well > --- > .gitignore | 2 ++ > Makefile.objs | 7 +++++- > qapi-schema.json | 31 ++++++++++++++++++++++++ > target-i386/cpu.c | 70 +++++++++++++++++++++++++++++++++++++++++++++---------- > 4 files changed, 97 insertions(+), 13 deletions(-) I'm not sure I'm the best person to review cpu.c, but I can at least review the interface from what libvirt plans on using: > +++ b/qapi-schema.json > @@ -3505,3 +3505,34 @@ > '*asl_compiler_rev': 'uint32', > '*file': 'str', > '*data': 'str' }} > + > +# @X86CPURegister32 > +# > +# A X86 32-bit register > +# > +# Since: 1.5 Yes, I'd still like to get this into 1.5. On some enums, we have called out doc-text for each enum value; but I'm fine with your choice here to omit that. > +## > +{ 'enum': 'X86CPURegister32', > + 'data': [ 'EAX', 'EBX', 'ECX', 'EDX', 'ESP', 'EBP', 'ESI', 'EDI' ] } Any reason you favored ALL-CAPS names? But it doesn't matter to me, as long as we stick to the name once baked into a release. > + > +## > +# @X86CPUFeatureWordInfo > +# > +# Information about a X86 CPU feature word > +# > +# @cpuid-input-eax: Input EAX value for CPUID instruction for that feature word > +# > +# @cpuid-input-ecx: #optional Input ECX value for CPUID instruction for that > +# feature word > +# > +# @cpuid-register: Output register containing the feature bits > +# > +# @features: value of output register, containing the feature bits > +# > +# Since: 1.5 > +## > +{ 'type': 'X86CPUFeatureWordInfo', > + 'data': { 'cpuid-input-eax': 'int', > + '*cpuid-input-ecx': 'int', > + 'cpuid-register': 'X86CPURegister32', > + 'features': 'int' } } Looks reasonable for the interface side of things. > > +static void x86_cpu_get_feature_words(Object *obj, Visitor *v, void *opaque, > + const char *name, Error **errp) Indentation looks off. Reviewed-by: Eric Blake <eblake@redhat.com>
diff --git a/.gitignore b/.gitignore index 487813a..71408e3 100644 --- a/.gitignore +++ b/.gitignore @@ -21,6 +21,8 @@ linux-headers/asm qapi-generated qapi-types.[ch] qapi-visit.[ch] +cpu-qapi-types.[ch] +cpu-qapi-visit.[ch] qmp-commands.h qmp-marshal.c qemu-doc.html diff --git a/Makefile.objs b/Makefile.objs index a473348..1835d37 100644 --- a/Makefile.objs +++ b/Makefile.objs @@ -78,10 +78,15 @@ common-obj-$(CONFIG_SMARTCARD_NSS) += $(libcacard-y) ###################################################################### # qapi -common-obj-y += qmp-marshal.o qapi-visit.o qapi-types.o +common-obj-y += qmp-marshal.o common-obj-y += qmp.o hmp.o endif +###################################################################### +# some qapi visitors are used by both system and user emulation: + +common-obj-y += qapi-visit.o qapi-types.o + ####################################################################### # Target-independent parts used in system and user emulation common-obj-y += qemu-log.o diff --git a/qapi-schema.json b/qapi-schema.json index 751d3c2..424434f 100644 --- a/qapi-schema.json +++ b/qapi-schema.json @@ -3505,3 +3505,34 @@ '*asl_compiler_rev': 'uint32', '*file': 'str', '*data': 'str' }} + +# @X86CPURegister32 +# +# A X86 32-bit register +# +# Since: 1.5 +## +{ 'enum': 'X86CPURegister32', + 'data': [ 'EAX', 'EBX', 'ECX', 'EDX', 'ESP', 'EBP', 'ESI', 'EDI' ] } + +## +# @X86CPUFeatureWordInfo +# +# Information about a X86 CPU feature word +# +# @cpuid-input-eax: Input EAX value for CPUID instruction for that feature word +# +# @cpuid-input-ecx: #optional Input ECX value for CPUID instruction for that +# feature word +# +# @cpuid-register: Output register containing the feature bits +# +# @features: value of output register, containing the feature bits +# +# Since: 1.5 +## +{ 'type': 'X86CPUFeatureWordInfo', + 'data': { 'cpuid-input-eax': 'int', + '*cpuid-input-ecx': 'int', + 'cpuid-register': 'X86CPURegister32', + 'features': 'int' } } diff --git a/target-i386/cpu.c b/target-i386/cpu.c index 314931e..757749c 100644 --- a/target-i386/cpu.c +++ b/target-i386/cpu.c @@ -30,6 +30,8 @@ #include "qemu/config-file.h" #include "qapi/qmp/qerror.h" +#include "qapi-types.h" +#include "qapi-visit.h" #include "qapi/visitor.h" #include "sysemu/arch_init.h" @@ -194,23 +196,34 @@ static FeatureWordInfo feature_word_info[FEATURE_WORDS] = { }, }; +typedef struct X86RegisterInfo32 { + /* Name of register */ + const char *name; + /* QAPI enum value register */ + X86CPURegister32 qapi_enum; +} X86RegisterInfo32; + +#define REGISTER(reg) \ + [R_##reg] = { .name = #reg, .qapi_enum = X86_C_P_U_REGISTER32_##reg } +X86RegisterInfo32 x86_reg_info_32[CPU_NB_REGS32] = { + REGISTER(EAX), + REGISTER(ECX), + REGISTER(EDX), + REGISTER(EBX), + REGISTER(ESP), + REGISTER(EBP), + REGISTER(ESI), + REGISTER(EDI), +}; +#undef REGISTER + + const char *get_register_name_32(unsigned int reg) { - static const char *reg_names[CPU_NB_REGS32] = { - [R_EAX] = "EAX", - [R_ECX] = "ECX", - [R_EDX] = "EDX", - [R_EBX] = "EBX", - [R_ESP] = "ESP", - [R_EBP] = "EBP", - [R_ESI] = "ESI", - [R_EDI] = "EDI", - }; - if (reg > CPU_NB_REGS32) { return NULL; } - return reg_names[reg]; + return x86_reg_info_32[reg].name; } /* collects per-function cpuid data @@ -1360,6 +1373,36 @@ static void x86_cpuid_set_tsc_freq(Object *obj, Visitor *v, void *opaque, cpu->env.tsc_khz = value / 1000; } +static void x86_cpu_get_feature_words(Object *obj, Visitor *v, void *opaque, + const char *name, Error **errp) +{ + X86CPU *cpu = X86_CPU(obj); + CPUX86State *env = &cpu->env; + FeatureWord w; + Error *err = NULL; + X86CPUFeatureWordInfo word_infos[FEATURE_WORDS] = { }; + X86CPUFeatureWordInfoList list_entries[FEATURE_WORDS] = { }; + X86CPUFeatureWordInfoList *list = NULL; + + for (w = 0; w < FEATURE_WORDS; w++) { + FeatureWordInfo *wi = &feature_word_info[w]; + X86CPUFeatureWordInfo *qwi = &word_infos[w]; + qwi->cpuid_input_eax = wi->cpuid_eax; + qwi->has_cpuid_input_ecx = wi->cpuid_needs_ecx; + qwi->cpuid_input_ecx = wi->cpuid_ecx; + qwi->cpuid_register = x86_reg_info_32[wi->cpuid_reg].qapi_enum; + qwi->features = env->features[w]; + + /* List will be in reverse order, but order shouldn't matter */ + list_entries[w].next = list; + list_entries[w].value = &word_infos[w]; + list = &list_entries[w]; + } + + visit_type_X86CPUFeatureWordInfoList(v, &list, "feature-words", &err); + error_propagate(errp, err); +} + static int cpu_x86_find_by_name(x86_def_t *x86_cpu_def, const char *name) { x86_def_t *def; @@ -2348,6 +2391,9 @@ static void x86_cpu_initfn(Object *obj) object_property_add(obj, "tsc-frequency", "int", x86_cpuid_get_tsc_freq, x86_cpuid_set_tsc_freq, NULL, NULL, NULL); + object_property_add(obj, "feature-words", "X86CPUFeatureWordInfo", + x86_cpu_get_feature_words, + NULL, NULL, NULL, NULL); env->cpuid_apic_id = x86_cpu_apic_id_from_index(cs->cpu_index);
This property will be useful for libvirt, as libvirt already has logic based on low-level feature bits (not feature names), so it will be really easy to convert the current libvirt logic to something using the "feature-words" property. The property will have two main use cases: - Checking host capabilities, by checking the features of the "host" CPU model - Checking which features are enabled on each CPU model Example output: $ ./QMP/qmp --path=/tmp/m qom-get --path=/machine/unattached/device[1] --property=feature-words item[0].cpuid-register: EDX item[0].cpuid-input-eax: 2147483658 item[0].features: 0 item[1].cpuid-register: EAX item[1].cpuid-input-eax: 1073741825 item[1].features: 0 item[2].cpuid-register: EDX item[2].cpuid-input-eax: 3221225473 item[2].features: 0 item[3].cpuid-register: ECX item[3].cpuid-input-eax: 2147483649 item[3].features: 101 item[4].cpuid-register: EDX item[4].cpuid-input-eax: 2147483649 item[4].features: 563346425 item[5].cpuid-register: EBX item[5].cpuid-input-eax: 7 item[5].features: 0 item[5].cpuid-input-ecx: 0 item[6].cpuid-register: ECX item[6].cpuid-input-eax: 1 item[6].features: 2155880449 item[7].cpuid-register: EDX item[7].cpuid-input-eax: 1 item[7].features: 126614521 Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> --- Changes v1 -> v2: * Merge the non-qapi and qapi patches, to keep series simpler * Use the feature word array series as base, so we don't have to set the feature word values one-by-one in the code * Change type name of property from "x86-cpu-feature-words" to "X86CPUFeatureWordInfo" * Remove cpu-qapi-schema.json and simply add the type definitions to qapi-schema.json, to keep the changes simpler * This required compiling qapi-types.o and qapi-visit.o into *-user as well --- .gitignore | 2 ++ Makefile.objs | 7 +++++- qapi-schema.json | 31 ++++++++++++++++++++++++ target-i386/cpu.c | 70 +++++++++++++++++++++++++++++++++++++++++++++---------- 4 files changed, 97 insertions(+), 13 deletions(-)