| Message ID | 20190416173634.31501-1-liran.alon@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: x86: Consider LAPIC TSC-Deadline timer expired if deadline too short | expand |
On Tue, Apr 16, 2019 at 08:36:34PM +0300, Liran Alon wrote: > If guest sets MSR_IA32_TSCDEADLINE to value such that in host > time-domain it's shorter than lapic_timer_advance_ns, we can > reach a case that we call hrtimer_start() with expiration time set at > the past. > > Because lapic_timer.timer is init with HRTIMER_MODE_ABS_PINNED, it > is not allowed to run in softirq and therefore will never expire. > > To avoid such a scenario, verify that deadline expiration time is set on > host time-domain further than (now + lapic_timer_advance_ns). > > A future patch can also consider adding a min_timer_deadline_ns module parameter, > similar to min_timer_period_us to avoid races that amount of ns it takes > to run logic could still call hrtimer_start() with expiration timer set > at the past. > > Reviewed-by: Joao Martins <joao.m.martins@oracle.com> > Signed-off-by: Liran Alon <liran.alon@oracle.com> > --- > arch/x86/kvm/lapic.c | 9 ++++++--- > 1 file changed, 6 insertions(+), 3 deletions(-) > > diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c > index 9f089e2e09d0..fbd37e07e90d 100644 > --- a/arch/x86/kvm/lapic.c > +++ b/arch/x86/kvm/lapic.c > @@ -1538,9 +1538,12 @@ static void start_sw_tscdeadline(struct kvm_lapic *apic) > > now = ktime_get(); > guest_tsc = kvm_read_l1_tsc(vcpu, rdtsc()); > - if (likely(tscdeadline > guest_tsc)) { > - ns = (tscdeadline - guest_tsc) * 1000000ULL; > - do_div(ns, this_tsc_khz); > + > + ns = (tscdeadline - guest_tsc) * 1000000ULL; > + do_div(ns, this_tsc_khz); This will do the division even if 'tscdeadline <= guest_tsc'. I don't see any reason to use ktime_sub_ns() since neither 'ns' or 'lapic_timer_advance_ns' is a ktime_t, e.g.: if (likely(tscdeadline > guest_tsc)) { ns = (tscdeadline - guest_tsc) * 1000000ULL; do_div(ns, this_tsc_khz); if (likely(ns > lapic_timer_advance_ns)) ns -= lapic_timer_advance_ns; else ns = 0; } else { ns = 0; } if (ns) { now = ktime_get(); expire = ktime_add_ns(now, ns); hrtimer_start(&apic->lapic_timer.timer, expire, HRTIMER_MODE_ABS_PINNED); } else apic_timer_expired(apic); > + > + if (likely(tscdeadline > guest_tsc) && > + likely(ns > lapic_timer_advance_ns)) { > expire = ktime_add_ns(now, ns); > expire = ktime_sub_ns(expire, lapic_timer_advance_ns); > hrtimer_start(&apic->lapic_timer.timer, > -- > 2.20.1 >
> On 23 Apr 2019, at 0:13, Sean Christopherson <sean.j.christopherson@intel.com> wrote: > > On Tue, Apr 16, 2019 at 08:36:34PM +0300, Liran Alon wrote: >> If guest sets MSR_IA32_TSCDEADLINE to value such that in host >> time-domain it's shorter than lapic_timer_advance_ns, we can >> reach a case that we call hrtimer_start() with expiration time set at >> the past. >> >> Because lapic_timer.timer is init with HRTIMER_MODE_ABS_PINNED, it >> is not allowed to run in softirq and therefore will never expire. >> >> To avoid such a scenario, verify that deadline expiration time is set on >> host time-domain further than (now + lapic_timer_advance_ns). >> >> A future patch can also consider adding a min_timer_deadline_ns module parameter, >> similar to min_timer_period_us to avoid races that amount of ns it takes >> to run logic could still call hrtimer_start() with expiration timer set >> at the past. >> >> Reviewed-by: Joao Martins <joao.m.martins@oracle.com> >> Signed-off-by: Liran Alon <liran.alon@oracle.com> >> --- >> arch/x86/kvm/lapic.c | 9 ++++++--- >> 1 file changed, 6 insertions(+), 3 deletions(-) >> >> diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c >> index 9f089e2e09d0..fbd37e07e90d 100644 >> --- a/arch/x86/kvm/lapic.c >> +++ b/arch/x86/kvm/lapic.c >> @@ -1538,9 +1538,12 @@ static void start_sw_tscdeadline(struct kvm_lapic *apic) >> >> now = ktime_get(); >> guest_tsc = kvm_read_l1_tsc(vcpu, rdtsc()); >> - if (likely(tscdeadline > guest_tsc)) { >> - ns = (tscdeadline - guest_tsc) * 1000000ULL; >> - do_div(ns, this_tsc_khz); >> + >> + ns = (tscdeadline - guest_tsc) * 1000000ULL; >> + do_div(ns, this_tsc_khz); > > This will do the division even if 'tscdeadline <= guest_tsc’. So? I don’t see an issue with that… Seems like a neglectable optimisation and code is very readable in my opinion as proposed in this patch. > I don't > see any reason to use ktime_sub_ns() since neither 'ns' or > 'lapic_timer_advance_ns' is a ktime_t, e.g.: > > if (likely(tscdeadline > guest_tsc)) { > ns = (tscdeadline - guest_tsc) * 1000000ULL; > do_div(ns, this_tsc_khz); > if (likely(ns > lapic_timer_advance_ns)) > ns -= lapic_timer_advance_ns; > else > ns = 0; > } else { > ns = 0; > } > if (ns) { > now = ktime_get(); > expire = ktime_add_ns(now, ns); > hrtimer_start(&apic->lapic_timer.timer, > expire, HRTIMER_MODE_ABS_PINNED); > } else > apic_timer_expired(apic); This would obviously work. It’s a matter of taste of course. I personally think it makes code unnecessarily less readable. I would let Paolo decide of course :) -Liran > >> + >> + if (likely(tscdeadline > guest_tsc) && >> + likely(ns > lapic_timer_advance_ns)) { >> expire = ktime_add_ns(now, ns); >> expire = ktime_sub_ns(expire, lapic_timer_advance_ns); >> hrtimer_start(&apic->lapic_timer.timer, >> -- >> 2.20.1 >>
diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c index 9f089e2e09d0..fbd37e07e90d 100644 --- a/arch/x86/kvm/lapic.c +++ b/arch/x86/kvm/lapic.c @@ -1538,9 +1538,12 @@ static void start_sw_tscdeadline(struct kvm_lapic *apic) now = ktime_get(); guest_tsc = kvm_read_l1_tsc(vcpu, rdtsc()); - if (likely(tscdeadline > guest_tsc)) { - ns = (tscdeadline - guest_tsc) * 1000000ULL; - do_div(ns, this_tsc_khz); + + ns = (tscdeadline - guest_tsc) * 1000000ULL; + do_div(ns, this_tsc_khz); + + if (likely(tscdeadline > guest_tsc) && + likely(ns > lapic_timer_advance_ns)) { expire = ktime_add_ns(now, ns); expire = ktime_sub_ns(expire, lapic_timer_advance_ns); hrtimer_start(&apic->lapic_timer.timer,