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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 15.01.13 at 01:54, Mukesh Rathor <mukesh.rathor@xxxxxxxxxx> wrote:
> On Mon, 14 Jan 2013 11:59:30 +0000 "Jan Beulich" <JBeulich@xxxxxxxx> wrote:
>> >>> On 12.01.13 at 03:01, Mukesh Rathor <mukesh.rathor@xxxxxxxxxx>
>> >>> 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
that, the sub-hypercall filtering you do there looks pretty fragile
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
ago that I consolidated three(?) incarnations into just one; I guess
you understand that I don't want to see another one appearing
without good reason).

Jan


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