From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ross Lagerwall Subject: Re: [PATCH] x86: Don't crash when mapping a page using EFI runtime page tables Date: Wed, 27 May 2015 13:03:14 +0100 Message-ID: <5565B282.4030705@citrix.com> References: <1431706091-32288-1-git-send-email-ross.lagerwall@citrix.com> <555A1A33020000780007B3EC@mail.emea.novell.com> <55659B2A.4020605@citrix.com> <5565CDC9020000780007E2BF@mail.emea.novell.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <5565CDC9020000780007E2BF@mail.emea.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: Andrew Cooper , Keir Fraser , xen-devel@lists.xen.org List-Id: xen-devel@lists.xenproject.org On 05/27/2015 12:59 PM, Jan Beulich wrote: >>>> On 27.05.15 at 12:23, wrote: >> On 05/18/2015 03:58 PM, Jan Beulich wrote: >>>>>> On 15.05.15 at 18:08, wrote: >>>> --- a/xen/arch/x86/domain_page.c >>>> +++ b/xen/arch/x86/domain_page.c >>>> @@ -32,20 +32,25 @@ static inline struct vcpu *mapcache_current_vcpu(void) >>>> return NULL; >>>> >>>> /* >>>> + * When using efi runtime page tables, we have the equivalent of the >> idle >>>> + * domain's page tables but current may point at another domain's VCPU. >>>> + * Return NULL as though current is not properly set up yet. >>>> + */ >>>> + if ( efi_enabled && read_cr3() == efi_rs_page_table() ) >>>> + return NULL; >>> >>> I'm okay with the patch in principle; what worries me is the CR3 read >>> that is now going to be necessary even in non-debug builds. With >>> this code being the only user of efi_rs_page_table(), I wonder if it >>> wouldn't make sense to alter that function to return non-zero only >>> when spin_is_locked(&efi_rs_lock), and then alter the code above >>> such that the CR3 read would happen only when we got a non-zero >>> value back. >> >> mapcache_current_vcpu() appears to be called from IRQ-enabled and >> IRQ-disabled callers which prevents us from using the spinlock. > > I didn't suggest to use any spin lock; I merely suggested checking > whether that particular one is being held by someone (to avoid the > CR3 read if that's not the case). spin_is_locked() calls check_lock() which causes a BUG_ON() even though you're not actually using the lock. -- Ross Lagerwall