[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH qom-cpu 05/11] target-i386: check/enforce: Fix C
From: |
Eduardo Habkost |
Subject: |
Re: [Qemu-devel] [PATCH qom-cpu 05/11] target-i386: check/enforce: Fix CPUID leaf numbers on error messages |
Date: |
Mon, 7 Jan 2013 09:54:23 -0200 |
User-agent: |
Mutt/1.5.21 (2010-09-15) |
On Sun, Jan 06, 2013 at 04:12:54PM +0200, Gleb Natapov wrote:
> On Fri, Jan 04, 2013 at 08:01:06PM -0200, Eduardo Habkost wrote:
> > The -cpu check/enforce warnings are printing incorrect information about the
> > missing flags. There are no feature flags on CPUID leaves 0 and 0x80000000,
> > but
> > there were references to 0 and 0x80000000 in the table at
> > kvm_check_features_against_host().
> >
> > This changes the model_features_t struct to contain the register number as
> > well, so the error messages print the correct CPUID leaf+register
> > information,
> > instead of wrong CPUID leaf numbers.
> >
> > This also changes the format of the error messages, so they follow the
> > "CPUID.<leaf>.<register>.<name> [bit <offset>]" convention used on Intel
> > documentation. Example output:
> >
> > $ qemu-system-x86_64 -machine pc-1.0,accel=kvm -cpu
> > Opteron_G4,+ia64,enforce
> > warning: host doesn't support requested feature: CPUID.01H:EDX.ia64
> > [bit 30]
> > warning: host doesn't support requested feature: CPUID.01H:ECX.xsave
> > [bit 26]
> > warning: host doesn't support requested feature: CPUID.01H:ECX.avx [bit
> > 28]
> > warning: host doesn't support requested feature:
> > CPUID.80000001H:ECX.abm [bit 5]
> > warning: host doesn't support requested feature:
> > CPUID.80000001H:ECX.sse4a [bit 6]
> > warning: host doesn't support requested feature:
> > CPUID.80000001H:ECX.misalignsse [bit 7]
> > warning: host doesn't support requested feature:
> > CPUID.80000001H:ECX.3dnowprefetch [bit 8]
> > warning: host doesn't support requested feature:
> > CPUID.80000001H:ECX.xop [bit 11]
> > warning: host doesn't support requested feature:
> > CPUID.80000001H:ECX.fma4 [bit 16]
> > Unable to find x86 CPU definition
> > $
> >
> > Signed-off-by: Eduardo Habkost <address@hidden>
> Reviewed-by: Gleb Natapov <address@hidden>
> But see the question below.
>
> > ---
> > Cc: Gleb Natapov <address@hidden>
> > Cc: Marcelo Tosatti <address@hidden>
> > Cc: address@hidden
> >
> > Changes v2:
> > - Coding style fixes
> > - Add assert() for invalid register numbers on
> > unavailable_host_feature()
> > ---
> > target-i386/cpu.c | 42 +++++++++++++++++++++++++++++++++---------
> > target-i386/cpu.h | 3 +++
> > 2 files changed, 36 insertions(+), 9 deletions(-)
> >
> > diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> > index e916ae0..c3e5db8 100644
> > --- a/target-i386/cpu.c
> > +++ b/target-i386/cpu.c
> > @@ -124,6 +124,25 @@ static const char *cpuid_7_0_ebx_feature_name[] = {
> > NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL,
> > };
> >
> > +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];
> > +}
> > +
> > /* collects per-function cpuid data
> > */
> > typedef struct model_features_t {
> > @@ -132,7 +151,8 @@ typedef struct model_features_t {
> > uint32_t check_feat;
> > const char **flag_names;
> > uint32_t cpuid;
> > - } model_features_t;
> > + int reg;
> > +} model_features_t;
> >
> > int check_cpuid = 0;
> > int enforce_cpuid = 0;
> > @@ -923,10 +943,13 @@ static int unavailable_host_feature(struct
> > model_features_t *f, uint32_t mask)
> >
> > for (i = 0; i < 32; ++i)
> > if (1 << i & mask) {
> > - fprintf(stderr, "warning: host cpuid %04x_%04x lacks requested"
> > - " flag '%s' [0x%08x]\n",
> > - f->cpuid >> 16, f->cpuid & 0xffff,
> > - f->flag_names[i] ? f->flag_names[i] : "[reserved]", mask);
> > + const char *reg = get_register_name_32(f->reg);
> > + assert(reg);
> > + fprintf(stderr, "warning: host doesn't support requested
> > feature: "
> > + "CPUID.%02XH:%s%s%s [bit %d]\n",
> > + f->cpuid, reg,
> > + f->flag_names[i] ? "." : "",
> > + f->flag_names[i] ? f->flag_names[i] : "", i);
> > break;
> > }
> > return 0;
> > @@ -945,13 +968,14 @@ static int kvm_check_features_against_host(x86_def_t
> > *guest_def)
> > int rv, i;
> > struct model_features_t ft[] = {
> > {&guest_def->features, &host_def.features,
> > - ~0, feature_name, 0x00000000},
> > + ~0, feature_name, 0x00000001, R_EDX},
> > {&guest_def->ext_features, &host_def.ext_features,
> > - ~CPUID_EXT_HYPERVISOR, ext_feature_name, 0x00000001},
> > + ~CPUID_EXT_HYPERVISOR, ext_feature_name, 0x00000001, R_ECX},
> > {&guest_def->ext2_features, &host_def.ext2_features,
> > - ~PPRO_FEATURES, ext2_feature_name, 0x80000000},
> > + ~PPRO_FEATURES, ext2_feature_name, 0x80000001, R_EDX},
> > {&guest_def->ext3_features, &host_def.ext3_features,
> > - ~CPUID_EXT3_SVM, ext3_feature_name, 0x80000001}};
> > + ~CPUID_EXT3_SVM, ext3_feature_name, 0x80000001, R_ECX}
> Why do we exclude PPRO_FEATURES/CPUID_EXT3_SVM from been checked?
I simply have no idea. I just have a few guesses:
- PPRO_FEATURES was supposed to be CPUID_EXT2_AMD_ALIASES, as
the user could want to expose vendor=AMD on a non-AMD host;
- Maybe CPUID_EXT3_SVM was being excluded because the old code checked
the host CPUID directly (instead of using GET_SUPPORTED_CPUID), so
the SVM bit in the host CPU wouldn't tell us anything about nested
SVM support.
They are removed on in separate patches that follow, and then the
check_feat field is removed completely.
>
> > + };
> >
> > assert(kvm_enabled());
> >
> > diff --git a/target-i386/cpu.h b/target-i386/cpu.h
> > index 27c8d0c..ab81a5c 100644
> > --- a/target-i386/cpu.h
> > +++ b/target-i386/cpu.h
> > @@ -1221,4 +1221,7 @@ void cpu_report_tpr_access(CPUX86State *env,
> > TPRAccess access);
> > void enable_kvm_pv_eoi(void);
> > void disable_kvm_mmu_op(void);
> >
> > +/* Return name of 32-bit register, from a R_* constant */
> > +const char *get_register_name_32(unsigned int reg);
> > +
> > #endif /* CPU_I386_H */
> > --
> > 1.7.11.7
>
> --
> Gleb.
--
Eduardo
- Re: [Qemu-devel] [PATCH qom-cpu 01/11] target-i386: Don't set any KVM flag by default if KVM is disabled, (continued)
[Qemu-devel] [PATCH qom-cpu 02/11] target-i386: Disable kvm_mmu_op by default on pc-1.4, Eduardo Habkost, 2013/01/04
[Qemu-devel] [PATCH qom-cpu 05/11] target-i386: check/enforce: Fix CPUID leaf numbers on error messages, Eduardo Habkost, 2013/01/04
[Qemu-devel] [PATCH qom-cpu 08/11] target-i386: check/enforce: Check SVM flag support as well, Eduardo Habkost, 2013/01/04
[Qemu-devel] [PATCH qom-cpu 11/11] target-i386: check/enforce: Check all feature words, Eduardo Habkost, 2013/01/04
Re: [Qemu-devel] [PATCH qom-cpu 00/11] disable-kvm_mmu + -cpu check/enforce fixes (v2), Andreas Färber, 2013/01/07