On 19.09.2011, at 21:12, Scott Wood wrote:
> On 09/19/2011 04:32 AM, Alexander Graf wrote:
>>
>> On 15.09.2011, at 22:52, Scott Wood wrote:
>>
>>> On 09/08/2011 10:39 AM, Alexander Graf wrote:
>>>>
>>>> On 08.09.2011, at 17:34, Scott Wood wrote:
>>>>
>>>>> On Wed, Sep 07, 2011 at 12:41:35PM +0200, Alexander Graf wrote:
>>>>>> Yes, but why can't we do this in the vcpu thread's context so we only
>>>>>> ever have a single instance accessing the vcpu struct? It makes a lot
>>>>>> of things a lot easier.
>>>>>
>>>>> Why? We don't do it for external interrupts. It would complicate
>>>>> locking, not simplify it -- we'd need to defer the execution to some
>>>>> context which can block, and then kick the guest out of guest mode, and
>>>>> make sure it doesn't reenter before we get the mutex...
>>>>
>>>> We need to kick the vcpu thread out of context anyways because we're
>>>> otherwise delaying delivery of the decr interrupt. So we can just as
>>>> well handle it all there then. Being lazy is good, but please not at
>>>> the cost of latency.
>>>
>>> That means we need a way to tell the thread to do this. Instead of
>>> using set_bit(), you want dedicated variables for pending_decr,
>>> pending_fit, pending_watchdog? And then we check them on every exit?
>>
>> Well, whatever we do we have to kvm_vcpu_kick() the receiving vcpu
>> out of context.
>
> Yes, the point is how the information about the event is communicated.
>
>> So we can just as well tell it to inject its own interrupt
>> and don't have to deal with atomic operations in most cases then, no?
>
> Either we use atomics, or we split each event into its own variable. We
> already use atomics for injecting other interrupts.
>
> It seems simpler to just use the architected format rather than invent
> something new. And if we do something non-architected, that makes it
> more likely that injection of the interrupt triggered by qemu setting
> the bit with sregs is broken (untested special case).
>
>>> Besides avoiding the overhead of an atomic operation (premature
>>> optimization), what does this buy us? The ability to move the
>>> dequeue-after-mask from kvmppc_booke_irqprio_deliver() to
>>> kvmppc_set_tcr() (only to be replaced by the check for pending_foo)?
>>>
>>> How is using set_bits() for TSR any different from the set_bit() we use
>>> in kvmppc_booke_queue_irqprio()?
>>
>> It keeps guest visible registers vcpu private. I want the vcpu thread to own all vcpu registers - and TSR is a vcpu register.
>
> It is private to the vcpu implementation. I don't get why it should be
> private to the thread just because it's an architected register -- would
> it be any different if we called it "not_tsr" (or "pending_timers")
> whose field "not_dis" (or a bit called BOOKE_TIMER_DECR) happened to be
> at the right location, and we happen to be able to very easily turn it
> into tsr when the guest wants to read it? :-)
I think I'm getting your point. So what we want is:
in timer handler:
set_bit(TSR_DIS, vcpu->arch.tsr);
kvm_make_request(KVM_REQ_PPC_TSR_UPDATE, vcpu);
kvm_vcpu_kick(vcpu);
in vcpu entry code:
if (vcpu->requests)
if (kvm_check_request(KVM_REQ_PPC_TSR_UPDATE, vcpu))
kvmppc_update_tsr(vcpu);
void kvmppc_update_tsr(struct kvm_vcpu *vcpu)
{
if (vcpu->arch.tsr & TSR_DIS &&
vcpu->arch.tcr & TCR_DIE) {
kvmppc_core_queue_dec(vcpu);
}
// XXX also implement dequeue!
}
in emulate code:
case SPR_TSR:
vcpu->arch.tsr &= ~TSR_DIS;
kvmppc_update_tsr(vcpu);
Right?
Alex
--
To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
