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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 16.01.13 at 01:37, Mukesh Rathor <mukesh.rathor@xxxxxxxxxx> wrote:
> On Mon, 14 Jan 2013 11:38:41 +0000
> "Jan Beulich" <JBeulich@xxxxxxxx> wrote:
> 
>> >>> On 12.01.13 at 02:51, Mukesh Rathor <mukesh.rathor@xxxxxxxxxx>
>> >>> 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.

I understand that this is difficult to break up. But I don't see why
comments can't be added when they (or adjusted multiple times to)
match reality.

>> 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?

No - I want you to add a single space of indentation for each
added level of opening parentheses that don't have seeing
their matching closing ones yet.

>> 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?

Are you telling me that there's nothing else pvh-specific that
needs declaring/defining? And even if so, xen/lib.h clearly is
the wrong place - I'm not even convinced PVH is a cross-
architecture concept...

>> > @@ -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?

Why would you want to close the road to run old guests?

Jan


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