From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Sahita, Ravi" Subject: Re: [PATCH v5 05/15] x86/altp2m: basic data structures and support routines. Date: Fri, 17 Jul 2015 22:36:42 +0000 Message-ID: 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> <55A790760200007800091BDD@mail.emea.novell.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <55A790760200007800091BDD@mail.emea.novell.com> Content-Language: en-US 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: Tim Deegan , "Sahita, Ravi" , Wei Liu , George Dunlap , Andrew Cooper , Ian Jackson , "White, Edmund H" , "xen-devel@lists.xen.org" , "tlengyel@novetta.com" , Daniel De Graaf List-Id: xen-devel@lists.xenproject.org >From: Jan Beulich [mailto:JBeulich@suse.com] >Sent: Thursday, July 16, 2015 2:08 AM > >>>> 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. > So in this case there isnt a graceful fallback; this case can happen only if there is a bug in the hypervisor - which should be reported via the BUG_ON. >>>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). Working through these as best as can (treating it as Category 4) Thanks, Ravi > >Jan