* Re: [PATCH 1/5] KVM: PPC: booke: Fix int_pending calculation for
2011-08-26 23:31 [PATCH 1/5] KVM: PPC: booke: Fix int_pending calculation for MSR[EE] Scott Wood
@ 2011-09-02 13:53 ` Alexander Graf
2011-09-02 18:17 ` Scott Wood
` (3 subsequent siblings)
4 siblings, 0 replies; 6+ messages in thread
From: Alexander Graf @ 2011-09-02 13:53 UTC (permalink / raw)
To: kvm-ppc
On 08/27/2011 01:31 AM, Scott Wood wrote:
> int_pending was only being lowered if a bit in pending_exceptions
> was cleared during exception delivery -- but for interrupts, we clear
> it during IACK/TSR emulation. This caused paravirt for enabling
> MSR[EE] to be ineffective.
But that means that int_pending can still be 1 even though there is none
pending as we don't get the call to deliver_interrupts when it gets
lowered. Please create a common function to remove a bit from
pending_exceptions and do the check there.
Alex
> Signed-off-by: Scott Wood<scottwood@freescale.com>
> ---
> arch/powerpc/kvm/booke.c | 6 +-----
> 1 files changed, 1 insertions(+), 5 deletions(-)
>
> diff --git a/arch/powerpc/kvm/booke.c b/arch/powerpc/kvm/booke.c
> index d967faf..aeb69b2 100644
> --- a/arch/powerpc/kvm/booke.c
> +++ b/arch/powerpc/kvm/booke.c
> @@ -292,7 +292,6 @@ static int kvmppc_booke_irqprio_deliver(struct kvm_vcpu *vcpu,
> void kvmppc_core_deliver_interrupts(struct kvm_vcpu *vcpu)
> {
> unsigned long *pending =&vcpu->arch.pending_exceptions;
> - unsigned long old_pending = vcpu->arch.pending_exceptions;
> unsigned int priority;
>
> priority = __ffs(*pending);
> @@ -306,10 +305,7 @@ void kvmppc_core_deliver_interrupts(struct kvm_vcpu *vcpu)
> }
>
> /* Tell the guest about our interrupt status */
> - if (*pending)
> - vcpu->arch.shared->int_pending = 1;
> - else if (old_pending)
> - vcpu->arch.shared->int_pending = 0;
> + vcpu->arch.shared->int_pending = !!*pending;
> }
>
> int kvmppc_vcpu_run(struct kvm_run *kvm_run, struct kvm_vcpu *vcpu)
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH 1/5] KVM: PPC: booke: Fix int_pending calculation for
2011-08-26 23:31 [PATCH 1/5] KVM: PPC: booke: Fix int_pending calculation for MSR[EE] Scott Wood
2011-09-02 13:53 ` [PATCH 1/5] KVM: PPC: booke: Fix int_pending calculation for Alexander Graf
@ 2011-09-02 18:17 ` Scott Wood
2011-09-02 19:25 ` [PATCH 1/5] KVM: PPC: booke: Fix int_pending calculation for MSR[EE] paravirt Alexander Graf
` (2 subsequent siblings)
4 siblings, 0 replies; 6+ messages in thread
From: Scott Wood @ 2011-09-02 18:17 UTC (permalink / raw)
To: kvm-ppc
On 09/02/2011 08:53 AM, Alexander Graf wrote:
> On 08/27/2011 01:31 AM, Scott Wood wrote:
>> int_pending was only being lowered if a bit in pending_exceptions
>> was cleared during exception delivery -- but for interrupts, we clear
>> it during IACK/TSR emulation. This caused paravirt for enabling
>> MSR[EE] to be ineffective.
>
> But that means that int_pending can still be 1 even though there is none
> pending as we don't get the call to deliver_interrupts when it gets
> lowered. Please create a common function to remove a bit from
> pending_exceptions and do the check there.
I can do that if you want, but kvmppc_core_deliver_interrupts() should
always get called before we return to the guest. Dequeues that are
asynchronous to a guest exit should be very rare, and would be cured on
the first subsequent guest exit.
-Scott
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH 1/5] KVM: PPC: booke: Fix int_pending calculation for MSR[EE] paravirt
2011-08-26 23:31 [PATCH 1/5] KVM: PPC: booke: Fix int_pending calculation for MSR[EE] Scott Wood
2011-09-02 13:53 ` [PATCH 1/5] KVM: PPC: booke: Fix int_pending calculation for Alexander Graf
2011-09-02 18:17 ` Scott Wood
@ 2011-09-02 19:25 ` Alexander Graf
2011-09-02 19:36 ` [PATCH 1/5] KVM: PPC: booke: Fix int_pending calculation for Scott Wood
2011-09-02 21:28 ` Scott Wood
4 siblings, 0 replies; 6+ messages in thread
From: Alexander Graf @ 2011-09-02 19:25 UTC (permalink / raw)
To: kvm-ppc
On 02.09.2011, at 20:17, Scott Wood wrote:
> On 09/02/2011 08:53 AM, Alexander Graf wrote:
>> On 08/27/2011 01:31 AM, Scott Wood wrote:
>>> int_pending was only being lowered if a bit in pending_exceptions
>>> was cleared during exception delivery -- but for interrupts, we clear
>>> it during IACK/TSR emulation. This caused paravirt for enabling
>>> MSR[EE] to be ineffective.
>>
>> But that means that int_pending can still be 1 even though there is none
>> pending as we don't get the call to deliver_interrupts when it gets
>> lowered. Please create a common function to remove a bit from
>> pending_exceptions and do the check there.
>
> I can do that if you want, but kvmppc_core_deliver_interrupts() should
> always get called before we return to the guest. Dequeues that are
> asynchronous to a guest exit should be very rare, and would be cured on
> the first subsequent guest exit.
Yes, but this means we have yet another subtile indirect assumption. The more we have those, the more people who work on the code need to know about the code to actually work on it. So the easier we can keep the scheme, the better IMHO.
So yes, please replace clear_bit(pending, ...) with clear_bit_pending(...) or so which then would also do the required magic to set the shared page values.
Alex
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/5] KVM: PPC: booke: Fix int_pending calculation for
2011-08-26 23:31 [PATCH 1/5] KVM: PPC: booke: Fix int_pending calculation for MSR[EE] Scott Wood
` (2 preceding siblings ...)
2011-09-02 19:25 ` [PATCH 1/5] KVM: PPC: booke: Fix int_pending calculation for MSR[EE] paravirt Alexander Graf
@ 2011-09-02 19:36 ` Scott Wood
2011-09-02 21:28 ` Scott Wood
4 siblings, 0 replies; 6+ messages in thread
From: Scott Wood @ 2011-09-02 19:36 UTC (permalink / raw)
To: kvm-ppc
On 09/02/2011 02:25 PM, Alexander Graf wrote:
>
> On 02.09.2011, at 20:17, Scott Wood wrote:
>
>> On 09/02/2011 08:53 AM, Alexander Graf wrote:
>>> On 08/27/2011 01:31 AM, Scott Wood wrote:
>>>> int_pending was only being lowered if a bit in pending_exceptions
>>>> was cleared during exception delivery -- but for interrupts, we clear
>>>> it during IACK/TSR emulation. This caused paravirt for enabling
>>>> MSR[EE] to be ineffective.
>>>
>>> But that means that int_pending can still be 1 even though there is none
>>> pending as we don't get the call to deliver_interrupts when it gets
>>> lowered. Please create a common function to remove a bit from
>>> pending_exceptions and do the check there.
>>
>> I can do that if you want, but kvmppc_core_deliver_interrupts() should
>> always get called before we return to the guest. Dequeues that are
>> asynchronous to a guest exit should be very rare, and would be cured on
>> the first subsequent guest exit.
>
> Yes, but this means we have yet another subtile indirect assumption. The more we have those, the more people who work on the code need to know about the code to actually work on it. So the easier we can keep the scheme, the better IMHO.
>
> So yes, please replace clear_bit(pending, ...) with clear_bit_pending(...) or so which then would also do the required magic to set the shared page values.
OK.
-Scott
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH 1/5] KVM: PPC: booke: Fix int_pending calculation for
2011-08-26 23:31 [PATCH 1/5] KVM: PPC: booke: Fix int_pending calculation for MSR[EE] Scott Wood
` (3 preceding siblings ...)
2011-09-02 19:36 ` [PATCH 1/5] KVM: PPC: booke: Fix int_pending calculation for Scott Wood
@ 2011-09-02 21:28 ` Scott Wood
4 siblings, 0 replies; 6+ messages in thread
From: Scott Wood @ 2011-09-02 21:28 UTC (permalink / raw)
To: kvm-ppc
On 09/02/2011 02:25 PM, Alexander Graf wrote:
>
> On 02.09.2011, at 20:17, Scott Wood wrote:
>
>> On 09/02/2011 08:53 AM, Alexander Graf wrote:
>>> On 08/27/2011 01:31 AM, Scott Wood wrote:
>>>> int_pending was only being lowered if a bit in pending_exceptions
>>>> was cleared during exception delivery -- but for interrupts, we clear
>>>> it during IACK/TSR emulation. This caused paravirt for enabling
>>>> MSR[EE] to be ineffective.
>>>
>>> But that means that int_pending can still be 1 even though there is none
>>> pending as we don't get the call to deliver_interrupts when it gets
>>> lowered. Please create a common function to remove a bit from
>>> pending_exceptions and do the check there.
>>
>> I can do that if you want, but kvmppc_core_deliver_interrupts() should
>> always get called before we return to the guest. Dequeues that are
>> asynchronous to a guest exit should be very rare, and would be cured on
>> the first subsequent guest exit.
>
> Yes, but this means we have yet another subtile indirect assumption.
> The more we have those, the more people who work on the code need to
> know about the code to actually work on it. So the easier we can keep
> the scheme, the better IMHO.
>
> So yes, please replace clear_bit(pending, ...) with
> clear_bit_pending(...) or so which then would also do the required
> magic to set the shared page values.
Then we need to deal with races, if another exception is queued after we
check pending_exceptions but before we clear int_pending, whereas the
guest exit path will always be invoked if we queue an extirq or timer.
What if we rename it from deliver_interrupts() to prepare_to_enter(), to
make it more obvious?
We also will want to check for MSR[WE] before returning to the guest,
rather than immediately when MSR is set, and this would be a place to
put that.
-Scott
^ permalink raw reply [flat|nested] 6+ messages in thread