Re: [PATCH 10/18 V2]: PVH xen: introduce vmx_pvh.c and pvh.c

[Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>]

On Mon, Apr 01, 2013 at 06:26:45PM -0700, Mukesh Rathor wrote:
> On Mon, 18 Mar 2013 12:32:06 -0400
> Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> wrote:
> 
> > 
> > > +}
> > > +
> > > +typedef unsigned long pvh_hypercall_t(
> > > +    unsigned long, unsigned long, unsigned long, unsigned long,
> > > unsigned long,
> > > +    unsigned long);
> > 
> > No way to re-use the something standard? I am not sure what is 'PVH'
> > specific to it? It looks like a garden variety normal hypercalls.
> 
> It does use standard do_xx calls, PV goes thru it's own table, HVM thru
> its own, and PVH thru its own. This was suggested to me early on, and
> it's easier this way since HVM and PVH are not same hcalls.

I meant the typedef.

> 
> > > +    rc = 0;
> > > +
> > > +    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 rc;
> > 
> > This function looks to return 0 (or X86EMUL_OKAY) irregardless of the
> > MSR? Perhaps just make it return X86EMUL_OKAY without bothering to
> > use 'rc'?
> > 
> > > +}
> > > +
> > > +/* returns : 0 success */
> > > +static int vmxit_msr_write(struct cpu_user_regs *regs)
> > > +{
> > > +    uint64_t msr_content = (uint32_t)regs->eax |
> > > ((uint64_t)regs->edx << 32);
> > > +    int rc=1;
> > 
> > X86EMUL_UNHANDLEABLE
> > 
> > > +
> > > +    dbgp1("PVH: msr write:0x%lx. eax:0x%lx edx:0x%lx\n",
> > > regs->ecx, 
> > > +          regs->eax,regs->edx);
> > > +
> > > +    if ( hvm_msr_write_intercept(regs->ecx, msr_content) ==
> > > X86EMUL_OKAY ) {
> > > +        vmx_update_guest_eip();
> > > +        rc = 0;
> > 
> > X86EMUL_OKAY
> 
> No, hide the X96EMUL_* from the caller of this function which deals
> with rc or non-zero.

In that case please make that part of the comment at the start. It
says: 0 success. But nothing about the failure path.

> 
> > > +    } else if (vector == TRAP_gp_fault) {
> > > +        regs->error_code = __vmread(VM_EXIT_INTR_ERROR_CODE);
> > > +        /* hvm_inject_hw_exception(TRAP_gp_fault,
> > > regs->error_code); */
> > 
> > So how come we don't inject it in?
> 
> Notice the exception bitmap doesn't allow GP vmexits. But it helps
> tremendously with debug to have this here. I often have this enabled
> for debug.

So #ifdef DEBUG ?

> 
> > > +        rc = 1;
> > 
> > Huh? Not X86_EMUL_OK?
> 
> No. there's no x86 emulation going on here??
> 
> 
> > > +            if (regp == NULL)
> > > +                break;
> > > +
> > > +            /* pl don't embed switch statements */
> > > +            if (cr == 0)
> > > +                rc = access_cr0(regs, acc_typ, regp);
> > > +            else if (cr == 3) {
> > > +                printk("PVH: d%d: unexpected cr3 access vmexit.
> > > rip:%lx\n", 
> > > +                       current->domain->domain_id, regs->rip);
> > > +                domain_crash_synchronous();
> > 
> > Uh? Why wouldn't we want to handle it?
> 
> Guest does it natively. 

Ah, please put that in a comment right above it.

> 
> 
> > > +}
> > > +
> > > +/* NOTE: a PVH sets IOPL natively by setting bits in the eflags
> > > and not by
> > > + * hypercalls used by a PV */
> > 
> > 
> > Ahh, so there are now five? PV hypercall families that should not be
> > used:
> > 
> >  - PHYSDEVOP_set_iopl (which I think in your earlier patch you did
> > not check for?)
> >  - HYPERVISOR_update_va_mapping (for all the MMU stuff)
> >  - HYPERVISOR_update_descriptor (segments and such)
> >  - HYPERVISOR_fpu_taskswitch (you are doing it in the above function)
> >  - HYPERVISOR_set_trap_table (again, LDT and GDT are now done via HVM)
> >  - HYPERVISOR_set_segment_base
> >  - HYPERVISOR_set_gdt
> >  - HYPERVISOR_tmem
> >  .. and host of other.
> > 
> > This should be documented somewhere in docs?
> > Perhaps in docs/misc/pvh.txt and just outline which ones are not to
> > be used anymore?
> 
> I am keeping track of all doc stuff, lets document in the end when I 
> enable PVH. We'll be changing stuff for a short while.

Why not make this 'doc stuff' part of the patches? That way when you
are done with one item you can have a patch to remove it out the TODO
list. Also this way other folks can look at it and if they have time
help you on some of the TODOs.

> 
> > Could you use 'cpuid' macro defined in processor.h?
> 
> since we know in this fucntion we are on intel, we can just do cpuid.
> the macro has somw quirks for cyrix cpus, and clears rcx. Besides this
> is user mode cpuid that will exactly be like this on a pure PV guest.

I am not following you. Is the reason you are doing this b/c the
macro clears rcx?


> 
> thanks,
> mukesh