From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Jan Beulich" Subject: Re: [PATCH v5 05/15] x86/altp2m: basic data structures and support routines. Date: Tue, 14 Jul 2015 14:13:24 +0100 Message-ID: <55A527140200007800090B74@mail.emea.novell.com> References: <1436832903-12639-1-git-send-email-edmund.h.white@intel.com> <1436832903-12639-6-git-send-email-edmund.h.white@intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <1436832903-12639-6-git-send-email-edmund.h.white@intel.com> Content-Disposition: inline List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Ed White Cc: Tim Deegan , Ravi Sahita , Wei Liu , George Dunlap , Andrew Cooper , Ian Jackson , xen-devel@lists.xen.org, tlengyel@novetta.com, Daniel De Graaf List-Id: xen-devel@lists.xenproject.org >>> On 14.07.15 at 02:14, wrote: > +void > +altp2m_vcpu_initialise(struct vcpu *v) > +{ > + if ( v != current ) > + vcpu_pause(v); > + > + altp2m_vcpu_reset(v); > + vcpu_altp2m(v).p2midx = 0; > + atomic_inc(&p2m_get_altp2m(v)->active_vcpus); > + > + altp2m_vcpu_update_eptp(v); > + > + if ( v != current ) > + vcpu_unpause(v); > +} > + > +void > +altp2m_vcpu_destroy(struct vcpu *v) > +{ > + struct p2m_domain *p2m; > + > + if ( v != current ) > + vcpu_pause(v); > + > + if ( (p2m = p2m_get_altp2m(v)) ) > + atomic_dec(&p2m->active_vcpus); > + > + altp2m_vcpu_reset(v); > + > + altp2m_vcpu_update_eptp(v); > + altp2m_vcpu_update_vmfunc_ve(v); > + > + if ( v != current ) > + vcpu_unpause(v); > +} There not being any caller of altp2m_vcpu_initialise() I can't judge about its pausing requirements, but for the destroy case I can't see what the pausing is good for. Considering its sole user it's also not really clear why the two update operations need to be done while destroying. > @@ -6498,6 +6500,25 @@ enum hvm_intblk nhvm_interrupt_blocked(struct vcpu *v) > return hvm_funcs.nhvm_intr_blocked(v); > } > > +void altp2m_vcpu_update_eptp(struct vcpu *v) > +{ > + if ( hvm_funcs.altp2m_vcpu_update_eptp ) > + hvm_funcs.altp2m_vcpu_update_eptp(v); > +} > + > +void altp2m_vcpu_update_vmfunc_ve(struct vcpu *v) > +{ > + if ( hvm_funcs.altp2m_vcpu_update_vmfunc_ve ) > + hvm_funcs.altp2m_vcpu_update_vmfunc_ve(v); > +} > + > +bool_t altp2m_vcpu_emulate_ve(struct vcpu *v) > +{ > + if ( hvm_funcs.altp2m_vcpu_emulate_ve ) > + return hvm_funcs.altp2m_vcpu_emulate_ve(v); > + return 0; > +} The patch context here suggests that you're using pretty outdated a tree - nhvm_interrupt_blocked() went away a week ago. In line with the commit doing so, all of the above should become inline functions. > +uint16_t p2m_find_altp2m_by_eptp(struct domain *d, uint64_t eptp) > +{ > + struct p2m_domain *p2m; > + struct ept_data *ept; > + uint16_t i; > + > + altp2m_list_lock(d); > + > + for ( i = 0; i < MAX_ALTP2M; i++ ) > + { > + if ( d->arch.altp2m_eptp[i] == INVALID_MFN ) > + continue; > + > + p2m = d->arch.altp2m_p2m[i]; > + ept = &p2m->ept; > + > + if ( eptp == ept_get_eptp(ept) ) > + goto out; > + } > + > + i = INVALID_ALTP2M; > + > +out: Labels should be indented by at least one space. Pending the rename, the function may also live in the wrong file (the use of ept_get_eptp() suggests so even if the function itself got renamed). > +bool_t p2m_switch_vcpu_altp2m_by_id(struct vcpu *v, uint16_t idx) > +{ > + struct domain *d = v->domain; > + bool_t rc = 0; > + > + if ( idx > MAX_ALTP2M ) > + return rc; > + > + altp2m_list_lock(d); > + > + if ( d->arch.altp2m_eptp[idx] != INVALID_MFN ) > + { > + if ( idx != vcpu_altp2m(v).p2midx ) > + { > + atomic_dec(&p2m_get_altp2m(v)->active_vcpus); > + vcpu_altp2m(v).p2midx = idx; > + atomic_inc(&p2m_get_altp2m(v)->active_vcpus); Are the two results of p2m_get_altp2m(v) here guaranteed to be distinct? If they aren't, is it safe to decrement first (potentially dropping the count to zero)? > @@ -722,6 +731,27 @@ void nestedp2m_write_p2m_entry(struct p2m_domain *p2m, unsigned long gfn, > l1_pgentry_t *p, l1_pgentry_t new, unsigned int level); > > /* > + * Alternate p2m: shadow p2m tables used for alternate memory views > + */ > + > +/* get current alternate p2m table */ > +static inline struct p2m_domain *p2m_get_altp2m(struct vcpu *v) > +{ > + struct domain *d = v->domain; > + uint16_t index = vcpu_altp2m(v).p2midx; > + > + ASSERT(index < MAX_ALTP2M); > + > + return (index == INVALID_ALTP2M) ? NULL : d->arch.altp2m_p2m[index]; > +} Looking at this again, I'm afraid I'd still prefer index < MAX_ALTP2M in the return statement (and the ASSERT() dropped): The ASSERT() does nothing in a debug=n build, and hence wouldn't shield us from possibly having to issue an XSA if somehow an access outside the array's bounds turned out possible. I've also avoided to repeat any of the un-addressed points that I raised against earlier versions. Jan