[PATCH 0/7] Consolidate vcpu ioctl locking

[PATCH 0/7] Consolidate vcpu ioctl locking

[Avi Kivity]

Avi Kivity (7):
  KVM: PPC: Add missing vcpu_load()/vcpu_put() in vcpu ioctls
  KVM: x86: Add missing locking to arch specific vcpu ioctls
  KVM: move vcpu locking to dispatcher for generic vcpu ioctls
  KVM: x86: Lock arch specific vcpu ioctls centrally
  KVM: s390: Centrally lock arch specific vcpu ioctls
  KVM: PPC: Centralize locking of arch specific vcpu ioctls
  KVM: Consolidate arch specific vcpu ioctl locking

 arch/ia64/kvm/kvm-ia64.c   |   11 -------
 arch/powerpc/kvm/book3s.c  |   10 +-----
 arch/powerpc/kvm/booke.c   |    5 ++-
 arch/powerpc/kvm/powerpc.c |    7 +---
 arch/s390/kvm/kvm-s390.c   |   55 ++++++++++-------------------------
 arch/x86/kvm/x86.c         |   69 +------------------------------------------
 virt/kvm/kvm_main.c        |   13 ++++++++
 7 files changed, 39 insertions(+), 131 deletions(-)

Re: [PATCH 0/7] Consolidate vcpu ioctl locking

[Alexander Graf]

On 13.05.2010, at 13:17, Avi Kivity wrote:

> 
> 
> Avi Kivity (7):
>  KVM: PPC: Add missing vcpu_load()/vcpu_put() in vcpu ioctls
>  KVM: x86: Add missing locking to arch specific vcpu ioctls
>  KVM: move vcpu locking to dispatcher for generic vcpu ioctls
>  KVM: x86: Lock arch specific vcpu ioctls centrally
>  KVM: s390: Centrally lock arch specific vcpu ioctls
>  KVM: PPC: Centralize locking of arch specific vcpu ioctls
>  KVM: Consolidate arch specific vcpu ioctl locking

Mind to give a high level overview on where you're moving which locks?

Alex

Re: [PATCH 0/7] Consolidate vcpu ioctl locking

[Avi Kivity]

On 05/13/2010 02:57 PM, Alexander Graf wrote:
>
> Mind to give a high level overview on where you're moving which locks?
>
>    

Um, looks like I forgot to fill in the patchset header.  Sorry.

The patches move all vcpu ioctl locking from the individual ioctl 
handlers (e.g. kvm_vcpu_ioctl_set_cpuid()) to the top-level vcpu ioctl 
handler (kvm_vcpu_ioctl()).  So tons of vcpu_load()/vcpu_put() pairs 
(some of the missing) get replaced by a single pair.

-- 
error compiling committee.c: too many arguments to function

Re: [PATCH 0/7] Consolidate vcpu ioctl locking

[Avi Kivity]

On 05/13/2010 03:01 PM, Avi Kivity wrote:
> On 05/13/2010 02:57 PM, Alexander Graf wrote:
>>
>> Mind to give a high level overview on where you're moving which locks?
>>
>
> Um, looks like I forgot to fill in the patchset header.  Sorry.

Gar, I actually wrote it but forgot to save the file.

-- 
error compiling committee.c: too many arguments to function

Re: [PATCH 0/7] Consolidate vcpu ioctl locking

[Avi Kivity]

On 05/13/2010 03:03 PM, Avi Kivity wrote:
> On 05/13/2010 03:01 PM, Avi Kivity wrote:
>> On 05/13/2010 02:57 PM, Alexander Graf wrote:
>>>
>>> Mind to give a high level overview on where you're moving which locks?
>>>
>>
>> Um, looks like I forgot to fill in the patchset header.  Sorry.
>
> Gar, I actually wrote it but forgot to save the file.
>

And it had some useful info:

[PATCH 0/7] Consolidate vcpu ioctl locking

In general, all vcpu ioctls need to take the vcpu mutex, but each one 
does it
(or not) individually.  This is cumbersome and error prone.

This patchset moves all locking to a central place.  This is complicated
by the fact that ppc's KVM_INTERRUPT and s390's KVM_S390_INTERRUPT break
the convention and need to run unlocked.

-- 
error compiling committee.c: too many arguments to function

