On 05/18/2015 03:58 PM, Jan Beulich wrote:
On 15.05.15 at 18:08, <ross.lagerw...@citrix.com> 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. Is reading cr3 that bad, given that this is a slow path anyway?
And with that I would then also like to fold the potential double CR3 reads in debug builds to at most one.
CR3 needs to be re-read after the lazy context switch to the idle domain is completed.
-- Ross Lagerwall _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
