Re: [RFC PATCH 10/16]: PVH xen: introduce vmx_pvh.c

[Mukesh Rathor <mukesh.rathor@oracle.com>]

On Tue, 15 Jan 2013 08:46:35 +0000
"Jan Beulich" <JBeulich@suse.com> wrote:

> >>> On 15.01.13 at 01:54, Mukesh Rathor <mukesh.rathor@oracle.com>
> >>> wrote:
> > On Mon, 14 Jan 2013 11:59:30 +0000 "Jan Beulich"
> > <JBeulich@suse.com> wrote:
> >> >>> On 12.01.13 at 03:01, Mukesh Rathor <mukesh.rathor@oracle.com>
> >> >>> wrote:
> >> > The heart of this patch is vmx exit handler for PVH guest. It is
> >> > nicely isolated in a separate module. A call to it is added to
> >> > vmx_pvh_vmexit_handler().
> >> 
> >> I'm sorry to say that, but this patch doesn't look worth commenting
> >> on in detail: It's completely unsorted (mixing VMX and generic
> >> stuff) and appears heavily redundant with other code. I think this
> >> needs to be sorted out cleanly first.
> > 
> > Not sure what you are referring to when you generic stuff, but it's 
> > all VMX stuff, mainly vmx exit handler. We had discussed it during 
> > the hackathon and the xen summit prior, and we wanted to keep
> > functions and code for PVH as much separate as possible to avoid
> > filling existing HVM code with if PVH statements. I can look into
> > moving some stuff to common code if you have issues with any
> > specific ones? Or do you not want a separate exit handler for PVH
> > case at all? I think keeping it separate is much better thing to
> > do.... 
> 
> The main thing are the hypercall wrappers - they're definitely not
> VMX-specific, and hence don't belong in VMX-specific code. Besides

Ah, I see. The HVM hcalls are in hvm.c and not vmx.c. Since PVH needs
slightly different hcalls and restricts certain ones ok for HVM, I really
prefer to not pollute hvm_do_hypercall() with if PVH everywhere. I could
add a new function to hvm.c, pvh_hvm_do_hypercall(), or create a new
file hvm_pvh.c and add it there. What would you suggest?


>too. But stuff like get_gpr_ptr() doesn't belong here either (and I
>actually doubt the function should be added in the first place - iirc
>we already have a function doing just that, and it wasn't that long

Yup, decode_register() does that, and it's non-static. I missed it
because it was added later, and wasn't in the tree I was using. I'll use
that, less code for me. 

thanks,
Mukesh