Re: [PATCH 0/7] Consolidate vcpu ioctl locking

[Alexander Graf]

On 13.05.2010, at 14:03, Avi Kivity wrote:

> On 05/13/2010 03:03 PM, Avi Kivity wrote:
>> On 05/13/2010 03:01 PM, Avi Kivity wrote:
>>> On 05/13/2010 02:57 PM, Alexander Graf wrote:
>>>> 
>>>> Mind to give a high level overview on where you're moving which locks?
>>>> 
>>> 
>>> Um, looks like I forgot to fill in the patchset header.  Sorry.
>> 
>> Gar, I actually wrote it but forgot to save the file.
>> 
> 
> And it had some useful info:
> 
> [PATCH 0/7] Consolidate vcpu ioctl locking
> 
> In general, all vcpu ioctls need to take the vcpu mutex, but each one does it
> (or not) individually.  This is cumbersome and error prone.
> 
> This patchset moves all locking to a central place.  This is complicated
> by the fact that ppc's KVM_INTERRUPT and s390's KVM_S390_INTERRUPT break
> the convention and need to run unlocked.

Why is the x86 non-kernel-pic path different?

Alex

Re: [PATCH 0/7] Consolidate vcpu ioctl locking

[Avi Kivity]

On 05/13/2010 03:18 PM, Alexander Graf wrote:
>
>> [PATCH 0/7] Consolidate vcpu ioctl locking
>>
>> In general, all vcpu ioctls need to take the vcpu mutex, but each one does it
>> (or not) individually.  This is cumbersome and error prone.
>>
>> This patchset moves all locking to a central place.  This is complicated
>> by the fact that ppc's KVM_INTERRUPT and s390's KVM_S390_INTERRUPT break
>> the convention and need to run unlocked.
>>      
> Why is the x86 non-kernel-pic path different?
>    

Userspace issues the ioctl from a vcpu thread.

It has to, btw, since whether an interrupt can be injected or not 
depends on vcpu-synchronous registers: eflags.if and tpr/cr8.

-- 
error compiling committee.c: too many arguments to function

Re: [PATCH 0/7] Consolidate vcpu ioctl locking

[Alexander Graf]

Am 13.05.2010 um 14:29 schrieb Avi Kivity <avi@redhat.com>:

> On 05/13/2010 03:18 PM, Alexander Graf wrote:
>>
>>> [PATCH 0/7] Consolidate vcpu ioctl locking
>>>
>>> In general, all vcpu ioctls need to take the vcpu mutex, but each  
>>> one does it
>>> (or not) individually.  This is cumbersome and error prone.
>>>
>>> This patchset moves all locking to a central place.  This is  
>>> complicated
>>> by the fact that ppc's KVM_INTERRUPT and s390's KVM_S390_INTERRUPT  
>>> break
>>> the convention and need to run unlocked.
>>>
>> Why is the x86 non-kernel-pic path different?
>>
>
> Userspace issues the ioctl from a vcpu thread.
>
> It has to, btw, since whether an interrupt can be injected or not  
> depends on vcpu-synchronous registers: eflags.if and tpr/cr8.

On ppc we don't have a tpr, but eflags.if is basically the same as  
msr.ee.

The major difference apparently is that on ppc we KVM_INTERRUPT pulls  
the interrupt line. On vcpu_run we then check whether msr.ee is set  
and if so, trigger the interrupt.

I wonder why we don't do the same for x86. The current limitation on  
userspace checking eflags and the tpr seems cumbersome.

Alex

>

Re: [PATCH 0/7] Consolidate vcpu ioctl locking

[Avi Kivity]

On 05/15/2010 11:26 AM, Alexander Graf wrote:
>
>> That means you never inject an interrupt from the iothread (or from a different vcpu thread)?
>>
>> If that's the case we might make it part of the API and require the ioctl to be issued from the vcpu thread.  We'd still be left with the s390 exception.
>>      
> Well I'm not sure that's guaranteed for MOL or Dolphin, but I guess the user base is small enough to ignore them.
>
> Either way, I'm actually rather unhappy with the way interrupts work right now. We're only injecting interrupts when in the main loop, which is rare if we did our homework right. So what I'd really like to see is that the MPIC on ppc directly invokes KVM_INTERRUPT to pull (and losen) the interrupt line. That way we can't just accidently miss interrupts.
>    

