From mboxrd@z Thu Jan 1 00:00:00 1970 From: Mukesh Rathor Subject: Re: [RFC PATCH 6/16]: PVH xen: Define pvh guest and header changes.. Date: Tue, 15 Jan 2013 16:37:24 -0800 Message-ID: <20130115163724.09a5b4d1@mantra.us.oracle.com> References: <20130111175138.26477708@mantra.us.oracle.com> <50F3FC5102000078000B5400@nat28.tlf.novell.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <50F3FC5102000078000B5400@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 Mon, 14 Jan 2013 11:38:41 +0000 "Jan Beulich" wrote: > >>> On 12.01.13 at 02:51, Mukesh Rathor > >>> wrote: > > +/* We need vcpu because during context switch, going from pure PV > > to PVH, > > + * in save_segments(), current has been updated to next, and no > > longer pointing > > + * to the pure PV. BTW, for PVH, we update regs->selectors on each > > vmexit */ #define read_segment_register(vcpu, regs, > > name) \ > > I can only hope that at the end of this patch set the comment > matches reality - at this point in the series it doesn't afaict. It's a big patch, tough to break to have things together this way. Each has to be compilable. It may help to apply all patches to the xen tree (c/s: 26124) and then cscope it? Just a thought. I realize it's tough to review, but not sure how else I can break it and still keep all parts small. > > --- a/xen/include/asm-x86/x86_64/regs.h Fri Jan 11 16:25:27 > > 2013 -0800 +++ b/xen/include/asm-x86/x86_64/regs.h Fri Jan > > 11 16:27:46 2013 -0800 @@ -11,9 +11,10 @@ > > #define ring_3(r) (((r)->cs & 3) == 3) > > > > If this BUG_ON() really has to stay here, you ought to add > white space inside the braces and around the !=. Ok, done. > As you add a level of parentheses, you also ought to adjust > indentation. It's already indented single space like the previous macro was. Do you want me to 4 space it? > At the very least, you want e.g. > > + do { PVH_ASSERT(!is_pvh_vcpu(v)); } while (0) Duh! fixed. > But the defines, if needed at all, are grossly misplaced in any case; > there ought to be a pvh header for such stuff. Well, I imagine those asserts while PVH is still being stabilized, and then removed. Do you still want me to create a new header with just 3 defines that will be deleted in near future? > > @@ -278,6 +281,7 @@ struct domain > > > > /* Is this an HVM guest? */ > > bool_t is_hvm; > > + bool_t is_pvh; /* see above for description */ > > These are mutually exclusive (also with PV), so perhaps better > to have a single enum-type variable? I imagine in future there would be no PV, so is_hvm==0 ==> pvh. Too optimistic? thanks, Mukesh