From mboxrd@z Thu Jan 1 00:00:00 1970 From: Mukesh Rathor Subject: Re: [RFC PATCH 8/16]: PVH xen: domain creation code changes Date: Thu, 24 Jan 2013 15:50:03 -0800 Message-ID: <20130124155003.2f01c4db@mantra.us.oracle.com> References: <20130111175731.78821167@mantra.us.oracle.com> <50F3FEE602000078000B540B@nat28.tlf.novell.com> <20130115165011.764c7180@mantra.us.oracle.com> <50F687A902000078000B6167@nat28.tlf.novell.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <50F687A902000078000B6167@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 Wed, 16 Jan 2013 09:57:45 +0000 "Jan Beulich" wrote: > >>> On 16.01.13 at 01:50, Mukesh Rathor > >>> wrote: > > On Mon, 14 Jan 2013 11:49:42 +0000 "Jan Beulich" > > 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