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: Thu, 16 Jul 2015 10:07:34 +0100 Message-ID: <55A790760200007800091BDD@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> <55A527140200007800090B74@mail.emea.novell.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: 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: Ravi Sahita Cc: Tim Deegan , Wei Liu , George Dunlap , Andrew Cooper , Ian Jackson , Edmund H White , "xen-devel@lists.xen.org" , "tlengyel@novetta.com" , Daniel De Graaf List-Id: xen-devel@lists.xenproject.org >>> On 16.07.15 at 10:57, wrote: >> From: Jan Beulich [mailto:JBeulich@suse.com] >>Sent: Tuesday, July 14, 2015 6:13 AM >>>>> On 14.07.15 at 02:14, wrote: >>> @@ -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. >> > > the assert was breaking v5 anyway. BUG_ON (with the right check) is probably > the right thing to do, as we do in the exit handling that checks for a VMFUNC > having changed the index. > So will make that change. But why use a BUG_ON() when you can deal with this more gracefully? Please try to avoid crashing the hypervisor when there are other ways to recover. >>I've also avoided to repeat any of the un-addressed points that I raised > against >>earlier versions. >> > > I went back and looked at the earlier versions of the comments on this patch > and afaict we have either addressed (accepted) those points or described why > they shouldn't cause changes with reasoning . so if I missed something please > let me know. Just one example of what wasn't done is the conversion of local variable, function return, and function parameter types from (bogus) uint8_t / uint16_t to unsigned int (iirc also in other patches). Jan