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

[Avi Kivity <avi-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>]

On 05/13/2010 10:49 PM, Alexander Graf wrote:
>
> Am 13.05.2010 um 14:29 schrieb Avi Kivity <avi-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>:
>
>> 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.

On x86 eflags.if is freely changeable by the guest, so if we want to 
queue an interrupt we have to IPI the vcpu to force it out of guest 
mode, so we can inspect eflags.  This means the vcpu thread has to be 
interrupted one way or another.

The tpr (really ppr) is even more problematic as it is maintained in 
userspace, not in the kernel (for non-kernel-irqchip).  It could in 
theory be inspected by another thread, but we wouldn't gain anything by 
it due to the requirement to IPI.

> void kvmppc_book3s_queue_irqprio(struct kvm_vcpu *vcpu, unsigned int vec)
> {
>     vcpu->stat.queue_intr++;
>
>     set_bit(kvmppc_book3s_vec2irqprio(vec),
> &vcpu->arch.pending_exceptions);
> #ifdef EXIT_DEBUG
>     printk(KERN_INFO "Queueing interrupt %x\n", vec);
> #endif
> }

Isn't this missing an IPI if the vcpu is in guest mode?


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