Re: [Xen-devel] [PATCH v3 3/3] Nested VMX: Fix IA32_VMX_CR4_FIXED1 msr emulation

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 11.09.13 at 04:52, Yang Zhang <yang.z.zhang@xxxxxxxxx> wrote:
> +        hvm_cpuid(0x0, &eax, &ebx, &ecx, &edx);
> +        if ( eax >= 0x7 )

Not a need to re-submit this patch, but for the future I'd recommend
trying to avoid deep scope nesting where possible. Here that's
pretty simple: If for whatever reason you dislike using the switch()
approach I suggested in the v2 review, invert the condition above,
and make the body of the if() simply "break;".

> +        {
> +            ecx = 0;
> +            hvm_cpuid(0x7, &ecx, &ebx, &ecx, &edx);

That's confusing: A casual reader might assume that the first use
of &ecx is actually a typo. Either use a dummy variable, or at
least be consistent and funnel _all_ unused output into the same
variable (i.e. hvm_cpuid(0x7, &ecx, &ebx, &ecx, &ecx)). But
again, the switch() approach suggested earlier would avoid all
that.

In the end, with more uses of hvm_cpuid() appearing, we may
want to make the function tolerate NULL inputs to allow to make
clear at the call site which of the outputs aren't cared about.

> +            if ( ebx & cpufeat_mask(X86_FEATURE_FSGSBASE) )
> +                data |= X86_CR4_FSGSBASE;
> +            if ( ebx & cpufeat_mask(X86_FEATURE_SMEP) )
> +                data |= X86_CR4_SMEP;
> +            if ( ebx & cpufeat_mask(X86_FEATURE_SMAP) )
> +                data |= X86_CR4_SMAP;
> +
> +            if ( eax >= 0xa )
> +            {

Same here then.

Jan

> +                hvm_cpuid(0xa, &eax, &ebx, &ecx, &edx);
> +                /* Check whether guest has the perf monitor feature. */
> +                if ( (eax & 0xff) && (eax & 0xff00) )
> +                    data |= X86_CR4_PCE;
> +            }
> +        }



_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel