Re: [PATCH] clear PF_VCPU in kvm_guest_exit()

Laurent Vivier
2007-10-17T15:10:14Z

Avi Kivity wrote:
> Laurent Vivier wrote:
>> clear PF_VCPU in kvm_guest_exit() and move kvm_guest_exit() after
>> local_irq_enable().
>>
>> According comments from Avi, we can clear PF_VCPU in kvm_guest_exit if we move
>> it after local_irq_enable().
>>
>> http://lkml.org/lkml/2007/10/15/114
>>
>> To simplify s390 port, we don't clear it in account_system_time().
>>
>> http://lkml.org/lkml/2007/10/15/183
>>
>> Signed-off-by: Laurent Vivier <Laurent.Vivier@bull.net>
>> ---
>> drivers/kvm/kvm.h | 1 +
>> drivers/kvm/kvm_main.c | 3 ++-
>> kernel/sched.c | 1 -
>> 3 files changed, 3 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/kvm/kvm.h b/drivers/kvm/kvm.h
>> index e9dbf67..e8b4902 100644
>> --- a/drivers/kvm/kvm.h
>> +++ b/drivers/kvm/kvm.h
>> @@ -677,6 +677,7 @@ static inline void kvm_guest_enter(void)
>>
>> static inline void kvm_guest_exit(void)
>> {
>> + current->flags &= ~PF_VCPU;
>> }
>>
>
>
> Ingo already added this.
>
>>
>> static inline int kvm_mmu_page_fault(struct kvm_vcpu *vcpu, gva_t gva,
>> diff --git a/drivers/kvm/kvm_main.c b/drivers/kvm/kvm_main.c
>> index 87275be..a9db477 100644
>> --- a/drivers/kvm/kvm_main.c
>> +++ b/drivers/kvm/kvm_main.c
>> @@ -2194,12 +2194,13 @@ again:
>>
>> kvm_x86_ops->run(vcpu, kvm_run);
>>
>> - kvm_guest_exit();
>> vcpu->guest_mode = 0;
>> local_irq_enable();
>>
>> ++vcpu->stat.exits;
>>
>>
>
> Need a barrier() here. Otherwise the compiler may reorder
> "kvm_guest_exit();" and "++vcpu->stats.exits;", making kvm_guest_exit()
> right after local_irq_enable(). If it inlines kvm_guest_exit(), and
> further encodes it in one instruction (both likely) we get:
>
> sti
> andl $something, (something_else)

For the moment it is:

5676: fb sti
5677: 48 8b 85 f8 fe ff ff mov 0xfffffffffffffef8(%rbp),%rax
567e: 8b 80 10 0c 00 00 mov 0xc10(%rax),%eax
5684: 8d 50 01 lea 0x1(%rax),%edx
5687: 48 8b 85 f8 fe ff ff mov 0xfffffffffffffef8(%rbp),%rax
568e: 89 90 10 0c 00 00 mov %edx,0xc10(%rax)
5694: 65 48 8b 04 25 00 00 mov %gs:0x0,%rax
569b: 00 00
569d: 48 89 45 a8 mov %rax,0xffffffffffffffa8(%rbp)
56a1: 48 8b 45 a8 mov 0xffffffffffffffa8(%rbp),%rax
56a5: 48 89 45 b0 mov %rax,0xffffffffffffffb0(%rbp)
56a9: 48 8b 45 b0 mov 0xffffffffffffffb0(%rbp),%rax
56ad: 48 89 c2 mov %rax,%rdx
56b0: 8b 42 14 mov 0x14(%rdx),%eax
56b3: 83 e0 ef and $0xffffffffffffffef,%eax
56b6: 89 42 14 mov %eax,0x14(%rdx)

So I think it is out of the interrupt shadow... but you're right I will resend
the patch adding barrier() (because in futur gcc can behave differently),
removing the part already added by Ingo and copying kvm-devel.

> The andl executes in interrupt shadow (that is, interrupts are still
> disabled) and the timer tick won't see PF_VCPU.
>
>> + kvm_guest_exit();
>> +
>> preempt_enable();
>>
>> /*
>>
>
>
> Please copy kvm-devel on kvm patches.
>

Laurent
--
---------------- Laurent.Vivier@bull.net -----------------
"Given enough eyeballs, all bugs are shallow" E. S. Raymond