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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 17.09.13 at 04:25, "Zhang, Yang Z" <yang.z.zhang@xxxxxxxxx> wrote:
> 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?

I case it came over ambiguously: The first comment alone was
meant to be indicated to be no reason for resubmitting. The
later comments, however, I would really want to see addressed
(if you don't, I'll likely end up polishing this, which would then still
want to be reviewed/tested by you again).

Jan


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