Re: [Xen-devel] [V11 PATCH 20/21] PVH xen: introduce vmexit handler for PVH

[Mukesh Rathor <mukesh.rathor@xxxxxxxxxx>]

On Fri, 23 Aug 2013 10:12:16 +0100
"Jan Beulich" <JBeulich@xxxxxxxx> wrote:

> >>> On 23.08.13 at 03:19, Mukesh Rathor <mukesh.rathor@xxxxxxxxxx>
> >>> wrote:
> > Changes in V11:
> >   - merge this with previous patch "prep changes".
> >   - allow invalid op emulation for kernel mode also.
> >   - Use CR0_READ_SHADOW instead of GUEST_CR0.
> > 
> > Signed-off-by: Mukesh Rathor <mukesh.rathor@xxxxxxxxxx>
> > Acked-by: Keir Fraser <keir@xxxxxxx>
> > Reviewed-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> > PV-HVM-Regression-Tested-by: Andrew Cooper
> > <andrew.cooper3@xxxxxxxxxx>
> 
> Again the changes above void the tags here.
> 
> > +static int vmxit_msr_read(struct cpu_user_regs *regs)
> > +{
> > +    u64 msr_content = 0;
> > +
> > +    switch ( regs->ecx )
> 
> Did you mean regs->_ecx?

Hmm.. don't understand why? HVM uses ecx:

  hvm_msr_read_intercept(regs->ecx, &msr_content) == X86EMUL_OKAY )


> > +    default:
> > +        /* PVH fixme: see hvm_msr_read_intercept(). */
> > +        rdmsrl(regs->ecx, msr_content);
> 
> So what does this comment refer to? There's no change to the
> referred to function here. And it seems rather questionable that
> reading the physical MSR values for everything but
> MSR_IA32_MISC_ENABLE is correct/secure. I appreciate the
> "fixme" annotation, but I'm afraid this is not sufficient here.

Yes, it needs to be revisited, best with AMD port so that a good
solution can be contrived for PVH.

> > +{
> > +    int vector = (__vmread(VM_EXIT_INTR_INFO)) &
> > INTR_INFO_VECTOR_MASK;
> > +    int rc = -ENOSYS;
> > +
> > +    dbgp1(" EXCPT: vec:%#x cs:%#lx rip:%#lx\n", vector,
> > +          __vmread(GUEST_CS_SELECTOR), regs->eip);
> 
> Do you continue to have these funny dbgp constructs in here. Are
> they supposed to go away before this gets committed? If not,
> please use a model similar to HVM_DBG_LOG().

Like the commit log says, it helps debug, but can be removed anytime.
I left it there thinking it might be useful for first couple months
while it gets thoroughly tested.

> > +static int vmxit_io_instr(struct cpu_user_regs *regs)
> > +{
> > +    struct segment_register seg;
> > +    int requested = (regs->rflags & X86_EFLAGS_IOPL) >> 12;
> > +    int curr_lvl = (regs->rflags & X86_EFLAGS_VM) ? 3 : 0;
> > +
> > +    if ( curr_lvl == 0 )
> > +    {
> > +        hvm_get_segment_register(current, x86_seg_ss, &seg);
> > +        curr_lvl = seg.attr.fields.dpl;
> > +    }
> > +    if ( requested >= curr_lvl && emulate_privileged_op(regs) )
> > +        return 0;
> > +
> > +    hvm_inject_hw_exception(TRAP_gp_fault, regs->error_code);
> 
> I don't think reg->error_code is valid here, I think this needs to be
> read from the VMCS.

Correct. Thats a bug. 

thanks
Mukesh



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