Re: [PATCH] KVM: arm/arm64: fix emulated ptimer irq injection
On Thu, Jun 13, 2019 at 11:01:41AM +0100, Marc Zyngier wrote:
> On Mon, 03 Jun 2019 13:14:40 +0100,
> Andrew Jones wrote:
> >
> > On Wed, May 29, 2019 at 12:03:11PM +0200, Christoffer Dall wrote:
> > > On Wed, May 29, 2019 at 10:13:21AM +0100, Marc Zyngier wrote:
> > > > On 29/05/2019 10:08, Christoffer Dall wrote:
> > > > > On Tue, May 28, 2019 at 05:08:53PM +0100, Marc Zyngier wrote:
> > > > >> On 28/05/2019 14:40, Andrew Jones wrote:
> > > > >>> On Tue, May 28, 2019 at 03:12:15PM +0200, Christoffer Dall wrote:
> > > > On Tue, May 28, 2019 at 01:25:52PM +0100, Marc Zyngier wrote:
> > > > > On 28/05/2019 12:01, Christoffer Dall wrote:
> > > > >> On Mon, May 27, 2019 at 01:46:19PM +0200, Andrew Jones wrote:
> > > > >>> The emulated ptimer needs to track the level changes, otherwise
> > > > >>> the
> > > > >>> the interrupt will never get deasserted, resulting in the guest
> > > > >>> getting
> > > > >>> stuck in an interrupt storm if it enables ptimer interrupts.
> > > > >>> This was
> > > > >>> found with kvm-unit-tests; the ptimer tests hung as soon as
> > > > >>> interrupts
> > > > >>> were enabled. Typical Linux guests don't have a problem as they
> > > > >>> prefer
> > > > >>> using the virtual timer.
> > > > >>>
> > > > >>> Fixes: bee038a674875 ("KVM: arm/arm64: Rework the timer code to
> > > > >>> use a timer_map")
> > > > >>> Signed-off-by: Andrew Jones
> > > > >>> ---
> > > > >>> virt/kvm/arm/arch_timer.c | 7 ++-
> > > > >>> 1 file changed, 6 insertions(+), 1 deletion(-)
> > > > >>>
> > > > >>> diff --git a/virt/kvm/arm/arch_timer.c
> > > > >>> b/virt/kvm/arm/arch_timer.c
> > > > >>> index 7fc272ecae16..9f5d8cc8b5e5 100644
> > > > >>> --- a/virt/kvm/arm/arch_timer.c
> > > > >>> +++ b/virt/kvm/arm/arch_timer.c
> > > > >>> @@ -324,10 +324,15 @@ static void kvm_timer_update_irq(struct
> > > > >>> kvm_vcpu *vcpu, bool new_level,
> > > > >>> static void timer_emulate(struct arch_timer_context *ctx)
> > > > >>> {
> > > > >>> bool should_fire = kvm_timer_should_fire(ctx);
> > > > >>> + struct timer_map map;
> > > > >>> +
> > > > >>> + get_timer_map(ctx->vcpu, );
> > > > >>>
> > > > >>> trace_kvm_timer_emulate(ctx, should_fire);
> > > > >>>
> > > > >>> - if (should_fire) {
> > > > >>> + if (ctx == map.emul_ptimer && should_fire !=
> > > > >>> ctx->irq.level) {
> > > > >>> + kvm_timer_update_irq(ctx->vcpu,
> > > > >>> !ctx->irq.level, ctx);
> > > > >>> + } else if (should_fire) {
> > > > >>> kvm_timer_update_irq(ctx->vcpu, true, ctx);
> > > > >>> return;
> > > > >>> }
> > > > >>
> > > > >> Hmm, this doesn't feel completely right.
> > > > >>>
> > > > >>> I won't try to argue that this is the right fix, as I haven't fully
> > > > >>> grasped how all this code works, but, afaict, this is how it worked
> > > > >>> prior to bee038a6.
> > > > >>>
> > > > >>
> > > > >> Lowering the line of an emulated timer should only ever happen
> > > > >> when the
> > > > >> guest (or user space) writes to one of the system registers for
> > > > >> that
> > > > >> timer, which should be trapped and that should cause an update
> > > > >> of the
> > > > >> line.
> > > > >>
> > > > >> Are we missing a call to kvm_timer_update_irq() from
> > > > >> kvm_arm_timer_set_reg() ?
> > > > >
> > > > > Which is exactly what we removed in 6bc210003dff, for good
> > > > > reasons.
> > > > >
> > > >
> > > > Ah well, I can be wrong twice. Or even three times.
> > > >
> > > > > Looking at kvm_arm_timer_write_sysreg(), we end-up calling
> > > > > kvm_timer_vcpu_load, but not updating the irq status.
> > > > >
> > > > > How about something like this instead (untested):
> > > > >
> > > > > diff --git a/virt/kvm/arm/arch_timer.c b/virt/kvm/arm/arch_timer.c
> > > > > index 7fc272ecae16..6a418dcc5433 100644
> > > > > --- a/virt/kvm/arm/arch_timer.c
> > > > > +++ b/virt/kvm/arm/arch_timer.c
> > > > > @@ -882,10 +882,14 @@ void kvm_arm_timer_write_sysreg(struct
> > > > > kvm_vcpu *vcpu,
> > > > > enum kvm_arch_timer_regs treg,
> > > > > u64 val)
> > > > > {
> > > > > + struct arch_timer_context *timer;
> > > > > +
> > > > > preempt_disable();
> > > > > kvm_timer_vcpu_put(vcpu);
> > > > >
> > > > > - kvm_arm_timer_write(vcpu, vcpu_get_timer(vcpu, tmr), treg, val);
> > > > > + timer = vcpu_get_timer(vcpu, tmr);
> > > > > + kvm_arm_timer_write(vcpu, timer, treg, val);
> > > > > + kvm_timer_update_irq(vcpu, kvm_timer_should_fire(timer), timer);
> > > > >
> > > > > kvm_timer_vcpu_load(vcpu);
> > > >
Re: [PATCH] KVM: arm/arm64: fix emulated ptimer irq injection
On Mon, 03 Jun 2019 13:14:40 +0100,
Andrew Jones wrote:
>
> On Wed, May 29, 2019 at 12:03:11PM +0200, Christoffer Dall wrote:
> > On Wed, May 29, 2019 at 10:13:21AM +0100, Marc Zyngier wrote:
> > > On 29/05/2019 10:08, Christoffer Dall wrote:
> > > > On Tue, May 28, 2019 at 05:08:53PM +0100, Marc Zyngier wrote:
> > > >> On 28/05/2019 14:40, Andrew Jones wrote:
> > > >>> On Tue, May 28, 2019 at 03:12:15PM +0200, Christoffer Dall wrote:
> > > On Tue, May 28, 2019 at 01:25:52PM +0100, Marc Zyngier wrote:
> > > > On 28/05/2019 12:01, Christoffer Dall wrote:
> > > >> On Mon, May 27, 2019 at 01:46:19PM +0200, Andrew Jones wrote:
> > > >>> The emulated ptimer needs to track the level changes, otherwise
> > > >>> the
> > > >>> the interrupt will never get deasserted, resulting in the guest
> > > >>> getting
> > > >>> stuck in an interrupt storm if it enables ptimer interrupts. This
> > > >>> was
> > > >>> found with kvm-unit-tests; the ptimer tests hung as soon as
> > > >>> interrupts
> > > >>> were enabled. Typical Linux guests don't have a problem as they
> > > >>> prefer
> > > >>> using the virtual timer.
> > > >>>
> > > >>> Fixes: bee038a674875 ("KVM: arm/arm64: Rework the timer code to
> > > >>> use a timer_map")
> > > >>> Signed-off-by: Andrew Jones
> > > >>> ---
> > > >>> virt/kvm/arm/arch_timer.c | 7 ++-
> > > >>> 1 file changed, 6 insertions(+), 1 deletion(-)
> > > >>>
> > > >>> diff --git a/virt/kvm/arm/arch_timer.c b/virt/kvm/arm/arch_timer.c
> > > >>> index 7fc272ecae16..9f5d8cc8b5e5 100644
> > > >>> --- a/virt/kvm/arm/arch_timer.c
> > > >>> +++ b/virt/kvm/arm/arch_timer.c
> > > >>> @@ -324,10 +324,15 @@ static void kvm_timer_update_irq(struct
> > > >>> kvm_vcpu *vcpu, bool new_level,
> > > >>> static void timer_emulate(struct arch_timer_context *ctx)
> > > >>> {
> > > >>> bool should_fire = kvm_timer_should_fire(ctx);
> > > >>> + struct timer_map map;
> > > >>> +
> > > >>> + get_timer_map(ctx->vcpu, );
> > > >>>
> > > >>> trace_kvm_timer_emulate(ctx, should_fire);
> > > >>>
> > > >>> - if (should_fire) {
> > > >>> + if (ctx == map.emul_ptimer && should_fire != ctx->irq.level) {
> > > >>> + kvm_timer_update_irq(ctx->vcpu, !ctx->irq.level, ctx);
> > > >>> + } else if (should_fire) {
> > > >>> kvm_timer_update_irq(ctx->vcpu, true, ctx);
> > > >>> return;
> > > >>> }
> > > >>
> > > >> Hmm, this doesn't feel completely right.
> > > >>>
> > > >>> I won't try to argue that this is the right fix, as I haven't fully
> > > >>> grasped how all this code works, but, afaict, this is how it worked
> > > >>> prior to bee038a6.
> > > >>>
> > > >>
> > > >> Lowering the line of an emulated timer should only ever happen
> > > >> when the
> > > >> guest (or user space) writes to one of the system registers for
> > > >> that
> > > >> timer, which should be trapped and that should cause an update of
> > > >> the
> > > >> line.
> > > >>
> > > >> Are we missing a call to kvm_timer_update_irq() from
> > > >> kvm_arm_timer_set_reg() ?
> > > >
> > > > Which is exactly what we removed in 6bc210003dff, for good reasons.
> > > >
> > >
> > > Ah well, I can be wrong twice. Or even three times.
> > >
> > > > Looking at kvm_arm_timer_write_sysreg(), we end-up calling
> > > > kvm_timer_vcpu_load, but not updating the irq status.
> > > >
> > > > How about something like this instead (untested):
> > > >
> > > > diff --git a/virt/kvm/arm/arch_timer.c b/virt/kvm/arm/arch_timer.c
> > > > index 7fc272ecae16..6a418dcc5433 100644
> > > > --- a/virt/kvm/arm/arch_timer.c
> > > > +++ b/virt/kvm/arm/arch_timer.c
> > > > @@ -882,10 +882,14 @@ void kvm_arm_timer_write_sysreg(struct
> > > > kvm_vcpu *vcpu,
> > > > enum kvm_arch_timer_regs treg,
> > > > u64 val)
> > > > {
> > > > + struct arch_timer_context *timer;
> > > > +
> > > > preempt_disable();
> > > > kvm_timer_vcpu_put(vcpu);
> > > >
> > > > - kvm_arm_timer_write(vcpu, vcpu_get_timer(vcpu, tmr), treg, val);
> > > > + timer = vcpu_get_timer(vcpu, tmr);
> > > > + kvm_arm_timer_write(vcpu, timer, treg, val);
> > > > + kvm_timer_update_irq(vcpu, kvm_timer_should_fire(timer), timer);
> > > >
> > > > kvm_timer_vcpu_load(vcpu);
> > > > preempt_enable();
> > > >
> > > >>>
> > > >>> Marc, I've tested this and it resolves the issue for me. If/when you
> > > >>> post
> > > >>> it you can add a t-b from me if you like.
> > > >>>
> > >
> > > Yes, that looks reasonable. Basically, in 6bc210003dff we should
> > > have
> > > only removed the call to timer_emulate, and not messed
Re: [PATCH] KVM: arm/arm64: fix emulated ptimer irq injection
On Wed, May 29, 2019 at 12:03:11PM +0200, Christoffer Dall wrote:
> On Wed, May 29, 2019 at 10:13:21AM +0100, Marc Zyngier wrote:
> > On 29/05/2019 10:08, Christoffer Dall wrote:
> > > On Tue, May 28, 2019 at 05:08:53PM +0100, Marc Zyngier wrote:
> > >> On 28/05/2019 14:40, Andrew Jones wrote:
> > >>> On Tue, May 28, 2019 at 03:12:15PM +0200, Christoffer Dall wrote:
> > On Tue, May 28, 2019 at 01:25:52PM +0100, Marc Zyngier wrote:
> > > On 28/05/2019 12:01, Christoffer Dall wrote:
> > >> On Mon, May 27, 2019 at 01:46:19PM +0200, Andrew Jones wrote:
> > >>> The emulated ptimer needs to track the level changes, otherwise the
> > >>> the interrupt will never get deasserted, resulting in the guest
> > >>> getting
> > >>> stuck in an interrupt storm if it enables ptimer interrupts. This
> > >>> was
> > >>> found with kvm-unit-tests; the ptimer tests hung as soon as
> > >>> interrupts
> > >>> were enabled. Typical Linux guests don't have a problem as they
> > >>> prefer
> > >>> using the virtual timer.
> > >>>
> > >>> Fixes: bee038a674875 ("KVM: arm/arm64: Rework the timer code to use
> > >>> a timer_map")
> > >>> Signed-off-by: Andrew Jones
> > >>> ---
> > >>> virt/kvm/arm/arch_timer.c | 7 ++-
> > >>> 1 file changed, 6 insertions(+), 1 deletion(-)
> > >>>
> > >>> diff --git a/virt/kvm/arm/arch_timer.c b/virt/kvm/arm/arch_timer.c
> > >>> index 7fc272ecae16..9f5d8cc8b5e5 100644
> > >>> --- a/virt/kvm/arm/arch_timer.c
> > >>> +++ b/virt/kvm/arm/arch_timer.c
> > >>> @@ -324,10 +324,15 @@ static void kvm_timer_update_irq(struct
> > >>> kvm_vcpu *vcpu, bool new_level,
> > >>> static void timer_emulate(struct arch_timer_context *ctx)
> > >>> {
> > >>> bool should_fire = kvm_timer_should_fire(ctx);
> > >>> + struct timer_map map;
> > >>> +
> > >>> + get_timer_map(ctx->vcpu, );
> > >>>
> > >>> trace_kvm_timer_emulate(ctx, should_fire);
> > >>>
> > >>> - if (should_fire) {
> > >>> + if (ctx == map.emul_ptimer && should_fire != ctx->irq.level) {
> > >>> + kvm_timer_update_irq(ctx->vcpu, !ctx->irq.level, ctx);
> > >>> + } else if (should_fire) {
> > >>> kvm_timer_update_irq(ctx->vcpu, true, ctx);
> > >>> return;
> > >>> }
> > >>
> > >> Hmm, this doesn't feel completely right.
> > >>>
> > >>> I won't try to argue that this is the right fix, as I haven't fully
> > >>> grasped how all this code works, but, afaict, this is how it worked
> > >>> prior to bee038a6.
> > >>>
> > >>
> > >> Lowering the line of an emulated timer should only ever happen when
> > >> the
> > >> guest (or user space) writes to one of the system registers for that
> > >> timer, which should be trapped and that should cause an update of the
> > >> line.
> > >>
> > >> Are we missing a call to kvm_timer_update_irq() from
> > >> kvm_arm_timer_set_reg() ?
> > >
> > > Which is exactly what we removed in 6bc210003dff, for good reasons.
> > >
> >
> > Ah well, I can be wrong twice. Or even three times.
> >
> > > Looking at kvm_arm_timer_write_sysreg(), we end-up calling
> > > kvm_timer_vcpu_load, but not updating the irq status.
> > >
> > > How about something like this instead (untested):
> > >
> > > diff --git a/virt/kvm/arm/arch_timer.c b/virt/kvm/arm/arch_timer.c
> > > index 7fc272ecae16..6a418dcc5433 100644
> > > --- a/virt/kvm/arm/arch_timer.c
> > > +++ b/virt/kvm/arm/arch_timer.c
> > > @@ -882,10 +882,14 @@ void kvm_arm_timer_write_sysreg(struct kvm_vcpu
> > > *vcpu,
> > > enum kvm_arch_timer_regs treg,
> > > u64 val)
> > > {
> > > + struct arch_timer_context *timer;
> > > +
> > > preempt_disable();
> > > kvm_timer_vcpu_put(vcpu);
> > >
> > > - kvm_arm_timer_write(vcpu, vcpu_get_timer(vcpu, tmr), treg, val);
> > > + timer = vcpu_get_timer(vcpu, tmr);
> > > + kvm_arm_timer_write(vcpu, timer, treg, val);
> > > + kvm_timer_update_irq(vcpu, kvm_timer_should_fire(timer), timer);
> > >
> > > kvm_timer_vcpu_load(vcpu);
> > > preempt_enable();
> > >
> > >>>
> > >>> Marc, I've tested this and it resolves the issue for me. If/when you
> > >>> post
> > >>> it you can add a t-b from me if you like.
> > >>>
> >
> > Yes, that looks reasonable. Basically, in 6bc210003dff we should have
> > only removed the call to timer_emulate, and not messed around with
> > kvm_timer_update_irq()?
> >
> > After this patch, we'll have moved the call to kvm_timer_update_irq()
> > from kvm_arm_timer_set_reg() to kvm_arm_timer_write_sysreg(). I can't
> > seem to decide if clearly belongs in one place or the other.
>
Re: [PATCH] KVM: arm/arm64: fix emulated ptimer irq injection
On Wed, May 29, 2019 at 10:13:21AM +0100, Marc Zyngier wrote:
> On 29/05/2019 10:08, Christoffer Dall wrote:
> > On Tue, May 28, 2019 at 05:08:53PM +0100, Marc Zyngier wrote:
> >> On 28/05/2019 14:40, Andrew Jones wrote:
> >>> On Tue, May 28, 2019 at 03:12:15PM +0200, Christoffer Dall wrote:
> On Tue, May 28, 2019 at 01:25:52PM +0100, Marc Zyngier wrote:
> > On 28/05/2019 12:01, Christoffer Dall wrote:
> >> On Mon, May 27, 2019 at 01:46:19PM +0200, Andrew Jones wrote:
> >>> The emulated ptimer needs to track the level changes, otherwise the
> >>> the interrupt will never get deasserted, resulting in the guest
> >>> getting
> >>> stuck in an interrupt storm if it enables ptimer interrupts. This was
> >>> found with kvm-unit-tests; the ptimer tests hung as soon as interrupts
> >>> were enabled. Typical Linux guests don't have a problem as they prefer
> >>> using the virtual timer.
> >>>
> >>> Fixes: bee038a674875 ("KVM: arm/arm64: Rework the timer code to use a
> >>> timer_map")
> >>> Signed-off-by: Andrew Jones
> >>> ---
> >>> virt/kvm/arm/arch_timer.c | 7 ++-
> >>> 1 file changed, 6 insertions(+), 1 deletion(-)
> >>>
> >>> diff --git a/virt/kvm/arm/arch_timer.c b/virt/kvm/arm/arch_timer.c
> >>> index 7fc272ecae16..9f5d8cc8b5e5 100644
> >>> --- a/virt/kvm/arm/arch_timer.c
> >>> +++ b/virt/kvm/arm/arch_timer.c
> >>> @@ -324,10 +324,15 @@ static void kvm_timer_update_irq(struct
> >>> kvm_vcpu *vcpu, bool new_level,
> >>> static void timer_emulate(struct arch_timer_context *ctx)
> >>> {
> >>> bool should_fire = kvm_timer_should_fire(ctx);
> >>> + struct timer_map map;
> >>> +
> >>> + get_timer_map(ctx->vcpu, );
> >>>
> >>> trace_kvm_timer_emulate(ctx, should_fire);
> >>>
> >>> - if (should_fire) {
> >>> + if (ctx == map.emul_ptimer && should_fire != ctx->irq.level) {
> >>> + kvm_timer_update_irq(ctx->vcpu, !ctx->irq.level, ctx);
> >>> + } else if (should_fire) {
> >>> kvm_timer_update_irq(ctx->vcpu, true, ctx);
> >>> return;
> >>> }
> >>
> >> Hmm, this doesn't feel completely right.
> >>>
> >>> I won't try to argue that this is the right fix, as I haven't fully
> >>> grasped how all this code works, but, afaict, this is how it worked
> >>> prior to bee038a6.
> >>>
> >>
> >> Lowering the line of an emulated timer should only ever happen when the
> >> guest (or user space) writes to one of the system registers for that
> >> timer, which should be trapped and that should cause an update of the
> >> line.
> >>
> >> Are we missing a call to kvm_timer_update_irq() from
> >> kvm_arm_timer_set_reg() ?
> >
> > Which is exactly what we removed in 6bc210003dff, for good reasons.
> >
>
> Ah well, I can be wrong twice. Or even three times.
>
> > Looking at kvm_arm_timer_write_sysreg(), we end-up calling
> > kvm_timer_vcpu_load, but not updating the irq status.
> >
> > How about something like this instead (untested):
> >
> > diff --git a/virt/kvm/arm/arch_timer.c b/virt/kvm/arm/arch_timer.c
> > index 7fc272ecae16..6a418dcc5433 100644
> > --- a/virt/kvm/arm/arch_timer.c
> > +++ b/virt/kvm/arm/arch_timer.c
> > @@ -882,10 +882,14 @@ void kvm_arm_timer_write_sysreg(struct kvm_vcpu
> > *vcpu,
> > enum kvm_arch_timer_regs treg,
> > u64 val)
> > {
> > + struct arch_timer_context *timer;
> > +
> > preempt_disable();
> > kvm_timer_vcpu_put(vcpu);
> >
> > - kvm_arm_timer_write(vcpu, vcpu_get_timer(vcpu, tmr), treg, val);
> > + timer = vcpu_get_timer(vcpu, tmr);
> > + kvm_arm_timer_write(vcpu, timer, treg, val);
> > + kvm_timer_update_irq(vcpu, kvm_timer_should_fire(timer), timer);
> >
> > kvm_timer_vcpu_load(vcpu);
> > preempt_enable();
> >
> >>>
> >>> Marc, I've tested this and it resolves the issue for me. If/when you post
> >>> it you can add a t-b from me if you like.
> >>>
>
> Yes, that looks reasonable. Basically, in 6bc210003dff we should have
> only removed the call to timer_emulate, and not messed around with
> kvm_timer_update_irq()?
>
> After this patch, we'll have moved the call to kvm_timer_update_irq()
> from kvm_arm_timer_set_reg() to kvm_arm_timer_write_sysreg(). I can't
> seem to decide if clearly belongs in one place or the other.
>
> >>>
> >>> Isn't kvm_arm_timer_set_reg() only for userspace setting of the register?
> >>> In this test case I don't think userspace is involved at that point.
> >>
> >> It still remains that userspace writing to any of the registers has an
> >> effect on the interrupt line. Or rather
Re: [PATCH] KVM: arm/arm64: fix emulated ptimer irq injection
On 29/05/2019 10:08, Christoffer Dall wrote:
> On Tue, May 28, 2019 at 05:08:53PM +0100, Marc Zyngier wrote:
>> On 28/05/2019 14:40, Andrew Jones wrote:
>>> On Tue, May 28, 2019 at 03:12:15PM +0200, Christoffer Dall wrote:
On Tue, May 28, 2019 at 01:25:52PM +0100, Marc Zyngier wrote:
> On 28/05/2019 12:01, Christoffer Dall wrote:
>> On Mon, May 27, 2019 at 01:46:19PM +0200, Andrew Jones wrote:
>>> The emulated ptimer needs to track the level changes, otherwise the
>>> the interrupt will never get deasserted, resulting in the guest getting
>>> stuck in an interrupt storm if it enables ptimer interrupts. This was
>>> found with kvm-unit-tests; the ptimer tests hung as soon as interrupts
>>> were enabled. Typical Linux guests don't have a problem as they prefer
>>> using the virtual timer.
>>>
>>> Fixes: bee038a674875 ("KVM: arm/arm64: Rework the timer code to use a
>>> timer_map")
>>> Signed-off-by: Andrew Jones
>>> ---
>>> virt/kvm/arm/arch_timer.c | 7 ++-
>>> 1 file changed, 6 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/virt/kvm/arm/arch_timer.c b/virt/kvm/arm/arch_timer.c
>>> index 7fc272ecae16..9f5d8cc8b5e5 100644
>>> --- a/virt/kvm/arm/arch_timer.c
>>> +++ b/virt/kvm/arm/arch_timer.c
>>> @@ -324,10 +324,15 @@ static void kvm_timer_update_irq(struct kvm_vcpu
>>> *vcpu, bool new_level,
>>> static void timer_emulate(struct arch_timer_context *ctx)
>>> {
>>> bool should_fire = kvm_timer_should_fire(ctx);
>>> + struct timer_map map;
>>> +
>>> + get_timer_map(ctx->vcpu, );
>>>
>>> trace_kvm_timer_emulate(ctx, should_fire);
>>>
>>> - if (should_fire) {
>>> + if (ctx == map.emul_ptimer && should_fire != ctx->irq.level) {
>>> + kvm_timer_update_irq(ctx->vcpu, !ctx->irq.level, ctx);
>>> + } else if (should_fire) {
>>> kvm_timer_update_irq(ctx->vcpu, true, ctx);
>>> return;
>>> }
>>
>> Hmm, this doesn't feel completely right.
>>>
>>> I won't try to argue that this is the right fix, as I haven't fully
>>> grasped how all this code works, but, afaict, this is how it worked
>>> prior to bee038a6.
>>>
>>
>> Lowering the line of an emulated timer should only ever happen when the
>> guest (or user space) writes to one of the system registers for that
>> timer, which should be trapped and that should cause an update of the
>> line.
>>
>> Are we missing a call to kvm_timer_update_irq() from
>> kvm_arm_timer_set_reg() ?
>
> Which is exactly what we removed in 6bc210003dff, for good reasons.
>
Ah well, I can be wrong twice. Or even three times.
> Looking at kvm_arm_timer_write_sysreg(), we end-up calling
> kvm_timer_vcpu_load, but not updating the irq status.
>
> How about something like this instead (untested):
>
> diff --git a/virt/kvm/arm/arch_timer.c b/virt/kvm/arm/arch_timer.c
> index 7fc272ecae16..6a418dcc5433 100644
> --- a/virt/kvm/arm/arch_timer.c
> +++ b/virt/kvm/arm/arch_timer.c
> @@ -882,10 +882,14 @@ void kvm_arm_timer_write_sysreg(struct kvm_vcpu
> *vcpu,
> enum kvm_arch_timer_regs treg,
> u64 val)
> {
> + struct arch_timer_context *timer;
> +
> preempt_disable();
> kvm_timer_vcpu_put(vcpu);
>
> - kvm_arm_timer_write(vcpu, vcpu_get_timer(vcpu, tmr), treg, val);
> + timer = vcpu_get_timer(vcpu, tmr);
> + kvm_arm_timer_write(vcpu, timer, treg, val);
> + kvm_timer_update_irq(vcpu, kvm_timer_should_fire(timer), timer);
>
> kvm_timer_vcpu_load(vcpu);
> preempt_enable();
>
>>>
>>> Marc, I've tested this and it resolves the issue for me. If/when you post
>>> it you can add a t-b from me if you like.
>>>
Yes, that looks reasonable. Basically, in 6bc210003dff we should have
only removed the call to timer_emulate, and not messed around with
kvm_timer_update_irq()?
After this patch, we'll have moved the call to kvm_timer_update_irq()
from kvm_arm_timer_set_reg() to kvm_arm_timer_write_sysreg(). I can't
seem to decide if clearly belongs in one place or the other.
>>>
>>> Isn't kvm_arm_timer_set_reg() only for userspace setting of the register?
>>> In this test case I don't think userspace is involved at that point.
>>
>> It still remains that userspace writing to any of the registers has an
>> effect on the interrupt line. Or rather that it should.
>>
>> And the more I look at this, the more I have the feeling this thing
>> should happen on kvm_timer_vcpu_load(), wherever the writes comes from.
>> It'd have slightly more overhead than doing it from every register
>> access path, but at least it'd be clearer... Untested, again.
>>
>> diff --git
Re: [PATCH] KVM: arm/arm64: fix emulated ptimer irq injection
On Tue, May 28, 2019 at 05:08:53PM +0100, Marc Zyngier wrote:
> On 28/05/2019 14:40, Andrew Jones wrote:
> > On Tue, May 28, 2019 at 03:12:15PM +0200, Christoffer Dall wrote:
> >> On Tue, May 28, 2019 at 01:25:52PM +0100, Marc Zyngier wrote:
> >>> On 28/05/2019 12:01, Christoffer Dall wrote:
> On Mon, May 27, 2019 at 01:46:19PM +0200, Andrew Jones wrote:
> > The emulated ptimer needs to track the level changes, otherwise the
> > the interrupt will never get deasserted, resulting in the guest getting
> > stuck in an interrupt storm if it enables ptimer interrupts. This was
> > found with kvm-unit-tests; the ptimer tests hung as soon as interrupts
> > were enabled. Typical Linux guests don't have a problem as they prefer
> > using the virtual timer.
> >
> > Fixes: bee038a674875 ("KVM: arm/arm64: Rework the timer code to use a
> > timer_map")
> > Signed-off-by: Andrew Jones
> > ---
> > virt/kvm/arm/arch_timer.c | 7 ++-
> > 1 file changed, 6 insertions(+), 1 deletion(-)
> >
> > diff --git a/virt/kvm/arm/arch_timer.c b/virt/kvm/arm/arch_timer.c
> > index 7fc272ecae16..9f5d8cc8b5e5 100644
> > --- a/virt/kvm/arm/arch_timer.c
> > +++ b/virt/kvm/arm/arch_timer.c
> > @@ -324,10 +324,15 @@ static void kvm_timer_update_irq(struct kvm_vcpu
> > *vcpu, bool new_level,
> > static void timer_emulate(struct arch_timer_context *ctx)
> > {
> > bool should_fire = kvm_timer_should_fire(ctx);
> > + struct timer_map map;
> > +
> > + get_timer_map(ctx->vcpu, );
> >
> > trace_kvm_timer_emulate(ctx, should_fire);
> >
> > - if (should_fire) {
> > + if (ctx == map.emul_ptimer && should_fire != ctx->irq.level) {
> > + kvm_timer_update_irq(ctx->vcpu, !ctx->irq.level, ctx);
> > + } else if (should_fire) {
> > kvm_timer_update_irq(ctx->vcpu, true, ctx);
> > return;
> > }
>
> Hmm, this doesn't feel completely right.
> >
> > I won't try to argue that this is the right fix, as I haven't fully
> > grasped how all this code works, but, afaict, this is how it worked
> > prior to bee038a6.
> >
>
> Lowering the line of an emulated timer should only ever happen when the
> guest (or user space) writes to one of the system registers for that
> timer, which should be trapped and that should cause an update of the
> line.
>
> Are we missing a call to kvm_timer_update_irq() from
> kvm_arm_timer_set_reg() ?
> >>>
> >>> Which is exactly what we removed in 6bc210003dff, for good reasons.
> >>>
> >>
> >> Ah well, I can be wrong twice. Or even three times.
> >>
> >>> Looking at kvm_arm_timer_write_sysreg(), we end-up calling
> >>> kvm_timer_vcpu_load, but not updating the irq status.
> >>>
> >>> How about something like this instead (untested):
> >>>
> >>> diff --git a/virt/kvm/arm/arch_timer.c b/virt/kvm/arm/arch_timer.c
> >>> index 7fc272ecae16..6a418dcc5433 100644
> >>> --- a/virt/kvm/arm/arch_timer.c
> >>> +++ b/virt/kvm/arm/arch_timer.c
> >>> @@ -882,10 +882,14 @@ void kvm_arm_timer_write_sysreg(struct kvm_vcpu
> >>> *vcpu,
> >>> enum kvm_arch_timer_regs treg,
> >>> u64 val)
> >>> {
> >>> + struct arch_timer_context *timer;
> >>> +
> >>> preempt_disable();
> >>> kvm_timer_vcpu_put(vcpu);
> >>>
> >>> - kvm_arm_timer_write(vcpu, vcpu_get_timer(vcpu, tmr), treg, val);
> >>> + timer = vcpu_get_timer(vcpu, tmr);
> >>> + kvm_arm_timer_write(vcpu, timer, treg, val);
> >>> + kvm_timer_update_irq(vcpu, kvm_timer_should_fire(timer), timer);
> >>>
> >>> kvm_timer_vcpu_load(vcpu);
> >>> preempt_enable();
> >>>
> >
> > Marc, I've tested this and it resolves the issue for me. If/when you post
> > it you can add a t-b from me if you like.
> >
> >>
> >> Yes, that looks reasonable. Basically, in 6bc210003dff we should have
> >> only removed the call to timer_emulate, and not messed around with
> >> kvm_timer_update_irq()?
> >>
> >> After this patch, we'll have moved the call to kvm_timer_update_irq()
> >> from kvm_arm_timer_set_reg() to kvm_arm_timer_write_sysreg(). I can't
> >> seem to decide if clearly belongs in one place or the other.
> >>
> >
> > Isn't kvm_arm_timer_set_reg() only for userspace setting of the register?
> > In this test case I don't think userspace is involved at that point.
>
> It still remains that userspace writing to any of the registers has an
> effect on the interrupt line. Or rather that it should.
>
> And the more I look at this, the more I have the feeling this thing
> should happen on kvm_timer_vcpu_load(), wherever the writes comes from.
> It'd have slightly more overhead than doing it from every register
> access path, but at least it'd be clearer... Untested, again.
>
> diff --git a/virt/kvm/arm/arch_timer.c b/virt/kvm/arm/arch_timer.c
> index
Re: [PATCH] KVM: arm/arm64: fix emulated ptimer irq injection
On Tue, May 28, 2019 at 05:08:53PM +0100, Marc Zyngier wrote:
> On 28/05/2019 14:40, Andrew Jones wrote:
> > On Tue, May 28, 2019 at 03:12:15PM +0200, Christoffer Dall wrote:
> >> On Tue, May 28, 2019 at 01:25:52PM +0100, Marc Zyngier wrote:
> >>> On 28/05/2019 12:01, Christoffer Dall wrote:
> On Mon, May 27, 2019 at 01:46:19PM +0200, Andrew Jones wrote:
> > The emulated ptimer needs to track the level changes, otherwise the
> > the interrupt will never get deasserted, resulting in the guest getting
> > stuck in an interrupt storm if it enables ptimer interrupts. This was
> > found with kvm-unit-tests; the ptimer tests hung as soon as interrupts
> > were enabled. Typical Linux guests don't have a problem as they prefer
> > using the virtual timer.
> >
> > Fixes: bee038a674875 ("KVM: arm/arm64: Rework the timer code to use a
> > timer_map")
> > Signed-off-by: Andrew Jones
> > ---
> > virt/kvm/arm/arch_timer.c | 7 ++-
> > 1 file changed, 6 insertions(+), 1 deletion(-)
> >
> > diff --git a/virt/kvm/arm/arch_timer.c b/virt/kvm/arm/arch_timer.c
> > index 7fc272ecae16..9f5d8cc8b5e5 100644
> > --- a/virt/kvm/arm/arch_timer.c
> > +++ b/virt/kvm/arm/arch_timer.c
> > @@ -324,10 +324,15 @@ static void kvm_timer_update_irq(struct kvm_vcpu
> > *vcpu, bool new_level,
> > static void timer_emulate(struct arch_timer_context *ctx)
> > {
> > bool should_fire = kvm_timer_should_fire(ctx);
> > + struct timer_map map;
> > +
> > + get_timer_map(ctx->vcpu, );
> >
> > trace_kvm_timer_emulate(ctx, should_fire);
> >
> > - if (should_fire) {
> > + if (ctx == map.emul_ptimer && should_fire != ctx->irq.level) {
> > + kvm_timer_update_irq(ctx->vcpu, !ctx->irq.level, ctx);
> > + } else if (should_fire) {
> > kvm_timer_update_irq(ctx->vcpu, true, ctx);
> > return;
> > }
>
> Hmm, this doesn't feel completely right.
> >
> > I won't try to argue that this is the right fix, as I haven't fully
> > grasped how all this code works, but, afaict, this is how it worked
> > prior to bee038a6.
> >
>
> Lowering the line of an emulated timer should only ever happen when the
> guest (or user space) writes to one of the system registers for that
> timer, which should be trapped and that should cause an update of the
> line.
>
> Are we missing a call to kvm_timer_update_irq() from
> kvm_arm_timer_set_reg() ?
> >>>
> >>> Which is exactly what we removed in 6bc210003dff, for good reasons.
> >>>
> >>
> >> Ah well, I can be wrong twice. Or even three times.
> >>
> >>> Looking at kvm_arm_timer_write_sysreg(), we end-up calling
> >>> kvm_timer_vcpu_load, but not updating the irq status.
> >>>
> >>> How about something like this instead (untested):
> >>>
> >>> diff --git a/virt/kvm/arm/arch_timer.c b/virt/kvm/arm/arch_timer.c
> >>> index 7fc272ecae16..6a418dcc5433 100644
> >>> --- a/virt/kvm/arm/arch_timer.c
> >>> +++ b/virt/kvm/arm/arch_timer.c
> >>> @@ -882,10 +882,14 @@ void kvm_arm_timer_write_sysreg(struct kvm_vcpu
> >>> *vcpu,
> >>> enum kvm_arch_timer_regs treg,
> >>> u64 val)
> >>> {
> >>> + struct arch_timer_context *timer;
> >>> +
> >>> preempt_disable();
> >>> kvm_timer_vcpu_put(vcpu);
> >>>
> >>> - kvm_arm_timer_write(vcpu, vcpu_get_timer(vcpu, tmr), treg, val);
> >>> + timer = vcpu_get_timer(vcpu, tmr);
> >>> + kvm_arm_timer_write(vcpu, timer, treg, val);
> >>> + kvm_timer_update_irq(vcpu, kvm_timer_should_fire(timer), timer);
> >>>
> >>> kvm_timer_vcpu_load(vcpu);
> >>> preempt_enable();
> >>>
> >
> > Marc, I've tested this and it resolves the issue for me. If/when you post
> > it you can add a t-b from me if you like.
> >
> >>
> >> Yes, that looks reasonable. Basically, in 6bc210003dff we should have
> >> only removed the call to timer_emulate, and not messed around with
> >> kvm_timer_update_irq()?
> >>
> >> After this patch, we'll have moved the call to kvm_timer_update_irq()
> >> from kvm_arm_timer_set_reg() to kvm_arm_timer_write_sysreg(). I can't
> >> seem to decide if clearly belongs in one place or the other.
> >>
> >
> > Isn't kvm_arm_timer_set_reg() only for userspace setting of the register?
> > In this test case I don't think userspace is involved at that point.
>
> It still remains that userspace writing to any of the registers has an
> effect on the interrupt line. Or rather that it should.
>
> And the more I look at this, the more I have the feeling this thing
> should happen on kvm_timer_vcpu_load(), wherever the writes comes from.
> It'd have slightly more overhead than doing it from every register
> access path, but at least it'd be clearer... Untested, again.
>
> diff --git a/virt/kvm/arm/arch_timer.c b/virt/kvm/arm/arch_timer.c
> index
Re: [PATCH] KVM: arm/arm64: fix emulated ptimer irq injection
On 28/05/2019 14:40, Andrew Jones wrote:
> On Tue, May 28, 2019 at 03:12:15PM +0200, Christoffer Dall wrote:
>> On Tue, May 28, 2019 at 01:25:52PM +0100, Marc Zyngier wrote:
>>> On 28/05/2019 12:01, Christoffer Dall wrote:
On Mon, May 27, 2019 at 01:46:19PM +0200, Andrew Jones wrote:
> The emulated ptimer needs to track the level changes, otherwise the
> the interrupt will never get deasserted, resulting in the guest getting
> stuck in an interrupt storm if it enables ptimer interrupts. This was
> found with kvm-unit-tests; the ptimer tests hung as soon as interrupts
> were enabled. Typical Linux guests don't have a problem as they prefer
> using the virtual timer.
>
> Fixes: bee038a674875 ("KVM: arm/arm64: Rework the timer code to use a
> timer_map")
> Signed-off-by: Andrew Jones
> ---
> virt/kvm/arm/arch_timer.c | 7 ++-
> 1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/virt/kvm/arm/arch_timer.c b/virt/kvm/arm/arch_timer.c
> index 7fc272ecae16..9f5d8cc8b5e5 100644
> --- a/virt/kvm/arm/arch_timer.c
> +++ b/virt/kvm/arm/arch_timer.c
> @@ -324,10 +324,15 @@ static void kvm_timer_update_irq(struct kvm_vcpu
> *vcpu, bool new_level,
> static void timer_emulate(struct arch_timer_context *ctx)
> {
> bool should_fire = kvm_timer_should_fire(ctx);
> + struct timer_map map;
> +
> + get_timer_map(ctx->vcpu, );
>
> trace_kvm_timer_emulate(ctx, should_fire);
>
> - if (should_fire) {
> + if (ctx == map.emul_ptimer && should_fire != ctx->irq.level) {
> + kvm_timer_update_irq(ctx->vcpu, !ctx->irq.level, ctx);
> + } else if (should_fire) {
> kvm_timer_update_irq(ctx->vcpu, true, ctx);
> return;
> }
Hmm, this doesn't feel completely right.
>
> I won't try to argue that this is the right fix, as I haven't fully
> grasped how all this code works, but, afaict, this is how it worked
> prior to bee038a6.
>
Lowering the line of an emulated timer should only ever happen when the
guest (or user space) writes to one of the system registers for that
timer, which should be trapped and that should cause an update of the
line.
Are we missing a call to kvm_timer_update_irq() from
kvm_arm_timer_set_reg() ?
>>>
>>> Which is exactly what we removed in 6bc210003dff, for good reasons.
>>>
>>
>> Ah well, I can be wrong twice. Or even three times.
>>
>>> Looking at kvm_arm_timer_write_sysreg(), we end-up calling
>>> kvm_timer_vcpu_load, but not updating the irq status.
>>>
>>> How about something like this instead (untested):
>>>
>>> diff --git a/virt/kvm/arm/arch_timer.c b/virt/kvm/arm/arch_timer.c
>>> index 7fc272ecae16..6a418dcc5433 100644
>>> --- a/virt/kvm/arm/arch_timer.c
>>> +++ b/virt/kvm/arm/arch_timer.c
>>> @@ -882,10 +882,14 @@ void kvm_arm_timer_write_sysreg(struct kvm_vcpu *vcpu,
>>> enum kvm_arch_timer_regs treg,
>>> u64 val)
>>> {
>>> + struct arch_timer_context *timer;
>>> +
>>> preempt_disable();
>>> kvm_timer_vcpu_put(vcpu);
>>>
>>> - kvm_arm_timer_write(vcpu, vcpu_get_timer(vcpu, tmr), treg, val);
>>> + timer = vcpu_get_timer(vcpu, tmr);
>>> + kvm_arm_timer_write(vcpu, timer, treg, val);
>>> + kvm_timer_update_irq(vcpu, kvm_timer_should_fire(timer), timer);
>>>
>>> kvm_timer_vcpu_load(vcpu);
>>> preempt_enable();
>>>
>
> Marc, I've tested this and it resolves the issue for me. If/when you post
> it you can add a t-b from me if you like.
>
>>
>> Yes, that looks reasonable. Basically, in 6bc210003dff we should have
>> only removed the call to timer_emulate, and not messed around with
>> kvm_timer_update_irq()?
>>
>> After this patch, we'll have moved the call to kvm_timer_update_irq()
>> from kvm_arm_timer_set_reg() to kvm_arm_timer_write_sysreg(). I can't
>> seem to decide if clearly belongs in one place or the other.
>>
>
> Isn't kvm_arm_timer_set_reg() only for userspace setting of the register?
> In this test case I don't think userspace is involved at that point.
It still remains that userspace writing to any of the registers has an
effect on the interrupt line. Or rather that it should.
And the more I look at this, the more I have the feeling this thing
should happen on kvm_timer_vcpu_load(), wherever the writes comes from.
It'd have slightly more overhead than doing it from every register
access path, but at least it'd be clearer... Untested, again.
diff --git a/virt/kvm/arm/arch_timer.c b/virt/kvm/arm/arch_timer.c
index 7fc272ecae16..8244e40af196 100644
--- a/virt/kvm/arm/arch_timer.c
+++ b/virt/kvm/arm/arch_timer.c
@@ -557,8 +557,12 @@ void kvm_timer_vcpu_load(struct kvm_vcpu *vcpu)
if (map.direct_ptimer)
timer_restore_state(map.direct_ptimer);
- if (map.emul_ptimer)
+ if (map.emul_ptimer) {
+
Re: [PATCH] KVM: arm/arm64: fix emulated ptimer irq injection
On Tue, May 28, 2019 at 03:12:15PM +0200, Christoffer Dall wrote:
> On Tue, May 28, 2019 at 01:25:52PM +0100, Marc Zyngier wrote:
> > On 28/05/2019 12:01, Christoffer Dall wrote:
> > > On Mon, May 27, 2019 at 01:46:19PM +0200, Andrew Jones wrote:
> > >> The emulated ptimer needs to track the level changes, otherwise the
> > >> the interrupt will never get deasserted, resulting in the guest getting
> > >> stuck in an interrupt storm if it enables ptimer interrupts. This was
> > >> found with kvm-unit-tests; the ptimer tests hung as soon as interrupts
> > >> were enabled. Typical Linux guests don't have a problem as they prefer
> > >> using the virtual timer.
> > >>
> > >> Fixes: bee038a674875 ("KVM: arm/arm64: Rework the timer code to use a
> > >> timer_map")
> > >> Signed-off-by: Andrew Jones
> > >> ---
> > >> virt/kvm/arm/arch_timer.c | 7 ++-
> > >> 1 file changed, 6 insertions(+), 1 deletion(-)
> > >>
> > >> diff --git a/virt/kvm/arm/arch_timer.c b/virt/kvm/arm/arch_timer.c
> > >> index 7fc272ecae16..9f5d8cc8b5e5 100644
> > >> --- a/virt/kvm/arm/arch_timer.c
> > >> +++ b/virt/kvm/arm/arch_timer.c
> > >> @@ -324,10 +324,15 @@ static void kvm_timer_update_irq(struct kvm_vcpu
> > >> *vcpu, bool new_level,
> > >> static void timer_emulate(struct arch_timer_context *ctx)
> > >> {
> > >> bool should_fire = kvm_timer_should_fire(ctx);
> > >> +struct timer_map map;
> > >> +
> > >> +get_timer_map(ctx->vcpu, );
> > >>
> > >> trace_kvm_timer_emulate(ctx, should_fire);
> > >>
> > >> -if (should_fire) {
> > >> +if (ctx == map.emul_ptimer && should_fire != ctx->irq.level) {
> > >> +kvm_timer_update_irq(ctx->vcpu, !ctx->irq.level, ctx);
> > >> +} else if (should_fire) {
> > >> kvm_timer_update_irq(ctx->vcpu, true, ctx);
> > >> return;
> > >> }
> > >
> > > Hmm, this doesn't feel completely right.
I won't try to argue that this is the right fix, as I haven't fully
grasped how all this code works, but, afaict, this is how it worked
prior to bee038a6.
> > >
> > > Lowering the line of an emulated timer should only ever happen when the
> > > guest (or user space) writes to one of the system registers for that
> > > timer, which should be trapped and that should cause an update of the
> > > line.
> > >
> > > Are we missing a call to kvm_timer_update_irq() from
> > > kvm_arm_timer_set_reg() ?
> >
> > Which is exactly what we removed in 6bc210003dff, for good reasons.
> >
>
> Ah well, I can be wrong twice. Or even three times.
>
> > Looking at kvm_arm_timer_write_sysreg(), we end-up calling
> > kvm_timer_vcpu_load, but not updating the irq status.
> >
> > How about something like this instead (untested):
> >
> > diff --git a/virt/kvm/arm/arch_timer.c b/virt/kvm/arm/arch_timer.c
> > index 7fc272ecae16..6a418dcc5433 100644
> > --- a/virt/kvm/arm/arch_timer.c
> > +++ b/virt/kvm/arm/arch_timer.c
> > @@ -882,10 +882,14 @@ void kvm_arm_timer_write_sysreg(struct kvm_vcpu *vcpu,
> > enum kvm_arch_timer_regs treg,
> > u64 val)
> > {
> > + struct arch_timer_context *timer;
> > +
> > preempt_disable();
> > kvm_timer_vcpu_put(vcpu);
> >
> > - kvm_arm_timer_write(vcpu, vcpu_get_timer(vcpu, tmr), treg, val);
> > + timer = vcpu_get_timer(vcpu, tmr);
> > + kvm_arm_timer_write(vcpu, timer, treg, val);
> > + kvm_timer_update_irq(vcpu, kvm_timer_should_fire(timer), timer);
> >
> > kvm_timer_vcpu_load(vcpu);
> > preempt_enable();
> >
Marc, I've tested this and it resolves the issue for me. If/when you post
it you can add a t-b from me if you like.
>
> Yes, that looks reasonable. Basically, in 6bc210003dff we should have
> only removed the call to timer_emulate, and not messed around with
> kvm_timer_update_irq()?
>
> After this patch, we'll have moved the call to kvm_timer_update_irq()
> from kvm_arm_timer_set_reg() to kvm_arm_timer_write_sysreg(). I can't
> seem to decide if clearly belongs in one place or the other.
>
Isn't kvm_arm_timer_set_reg() only for userspace setting of the register?
In this test case I don't think userspace is involved at that point.
Thanks,
drew
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH] KVM: arm/arm64: fix emulated ptimer irq injection
On Tue, May 28, 2019 at 01:25:52PM +0100, Marc Zyngier wrote:
> On 28/05/2019 12:01, Christoffer Dall wrote:
> > On Mon, May 27, 2019 at 01:46:19PM +0200, Andrew Jones wrote:
> >> The emulated ptimer needs to track the level changes, otherwise the
> >> the interrupt will never get deasserted, resulting in the guest getting
> >> stuck in an interrupt storm if it enables ptimer interrupts. This was
> >> found with kvm-unit-tests; the ptimer tests hung as soon as interrupts
> >> were enabled. Typical Linux guests don't have a problem as they prefer
> >> using the virtual timer.
> >>
> >> Fixes: bee038a674875 ("KVM: arm/arm64: Rework the timer code to use a
> >> timer_map")
> >> Signed-off-by: Andrew Jones
> >> ---
> >> virt/kvm/arm/arch_timer.c | 7 ++-
> >> 1 file changed, 6 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/virt/kvm/arm/arch_timer.c b/virt/kvm/arm/arch_timer.c
> >> index 7fc272ecae16..9f5d8cc8b5e5 100644
> >> --- a/virt/kvm/arm/arch_timer.c
> >> +++ b/virt/kvm/arm/arch_timer.c
> >> @@ -324,10 +324,15 @@ static void kvm_timer_update_irq(struct kvm_vcpu
> >> *vcpu, bool new_level,
> >> static void timer_emulate(struct arch_timer_context *ctx)
> >> {
> >>bool should_fire = kvm_timer_should_fire(ctx);
> >> + struct timer_map map;
> >> +
> >> + get_timer_map(ctx->vcpu, );
> >>
> >>trace_kvm_timer_emulate(ctx, should_fire);
> >>
> >> - if (should_fire) {
> >> + if (ctx == map.emul_ptimer && should_fire != ctx->irq.level) {
> >> + kvm_timer_update_irq(ctx->vcpu, !ctx->irq.level, ctx);
> >> + } else if (should_fire) {
> >>kvm_timer_update_irq(ctx->vcpu, true, ctx);
> >>return;
> >>}
> >
> > Hmm, this doesn't feel completely right.
> >
> > Lowering the line of an emulated timer should only ever happen when the
> > guest (or user space) writes to one of the system registers for that
> > timer, which should be trapped and that should cause an update of the
> > line.
> >
> > Are we missing a call to kvm_timer_update_irq() from
> > kvm_arm_timer_set_reg() ?
>
> Which is exactly what we removed in 6bc210003dff, for good reasons.
>
Ah well, I can be wrong twice. Or even three times.
> Looking at kvm_arm_timer_write_sysreg(), we end-up calling
> kvm_timer_vcpu_load, but not updating the irq status.
>
> How about something like this instead (untested):
>
> diff --git a/virt/kvm/arm/arch_timer.c b/virt/kvm/arm/arch_timer.c
> index 7fc272ecae16..6a418dcc5433 100644
> --- a/virt/kvm/arm/arch_timer.c
> +++ b/virt/kvm/arm/arch_timer.c
> @@ -882,10 +882,14 @@ void kvm_arm_timer_write_sysreg(struct kvm_vcpu *vcpu,
> enum kvm_arch_timer_regs treg,
> u64 val)
> {
> + struct arch_timer_context *timer;
> +
> preempt_disable();
> kvm_timer_vcpu_put(vcpu);
>
> - kvm_arm_timer_write(vcpu, vcpu_get_timer(vcpu, tmr), treg, val);
> + timer = vcpu_get_timer(vcpu, tmr);
> + kvm_arm_timer_write(vcpu, timer, treg, val);
> + kvm_timer_update_irq(vcpu, kvm_timer_should_fire(timer), timer);
>
> kvm_timer_vcpu_load(vcpu);
> preempt_enable();
>
Yes, that looks reasonable. Basically, in 6bc210003dff we should have
only removed the call to timer_emulate, and not messed around with
kvm_timer_update_irq()?
After this patch, we'll have moved the call to kvm_timer_update_irq()
from kvm_arm_timer_set_reg() to kvm_arm_timer_write_sysreg(). I can't
seem to decide if clearly belongs in one place or the other.
Thanks,
Christoffer
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH] KVM: arm/arm64: fix emulated ptimer irq injection
On 28/05/2019 12:01, Christoffer Dall wrote:
> On Mon, May 27, 2019 at 01:46:19PM +0200, Andrew Jones wrote:
>> The emulated ptimer needs to track the level changes, otherwise the
>> the interrupt will never get deasserted, resulting in the guest getting
>> stuck in an interrupt storm if it enables ptimer interrupts. This was
>> found with kvm-unit-tests; the ptimer tests hung as soon as interrupts
>> were enabled. Typical Linux guests don't have a problem as they prefer
>> using the virtual timer.
>>
>> Fixes: bee038a674875 ("KVM: arm/arm64: Rework the timer code to use a
>> timer_map")
>> Signed-off-by: Andrew Jones
>> ---
>> virt/kvm/arm/arch_timer.c | 7 ++-
>> 1 file changed, 6 insertions(+), 1 deletion(-)
>>
>> diff --git a/virt/kvm/arm/arch_timer.c b/virt/kvm/arm/arch_timer.c
>> index 7fc272ecae16..9f5d8cc8b5e5 100644
>> --- a/virt/kvm/arm/arch_timer.c
>> +++ b/virt/kvm/arm/arch_timer.c
>> @@ -324,10 +324,15 @@ static void kvm_timer_update_irq(struct kvm_vcpu
>> *vcpu, bool new_level,
>> static void timer_emulate(struct arch_timer_context *ctx)
>> {
>> bool should_fire = kvm_timer_should_fire(ctx);
>> +struct timer_map map;
>> +
>> +get_timer_map(ctx->vcpu, );
>>
>> trace_kvm_timer_emulate(ctx, should_fire);
>>
>> -if (should_fire) {
>> +if (ctx == map.emul_ptimer && should_fire != ctx->irq.level) {
>> +kvm_timer_update_irq(ctx->vcpu, !ctx->irq.level, ctx);
>> +} else if (should_fire) {
>> kvm_timer_update_irq(ctx->vcpu, true, ctx);
>> return;
>> }
>
> Hmm, this doesn't feel completely right.
>
> Lowering the line of an emulated timer should only ever happen when the
> guest (or user space) writes to one of the system registers for that
> timer, which should be trapped and that should cause an update of the
> line.
>
> Are we missing a call to kvm_timer_update_irq() from
> kvm_arm_timer_set_reg() ?
Which is exactly what we removed in 6bc210003dff, for good reasons.
Looking at kvm_arm_timer_write_sysreg(), we end-up calling kvm_timer_vcpu_load,
but not updating the irq status.
How about something like this instead (untested):
diff --git a/virt/kvm/arm/arch_timer.c b/virt/kvm/arm/arch_timer.c
index 7fc272ecae16..6a418dcc5433 100644
--- a/virt/kvm/arm/arch_timer.c
+++ b/virt/kvm/arm/arch_timer.c
@@ -882,10 +882,14 @@ void kvm_arm_timer_write_sysreg(struct kvm_vcpu *vcpu,
enum kvm_arch_timer_regs treg,
u64 val)
{
+ struct arch_timer_context *timer;
+
preempt_disable();
kvm_timer_vcpu_put(vcpu);
- kvm_arm_timer_write(vcpu, vcpu_get_timer(vcpu, tmr), treg, val);
+ timer = vcpu_get_timer(vcpu, tmr);
+ kvm_arm_timer_write(vcpu, timer, treg, val);
+ kvm_timer_update_irq(vcpu, kvm_timer_should_fire(timer), timer);
kvm_timer_vcpu_load(vcpu);
preempt_enable();
Thanks,
M.
--
Jazz is not dead. It just smells funny...
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH] KVM: arm/arm64: fix emulated ptimer irq injection
On Mon, May 27, 2019 at 01:46:19PM +0200, Andrew Jones wrote:
> The emulated ptimer needs to track the level changes, otherwise the
> the interrupt will never get deasserted, resulting in the guest getting
> stuck in an interrupt storm if it enables ptimer interrupts. This was
> found with kvm-unit-tests; the ptimer tests hung as soon as interrupts
> were enabled. Typical Linux guests don't have a problem as they prefer
> using the virtual timer.
>
> Fixes: bee038a674875 ("KVM: arm/arm64: Rework the timer code to use a
> timer_map")
> Signed-off-by: Andrew Jones
> ---
> virt/kvm/arm/arch_timer.c | 7 ++-
> 1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/virt/kvm/arm/arch_timer.c b/virt/kvm/arm/arch_timer.c
> index 7fc272ecae16..9f5d8cc8b5e5 100644
> --- a/virt/kvm/arm/arch_timer.c
> +++ b/virt/kvm/arm/arch_timer.c
> @@ -324,10 +324,15 @@ static void kvm_timer_update_irq(struct kvm_vcpu *vcpu,
> bool new_level,
> static void timer_emulate(struct arch_timer_context *ctx)
> {
> bool should_fire = kvm_timer_should_fire(ctx);
> + struct timer_map map;
> +
> + get_timer_map(ctx->vcpu, );
>
> trace_kvm_timer_emulate(ctx, should_fire);
>
> - if (should_fire) {
> + if (ctx == map.emul_ptimer && should_fire != ctx->irq.level) {
> + kvm_timer_update_irq(ctx->vcpu, !ctx->irq.level, ctx);
> + } else if (should_fire) {
> kvm_timer_update_irq(ctx->vcpu, true, ctx);
> return;
> }
Hmm, this doesn't feel completely right.
Lowering the line of an emulated timer should only ever happen when the
guest (or user space) writes to one of the system registers for that
timer, which should be trapped and that should cause an update of the
line.
Are we missing a call to kvm_timer_update_irq() from
kvm_arm_timer_set_reg() ?
Thanks,
Christoffer
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
[PATCH] KVM: arm/arm64: fix emulated ptimer irq injection
The emulated ptimer needs to track the level changes, otherwise the
the interrupt will never get deasserted, resulting in the guest getting
stuck in an interrupt storm if it enables ptimer interrupts. This was
found with kvm-unit-tests; the ptimer tests hung as soon as interrupts
were enabled. Typical Linux guests don't have a problem as they prefer
using the virtual timer.
Fixes: bee038a674875 ("KVM: arm/arm64: Rework the timer code to use a
timer_map")
Signed-off-by: Andrew Jones
---
virt/kvm/arm/arch_timer.c | 7 ++-
1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/virt/kvm/arm/arch_timer.c b/virt/kvm/arm/arch_timer.c
index 7fc272ecae16..9f5d8cc8b5e5 100644
--- a/virt/kvm/arm/arch_timer.c
+++ b/virt/kvm/arm/arch_timer.c
@@ -324,10 +324,15 @@ static void kvm_timer_update_irq(struct kvm_vcpu *vcpu,
bool new_level,
static void timer_emulate(struct arch_timer_context *ctx)
{
bool should_fire = kvm_timer_should_fire(ctx);
+ struct timer_map map;
+
+ get_timer_map(ctx->vcpu, );
trace_kvm_timer_emulate(ctx, should_fire);
- if (should_fire) {
+ if (ctx == map.emul_ptimer && should_fire != ctx->irq.level) {
+ kvm_timer_update_irq(ctx->vcpu, !ctx->irq.level, ctx);
+ } else if (should_fire) {
kvm_timer_update_irq(ctx->vcpu, true, ctx);
return;
}
--
2.18.1
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
