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

[Mukesh Rathor <mukesh.rathor@xxxxxxxxxx>]

On Mon, 18 Mar 2013 12:32:06 -0400
Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx> 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.

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

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

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


> > +}
> > +
> > +/* 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.

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

thanks,
mukesh

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