| Message ID | 20171204200506.3224-8-cdall@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, Dec 04, 2017 at 09:05:05PM +0100, Christoffer Dall wrote: > From: Christoffer Dall <christoffer.dall@linaro.org> > > The VGIC can now support the life-cycle of mapped level-triggered > interrupts, and we no longer have to read back the timer state on every > exit from the VM if we had an asserted timer interrupt signal, because > the VGIC already knows if we hit the unlikely case where the guest > disables the timer without ACKing the virtual timer interrupt. > > This means we rework a bit of the code to factor out the functionality > to snapshot the timer state from vtimer_save_state(), and we can reuse > this functionality in the sync path when we have an irqchip in > userspace, and also to support our implementation of the > get_input_level() function for the timer. > > This change also means that we can no longer rely on the timer's view of > the interrupt line to set the active state, because we no longer > maintain this state for mapped interrupts when exiting from the guest. > Instead, we only set the active state if the virtual interrupt is > active, and otherwise we simply let the timer fire again and raise the > virtual interrupt from the ISR. > > Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org> > --- > include/kvm/arm_arch_timer.h | 2 ++ > virt/kvm/arm/arch_timer.c | 75 +++++++++++++++++++++----------------------- > 2 files changed, 38 insertions(+), 39 deletions(-) > > diff --git a/include/kvm/arm_arch_timer.h b/include/kvm/arm_arch_timer.h > index 01ee473517e2..f57f795d704c 100644 > --- a/include/kvm/arm_arch_timer.h > +++ b/include/kvm/arm_arch_timer.h > @@ -90,6 +90,8 @@ void kvm_timer_vcpu_put(struct kvm_vcpu *vcpu); > > void kvm_timer_init_vhe(void); > > +bool kvm_arch_timer_get_input_level(int vintid); > + > #define vcpu_vtimer(v) (&(v)->arch.timer_cpu.vtimer) > #define vcpu_ptimer(v) (&(v)->arch.timer_cpu.ptimer) > [...] > +bool kvm_arch_timer_get_input_level(int vintid) > +{ > + struct kvm_vcpu *vcpu = kvm_arm_get_running_vcpu(); > + struct arch_timer_context *timer; > + > + if (vintid == vcpu_vtimer(vcpu)->irq.irq) > + timer = vcpu_vtimer(vcpu); > + else > + BUG(); /* We only map the vtimer so far */ > + > + if (timer->loaded) > + __timer_snapshot_state(timer); > + > + return kvm_timer_should_fire(timer); > +} I think it worth to reword to highlight your intention about BUG, and save 1 call to vcpu_vtimer() bool kvm_arch_timer_get_input_level(int vintid) { struct kvm_vcpu *vcpu = kvm_arm_get_running_vcpu(); struct arch_timer_context *timer = vcpu_vtimer(vcpu); /* We only map the vtimer so far */ BUG_ON(vintid != timer->irq.irq) if (timer->loaded) __timer_snapshot_state(timer); return kvm_timer_should_fire(timer); } > int kvm_timer_enable(struct kvm_vcpu *vcpu) > { > struct arch_timer_cpu *timer = &vcpu->arch.timer_cpu; > @@ -841,7 +838,7 @@ int kvm_timer_enable(struct kvm_vcpu *vcpu) > } > > ret = kvm_vgic_map_phys_irq(vcpu, host_vtimer_irq, vtimer->irq.irq, > - NULL); > + kvm_arch_timer_get_input_level); > if (ret) > return ret; > > -- > 2.14.2
On Tue, Dec 05, 2017 at 06:24:46PM +0300, Yury Norov wrote: > On Mon, Dec 04, 2017 at 09:05:05PM +0100, Christoffer Dall wrote: > > From: Christoffer Dall <christoffer.dall@linaro.org> > > > > The VGIC can now support the life-cycle of mapped level-triggered > > interrupts, and we no longer have to read back the timer state on every > > exit from the VM if we had an asserted timer interrupt signal, because > > the VGIC already knows if we hit the unlikely case where the guest > > disables the timer without ACKing the virtual timer interrupt. > > > > This means we rework a bit of the code to factor out the functionality > > to snapshot the timer state from vtimer_save_state(), and we can reuse > > this functionality in the sync path when we have an irqchip in > > userspace, and also to support our implementation of the > > get_input_level() function for the timer. > > > > This change also means that we can no longer rely on the timer's view of > > the interrupt line to set the active state, because we no longer > > maintain this state for mapped interrupts when exiting from the guest. > > Instead, we only set the active state if the virtual interrupt is > > active, and otherwise we simply let the timer fire again and raise the > > virtual interrupt from the ISR. > > > > Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org> > > --- > > include/kvm/arm_arch_timer.h | 2 ++ > > virt/kvm/arm/arch_timer.c | 75 +++++++++++++++++++++----------------------- > > 2 files changed, 38 insertions(+), 39 deletions(-) > > > > diff --git a/include/kvm/arm_arch_timer.h b/include/kvm/arm_arch_timer.h > > index 01ee473517e2..f57f795d704c 100644 > > --- a/include/kvm/arm_arch_timer.h > > +++ b/include/kvm/arm_arch_timer.h > > @@ -90,6 +90,8 @@ void kvm_timer_vcpu_put(struct kvm_vcpu *vcpu); > > > > void kvm_timer_init_vhe(void); > > > > +bool kvm_arch_timer_get_input_level(int vintid); > > + > > #define vcpu_vtimer(v) (&(v)->arch.timer_cpu.vtimer) > > #define vcpu_ptimer(v) (&(v)->arch.timer_cpu.ptimer) > > > > [...] > > > +bool kvm_arch_timer_get_input_level(int vintid) > > +{ > > + struct kvm_vcpu *vcpu = kvm_arm_get_running_vcpu(); > > + struct arch_timer_context *timer; > > + > > + if (vintid == vcpu_vtimer(vcpu)->irq.irq) > > + timer = vcpu_vtimer(vcpu); > > + else > > + BUG(); /* We only map the vtimer so far */ > > + > > + if (timer->loaded) > > + __timer_snapshot_state(timer); > > + > > + return kvm_timer_should_fire(timer); > > +} > > I think it worth to reword to highlight your intention about BUG, > and save 1 call to vcpu_vtimer() > > bool kvm_arch_timer_get_input_level(int vintid) > { > struct kvm_vcpu *vcpu = kvm_arm_get_running_vcpu(); > struct arch_timer_context *timer = vcpu_vtimer(vcpu); > > /* We only map the vtimer so far */ > BUG_ON(vintid != timer->irq.irq) > > if (timer->loaded) > __timer_snapshot_state(timer); > > return kvm_timer_should_fire(timer); > } > Besides the incredible bikesheding nature of your comments, I disagree. The current code suggests where to add functionality once we move to using the physical timer hardware to drive an EL1 physical timer on VHE systems, and is purposely written this way. I'm sure we have real bugs and real issues in the code, perhaps you could spend your energy looking for those, and if you cannot find any, then provide a reviewed-by instead of these pointless cosmetic adjustments. Thanks, -Christoffer
On Wed, Dec 06, 2017 at 11:59:04AM +0100, Christoffer Dall wrote: > On Tue, Dec 05, 2017 at 06:24:46PM +0300, Yury Norov wrote: > > On Mon, Dec 04, 2017 at 09:05:05PM +0100, Christoffer Dall wrote: > > > From: Christoffer Dall <christoffer.dall@linaro.org> > > > > > > The VGIC can now support the life-cycle of mapped level-triggered > > > interrupts, and we no longer have to read back the timer state on every > > > exit from the VM if we had an asserted timer interrupt signal, because > > > the VGIC already knows if we hit the unlikely case where the guest > > > disables the timer without ACKing the virtual timer interrupt. > > > > > > This means we rework a bit of the code to factor out the functionality > > > to snapshot the timer state from vtimer_save_state(), and we can reuse > > > this functionality in the sync path when we have an irqchip in > > > userspace, and also to support our implementation of the > > > get_input_level() function for the timer. > > > > > > This change also means that we can no longer rely on the timer's view of > > > the interrupt line to set the active state, because we no longer > > > maintain this state for mapped interrupts when exiting from the guest. > > > Instead, we only set the active state if the virtual interrupt is > > > active, and otherwise we simply let the timer fire again and raise the > > > virtual interrupt from the ISR. > > > > > > Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org> > > > --- > > > include/kvm/arm_arch_timer.h | 2 ++ > > > virt/kvm/arm/arch_timer.c | 75 +++++++++++++++++++++----------------------- > > > 2 files changed, 38 insertions(+), 39 deletions(-) > > > > > > diff --git a/include/kvm/arm_arch_timer.h b/include/kvm/arm_arch_timer.h > > > index 01ee473517e2..f57f795d704c 100644 > > > --- a/include/kvm/arm_arch_timer.h > > > +++ b/include/kvm/arm_arch_timer.h > > > @@ -90,6 +90,8 @@ void kvm_timer_vcpu_put(struct kvm_vcpu *vcpu); > > > > > > void kvm_timer_init_vhe(void); > > > > > > +bool kvm_arch_timer_get_input_level(int vintid); > > > + > > > #define vcpu_vtimer(v) (&(v)->arch.timer_cpu.vtimer) > > > #define vcpu_ptimer(v) (&(v)->arch.timer_cpu.ptimer) > > > > > > > [...] > > > > > +bool kvm_arch_timer_get_input_level(int vintid) > > > +{ > > > + struct kvm_vcpu *vcpu = kvm_arm_get_running_vcpu(); > > > + struct arch_timer_context *timer; > > > + > > > + if (vintid == vcpu_vtimer(vcpu)->irq.irq) > > > + timer = vcpu_vtimer(vcpu); > > > + else > > > + BUG() We only map the vtimer so far */ > > > + > > > + if (timer->loaded) > > > + __timer_snapshot_state(timer); > > > + > > > + return kvm_timer_should_fire(timer); > > > +} > > > > I think it worth to reword to highlight your intention about BUG, > > and save 1 call to vcpu_vtimer() > > > > bool kvm_arch_timer_get_input_level(int vintid) > > { > > struct kvm_vcpu *vcpu = kvm_arm_get_running_vcpu(); > > struct arch_timer_context *timer = vcpu_vtimer(vcpu); > > > > /* We only map the vtimer so far */ > > BUG_ON(vintid != timer->irq.irq) > > > > if (timer->loaded) > > __timer_snapshot_state(timer); > > > > return kvm_timer_should_fire(timer); > > } > > > > Besides the incredible bikesheding nature of your comments, I disagree. > The current code suggests where to add functionality once we move to > using the physical timer hardware to drive an EL1 physical timer on VHE > systems, and is purposely written this way. > > I'm sure we have real bugs and real issues in the code, perhaps you > could spend your energy looking for those, and if you cannot find any, > then provide a reviewed-by instead of these pointless cosmetic > adjustments. OK. I understood. Let me elaborate. 0. As you say in patch 0, This series is based on 4.15-rc, so I decided that the code above is assumed to be release version. You may change it in future, or may not, but the code will exist as is in mainline kernel for some time, right? 1. BUG() is needed to kill system, and it really kills it. This is wrong to kill system in else-branch of some minor helper due to unimplemented feature. You should use pr_err() or WARN_ON() instead. The best - return reasonable error and do everything possible on upper levels to keep system alive. What's really wrong in my comment is that I didn't suggest you switch to WARN_ON(). Did you follow this thread? https://lkml.org/lkml/2016/10/4/1 2. Calling the same function with the same argument twice in code path is also an issue. Besides the nasty feeling of that code, it might be dangerous. The most obvious scenario is when it returns different values due to change of internal state. I realize that vcpu_vtimer() is just a pointer dereference. But it may bite you painfully as well. I know it for sure because it bitten me once. Consider racing scenario in this patch: https://patchwork.kernel.org/patch/9974327/ It may never become the problem, or may become one day. But fix is clear and obvious - why not taking it? 3. I will be really happy to provide tested-by and reviewed-by. But for doing that I need to actually test and review. It would be extremely helpful if you share your testing modules and scripts. I have access to several Cavium machines and can do before/after measurements. Right now I have few tests, but kvm is very complex system, and I'm not sure I measure what I'm actually going to. Guidance from KVM experts is what really lacks. Thanks, Yury
On Wed, Dec 06, 2017 at 05:17:28PM +0300, Yury Norov wrote: > On Wed, Dec 06, 2017 at 11:59:04AM +0100, Christoffer Dall wrote: > > On Tue, Dec 05, 2017 at 06:24:46PM +0300, Yury Norov wrote: > > > On Mon, Dec 04, 2017 at 09:05:05PM +0100, Christoffer Dall wrote: > > > > From: Christoffer Dall <christoffer.dall@linaro.org> > > > > > > > > The VGIC can now support the life-cycle of mapped level-triggered > > > > interrupts, and we no longer have to read back the timer state on every > > > > exit from the VM if we had an asserted timer interrupt signal, because > > > > the VGIC already knows if we hit the unlikely case where the guest > > > > disables the timer without ACKing the virtual timer interrupt. > > > > > > > > This means we rework a bit of the code to factor out the functionality > > > > to snapshot the timer state from vtimer_save_state(), and we can reuse > > > > this functionality in the sync path when we have an irqchip in > > > > userspace, and also to support our implementation of the > > > > get_input_level() function for the timer. > > > > > > > > This change also means that we can no longer rely on the timer's view of > > > > the interrupt line to set the active state, because we no longer > > > > maintain this state for mapped interrupts when exiting from the guest. > > > > Instead, we only set the active state if the virtual interrupt is > > > > active, and otherwise we simply let the timer fire again and raise the > > > > virtual interrupt from the ISR. > > > > > > > > Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org> > > > > --- > > > > include/kvm/arm_arch_timer.h | 2 ++ > > > > virt/kvm/arm/arch_timer.c | 75 +++++++++++++++++++++----------------------- > > > > 2 files changed, 38 insertions(+), 39 deletions(-) > > > > > > > > diff --git a/include/kvm/arm_arch_timer.h b/include/kvm/arm_arch_timer.h > > > > index 01ee473517e2..f57f795d704c 100644 > > > > --- a/include/kvm/arm_arch_timer.h > > > > +++ b/include/kvm/arm_arch_timer.h > > > > @@ -90,6 +90,8 @@ void kvm_timer_vcpu_put(struct kvm_vcpu *vcpu); > > > > > > > > void kvm_timer_init_vhe(void); > > > > > > > > +bool kvm_arch_timer_get_input_level(int vintid); > > > > + > > > > #define vcpu_vtimer(v) (&(v)->arch.timer_cpu.vtimer) > > > > #define vcpu_ptimer(v) (&(v)->arch.timer_cpu.ptimer) > > > > > > > > > > [...] > > > > > > > +bool kvm_arch_timer_get_input_level(int vintid) > > > > +{ > > > > + struct kvm_vcpu *vcpu = kvm_arm_get_running_vcpu(); > > > > + struct arch_timer_context *timer; > > > > + > > > > + if (vintid == vcpu_vtimer(vcpu)->irq.irq) > > > > + timer = vcpu_vtimer(vcpu); > > > > + else > > > > + BUG() We only map the vtimer so far */ > > > > + > > > > + if (timer->loaded) > > > > + __timer_snapshot_state(timer); > > > > + > > > > + return kvm_timer_should_fire(timer); > > > > +} > > > > > > I think it worth to reword to highlight your intention about BUG, > > > and save 1 call to vcpu_vtimer() > > > > > > bool kvm_arch_timer_get_input_level(int vintid) > > > { > > > struct kvm_vcpu *vcpu = kvm_arm_get_running_vcpu(); > > > struct arch_timer_context *timer = vcpu_vtimer(vcpu); > > > > > > /* We only map the vtimer so far */ > > > BUG_ON(vintid != timer->irq.irq) > > > > > > if (timer->loaded) > > > __timer_snapshot_state(timer); > > > > > > return kvm_timer_should_fire(timer); > > > } > > > > > > > Besides the incredible bikesheding nature of your comments, I disagree. > > The current code suggests where to add functionality once we move to > > using the physical timer hardware to drive an EL1 physical timer on VHE > > systems, and is purposely written this way. > > > > I'm sure we have real bugs and real issues in the code, perhaps you > > could spend your energy looking for those, and if you cannot find any, > > then provide a reviewed-by instead of these pointless cosmetic > > adjustments. > > OK. I understood. Let me elaborate. > > 0. As you say in patch 0, This series is based on 4.15-rc, so I decided that > the code above is assumed to be release version. You may change it in > future, or may not, but the code will exist as is in mainline kernel for > some time, right? When the code is properly reviewed and nobody raises relevant objections, the maintainers of the subsystem can merge the code. > > 1. BUG() is needed to kill system, and it really kills it. This is wrong > to kill system in else-branch of some minor helper due to unimplemented > feature. You should use pr_err() or WARN_ON() instead. The best - return > reasonable error and do everything possible on upper levels to keep system > alive. What's really wrong in my comment is that I didn't suggest you switch > to WARN_ON(). > You're missing a key point. If the system can reasonably recover from a failure or some condition, it's definitely preferred to use a print or a warning. However, if an internal function is called with an unsupported value, when managing the operation of hardware, the most sane thing to do is to panic, because the system is in an enitrely unsupported state. > > 2. Calling the same function with the same argument twice in code path is > also an issue. Besides the nasty feeling of that code, it might be dangerous. > The most obvious scenario is when it returns different values due to change > of internal state. I realize that vcpu_vtimer() is just a pointer dereference. > But it may bite you painfully as well. I know it for sure because it bitten > me once. Consider racing scenario in this patch: > https://patchwork.kernel.org/patch/9974327/ Seriously, what make you think I need a programming lesson from you? You are taking a specific lesson and generalizing it, and then applying it to a piece of code you admit to not understanding. This is not helpful. I really don't care about your nasty feelings at this point, and claiming that calling the same function twice is in general a problem is a ridiculous statement. > > It may never become the problem, or may become one day. But fix is > clear and obvious - why not taking it? I think I've answered this already, but that's because this function will eventually become: bool kvm_arch_timer_get_input_level(int vintid) { struct kvm_vcpu *vcpu = kvm_arm_get_running_vcpu(); struct arch_timer_context *timer; if (vintid == vcpu_vtimer(vcpu)->irq.irq) timer = vcpu_vtimer(vcpu); else timer = vcpu_ptimer(vcpu); if (timer->loaded) __timer_snapshot_state(timer); return kvm_timer_should_fire(timer); } > > 3. I will be really happy to provide tested-by and reviewed-by. But > for doing that I need to actually test and review. It would be extremely > helpful if you share your testing modules and scripts. I have access > to several Cavium machines and can do before/after measurements. Right > now I have few tests, but kvm is very complex system, and I'm not sure I > measure what I'm actually going to. Guidance from KVM experts is what > really lacks. > If you don't understand what KVM is and how to use it, I'm afraid your input is not very valuable. Until you change your attitude and make some attempt to understand how we work, and try to actually understand the code you comment on, I'll be inclined to ignore future posting from you. -Christoffer
diff --git a/include/kvm/arm_arch_timer.h b/include/kvm/arm_arch_timer.h index 01ee473517e2..f57f795d704c 100644 --- a/include/kvm/arm_arch_timer.h +++ b/include/kvm/arm_arch_timer.h @@ -90,6 +90,8 @@ void kvm_timer_vcpu_put(struct kvm_vcpu *vcpu); void kvm_timer_init_vhe(void); +bool kvm_arch_timer_get_input_level(int vintid); + #define vcpu_vtimer(v) (&(v)->arch.timer_cpu.vtimer) #define vcpu_ptimer(v) (&(v)->arch.timer_cpu.ptimer) diff --git a/virt/kvm/arm/arch_timer.c b/virt/kvm/arm/arch_timer.c index e78ba5e20f74..82d4963f63b8 100644 --- a/virt/kvm/arm/arch_timer.c +++ b/virt/kvm/arm/arch_timer.c @@ -343,6 +343,12 @@ static void kvm_timer_update_state(struct kvm_vcpu *vcpu) phys_timer_emulate(vcpu); } +static void __timer_snapshot_state(struct arch_timer_context *timer) +{ + timer->cnt_ctl = read_sysreg_el0(cntv_ctl); + timer->cnt_cval = read_sysreg_el0(cntv_cval); +} + static void vtimer_save_state(struct kvm_vcpu *vcpu) { struct arch_timer_cpu *timer = &vcpu->arch.timer_cpu; @@ -354,10 +360,8 @@ static void vtimer_save_state(struct kvm_vcpu *vcpu) if (!vtimer->loaded) goto out; - if (timer->enabled) { - vtimer->cnt_ctl = read_sysreg_el0(cntv_ctl); - vtimer->cnt_cval = read_sysreg_el0(cntv_cval); - } + if (timer->enabled) + __timer_snapshot_state(vtimer); /* Disable the virtual timer */ write_sysreg_el0(0, cntv_ctl); @@ -454,8 +458,7 @@ static void kvm_timer_vcpu_load_vgic(struct kvm_vcpu *vcpu) bool phys_active; int ret; - phys_active = vtimer->irq.level || - kvm_vgic_map_is_active(vcpu, vtimer->irq.irq); + phys_active = kvm_vgic_map_is_active(vcpu, vtimer->irq.irq); ret = irq_set_irqchip_state(host_vtimer_irq, IRQCHIP_STATE_ACTIVE, @@ -541,27 +544,19 @@ void kvm_timer_vcpu_put(struct kvm_vcpu *vcpu) set_cntvoff(0); } -static void unmask_vtimer_irq(struct kvm_vcpu *vcpu) +/* + * With a userspace irqchip we have to check if the guest de-asserted the + * timer and if so, unmask the timer irq signal on the host interrupt + * controller to ensure that we see future timer signals. + */ +static void unmask_vtimer_irq_user(struct kvm_vcpu *vcpu) { struct arch_timer_context *vtimer = vcpu_vtimer(vcpu); if (unlikely(!irqchip_in_kernel(vcpu->kvm))) { - kvm_vtimer_update_mask_user(vcpu); - return; - } - - /* - * If the guest disabled the timer without acking the interrupt, then - * we must make sure the physical and virtual active states are in - * sync by deactivating the physical interrupt, because otherwise we - * wouldn't see the next timer interrupt in the host. - */ - if (!kvm_vgic_map_is_active(vcpu, vtimer->irq.irq)) { - int ret; - ret = irq_set_irqchip_state(host_vtimer_irq, - IRQCHIP_STATE_ACTIVE, - false); - WARN_ON(ret); + __timer_snapshot_state(vtimer); + if (!kvm_timer_should_fire(vtimer)) + kvm_vtimer_update_mask_user(vcpu); } } @@ -574,21 +569,7 @@ static void unmask_vtimer_irq(struct kvm_vcpu *vcpu) */ void kvm_timer_sync_hwstate(struct kvm_vcpu *vcpu) { - struct arch_timer_context *vtimer = vcpu_vtimer(vcpu); - - /* - * If we entered the guest with the vtimer output asserted we have to - * check if the guest has modified the timer so that we should lower - * the line at this point. - */ - if (vtimer->irq.level) { - vtimer->cnt_ctl = read_sysreg_el0(cntv_ctl); - vtimer->cnt_cval = read_sysreg_el0(cntv_cval); - if (!kvm_timer_should_fire(vtimer)) { - kvm_timer_update_irq(vcpu, false, vtimer); - unmask_vtimer_irq(vcpu); - } - } + unmask_vtimer_irq_user(vcpu); } int kvm_timer_vcpu_reset(struct kvm_vcpu *vcpu) @@ -819,6 +800,22 @@ static bool timer_irqs_are_valid(struct kvm_vcpu *vcpu) return true; } +bool kvm_arch_timer_get_input_level(int vintid) +{ + struct kvm_vcpu *vcpu = kvm_arm_get_running_vcpu(); + struct arch_timer_context *timer; + + if (vintid == vcpu_vtimer(vcpu)->irq.irq) + timer = vcpu_vtimer(vcpu); + else + BUG(); /* We only map the vtimer so far */ + + if (timer->loaded) + __timer_snapshot_state(timer); + + return kvm_timer_should_fire(timer); +} + int kvm_timer_enable(struct kvm_vcpu *vcpu) { struct arch_timer_cpu *timer = &vcpu->arch.timer_cpu; @@ -841,7 +838,7 @@ int kvm_timer_enable(struct kvm_vcpu *vcpu) } ret = kvm_vgic_map_phys_irq(vcpu, host_vtimer_irq, vtimer->irq.irq, - NULL); + kvm_arch_timer_get_input_level); if (ret) return ret;