on x86 we signal the vcpu thread to pull it out of the main loop and 
poll the apic.

> What happens now with ppc64 guests that have EE lazily disabled is:
>
> * device in userspace wants to trigger an interrupt
> * mpic pulls external interrupt line
>    

<signal>

> * kvm_arch_pre_run sees that external interrupt line is pulled, issues ioctl(KVM_INTERRUPT), kernel space sets trigger_interrupt=1
> * we enter the guest
> * we inject an external interrupt, set trigger_interrupt=0
> * guest gets the interrupt, sees it's lazily disabled, sets msr.ee=0
> * guest moves on, sets msr.ee=1 again later
> *** guest expects the interrupt to trigger again, but we don't know that it's still pending
>    

Why does the guest expect the interrupt to trigger again?  Is it level 
triggered until acknowledged?

That's exactly why x86 has run->request_interrupt_window, 
run->ready_for_interrupt_injection, and run->if_flag.

> ->  we need to exit to userspace to realize that the interrupt is still active
>
> This is fundamentally broken. What I'd like to see is:
>
> * device in userspace wants to trigger an interrupt
> * mpic pulls external interrupt line, automatically issues ioctl(KVM_INTERRUPT)
> * we enter the guest
> * we inject the external interrupt
> * guest gets the interrupt, sees it's lazily disabled, sets msr.ee=0
> * guest moves on, sets msr.ee=1 again later
> * we inject the external interrupt again, since it's still active
> * guest acknowledges the interrupt with the mpic, issues ioctl(KVM_INTERRUPT, disable)
> ->  all is great
>    

This is similar to KVM_IRQ_LINE.

> For that to work we need to enable triggering KVM_INTERRUPT from a non-vcpu thread.
>    

KVM_IRQ_LINE is a vm ioctl instead of a vcpu ioctl.

The motivation for the strict 'issue vcpu ioctls from vcpu thread' rule 
is to prepare the way for a syscall (instead of ioctl) API.  Then we 
lost the fd argument, and choosing the vcpu is done by associating it 
with the current task.  That allows us to get rid of vcpu->mutex and 
fget_light() (as well as the ioctl dispatch).

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

Re: [PATCH 0/7] Consolidate vcpu ioctl locking

[Alexander Graf]

On 15.05.2010, at 19:30, Avi Kivity wrote:

> On 05/15/2010 11:26 AM, Alexander Graf wrote:
>> 
>>> That means you never inject an interrupt from the iothread (or from a different vcpu thread)?
>>> 
>>> If that's the case we might make it part of the API and require the ioctl to be issued from the vcpu thread.  We'd still be left with the s390 exception.
>>>     
>> Well I'm not sure that's guaranteed for MOL or Dolphin, but I guess the user base is small enough to ignore them.
>> 
>> Either way, I'm actually rather unhappy with the way interrupts work right now. We're only injecting interrupts when in the main loop, which is rare if we did our homework right. So what I'd really like to see is that the MPIC on ppc directly invokes KVM_INTERRUPT to pull (and losen) the interrupt line. That way we can't just accidently miss interrupts.
>>   
> 
> on x86 we signal the vcpu thread to pull it out of the main loop and poll the apic.

Hrm, makes sense. Though it's additional overhead of a task switch. Why take the burden if you don't have to?

> 
>> What happens now with ppc64 guests that have EE lazily disabled is:
>> 
>> * device in userspace wants to trigger an interrupt
>> * mpic pulls external interrupt line
>>   
> 
> <signal>
> 
>> * kvm_arch_pre_run sees that external interrupt line is pulled, issues ioctl(KVM_INTERRUPT), kernel space sets trigger_interrupt=1
>> * we enter the guest
>> * we inject an external interrupt, set trigger_interrupt=0
>> * guest gets the interrupt, sees it's lazily disabled, sets msr.ee=0
>> * guest moves on, sets msr.ee=1 again later
>> *** guest expects the interrupt to trigger again, but we don't know that it's still pending
>>   
> 
> Why does the guest expect the interrupt to trigger again?  Is it level triggered until acknowledged?

IIUC, yes.

> That's exactly why x86 has run->request_interrupt_window, run->ready_for_interrupt_injection, and run->if_flag.

