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

["Zhang, Yang Z" <yang.z.zhang@xxxxxxxxx>]

Jan Beulich wrote on 2013-09-11:
>>>> 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;
>> +            }
>> +        }
>
Should I resend this or just the first patch is enough?

Best regards,
Yang



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