Re: [RFC PATCH 6/16]: PVH xen: Define pvh guest and header changes..

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

On Mon, 14 Jan 2013 11:38:41 +0000
"Jan Beulich" <JBeulich@suse.com> wrote:

> >>> On 12.01.13 at 02:51, Mukesh Rathor <mukesh.rathor@oracle.com>
> >>> 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