Re: [Xen-devel] [PATCH 18/18] PVH xen: introduce vmx_pvh.c

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 25.06.13 at 02:01, Mukesh Rathor <mukesh.rathor@xxxxxxxxxx> wrote:
> --- /dev/null
> +++ b/xen/arch/x86/hvm/vmx/vmx_pvh.c

Just pvh.c please - the directory we're in already tells us that this is
VMX.

> @@ -0,0 +1,523 @@
> +/*
> + * Copyright (C) 2013, Mukesh Rathor, Oracle Corp.  All rights reserved.
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public
> + * License v2 as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + * General Public License for more details.
> + */
> +
> +#include <xen/hypercall.h>
> +#include <xen/guest_access.h>
> +#include <asm/p2m.h>
> +#include <asm/traps.h>
> +#include <asm/hvm/vmx/vmx.h>
> +#include <public/sched.h>
> +#include <asm/hvm/nestedhvm.h>
> +#include <asm/xstate.h>
> +
> +#ifndef NDEBUG
> +int pvhdbg = 0;

This apparently not being declared in a header suggests that it
should be static.

> +#define dbgp1(...) do { (pvhdbg == 1) ? printk(__VA_ARGS__) : 0; } while ( 0 
> )
> +#else
> +#define dbgp1(...) ((void)0)
> +#endif
> +
> +
> +/* NOTE: this does NOT read the CS. */

Should probably also say why.

> +static void read_vmcs_selectors(struct cpu_user_regs *regs)
> +{
> +    regs->ss = __vmread(GUEST_SS_SELECTOR);
> +    regs->ds = __vmread(GUEST_DS_SELECTOR);
> +    regs->es = __vmread(GUEST_ES_SELECTOR);
> +    regs->gs = __vmread(GUEST_GS_SELECTOR);
> +    regs->fs = __vmread(GUEST_FS_SELECTOR);
> +}

By only conditionally reading the selector registers, how do you
guarantee that read_segment_register() would always read
valid values? I think that macro needs to not look at "regs->?s"
at all...

> +static int vmxit_debug(struct cpu_user_regs *regs)
> +{
> +    struct vcpu *vp = current;
> +    unsigned long exit_qualification = __vmread(EXIT_QUALIFICATION);
> +
> +    write_debugreg(6, exit_qualification | 0xffff0ff0);
> +
> +    /* gdbsx or another debugger. */
> +    if ( vp->domain->domain_id != 0 &&    /* never pause dom0 */
> +         guest_kernel_mode(vp, regs) &&  vp->domain->debugger_attached )
> +

Bogus double blank, and bogus blank line.

> +        domain_pause_for_debugger();
> +    else
> +        hvm_inject_hw_exception(TRAP_debug, HVM_DELIVER_NO_ERROR_CODE);
> +
> +    return 0;
> +}
[...]
> +static int vmxit_invalid_op(struct cpu_user_regs *regs)
> +{
> +    if ( guest_kernel_mode(current, regs) || 
> !emulate_forced_invalid_op(regs) )

Did you perhaps mean !guest_kernel_mode()?

> +    default:
> +        gdprintk(XENLOG_G_WARNING,
> +                 "PVH: Unhandled trap:%d. IP:%lx\n", vector, regs->eip);

gdprintk() shouldn't be used with XENLOG_G_*.

> +void vmx_pvh_vmexit_handler(struct cpu_user_regs *regs)
> +{
> +    unsigned long exit_qualification;
> +    unsigned int exit_reason = __vmread(VM_EXIT_REASON);
> +    int rc=0, ccpu = smp_processor_id();
> +    struct vcpu *v = current;
> +
> +    dbgp1("PVH:[%d]left VMCS exitreas:%d RIP:%lx RSP:%lx EFLAGS:%lx 
> CR0:%lx\n",
> +          ccpu, exit_reason, regs->rip, regs->rsp, regs->rflags,
> +          __vmread(GUEST_CR0));
> +
> +    /* For guest_kernel_mode which is called from most places below. */
> +    regs->cs = __vmread(GUEST_CS_SELECTOR);

Which raises the question of whether your uses of
guest_kernel_mode() are appropriate in the first place: Before this
series there's no use at all under xen/arch/x86/hvm/.

And if it is, I'd like to point out once again that this check should be
looking at SS.DPL, not CS.RPL.

> +    if ( rc )
> +    {
> +        exit_qualification = __vmread(EXIT_QUALIFICATION);
> +        gdprintk(XENLOG_G_WARNING,
> +                 "PVH: [%d] exit_reas:%d 0x%x qual:%ld 0x%lx cr0:0x%016lx\n",

Please use %#x and alike in favor of 0x%x etc.

> +int vmx_pvh_set_vcpu_info(struct vcpu *v, struct vcpu_guest_context *ctxtp)
> +{
> +    if ( v->vcpu_id == 0 )
> +        return 0;
> +
> +    vmx_vmcs_enter(v);
> +    __vmwrite(GUEST_GDTR_BASE, ctxtp->gdt.pvh.addr);
> +    __vmwrite(GUEST_GDTR_LIMIT, ctxtp->gdt.pvh.limit);
> +    __vmwrite(GUEST_GS_BASE, ctxtp->gs_base_user);
> +
> +    __vmwrite(GUEST_CS_SELECTOR, ctxtp->user_regs.cs);
> +    __vmwrite(GUEST_DS_SELECTOR, ctxtp->user_regs.ds);
> +    __vmwrite(GUEST_ES_SELECTOR, ctxtp->user_regs.es);
> +    __vmwrite(GUEST_SS_SELECTOR, ctxtp->user_regs.ss);
> +    __vmwrite(GUEST_GS_SELECTOR, ctxtp->user_regs.gs);

How does this work without also writing the "hidden" register
fields?

> +    if ( vmx_add_guest_msr(MSR_SHADOW_GS_BASE) )
> +    {
> +        vmx_vmcs_exit(v);
> +        return -EINVAL;
> +    }
> +    vmx_write_guest_msr(MSR_SHADOW_GS_BASE, ctxtp->gs_base_kernel);

So you write both GS bases, but not the base of FS (and above
its selector is being skipped too)?

And there are other parts of struct vcpu_guest_context that
I don't see getting mirrored - are all of them getting handled
elsewhere?

Jan

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