So how does x86 userspace get notified when it has an interrupt pending but couldn't inject it? Without a notification, we delay interrupts by quite some time.

> 
>> ->  we need to exit to userspace to realize that the interrupt is still active
>> 
>> This is fundamentally broken. What I'd like to see is:
>> 
>> * device in userspace wants to trigger an interrupt
>> * mpic pulls external interrupt line, automatically issues ioctl(KVM_INTERRUPT)
>> * we enter the guest
>> * we inject the external interrupt
>> * guest gets the interrupt, sees it's lazily disabled, sets msr.ee=0
>> * guest moves on, sets msr.ee=1 again later
>> * we inject the external interrupt again, since it's still active
>> * guest acknowledges the interrupt with the mpic, issues ioctl(KVM_INTERRUPT, disable)
>> ->  all is great
>>   
> 
> This is similar to KVM_IRQ_LINE.

Well, KVM_IRQ_LINE would be the in-kernel pic, no? That'd mean I'd have to reimplement the mpic in kernel space - which might eventually be a good idea when going for SMP, but I'd first like to see if I can keep the current interrupt injection path efficient.

> 
>> For that to work we need to enable triggering KVM_INTERRUPT from a non-vcpu thread.
>>   
> 
> KVM_IRQ_LINE is a vm ioctl instead of a vcpu ioctl.
> 
> The motivation for the strict 'issue vcpu ioctls from vcpu thread' rule is to prepare the way for a syscall (instead of ioctl) API.  Then we lost the fd argument, and choosing the vcpu is done by associating it with the current task.  That allows us to get rid of vcpu->mutex and fget_light() (as well as the ioctl dispatch).

If we define the API to only work on the current vcpu with a few excetions where we're safe (KVM_INTERRUPT), that'd get rid of the mutex too, no?
What does fget_light do?
And does the ioctl dispatch cost that much? I like the flexibility of it to be honest.


Alex

Re: [PATCH 0/7] Consolidate vcpu ioctl locking

[Avi Kivity]

On 05/16/2010 04:00 AM, Alexander Graf wrote:
> On 15.05.2010, at 19:30, Avi Kivity wrote:
>
>    
>> On 05/15/2010 11:26 AM, Alexander Graf wrote:
>>      
>>>        
>>>> That means you never inject an interrupt from the iothread (or from a different vcpu thread)?
>>>>
>>>> If that's the case we might make it part of the API and require the ioctl to be issued from the vcpu thread.  We'd still be left with the s390 exception.
>>>>
>>>>          
>>> Well I'm not sure that's guaranteed for MOL or Dolphin, but I guess the user base is small enough to ignore them.
>>>
>>> Either way, I'm actually rather unhappy with the way interrupts work right now. We're only injecting interrupts when in the main loop, which is rare if we did our homework right. So what I'd really like to see is that the MPIC on ppc directly invokes KVM_INTERRUPT to pull (and losen) the interrupt line. That way we can't just accidently miss interrupts.
>>>
>>>        
>> on x86 we signal the vcpu thread to pull it out of the main loop and poll the apic.
>>      
> Hrm, makes sense. Though it's additional overhead of a task switch. Why take the burden if you don't have to?
>    

That's what the world looked like in 2006.

We could change it, but there's not much point, since having the local 
apic in the kernel is pretty much a requirement for reasonable performance.

>> That's exactly why x86 has run->request_interrupt_window, run->ready_for_interrupt_injection, and run->if_flag.
>>      
> So how does x86 userspace get notified when it has an interrupt pending but couldn't inject it? Without a notification, we delay interrupts by quite some time.
>    

run->ready_for_interrupt_injection and run->request_irq_window.

>>> ->   we need to exit to userspace to realize that the interrupt is still active
>>>
>>> This is fundamentally broken. What I'd like to see is:
>>>
>>> * device in userspace wants to trigger an interrupt
>>> * mpic pulls external interrupt line, automatically issues ioctl(KVM_INTERRUPT)
>>> * we enter the guest
>>> * we inject the external interrupt
>>> * guest gets the interrupt, sees it's lazily disabled, sets msr.ee=0
>>> * guest moves on, sets msr.ee=1 again later
>>> * we inject the external interrupt again, since it's still active
>>> * guest acknowledges the interrupt with the mpic, issues ioctl(KVM_INTERRUPT, disable)
>>> ->   all is great
>>>
>>>        
>> This is similar to KVM_IRQ_LINE.
>>      
> Well, KVM_IRQ_LINE would be the in-kernel pic, no? That'd mean I'd have to reimplement the mpic in kernel space - which might eventually be a good idea when going for SMP, but I'd first like to see if I can keep the current interrupt injection path efficient.
>    

