From mboxrd@z Thu Jan 1 00:00:00 1970 From: Mukesh Rathor Subject: Re: [RFC PATCH 10/16]: PVH xen: introduce vmx_pvh.c Date: Wed, 23 Jan 2013 17:59:50 -0800 Message-ID: <20130123175950.772a3526@mantra.us.oracle.com> References: <20130111180110.55ce77aa@mantra.us.oracle.com> <50F4013202000078000B5421@nat28.tlf.novell.com> <20130114165408.0778f73b@mantra.us.oracle.com> <50F5257B02000078000B5AE9@nat28.tlf.novell.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <50F5257B02000078000B5AE9@nat28.tlf.novell.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Jan Beulich Cc: xen-devel List-Id: xen-devel@lists.xenproject.org On Tue, 15 Jan 2013 08:46:35 +0000 "Jan Beulich" wrote: > >>> On 15.01.13 at 01:54, Mukesh Rathor > >>> wrote: > > On Mon, 14 Jan 2013 11:59:30 +0000 "Jan Beulich" > > wrote: > >> >>> On 12.01.13 at 03:01, Mukesh Rathor > >> >>> 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