Re: [Xen-devel] [V10 PATCH 23/23] PVH xen: introduce vmexit handler for PVH

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 24.07.13 at 03:59, Mukesh Rathor <mukesh.rathor@xxxxxxxxxx> wrote:
> +static int vmxit_msr_read(struct cpu_user_regs *regs)
> +{
> +    u64 msr_content = 0;
> +
> +    switch ( regs->ecx )
> +    {
> +    case MSR_IA32_MISC_ENABLE:
> +        rdmsrl(MSR_IA32_MISC_ENABLE, msr_content);
> +        msr_content |= MSR_IA32_MISC_ENABLE_BTS_UNAVAIL |
> +                       MSR_IA32_MISC_ENABLE_PEBS_UNAVAIL;
> +        break;
> +
> +    default:
> +        /* PVH fixme: see hvm_msr_read_intercept(). */
> +        rdmsrl(regs->ecx, msr_content);
> +        break;
> +    }
> +    regs->eax = (uint32_t)msr_content;
> +    regs->edx = (uint32_t)(msr_content >> 32);
> +    vmx_update_guest_eip();
> +
> +    dbgp1("msr read c:%lx a:%lx d:%lx RIP:%lx RSP:%lx\n", regs->ecx, 
> regs->eax,
> +          regs->edx, regs->rip, regs->rsp);
> +
> +    return 0;
> +}
> +
> +/* Returns : 0 == msr written successfully. */
> +static int vmxit_msr_write(struct cpu_user_regs *regs)
> +{
> +    uint64_t msr_content = (uint32_t)regs->eax | ((uint64_t)regs->edx << 32);

... = regs->_eax | (regs->rdx << 32);

would do - the construct you likely copied from elsewhere was only
necessary when we still had 32-bit support in the tree.

> +
> +    dbgp1("PVH: msr write:0x%lx. eax:0x%lx edx:0x%lx\n", regs->ecx,
> +          regs->eax, regs->edx);

Please settle on whether you use 0x prefixes or not (a few lines up
you don't), and if you do please use %#lx instead of spelling out
the 0x (a cleanup patch to that effect went in not too long ago).

> +static int vmxit_exception(struct cpu_user_regs *regs)
> +{
> +    int vector = (__vmread(VM_EXIT_INTR_INFO)) & INTR_INFO_VECTOR_MASK;
> +    int rc = -ENOSYS;
> +
> +    dbgp1(" EXCPT: vec:%d cs:%lx r.IP:%lx\n", vector,
> +          __vmread(GUEST_CS_SELECTOR), regs->eip);

Vectors printed in decimal are almost always useless.

And - what's "r.IP"? If you say "cs:", you should likewise say "rip:"
or "ip:".

> +static int access_cr0(struct cpu_user_regs *regs, uint acc_typ, uint64_t 
> *regp)
> +{
> +    struct vcpu *vp = current;
> +
> +    if ( acc_typ == VMX_CONTROL_REG_ACCESS_TYPE_MOV_TO_CR )
> +    {
> +        unsigned long new_cr0 = *regp;
> +        unsigned long old_cr0 = __vmread(GUEST_CR0);
> +
> +        dbgp1("PVH:writing to CR0. RIP:%lx val:0x%lx\n", regs->rip, *regp);

Here you're mixing 0x and not-0x even within a single message.
Such style can only help confusion.

Jan


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