Re: [PATCH v13 3/7] KVM: x86: flush TLBs last before returning from KVM_GET_DIRTY_LOG

[Mario Smarduch <m.smarduch@samsung.com>]

On 11/06/2014 11:44 PM, Paolo Bonzini wrote:
> 
> 
> On 07/11/2014 01:40, Mario Smarduch wrote:
>> In the next patches, we will move parts of x86's kvm_vm_ioctl_get_dirty_log
>> implementation to generic code; leave the arch-specific code at the end,
>> similar to the existing generic function kvm_get_dirty_log.
>>
>> Reviewed-by: Mario Smarduch <m.smarduch@samsung.com>
>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> 
> You should have a "Signed-off-by: Mario Smarduch ..." here, and you 
> should also add my authorship.  You can do the latter with "git rebase 
> -i", adding
> 
>   x git commit --amend -C HEAD --author 'Paolo Bonzini <pbonzini@redhat.com>'
> 
> after this patch.  If you're using a patch queue instead (quilt or
> similar) you can just edit the "From" line in the patch.
> 
> I guess if you reply to the patch with just
> 
>    Signed-off-by: Mario Smarduch <m.smarduch@samsung.com>
> 
> the ARM maintainers can do the above for you, if there's no need for
> a v14.
> 
> I'll send an eighth patch to actually switch x86 to the new function.
> Again, the maintainers can apply it in the right place, but please
> include it yourself if you have to do a v14.
> 
> Thanks,
> 
> Paolo

I kind of thought something was wrong :) Thanks for the explanation,
until I get the hang of it I'll defer to ARM maintainers. Maybe later
update SubmittingPatches documentation.

- Mario
> 
>> ---
>>  arch/x86/kvm/x86.c |   22 ++++++++++------------
>>  1 file changed, 10 insertions(+), 12 deletions(-)
>>
>> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
>> index 8f1e22d..dc8e66b 100644
>> --- a/arch/x86/kvm/x86.c
>> +++ b/arch/x86/kvm/x86.c
>> @@ -3606,13 +3606,13 @@ static int kvm_vm_ioctl_reinject(struct kvm *kvm,
>>   *
>>   *   1. Take a snapshot of the bit and clear it if needed.
>>   *   2. Write protect the corresponding page.
>> - *   3. Flush TLB's if needed.
>> - *   4. Copy the snapshot to the userspace.
>> + *   3. Copy the snapshot to the userspace.
>> + *   4. Flush TLB's if needed.
>>   *
>> - * Between 2 and 3, the guest may write to the page using the remaining TLB
>> - * entry.  This is not a problem because the page will be reported dirty at
>> - * step 4 using the snapshot taken before and step 3 ensures that successive
>> - * writes will be logged for the next call.
>> + * Between 2 and 4, the guest may write to the page using the remaining TLB
>> + * entry.  This is not a problem because the page is reported dirty using
>> + * the snapshot taken before and step 4 ensures that writes done after
>> + * exiting to userspace will be logged for the next call.
>>   */
>>  int kvm_vm_ioctl_get_dirty_log(struct kvm *kvm, struct kvm_dirty_log *log)
>>  {
>> @@ -3661,6 +3661,10 @@ int kvm_vm_ioctl_get_dirty_log(struct kvm *kvm, struct kvm_dirty_log *log)
>>  
>>  	spin_unlock(&kvm->mmu_lock);
>>  
>> +	r = 0;
>> +	if (copy_to_user(log->dirty_bitmap, dirty_bitmap_buffer, n))
>> +		r = -EFAULT;
>> +
>>  	/* See the comments in kvm_mmu_slot_remove_write_access(). */
>>  	lockdep_assert_held(&kvm->slots_lock);
>>  
>> @@ -3670,12 +3674,6 @@ int kvm_vm_ioctl_get_dirty_log(struct kvm *kvm, struct kvm_dirty_log *log)
>>  	 */
>>  	if (is_dirty)
>>  		kvm_flush_remote_tlbs(kvm);
>> -
>> -	r = -EFAULT;
>> -	if (copy_to_user(log->dirty_bitmap, dirty_bitmap_buffer, n))
>> -		goto out;
>> -
>> -	r = 0;
>>  out:
>>  	mutex_unlock(&kvm->slots_lock);
>>  	return r;
>>