From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id 94719C433EF for ; Sat, 5 Mar 2022 09:37:22 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S230511AbiCEJiJ (ORCPT ); Sat, 5 Mar 2022 04:38:09 -0500 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:45346 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S230437AbiCEJiE (ORCPT ); Sat, 5 Mar 2022 04:38:04 -0500 Received: from mail-yw1-x112a.google.com (mail-yw1-x112a.google.com [IPv6:2607:f8b0:4864:20::112a]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 5D93C23D035 for ; Sat, 5 Mar 2022 01:37:14 -0800 (PST) Received: by mail-yw1-x112a.google.com with SMTP id 00721157ae682-2dbd97f9bfcso115726807b3.9 for ; Sat, 05 Mar 2022 01:37:14 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=atishpatra.org; s=google; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc:content-transfer-encoding; bh=xOIZsf+3vy93U49coHoC6ZI/OVtVYKIXvQZyaHv+im8=; b=ftLvrvtxT3HQer2Io3LhFNMGAthfS+5BBgk9Yaz67IiFHr9GMM98KEUo+L8E5U9JcB L07iSdMqjd77frnGHiIyGZYNQexB2QIKBPqNq6I50ax93/FwZ/HZGVaeAOrDrwZP4N5h gq+6niThfy4iyCLeiSu2LEX4D4ggeKVWZBi1Y= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc:content-transfer-encoding; bh=xOIZsf+3vy93U49coHoC6ZI/OVtVYKIXvQZyaHv+im8=; b=cxsl8qHZf8Uo4D/WBSWKDUvLyzbRV+bsQTY4ysbhVftY45VwQOTNgHl2OkSuicXThQ PEY+SoR84iKz+eX8ODIH+GrrcepUjowV226XtFb1tDFlc4SOLDxlXch+ADh9K2zOj1Z6 m+j6mbyHWZPmIsc8hpM0M1nqtp3NV0ISjTLHEbFZOlOaDkv2GYnMwpbJF8WLATshXYPr CXxzLHL9e46J/ITtL7U3azA/05I4sI3k1xGwowMAQBbFBS9tNPk65Ts1xSeTsEIaZstb O3m5iDHqVNuR7+d0oHTQcRbx5iz78ep7R6OTY8gPtbNaC8ErK970BTYTrbTEhw8LntKB PdBw== X-Gm-Message-State: AOAM530YZUSuxFhKsGk0qwTyvBZOuHYU8fxIt7I8WEkpoa/SB4S0E8sG kCWx5huElkNg26XvnArUGW8troLurNMvvBOuL2EG X-Google-Smtp-Source: ABdhPJxGDNQB8Dmal092I+k/ZmkaIQvE13UwQHqvP+D4rVN26EZmKK86CJu2Nb3PYuKizdy6Cwbsat1AxjNAeaw7cZ4= X-Received: by 2002:a81:6307:0:b0:2d6:6aee:dc75 with SMTP id x7-20020a816307000000b002d66aeedc75mr1989909ywb.249.1646473033441; Sat, 05 Mar 2022 01:37:13 -0800 (PST) MIME-Version: 1.0 References: <20220304201020.810380-1-atishp@rivosinc.com> <20220304201020.810380-8-atishp@rivosinc.com> In-Reply-To: From: Atish Patra Date: Sat, 5 Mar 2022 01:37:02 -0800 Message-ID: Subject: Re: [RFC PATCH v2 7/7] RISC-V: KVM: Support sstc extension To: Jessica Clarke Cc: Atish Patra , "linux-kernel@vger.kernel.org List" , Anup Patel , Damien Le Moal , devicetree , Jisheng Zhang , Krzysztof Kozlowski , kvm-riscv@lists.infradead.org, linux-riscv , Palmer Dabbelt , Paul Walmsley , Rob Herring Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Fri, Mar 4, 2022 at 12:38 PM Jessica Clarke wrote: > > On 4 Mar 2022, at 20:10, Atish Patra wrote: > > > > Sstc extension allows the guest to program the vstimecmp CSR directly > > instead of making an SBI call to the hypervisor to program the next > > event. The timer interrupt is also directly injected to the guest by > > the hardware in this case. To maintain backward compatibility, the > > hypervisors also update the vstimecmp in an SBI set_time call if > > the hardware supports it. Thus, the older kernels in guest also > > take advantage of the sstc extension. > > Same comment as the OpenSBI patch. I have replied to your comment in OpenSBI. > This changes the semantics of the > SBI call from only touching M-mode (or HS-mode in this case) state > (minus STIP as explicitly requested) to also touching S-mode (or > VS-mode in this case) visible and controlled state, which to me goes > against the spec as any clobbered state needs to be explicitly > specified, but is not in the current frozen 0.3 spec. All this does is > optimise for legacy systems by adding code complexity, anyway, so I > fail to see why it=E2=80=99s really needed, if they want to go faster the= y can > just adopt Sstc. You can=E2=80=99t get rid of the existing mechanism so l= ong as > you want to support non-Sstc hardware so it=E2=80=99s just adding a third > poorly-motivated case that to me goes against the spec. > In hypervisor, STIP bit in hvip is still writable. Thus, hypervisor can continue to use the hrtimer and inject the guest timer interrupt via hvip even though it is suboptimal. But I agree with your point in general. To summarize, guest timer interrupts can be managed by the hypervisor for older guest kernel without sstc support on sstc enabled hardware in the following ways: 1. In SBI call handler, update vstimecmp directly so that guest can receive timer interrupt directly (as implemented in this patch) or 2. In SBI call handler, hypervisor will setup an hrtimer and inject the guest timer interrupt via hvip when the hrtimer expired. (current behavior) Personally, I am okay with either approach. Any other thoughts ? > Jess > > > Signed-off-by: Atish Patra > > --- > > arch/riscv/include/asm/kvm_host.h | 1 + > > arch/riscv/include/asm/kvm_vcpu_timer.h | 8 +- > > arch/riscv/include/uapi/asm/kvm.h | 1 + > > arch/riscv/kvm/main.c | 12 ++- > > arch/riscv/kvm/vcpu.c | 4 +- > > arch/riscv/kvm/vcpu_timer.c | 138 +++++++++++++++++++++++- > > 6 files changed, 158 insertions(+), 6 deletions(-) > > > > diff --git a/arch/riscv/include/asm/kvm_host.h b/arch/riscv/include/asm= /kvm_host.h > > index 99ef6a120617..2ed93cdb334f 100644 > > --- a/arch/riscv/include/asm/kvm_host.h > > +++ b/arch/riscv/include/asm/kvm_host.h > > @@ -135,6 +135,7 @@ struct kvm_vcpu_csr { > > unsigned long hvip; > > unsigned long vsatp; > > unsigned long scounteren; > > + u64 vstimecmp; > > }; > > > > struct kvm_vcpu_arch { > > diff --git a/arch/riscv/include/asm/kvm_vcpu_timer.h b/arch/riscv/inclu= de/asm/kvm_vcpu_timer.h > > index 375281eb49e0..a24a265f3ccb 100644 > > --- a/arch/riscv/include/asm/kvm_vcpu_timer.h > > +++ b/arch/riscv/include/asm/kvm_vcpu_timer.h > > @@ -28,6 +28,11 @@ struct kvm_vcpu_timer { > > u64 next_cycles; > > /* Underlying hrtimer instance */ > > struct hrtimer hrt; > > + > > + /* Flag to check if sstc is enabled or not */ > > + bool sstc_enabled; > > + /* A function pointer to switch between stimecmp or hrtimer at ru= ntime */ > > + int (*timer_next_event)(struct kvm_vcpu *vcpu, u64 ncycles); > > }; > > > > int kvm_riscv_vcpu_timer_next_event(struct kvm_vcpu *vcpu, u64 ncycles)= ; > > @@ -39,6 +44,7 @@ int kvm_riscv_vcpu_timer_init(struct kvm_vcpu *vcpu); > > int kvm_riscv_vcpu_timer_deinit(struct kvm_vcpu *vcpu); > > int kvm_riscv_vcpu_timer_reset(struct kvm_vcpu *vcpu); > > void kvm_riscv_vcpu_timer_restore(struct kvm_vcpu *vcpu); > > +void kvm_riscv_vcpu_timer_save(struct kvm_vcpu *vcpu); > > int kvm_riscv_guest_timer_init(struct kvm *kvm); > > - > > +bool kvm_riscv_vcpu_timer_pending(struct kvm_vcpu *vcpu); > > #endif > > diff --git a/arch/riscv/include/uapi/asm/kvm.h b/arch/riscv/include/uap= i/asm/kvm.h > > index 92bd469e2ba6..d2f02ba1947a 100644 > > --- a/arch/riscv/include/uapi/asm/kvm.h > > +++ b/arch/riscv/include/uapi/asm/kvm.h > > @@ -96,6 +96,7 @@ enum KVM_RISCV_ISA_EXT_ID { > > KVM_RISCV_ISA_EXT_H, > > KVM_RISCV_ISA_EXT_I, > > KVM_RISCV_ISA_EXT_M, > > + KVM_RISCV_ISA_EXT_SSTC, > > KVM_RISCV_ISA_EXT_MAX, > > }; > > > > diff --git a/arch/riscv/kvm/main.c b/arch/riscv/kvm/main.c > > index 2e5ca43c8c49..83c4db7fc35f 100644 > > --- a/arch/riscv/kvm/main.c > > +++ b/arch/riscv/kvm/main.c > > @@ -32,7 +32,7 @@ int kvm_arch_hardware_setup(void *opaque) > > > > int kvm_arch_hardware_enable(void) > > { > > - unsigned long hideleg, hedeleg; > > + unsigned long hideleg, hedeleg, henvcfg; > > > > hedeleg =3D 0; > > hedeleg |=3D (1UL << EXC_INST_MISALIGNED); > > @@ -51,6 +51,16 @@ int kvm_arch_hardware_enable(void) > > > > csr_write(CSR_HCOUNTEREN, -1UL); > > > > + if (riscv_isa_extension_available(NULL, SSTC)) { > > +#ifdef CONFIG_64BIT > > + henvcfg =3D csr_read(CSR_HENVCFG); > > + csr_write(CSR_HENVCFG, henvcfg | 1UL< > +#else > > + henvcfg =3D csr_read(CSR_HENVCFGH); > > + csr_write(CSR_HENVCFGH, henvcfg | 1UL< > +#endif > > + } > > + > > csr_write(CSR_HVIP, 0); > > > > return 0; > > diff --git a/arch/riscv/kvm/vcpu.c b/arch/riscv/kvm/vcpu.c > > index a3ae7042c696..f7c08a182e3a 100644 > > --- a/arch/riscv/kvm/vcpu.c > > +++ b/arch/riscv/kvm/vcpu.c > > @@ -143,7 +143,7 @@ void kvm_arch_vcpu_destroy(struct kvm_vcpu *vcpu) > > > > int kvm_cpu_has_pending_timer(struct kvm_vcpu *vcpu) > > { > > - return kvm_riscv_vcpu_has_interrupts(vcpu, 1UL << IRQ_VS_TIMER); > > + return kvm_riscv_vcpu_timer_pending(vcpu); > > } > > > > void kvm_arch_vcpu_blocking(struct kvm_vcpu *vcpu) > > @@ -374,6 +374,7 @@ static unsigned long kvm_isa_ext_arr[] =3D { > > RISCV_ISA_EXT_h, > > RISCV_ISA_EXT_i, > > RISCV_ISA_EXT_m, > > + RISCV_ISA_EXT_SSTC, > > }; > > > > static int kvm_riscv_vcpu_get_reg_isa_ext(struct kvm_vcpu *vcpu, > > @@ -757,6 +758,7 @@ void kvm_arch_vcpu_put(struct kvm_vcpu *vcpu) > > vcpu->arch.isa); > > kvm_riscv_vcpu_host_fp_restore(&vcpu->arch.host_context); > > > > + kvm_riscv_vcpu_timer_save(vcpu); > > csr_write(CSR_HGATP, 0); > > > > csr->vsstatus =3D csr_read(CSR_VSSTATUS); > > diff --git a/arch/riscv/kvm/vcpu_timer.c b/arch/riscv/kvm/vcpu_timer.c > > index 5c4c37ff2d48..d226a931de92 100644 > > --- a/arch/riscv/kvm/vcpu_timer.c > > +++ b/arch/riscv/kvm/vcpu_timer.c > > @@ -69,7 +69,18 @@ static int kvm_riscv_vcpu_timer_cancel(struct kvm_vc= pu_timer *t) > > return 0; > > } > > > > -int kvm_riscv_vcpu_timer_next_event(struct kvm_vcpu *vcpu, u64 ncycles= ) > > +static int kvm_riscv_vcpu_update_vstimecmp(struct kvm_vcpu *vcpu, u64 = ncycles) > > +{ > > +#if __riscv_xlen =3D=3D 32 > > + csr_write(CSR_VSTIMECMP, ncycles & 0xFFFFFFFF); > > + csr_write(CSR_VSTIMECMPH, ncycles >> 32); > > +#else > > + csr_write(CSR_VSTIMECMP, ncycles); > > +#endif > > + return 0; > > +} > > + > > +static int kvm_riscv_vcpu_update_hrtimer(struct kvm_vcpu *vcpu, u64 nc= ycles) > > { > > struct kvm_vcpu_timer *t =3D &vcpu->arch.timer; > > struct kvm_guest_timer *gt =3D &vcpu->kvm->arch.timer; > > @@ -88,6 +99,68 @@ int kvm_riscv_vcpu_timer_next_event(struct kvm_vcpu = *vcpu, u64 ncycles) > > return 0; > > } > > > > +int kvm_riscv_vcpu_timer_next_event(struct kvm_vcpu *vcpu, u64 ncycles= ) > > +{ > > + struct kvm_vcpu_timer *t =3D &vcpu->arch.timer; > > + > > + return t->timer_next_event(vcpu, ncycles); > > +} > > + > > +static enum hrtimer_restart kvm_riscv_vcpu_vstimer_expired(struct hrti= mer *h) > > +{ > > + u64 delta_ns; > > + struct kvm_vcpu_timer *t =3D container_of(h, struct kvm_vcpu_time= r, hrt); > > + struct kvm_vcpu *vcpu =3D container_of(t, struct kvm_vcpu, arch.t= imer); > > + struct kvm_guest_timer *gt =3D &vcpu->kvm->arch.timer; > > + > > + if (kvm_riscv_current_cycles(gt) < t->next_cycles) { > > + delta_ns =3D kvm_riscv_delta_cycles2ns(t->next_cycles, gt= , t); > > + hrtimer_forward_now(&t->hrt, ktime_set(0, delta_ns)); > > + return HRTIMER_RESTART; > > + } > > + > > + t->next_set =3D false; > > + kvm_vcpu_kick(vcpu); > > + > > + return HRTIMER_NORESTART; > > +} > > + > > +bool kvm_riscv_vcpu_timer_pending(struct kvm_vcpu *vcpu) > > +{ > > + struct kvm_vcpu_timer *t =3D &vcpu->arch.timer; > > + struct kvm_guest_timer *gt =3D &vcpu->kvm->arch.timer; > > + u64 vstimecmp_val =3D vcpu->arch.guest_csr.vstimecmp; > > + > > + if (!kvm_riscv_delta_cycles2ns(vstimecmp_val, gt, t) || > > + kvm_riscv_vcpu_has_interrupts(vcpu, 1UL << IRQ_VS_TIMER)) > > + return true; > > + else > > + return false; > > +} > > + > > +static void kvm_riscv_vcpu_timer_blocking(struct kvm_vcpu *vcpu) > > +{ > > + struct kvm_vcpu_timer *t =3D &vcpu->arch.timer; > > + struct kvm_guest_timer *gt =3D &vcpu->kvm->arch.timer; > > + u64 delta_ns; > > + u64 vstimecmp_val =3D vcpu->arch.guest_csr.vstimecmp; > > + > > + if (!t->init_done) > > + return; > > + > > + delta_ns =3D kvm_riscv_delta_cycles2ns(vstimecmp_val, gt, t); > > + if (delta_ns) { > > + t->next_cycles =3D vstimecmp_val; > > + hrtimer_start(&t->hrt, ktime_set(0, delta_ns), HRTIMER_MO= DE_REL); > > + t->next_set =3D true; > > + } > > +} > > + > > +static void kvm_riscv_vcpu_timer_unblocking(struct kvm_vcpu *vcpu) > > +{ > > + kvm_riscv_vcpu_timer_cancel(&vcpu->arch.timer); > > +} > > + > > int kvm_riscv_vcpu_get_reg_timer(struct kvm_vcpu *vcpu, > > const struct kvm_one_reg *reg) > > { > > @@ -180,10 +253,20 @@ int kvm_riscv_vcpu_timer_init(struct kvm_vcpu *vc= pu) > > return -EINVAL; > > > > hrtimer_init(&t->hrt, CLOCK_MONOTONIC, HRTIMER_MODE_REL); > > - t->hrt.function =3D kvm_riscv_vcpu_hrtimer_expired; > > t->init_done =3D true; > > t->next_set =3D false; > > > > + /* Enable sstc for every vcpu if available in hardware */ > > + if (riscv_isa_extension_available(NULL, SSTC)) { > > + t->sstc_enabled =3D true; > > + t->hrt.function =3D kvm_riscv_vcpu_vstimer_expired; > > + t->timer_next_event =3D kvm_riscv_vcpu_update_vstimecmp; > > + } else { > > + t->sstc_enabled =3D false; > > + t->hrt.function =3D kvm_riscv_vcpu_hrtimer_expired; > > + t->timer_next_event =3D kvm_riscv_vcpu_update_hrtimer; > > + } > > + > > return 0; > > } > > > > @@ -202,7 +285,7 @@ int kvm_riscv_vcpu_timer_reset(struct kvm_vcpu *vcp= u) > > return kvm_riscv_vcpu_timer_cancel(&vcpu->arch.timer); > > } > > > > -void kvm_riscv_vcpu_timer_restore(struct kvm_vcpu *vcpu) > > +static void kvm_riscv_vcpu_update_timedelta(struct kvm_vcpu *vcpu) > > { > > struct kvm_guest_timer *gt =3D &vcpu->kvm->arch.timer; > > > > @@ -214,6 +297,55 @@ void kvm_riscv_vcpu_timer_restore(struct kvm_vcpu = *vcpu) > > #endif > > } > > > > +void kvm_riscv_vcpu_timer_restore(struct kvm_vcpu *vcpu) > > +{ > > + struct kvm_vcpu_csr *csr; > > + struct kvm_vcpu_timer *t =3D &vcpu->arch.timer; > > + > > + kvm_riscv_vcpu_update_timedelta(vcpu); > > + > > + if (!t->sstc_enabled) > > + return; > > + > > + csr =3D &vcpu->arch.guest_csr; > > +#ifdef CONFIG_64BIT > > + csr_write(CSR_VSTIMECMP, csr->vstimecmp); > > +#else > > + csr_write(CSR_VSTIMECMP, (u32)csr->vstimecmp); > > + csr_write(CSR_VSTIMECMPH, (u32)(csr->vstimecmp >> 32)); > > +#endif > > + > > + /* timer should be enabled for the remaining operations */ > > + if (unlikely(!t->init_done)) > > + return; > > + > > + kvm_riscv_vcpu_timer_unblocking(vcpu); > > +} > > + > > +void kvm_riscv_vcpu_timer_save(struct kvm_vcpu *vcpu) > > +{ > > + struct kvm_vcpu_csr *csr; > > + struct kvm_vcpu_timer *t =3D &vcpu->arch.timer; > > + > > + if (!t->sstc_enabled) > > + return; > > + > > + csr =3D &vcpu->arch.guest_csr; > > + t =3D &vcpu->arch.timer; > > +#ifdef CONFIG_64BIT > > + csr->vstimecmp =3D csr_read(CSR_VSTIMECMP); > > +#else > > + csr->vstimecmp =3D csr_read(CSR_VSTIMECMP); > > + csr->vstimecmp |=3D (u64)csr_read(CSR_VSTIMECMPH) << 32; > > +#endif > > + /* timer should be enabled for the remaining operations */ > > + if (unlikely(!t->init_done)) > > + return; > > + > > + if (kvm_vcpu_is_blocking(vcpu)) > > + kvm_riscv_vcpu_timer_blocking(vcpu); > > +} > > + > > int kvm_riscv_guest_timer_init(struct kvm *kvm) > > { > > struct kvm_guest_timer *gt =3D &kvm->arch.timer; > > -- > > 2.30.2 > > > > > > _______________________________________________ > > linux-riscv mailing list > > linux-riscv@lists.infradead.org > > http://lists.infradead.org/mailman/listinfo/linux-riscv > -- Regards, Atish