Re: [Xen-devel] [PATCH] x86: Don't crash when mapping a page using EFI runtime page tables

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 27.05.15 at 12:23, <ross.lagerwall@xxxxxxxxxx> wrote:
> On 05/18/2015 03:58 PM, Jan Beulich wrote:
>>>>> On 15.05.15 at 18:08, <ross.lagerwall@xxxxxxxxxx> 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).

> Is reading cr3 that bad, given that this is a slow path anyway?

I never easily agree to making a code path slower that may be
executed frequently. And looking at the most likely path through
the function, I don't think it's all that slow 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.

That's a good point.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel