Re: [PATCH 5/5] KVM: PPC: booke: Improve timer register emulation

[Scott Wood <scottwood@freescale.com>]

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? :-)

-Scott