Re: [Xen-devel] [PATCH v2 08/10] nEPT: handle invept instruction from L1 VMM

["Zhang, Xiantao" <xiantao.zhang@xxxxxxxxx>]

Thanks, Jan! 
>>: handle invept instruction from L1 VMM
> 
> >>> On 19.12.12 at 20:44, Xiantao Zhang <xiantao.zhang@xxxxxxxxx> wrote:
> > --- a/xen/arch/x86/hvm/vmx/vmx.c
> > +++ b/xen/arch/x86/hvm/vmx/vmx.c
> > @@ -2572,11 +2572,13 @@ void vmx_vmexit_handler(struct
> cpu_user_regs *regs)
> >          if ( nvmx_handle_vmresume(regs) == X86EMUL_OKAY )
> >              update_guest_eip();
> >          break;
> > -
> > +    case EXIT_REASON_INVEPT:
> > +        if ( nvmx_handle_invept(regs) == X86EMUL_OKAY )
> > +            update_guest_eip();
> > +        break;
> 
> In (potentially going to become) long switch statements, please don't drop
> the blank lines between individual cases - instead of dropping the line here,
> you wold want to insert another one below the new separately handled case.

Okay. 

> >      case EXIT_REASON_MWAIT_INSTRUCTION:
> >      case EXIT_REASON_MONITOR_INSTRUCTION:
> >      case EXIT_REASON_GETSEC:
> > -    case EXIT_REASON_INVEPT:
> >      case EXIT_REASON_INVVPID:
> >          /*
> >           * We should never exit on GETSEC because CR4.SMXE is always
> > 0 when
> > --- a/xen/arch/x86/hvm/vmx/vvmx.c
> > +++ b/xen/arch/x86/hvm/vmx/vvmx.c
> > @@ -1356,6 +1356,45 @@ int nvmx_handle_vmwrite(struct cpu_user_regs
> *regs)
> >      return X86EMUL_OKAY;
> >  }
> >
> > +int nvmx_handle_invept(struct cpu_user_regs *regs) {
> > +    struct vmx_inst_decoded decode;
> > +    unsigned long eptp;
> > +    u64 inv_type;
> > +
> > +    if ( !cpu_has_vmx_ept )
> > +        return X86EMUL_EXCEPTION;
> > +
> > +    if ( decode_vmx_inst(regs, &decode, &eptp, 0)
> > +             != X86EMUL_OKAY )
> > +        return X86EMUL_EXCEPTION;
> > +
> > +    inv_type = reg_read(regs, decode.reg2);
> > +    gdprintk(XENLOG_DEBUG,"inv_type:%ld, eptp:%lx\n", inv_type,
> > + eptp);
> 
> An unconditional printk() on an operation potentially happening quite
> frequently? Even with XENLOG_DEBUG this is not acceptable imo.

Okay, I will remove it. 

> > +
> > +    switch ( inv_type ) {
> > +    case INVEPT_SINGLE_CONTEXT:
> > +        {
> > +            struct p2m_domain *p2m = vcpu_nestedhvm(current).nv_p2m;
> > +            if ( p2m )
> > +            {
> > +               p2m_flush(current, p2m);
> 
> Despite your comment in 00/10, there still is a whitespace issues at least 
> here
> (didn't look that closely elsewhere).

Fixed.

> > +                ept_sync_domain(p2m);
> > +            }
> > +        }
> > +        break;
> > +    case INVEPT_ALL_CONTEXT:
> > +        p2m_flush_nestedp2m(current->domain);
> > +        __invept(INVEPT_ALL_CONTEXT, 0, 0);
> > +        break;
> > +    default:
> > +        return X86EMUL_EXCEPTION;
> > +    }
> > +    vmreturn(regs, VMSUCCEED);
> > +    return X86EMUL_OKAY;
> > +}
> > +
> > +
> >  #define __emul_value(enable1, default1) \
> >      ((enable1 | default1) << 32 | (default1))
> >
> > --- a/xen/arch/x86/mm/p2m.c
> > +++ b/xen/arch/x86/mm/p2m.c
> > @@ -1465,7 +1465,7 @@ p2m_flush_table(struct p2m_domain *p2m)  void
> > p2m_flush(struct vcpu *v, struct p2m_domain *p2m)  {
> > -    ASSERT(v->domain == p2m->domain);
> > +    ASSERT(p2m && v->domain == p2m->domain);
> 
> How is this change related to the rest of the patch? 
I will remove it, and let caller check whether p2m is NULL.  Originally,  this 
is to fix a Xen booting issue. 
Xiantao


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