Re: [PATCH] Use cmpxchg for pte updates on walk_addr()

[Avi Kivity <avi-atKUWr5tajBWk0Htik3J/w@public.gmane.org>]

Marcelo Tosatti wrote:
> On Fri, Dec 07, 2007 at 07:06:26AM +0200, Avi Kivity wrote:
>   
>> Marcelo Tosatti wrote:
>>     
>>> Right, patch at end of the message restarts the process if the pte
>>> changes under the walker. The goto is pretty ugly, but I fail to see any
>>> elegant way of doing that. Ideas?
>>>
>>>  
>>>       
>> goto is fine for that.  But there's a subtle livelock here: suppose vcpu 
>> 0 is in guest mode with continuously updating a memory location.  vcpu 1 
>> is faulting with that memory location acting as a pte.  While we're in 
>> kernel mode, we aren't responding to signals like we should; so we need 
>> to abort the walk and let the guest retry; that way we go through the 
>> signal_pending() check.
>>
>> However, this is an intrusive change, so let's start with the goto and 
>> drop it later in favor or an abort.
>>
>>     
>>>>> @@ -1510,6 +1510,9 @@ static int emulator_write_phys(struct kvm_vcpu 
>>>>> *vcpu, gpa_t gpa,
>>>>> {
>>>>> 	int ret;
>>>>>
>>>>> +	/* No need for kvm_cmpxchg_guest_pte here, its the guest 
>>>>> + 	 * responsability to synchronize pte updates and page faults.
>>>>> +	 */
>>>>> 	ret = kvm_write_guest(vcpu->kvm, gpa, val, bytes);
>>>>> 	if (ret < 0)
>>>>> 		return 0;
>>>>>      
>>>>>           
>>>> Hmm.  What if an i386 pae guest carefully uses cmpxchg8b to atomically 
>>>> set a pte?  kvm_write_guest() doesn't guarantee atomicity, so an 
>>>> intended atomic write can be seen splitted by the guest walker doing a 
>>>> concurrent walk.
>>>>    
>>>>         
>>> True, an atomic write is needed... a separate patch for that seems more
>>> appropriate.
>>>
>>>
>>>  
>>>       
>> Yes.
>>     
>
> Hi Avi,
>
> How about the following
>
> diff --git a/drivers/kvm/x86.c b/drivers/kvm/x86.c
> index c70ac33..8678651 100644
> --- a/drivers/kvm/x86.c
> +++ b/drivers/kvm/x86.c
> @@ -25,6 +25,7 @@
>  #include <linux/vmalloc.h>
>  #include <linux/module.h>
>  #include <linux/mman.h>
> +#include <linux/highmem.h>
>  
>  #include <asm/uaccess.h>
>  #include <asm/msr.h>
> @@ -1589,6 +1590,34 @@ static int emulator_cmpxchg_emulated(unsigned long addr,
>  		reported = 1;
>  		printk(KERN_WARNING "kvm: emulating exchange as write\n");
>  	}
> +
> +	/* guests cmpxchg8b have to be emulated atomically */
> +	if (bytes == 8) {
>   

It's only needed on i386, as 8-byte copy_to_user should be atomic on x86_64.


-- 
Do not meddle in the internals of kernels, for they are subtle and quick to panic.


-------------------------------------------------------------------------
SF.Net email is sponsored by:
Check out the new SourceForge.net Marketplace.
It's the best place to buy or sell services for
just about anything Open Source.
http://sourceforge.net/services/buy/index.php