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

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

On Wed, 16 Jan 2013 09:57:45 +0000
"Jan Beulich" <JBeulich@suse.com> wrote:

> >>> On 16.01.13 at 01:50, Mukesh Rathor <mukesh.rathor@oracle.com>
> >>> wrote:
> > On Mon, 14 Jan 2013 11:49:42 +0000 "Jan Beulich"
> > <JBeulich@suse.com> wrote:
> >> So you add these hooks, call them unconditionally, yet neither VMX

No, both are conditional calls:

+    if ( is_pvh_vcpu(v) )
+    {
+        /* guest is bringing up non-boot SMP vcpu */
+        if ( (rc=hvm_pvh_set_vcpu_info(v, c.nat)) != 0 )
+            return rc;
+    }
+

So, if someone applies partial patches and then tries to boot PVH guest,
then it'll hit NULL ptr.


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

Hmm.. I guess in this particular patch then I could just create null
functions and not call vmx/svm ones, and change that later in the patch 
when I introduce the actual vmx/svm functions. For SVM, they will be stubs
since SVM is not implmented for PVH right now. Ok, I'll do that.


> >> >  #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).

Only if the ptr to struct is define in a meaningful way, like

     struct pvh_hvm_vcpu_ext *pvhp;

But I rarely see that. Furthermore, some smart guy will add a field
"int size". Good luck greping/cscoping that! So, setting a precedent
to prefix pvh would be a good idea IMO.

I changed it to vcpu_info_mfn anyways.

thanks
Mukesh