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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> 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?

> +    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.

> +/* Returns : 0 == msr written successfully. */
> +static int vmxit_msr_write(struct cpu_user_regs *regs)
> +{
> +    uint64_t msr_content = regs->eax | (regs->edx << 32);

And similarly to the above regs->_eax?

> +static int vmxit_debug(struct cpu_user_regs *regs)
> +{
> +    struct vcpu *vp = current;

This variable is to be named either "v" or, preferably, "curr". Same
further down.

> +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:%#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().

> +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.

Jan


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