Re: [Xen-devel] [RFC PATCH 8/16]: PVH xen: domain creation code changes

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 16.01.13 at 01:50, Mukesh Rathor <mukesh.rathor@xxxxxxxxxx> wrote:
> On Mon, 14 Jan 2013 11:49:42 +0000 "Jan Beulich" <JBeulich@xxxxxxxx> wrote:
>> So you add these hooks, call them unconditionally, yet neither VMX
>> nor SVM implement them? What's the purpose? Series of patches
>> are expected to be consistent at each patch boundary.
> 
> I'm told to keep patch sizes small, so I try to group together
> changes. The functions are small/generic enough I figured it would 
> be OK.

Sure, but the code needs to be correct at patch boundaries. And
a NULL pointer dereference doesn't count as correct to me, the
more that I don't think the patch set deals with SVM, and hence
there the NULL pointer dereference (at the end of your patch
set) likely has paths reaching it that cannot be easily shown to be
dead under SVM.

>> > --- a/xen/include/asm-x86/hvm/vcpu.h       Fri Jan 11 16:29:49
>> > 2013 -0800 +++ b/xen/include/asm-x86/hvm/vcpu.h    Fri Jan 11
>> > 16:31:33 2013 -0800 @@ -104,6 +104,13 @@ struct nestedvcpu {
>> >  
>> >  #define vcpu_nestedhvm(v) ((v)->arch.hvm_vcpu.nvcpu)
>> >  
>> > +/* add any PVH specific fields here */
>> > +struct pvh_hvm_vcpu_ext
>> > +{
>> > +    /* Guest-specified relocation of vcpu_info. */
>> > +    unsigned long pvh_vcpu_info_mfn;
>> 
>> Isn't that a field equivalent to v->arch.pv_vcpu.vcpu_info_mfn?
>> Preferably they would be shared then, or if not, having "pvh" in
>> the containing structure's field name and the field name here is
>> clearly one too much.
> 
> No, it's a union, so can't use pv_vcpu.vcpu_info_mfn. I like the
> 3 char prefix to field name so grep/cscope can find it easily.

Sure, it's a matter of taste to some degree. But I personally
dislike that sort of redundancy (the expressions actually using
this look pretty odd), except when the untagged name is
really very generic (which isn't the case here).

Jan


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