[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 2/6] target-i386: sanitize AMD's ext2_features a
From: |
Igor Mammedov |
Subject: |
Re: [Qemu-devel] [PATCH 2/6] target-i386: sanitize AMD's ext2_features at realize time |
Date: |
Tue, 11 Dec 2012 15:08:56 +0100 |
On Tue, 11 Dec 2012 11:31:45 -0200
Eduardo Habkost <address@hidden> wrote:
[...]
> > ---
> > target-i386/cpu.c | 21 +++++++++++----------
> > 1 files changed, 11 insertions(+), 10 deletions(-)
> >
> > diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> > index 63aae86..64b7637 100644
> > --- a/target-i386/cpu.c
> > +++ b/target-i386/cpu.c
> > @@ -1539,16 +1539,6 @@ int cpu_x86_register(X86CPU *cpu, const char
> > *cpu_model) object_property_set_int(OBJECT(cpu), (int64_t)def->tsc_khz *
> > 1000, "tsc-frequency", &error);
> >
> > - /* On AMD CPUs, some CPUID[8000_0001].EDX bits must match the bits on
> > - * CPUID[1].EDX.
> > - */
> > - if (env->cpuid_vendor1 == CPUID_VENDOR_AMD_1 &&
> > - env->cpuid_vendor2 == CPUID_VENDOR_AMD_2 &&
> > - env->cpuid_vendor3 == CPUID_VENDOR_AMD_3) {
> > - env->cpuid_ext2_features &= ~CPUID_EXT2_AMD_ALIASES;
> > - env->cpuid_ext2_features |= (def->features &
> > CPUID_EXT2_AMD_ALIASES);
> > - }
> > -
> > object_property_set_str(OBJECT(cpu), def->model_id, "model-id",
> > &error); if (error) {
> > fprintf(stderr, "%s\n", error_get_pretty(error));
> > @@ -2062,6 +2052,17 @@ void x86_cpu_realize(Object *obj, Error **errp)
> > env->cpuid_level = 7;
> > }
> >
> > + /* On AMD CPUs, some CPUID[8000_0001].EDX bits must match the bits on
> > + * CPUID[1].EDX.
> > + */
> > + if (env->cpuid_vendor1 == CPUID_VENDOR_AMD_1 &&
> > + env->cpuid_vendor2 == CPUID_VENDOR_AMD_2 &&
> > + env->cpuid_vendor3 == CPUID_VENDOR_AMD_3) {
>
> I would add extra indentation space here, to make it not align with the
> statements below, making the condition visually distinct from the body,
> like in the original code you are moving.
I've thought people would object to ident here, that's why I've changed
original indentation to a more consistent with rules.
BTW: git grep -A 3 "if (.*[^{)]$"
shows that many places use this style including fairly recent ones:
aio-posix.c first hit
and we have in target-i386/cpu.c
if (x86_cpu_def->vendor1 == CPUID_VENDOR_VIA_1 &
...
with the same alignment style and at least one more similar.
Lets leave it this way to be consistent with the rest of the code.
>
> > + env->cpuid_ext2_features &= ~CPUID_EXT2_AMD_ALIASES;
> > + env->cpuid_ext2_features |= (env->cpuid_features
> > + & CPUID_EXT2_AMD_ALIASES);
>
> Weird spacing around the "&" above (3 spaces indent, 2 spaces after the
> "&").
>
> I would align this as:
>
> env->cpuid_ext2_features |= (env->cpuid_features &
> CPUID_EXT2_AMD_ALIASES);
Thanks, fixed.
>
>
> As the above issues are only cosmetic:
>
> Reviewed-by: Eduardo Habkost <address@hidden>
>
>
> > + }
> > +
> > if (!kvm_enabled()) {
> > env->cpuid_features &= TCG_FEATURES;
> > env->cpuid_ext_features &= TCG_EXT_FEATURES;
> > --
> > 1.7.1
> >
>
- Re: [Qemu-devel] [PATCH 1/6] target-i386: filter out not TCG features if running without kvm at realize time, (continued)