Sure.

>>> For that to work we need to enable triggering KVM_INTERRUPT from a non-vcpu thread.
>>>
>>>        
>> KVM_IRQ_LINE is a vm ioctl instead of a vcpu ioctl.
>>
>> The motivation for the strict 'issue vcpu ioctls from vcpu thread' rule is to prepare the way for a syscall (instead of ioctl) API.  Then we lost the fd argument, and choosing the vcpu is done by associating it with the current task.  That allows us to get rid of vcpu->mutex and fget_light() (as well as the ioctl dispatch).
>>      
> If we define the API to only work on the current vcpu with a few excetions where we're safe (KVM_INTERRUPT), that'd get rid of the mutex too, no?
>    

Yes.  Need to document it though.

> What does fget_light do?
>    

Make sure that the vcpu fd doesn't disappear.

> And does the ioctl dispatch cost that much? I like the flexibility of it to be honest.
>    

Not much, which is why there's no movement in that direction.

-- 
error compiling committee.c: too many arguments to function

Re: [PATCH 0/7] Consolidate vcpu ioctl locking

[Alexander Graf]

On 16.05.2010, at 10:23, Avi Kivity wrote:

> On 05/16/2010 04:00 AM, Alexander Graf wrote:
>> On 15.05.2010, at 19:30, Avi Kivity wrote:
>> 
>>   
>>> On 05/15/2010 11:26 AM, Alexander Graf wrote:
>>>     
>>>>       
>>>>> That means you never inject an interrupt from the iothread (or from a different vcpu thread)?
>>>>> 
>>>>> If that's the case we might make it part of the API and require the ioctl to be issued from the vcpu thread.  We'd still be left with the s390 exception.
>>>>> 
>>>>>         
>>>> Well I'm not sure that's guaranteed for MOL or Dolphin, but I guess the user base is small enough to ignore them.
>>>> 
>>>> Either way, I'm actually rather unhappy with the way interrupts work right now. We're only injecting interrupts when in the main loop, which is rare if we did our homework right. So what I'd really like to see is that the MPIC on ppc directly invokes KVM_INTERRUPT to pull (and losen) the interrupt line. That way we can't just accidently miss interrupts.
>>>> 
>>>>       
>>> on x86 we signal the vcpu thread to pull it out of the main loop and poll the apic.
>>>     
>> Hrm, makes sense. Though it's additional overhead of a task switch. Why take the burden if you don't have to?
>>   
> 
> That's what the world looked like in 2006.
> 
> We could change it, but there's not much point, since having the local apic in the kernel is pretty much a requirement for reasonable performance.

Well, I'm not convinced yet that's the case for PPC as well. The timer is in-cpu anyways and I don't see why IPIs should be slow with a userspace pic - if we keep the overhead low.

So let me think this through. With remote interrupt injection we have.

* thread 1 does vcpu_run
* thread 2 triggers KVM_INTERRUPT on fd
* thread 2 signals thread 1 so we're sure the interrupt gets injected
* thread 1 exits into qemu
* thread 1 goes back into the vcpu, triggering an interrupt

Without we have:

* thread 1 does vcpu_run
* thread 2 wants to trigger an an interrupt, sets the qemu internal bit
* thread 2 signals thread 1 so we're sure the interrupt gets processed
* thread 1 exits into qemu
* thread 1 triggers KVM_INTERRUPT on fd
* thread 1 goes into the vcpu

So we don't really buy anything from doing the remote injection. Hrm.

What's somewhat striking me here though is - why do we need KVM_INTERRUPT when there's all those kvm_run fields? Can't we just do interrupt injection by setting run->trigger_interrupt? There's only a single "interrupt line" on the CPU anyways. That way we'd save the ioctl and get rid of the locking problem altogether.

Alex

Re: [PATCH 0/7] Consolidate vcpu ioctl locking

[Avi Kivity]

