From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:35615) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TiQWA-00012s-R9 for qemu-devel@nongnu.org; Tue, 11 Dec 2012 09:09:07 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1TiQW4-00025z-S7 for qemu-devel@nongnu.org; Tue, 11 Dec 2012 09:09:06 -0500 Received: from mx1.redhat.com ([209.132.183.28]:7137) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TiQW4-00025r-KD for qemu-devel@nongnu.org; Tue, 11 Dec 2012 09:09:00 -0500 Date: Tue, 11 Dec 2012 15:08:56 +0100 From: Igor Mammedov Message-ID: <20121211150856.1b6f44a3@nial.usersys.redhat.com> In-Reply-To: <20121211133145.GX4255@otherpad.lan.raisama.net> References: <1355220666-31722-1-git-send-email-imammedo@redhat.com> <1355220666-31722-3-git-send-email-imammedo@redhat.com> <20121211133145.GX4255@otherpad.lan.raisama.net> Mime-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 2/6] target-i386: sanitize AMD's ext2_features at realize time List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Eduardo Habkost Cc: Don@CloudSwitch.com, qemu-devel@nongnu.org, afaerber@suse.de On Tue, 11 Dec 2012 11:31:45 -0200 Eduardo Habkost 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 > > > > + } > > + > > if (!kvm_enabled()) { > > env->cpuid_features &= TCG_FEATURES; > > env->cpuid_ext_features &= TCG_EXT_FEATURES; > > -- > > 1.7.1 > > >