| Message ID | 20160831235210.1976-1-tamas.lengyel@zentific.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 09/01/16 02:52, Tamas K Lengyel wrote: > Extend the CPUID monitor event to include EAX and ECX values that were used > when CPUID was executed. This is useful in identifying which leaf was queried. > We also adjust the xen-access output format to more closely resemble the output > of the Linux cpuid tool's raw format. > > Signed-off-by: Tamas K Lengyel <tamas.lengyel@zentific.com> > --- > Cc: Razvan Cojocaru <rcojocaru@bitdefender.com> > Cc: Ian Jackson <ian.jackson@eu.citrix.com> > Cc: Wei Liu <wei.liu2@citrix.com> > Cc: Jan Beulich <jbeulich@suse.com> > Cc: Andrew Cooper <andrew.cooper3@citrix.com> > Cc: Jun Nakajima <jun.nakajima@intel.com> > Cc: Kevin Tian <kevin.tian@intel.com> > --- > tools/tests/xen-access/xen-access.c | 4 +++- > xen/arch/x86/hvm/monitor.c | 5 ++++- > xen/arch/x86/hvm/vmx/vmx.c | 7 ++++++- > xen/include/asm-x86/hvm/monitor.h | 3 ++- > xen/include/public/vm_event.h | 7 +++++++ > 5 files changed, 22 insertions(+), 4 deletions(-) > > diff --git a/tools/tests/xen-access/xen-access.c b/tools/tests/xen-access/xen-access.c > index ebb63b1..b51b62c 100644 > --- a/tools/tests/xen-access/xen-access.c > +++ b/tools/tests/xen-access/xen-access.c > @@ -735,10 +735,12 @@ int main(int argc, char *argv[]) > break; > case VM_EVENT_REASON_CPUID: > printf("CPUID executed: rip=%016"PRIx64", vcpu %d. Insn length: %"PRIu32" " \ > - "EAX: 0x%"PRIx64" EBX: 0x%"PRIx64" ECX: 0x%"PRIx64" EDX: 0x%"PRIx64"\n", > + "0x%"PRIx32" 0x%"PRIx32": EAX=0x%"PRIx64" EBX=0x%"PRIx64" ECX=0x%"PRIx64" EDX=0x%"PRIx64"\n", > req.data.regs.x86.rip, > req.vcpu_id, > req.u.cpuid.insn_length, > + req.u.cpuid.eax, > + req.u.cpuid.ecx, > req.data.regs.x86.rax, > req.data.regs.x86.rbx, > req.data.regs.x86.rcx, > diff --git a/xen/arch/x86/hvm/monitor.c b/xen/arch/x86/hvm/monitor.c > index 7277c12..6540e2b 100644 > --- a/xen/arch/x86/hvm/monitor.c > +++ b/xen/arch/x86/hvm/monitor.c > @@ -136,7 +136,8 @@ int hvm_monitor_debug(unsigned long rip, enum hvm_monitor_debug_type type, > return monitor_traps(curr, sync, &req); > } > > -int hvm_monitor_cpuid(unsigned long insn_length) > +int hvm_monitor_cpuid(unsigned long insn_length, unsigned int eax, > + unsigned int ecx) > { > struct vcpu *curr = current; > struct arch_domain *ad = &curr->domain->arch; > @@ -148,6 +149,8 @@ int hvm_monitor_cpuid(unsigned long insn_length) > req.reason = VM_EVENT_REASON_CPUID; > req.vcpu_id = curr->vcpu_id; > req.u.cpuid.insn_length = insn_length; > + req.u.cpuid.eax = eax; > + req.u.cpuid.ecx = ecx; > > return monitor_traps(curr, 1, &req); > } > diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c > index 3d330b6..6df7612 100644 > --- 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; > > eax = regs->eax; > ebx = regs->ebx; > ecx = regs->ecx; > edx = regs->edx; > > + /* The CPUID monitor needs both the old and new values of EAX and ECX */ > + _eax = regs->eax; > + _ecx = regs->ecx; > + > vmx_cpuid_intercept(&eax, &ebx, &ecx, &edx); > > regs->eax = eax; > @@ -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);; > } > > static void vmx_dr_access(unsigned long exit_qualification, > diff --git a/xen/include/asm-x86/hvm/monitor.h b/xen/include/asm-x86/hvm/monitor.h > index a92f3fc..cdcbeca 100644 > --- a/xen/include/asm-x86/hvm/monitor.h > +++ b/xen/include/asm-x86/hvm/monitor.h > @@ -40,7 +40,8 @@ bool_t hvm_monitor_cr(unsigned int index, unsigned long value, > void hvm_monitor_msr(unsigned int msr, uint64_t value); > int hvm_monitor_debug(unsigned long rip, enum hvm_monitor_debug_type type, > unsigned long trap_type, unsigned long insn_length); > -int hvm_monitor_cpuid(unsigned long insn_length); > +int hvm_monitor_cpuid(unsigned long insn_length, unsigned int eax, > + unsigned int ecx); > > #endif /* __ASM_X86_HVM_MONITOR_H__ */ > > diff --git a/xen/include/public/vm_event.h b/xen/include/public/vm_event.h > index 64e6857..e0fee23 100644 > --- 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. > + */ > + uint32_t eax; > + uint32_t ecx; > uint32_t _pad; > }; Would it not be clearer if you named these old_eax and old_ecx? In user code it would be hard to choose between these and the values in vm_event_regs_x86, and then hard to understand why the choice was made reading the user code later on (unless a comment is added) and so on. It doesn't have to be old_eax and old_ecx, any other naming that prevents cofusion would be great. Thanks, Razvan
>>> On 01.09.16 at 09:26, <rcojocaru@bitdefender.com> wrote: > On 09/01/16 02:52, Tamas K Lengyel wrote: >> --- 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. >> + */ >> + uint32_t eax; >> + uint32_t ecx; >> uint32_t _pad; >> }; > > Would it not be clearer if you named these old_eax and old_ecx? In user > code it would be hard to choose between these and the values in > vm_event_regs_x86, and then hard to understand why the choice was made > reading the user code later on (unless a comment is added) and so on. > > It doesn't have to be old_eax and old_ecx, any other naming that > prevents cofusion would be great. What about leaf and subleaf? Jan
>>> On 01.09.16 at 01:52, <tamas.lengyel@zentific.com> 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? > @@ -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. > --- 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"? Jan
On 09/01/2016 10:58 AM, Jan Beulich wrote: >>>> On 01.09.16 at 09:26, <rcojocaru@bitdefender.com> wrote: >> On 09/01/16 02:52, Tamas K Lengyel wrote: >>> --- 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. >>> + */ >>> + uint32_t eax; >>> + uint32_t ecx; >>> uint32_t _pad; >>> }; >> >> Would it not be clearer if you named these old_eax and old_ecx? In user >> code it would be hard to choose between these and the values in >> vm_event_regs_x86, and then hard to understand why the choice was made >> reading the user code later on (unless a comment is added) and so on. >> >> It doesn't have to be old_eax and old_ecx, any other naming that >> prevents cofusion would be great. > > What about leaf and subleaf? That's perfect. Thanks, Razvan
On Thu, Sep 1, 2016 at 2:01 AM, Jan Beulich <JBeulich@suse.com> wrote: >>>> On 01.09.16 at 01:52, <tamas.lengyel@zentific.com> 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
On Thu, Sep 1, 2016 at 2:02 AM, Razvan Cojocaru <rcojocaru@bitdefender.com> wrote: > On 09/01/2016 10:58 AM, Jan Beulich wrote: >>>>> On 01.09.16 at 09:26, <rcojocaru@bitdefender.com> wrote: >>> On 09/01/16 02:52, Tamas K Lengyel wrote: >>>> --- 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. >>>> + */ >>>> + uint32_t eax; >>>> + uint32_t ecx; >>>> uint32_t _pad; >>>> }; >>> >>> Would it not be clearer if you named these old_eax and old_ecx? In user >>> code it would be hard to choose between these and the values in >>> vm_event_regs_x86, and then hard to understand why the choice was made >>> reading the user code later on (unless a comment is added) and so on. >>> >>> It doesn't have to be old_eax and old_ecx, any other naming that >>> prevents cofusion would be great. >> >> What about leaf and subleaf? > > That's perfect. > Sounds good to me too! Thanks, Tamas
diff --git a/tools/tests/xen-access/xen-access.c b/tools/tests/xen-access/xen-access.c index ebb63b1..b51b62c 100644 --- a/tools/tests/xen-access/xen-access.c +++ b/tools/tests/xen-access/xen-access.c @@ -735,10 +735,12 @@ int main(int argc, char *argv[]) break; case VM_EVENT_REASON_CPUID: printf("CPUID executed: rip=%016"PRIx64", vcpu %d. Insn length: %"PRIu32" " \ - "EAX: 0x%"PRIx64" EBX: 0x%"PRIx64" ECX: 0x%"PRIx64" EDX: 0x%"PRIx64"\n", + "0x%"PRIx32" 0x%"PRIx32": EAX=0x%"PRIx64" EBX=0x%"PRIx64" ECX=0x%"PRIx64" EDX=0x%"PRIx64"\n", req.data.regs.x86.rip, req.vcpu_id, req.u.cpuid.insn_length, + req.u.cpuid.eax, + req.u.cpuid.ecx, req.data.regs.x86.rax, req.data.regs.x86.rbx, req.data.regs.x86.rcx, diff --git a/xen/arch/x86/hvm/monitor.c b/xen/arch/x86/hvm/monitor.c index 7277c12..6540e2b 100644 --- a/xen/arch/x86/hvm/monitor.c +++ b/xen/arch/x86/hvm/monitor.c @@ -136,7 +136,8 @@ int hvm_monitor_debug(unsigned long rip, enum hvm_monitor_debug_type type, return monitor_traps(curr, sync, &req); } -int hvm_monitor_cpuid(unsigned long insn_length) +int hvm_monitor_cpuid(unsigned long insn_length, unsigned int eax, + unsigned int ecx) { struct vcpu *curr = current; struct arch_domain *ad = &curr->domain->arch; @@ -148,6 +149,8 @@ int hvm_monitor_cpuid(unsigned long insn_length) req.reason = VM_EVENT_REASON_CPUID; req.vcpu_id = curr->vcpu_id; req.u.cpuid.insn_length = insn_length; + req.u.cpuid.eax = eax; + req.u.cpuid.ecx = ecx; return monitor_traps(curr, 1, &req); } diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c index 3d330b6..6df7612 100644 --- 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; eax = regs->eax; ebx = regs->ebx; ecx = regs->ecx; edx = regs->edx; + /* The CPUID monitor needs both the old and new values of EAX and ECX */ + _eax = regs->eax; + _ecx = regs->ecx; + vmx_cpuid_intercept(&eax, &ebx, &ecx, &edx); regs->eax = eax; @@ -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);; } static void vmx_dr_access(unsigned long exit_qualification, diff --git a/xen/include/asm-x86/hvm/monitor.h b/xen/include/asm-x86/hvm/monitor.h index a92f3fc..cdcbeca 100644 --- a/xen/include/asm-x86/hvm/monitor.h +++ b/xen/include/asm-x86/hvm/monitor.h @@ -40,7 +40,8 @@ bool_t hvm_monitor_cr(unsigned int index, unsigned long value, void hvm_monitor_msr(unsigned int msr, uint64_t value); int hvm_monitor_debug(unsigned long rip, enum hvm_monitor_debug_type type, unsigned long trap_type, unsigned long insn_length); -int hvm_monitor_cpuid(unsigned long insn_length); +int hvm_monitor_cpuid(unsigned long insn_length, unsigned int eax, + unsigned int ecx); #endif /* __ASM_X86_HVM_MONITOR_H__ */ diff --git a/xen/include/public/vm_event.h b/xen/include/public/vm_event.h index 64e6857..e0fee23 100644 --- 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. + */ + uint32_t eax; + uint32_t ecx; uint32_t _pad; };
Extend the CPUID monitor event to include EAX and ECX values that were used when CPUID was executed. This is useful in identifying which leaf was queried. We also adjust the xen-access output format to more closely resemble the output of the Linux cpuid tool's raw format. Signed-off-by: Tamas K Lengyel <tamas.lengyel@zentific.com> --- Cc: Razvan Cojocaru <rcojocaru@bitdefender.com> Cc: Ian Jackson <ian.jackson@eu.citrix.com> Cc: Wei Liu <wei.liu2@citrix.com> Cc: Jan Beulich <jbeulich@suse.com> Cc: Andrew Cooper <andrew.cooper3@citrix.com> Cc: Jun Nakajima <jun.nakajima@intel.com> Cc: Kevin Tian <kevin.tian@intel.com> --- tools/tests/xen-access/xen-access.c | 4 +++- xen/arch/x86/hvm/monitor.c | 5 ++++- xen/arch/x86/hvm/vmx/vmx.c | 7 ++++++- xen/include/asm-x86/hvm/monitor.h | 3 ++- xen/include/public/vm_event.h | 7 +++++++ 5 files changed, 22 insertions(+), 4 deletions(-)