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: Mon, 20 Jul 2015 07:20:27 +0100 Message-ID: <55ACAF4B0200007800092D71@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> <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: 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 18.07.15 at 00:36, wrote: >> 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. Generally (as an example), if a hypervisor bug can be confined to a guest, killing just the guest instead of the hypervisor would still be preferred (allowing the admin to attempt to gracefully shut down other guests before updating/restarting). Jan