On 05/16/2010 12:01 PM, Alexander Graf wrote:
>
>> That's what the world looked like in 2006.
>>
>> We could change it, but there's not much point, since having the local apic in the kernel is pretty much a requirement for reasonable performance.
>>      
> Well, I'm not convinced yet that's the case for PPC as well. The timer is in-cpu anyways and I don't see why IPIs should be slow with a userspace pic - if we keep the overhead low.
>    

If it's at all possible keep the mpic out.  I am _not_ advocating 
pushing ppc's mpic into the kernel.

> So let me think this through. With remote interrupt injection we have.
>
> * thread 1 does vcpu_run
> * thread 2 triggers KVM_INTERRUPT on fd
> * thread 2 signals thread 1 so we're sure the interrupt gets injected
> * thread 1 exits into qemu
>    

This doesn't seem necessary.  The kernel can own the interrupt line, so 
it remembers it from the last KVM_INTERRUPT.

> * thread 1 goes back into the vcpu, triggering an interrupt
>
> Without we have:
>
> * thread 1 does vcpu_run
> * thread 2 wants to trigger an an interrupt, sets the qemu internal bit
> * thread 2 signals thread 1 so we're sure the interrupt gets processed
> * thread 1 exits into qemu
> * thread 1 triggers KVM_INTERRUPT on fd
> * thread 1 goes into the vcpu
>
> So we don't really buy anything from doing the remote injection. Hrm.
>    

Not if you make interrupt injection a lightweight exit.

> What's somewhat striking me here though is - why do we need KVM_INTERRUPT when there's all those kvm_run fields? Can't we just do interrupt injection by setting run->trigger_interrupt? There's only a single "interrupt line" on the CPU anyways. That way we'd save the ioctl and get rid of the locking problem altogether.
>    

That's what x86 does.  However, it's synchronous.

-- 
error compiling committee.c: too many arguments to function

Re: [PATCH 0/7] Consolidate vcpu ioctl locking

[Alexander Graf]

On 16.05.2010, at 11:09, Avi Kivity wrote:

> On 05/16/2010 12:01 PM, Alexander Graf wrote:
>> 
>>> That's what the world looked like in 2006.
>>> 
>>> We could change it, but there's not much point, since having the local apic in the kernel is pretty much a requirement for reasonable performance.
>>>     
>> Well, I'm not convinced yet that's the case for PPC as well. The timer is in-cpu anyways and I don't see why IPIs should be slow with a userspace pic - if we keep the overhead low.
>>   
> 
> If it's at all possible keep the mpic out.  I am _not_ advocating pushing ppc's mpic into the kernel.
> 
>> So let me think this through. With remote interrupt injection we have.
>> 
>> * thread 1 does vcpu_run
>> * thread 2 triggers KVM_INTERRUPT on fd
>> * thread 2 signals thread 1 so we're sure the interrupt gets injected
>> * thread 1 exits into qemu
>>   
> 
> This doesn't seem necessary.  The kernel can own the interrupt line, so it remembers it from the last KVM_INTERRUPT.

It's not? On signals we always exit to userspace, no?

> 
>> * thread 1 goes back into the vcpu, triggering an interrupt
>> 
>> Without we have:
>> 
>> * thread 1 does vcpu_run
>> * thread 2 wants to trigger an an interrupt, sets the qemu internal bit
>> * thread 2 signals thread 1 so we're sure the interrupt gets processed
>> * thread 1 exits into qemu
>> * thread 1 triggers KVM_INTERRUPT on fd
>> * thread 1 goes into the vcpu
>> 
>> So we don't really buy anything from doing the remote injection. Hrm.
>>   
> 
> Not if you make interrupt injection a lightweight exit.

Please elaborate.

> 
>> What's somewhat striking me here though is - why do we need KVM_INTERRUPT when there's all those kvm_run fields? Can't we just do interrupt injection by setting run->trigger_interrupt? There's only a single "interrupt line" on the CPU anyways. That way we'd save the ioctl and get rid of the locking problem altogether.
>>   
> 
> That's what x86 does.  However, it's synchronous.

For everyone except for the vcpu thread executing the interrupt, it's asynchronous, right? The same applies to an in-kernel pic.


Alex

Re: [PATCH 0/7] Consolidate vcpu ioctl locking

[Avi Kivity]

