Re: [Xen-devel] [PATCH] x86/monitor: Include EAX/ECX in CPUID monitor events

[Tamas K Lengyel <tamas.lengyel@xxxxxxxxxxxx>]

On Thu, Sep 1, 2016 at 2:01 AM, Jan Beulich <JBeulich@xxxxxxxx> wrote:
>>>> On 01.09.16 at 01:52, <tamas.lengyel@xxxxxxxxxxxx> wrote:
>> --- a/xen/arch/x86/hvm/vmx/vmx.c
>> +++ b/xen/arch/x86/hvm/vmx/vmx.c
>> @@ -2402,12 +2402,17 @@ static void vmx_cpuid_intercept(
>>  static int vmx_do_cpuid(struct cpu_user_regs *regs)
>>  {
>>      unsigned int eax, ebx, ecx, edx;
>> +    unsigned int _eax, _ecx;
>
> Please avoid adding new name space violations like this: Identifiers
> starting with an underscore and a lower case letter are intended to
> be used for file scope symbols. May I, just like for the public header,
> suggest using leaf and subleaf here?

Yeap, makes sense.

>
>> @@ -2415,7 +2420,7 @@ static int vmx_do_cpuid(struct cpu_user_regs *regs)
>>      regs->ecx = ecx;
>>      regs->edx = edx;
>>
>> -    return hvm_monitor_cpuid(get_instruction_length());
>> +    return hvm_monitor_cpuid(get_instruction_length(), _eax, _ecx);;
>
> Stray semicolon.

Ack,

>
>> --- a/xen/include/public/vm_event.h
>> +++ b/xen/include/public/vm_event.h
>> @@ -226,6 +226,13 @@ struct vm_event_mov_to_msr {
>>
>>  struct vm_event_cpuid {
>>      uint32_t insn_length;
>> +    /*
>> +     * Value of EAX and ECX when CPUID was executed.
>> +     * Note that the resulting register values are accessible in
>> +     * vm_event_regs_x86.
>> +     */
>
> "resulting" is a little ambiguous. How about "output"?

So the point of the comment really was just to disambiguate the
eax/ecx here from the other set of registers. Now that the names
changed the comment is not needed.

Thanks,
Tamas

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