On 05/16/2010 12:35 PM, Alexander Graf wrote:
>
>>
>>> So let me think this through. With remote interrupt injection we have.
>>>
>>> * thread 1 does vcpu_run
>>> * thread 2 triggers KVM_INTERRUPT on fd
>>> * thread 2 signals thread 1 so we're sure the interrupt gets injected
>>> * thread 1 exits into qemu
>>>
>>>        
>> This doesn't seem necessary.  The kernel can own the interrupt line, so it remembers it from the last KVM_INTERRUPT.
>>      
> It's not?

With s/signals/IPIs/.

> On signals we always exit to userspace, no?
>    

Yes (if the signal isn't blocked).

>>> * thread 1 goes back into the vcpu, triggering an interrupt
>>>
>>> Without we have:
>>>
>>> * thread 1 does vcpu_run
>>> * thread 2 wants to trigger an an interrupt, sets the qemu internal bit
>>> * thread 2 signals thread 1 so we're sure the interrupt gets processed
>>> * thread 1 exits into qemu
>>> * thread 1 triggers KVM_INTERRUPT on fd
>>> * thread 1 goes into the vcpu
>>>
>>> So we don't really buy anything from doing the remote injection. Hrm.
>>>
>>>        
>> Not if you make interrupt injection a lightweight exit.
>>      
> Please elaborate.
>    

1: vcpu_run
2: KVM_INTERRUPT
2k: sets flag, if msr.ee IPIs 1 or wakes up 1 if halted
1k: notices flag, if msr.ee injects interrupt
...
1g: acks
1k: forwards ack to userspace
1: completes interrupt


>>> What's somewhat striking me here though is - why do we need KVM_INTERRUPT when there's all those kvm_run fields? Can't we just do interrupt injection by setting run->trigger_interrupt? There's only a single "interrupt line" on the CPU anyways. That way we'd save the ioctl and get rid of the locking problem altogether.
>>>
>>>        
>> That's what x86 does.  However, it's synchronous.
>>      
> For everyone except for the vcpu thread executing the interrupt, it's asynchronous, right?

For everyone other than the vcpu thread, it's off limits.  kvm_run is 
only read on KVM_RUN entries and written on KVM_RUN exits.

> The same applies to an in-kernel pic.
>    

The in-kernel pic doesn't use kvm_run.


-- 
error compiling committee.c: too many arguments to function

Re: [PATCH 0/7] Consolidate vcpu ioctl locking

[Alexander Graf]

On 16.05.2010, at 11:47, Avi Kivity wrote:

> 1: vcpu_run
> 2: KVM_INTERRUPT
> 2k: sets flag, if msr.ee IPIs 1 or wakes up 1 if halted

Doesn't that break when we have a while(1) loop in the guest with msr.ee=0 while no timer is scheduled on the host? But then again with msr.ee=0 we don't get interrupts in the guest and to set msr.ee=1 we trap. Yeah, that would work.

> 1k: notices flag, if msr.ee injects interrupt
> ...
> 1g: acks

The ack is done in userspace by the mpic, so we can just complete the interrupt there.

> 1k: forwards ack to userspace
> 1: completes interrupt


So if I just have a field kvm_run->external_active I could set that to =1 on KVM_INTERRUPT including the above logic. To acknowledge it userspace would then do something like this in kvm_arch_pre_run:

    if (kvm_run->external_active &&
        !((env->interrupt_request & CPU_INTERRUPT_HARD) &&
          (env->irq_input_state & (1<<PPC_INPUT_INT))))
    {
        kvm_run->external_active = 0;
    }

The big question is how to make such a change backwards compatible. But I guess I could just reuse the feature enabling framework. Well, sounds like we're getting closer.


Alex

Re: [PATCH 0/7] Consolidate vcpu ioctl locking

[Carsten Otte]

On 15.05.2010 10:26, Alexander Graf wrote:
> On S390, I'm also still sceptical if the implementation we have really works. A device injects an S390_INTERRUPT with its address and on the next vcpu_run, an according interrupt is issued. But what happens if two devices trigger an S390_INTERRUPT before the vcpu_run? We'd have lost an interrupt by then...
We're safe on that: the interrupt info field in both struct kvm (for 
floating interrupts) and struct vcpu (for cpu local interrupts) have 
their own locking and can queue up interrupts.

cheers,
Carsten