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 X-Spam-Level: X-Spam-Status: No, score=-23.3 required=3.0 tests=BAYES_00,DKIMWL_WL_MED, DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS, INCLUDES_CR_TRAILER,INCLUDES_PATCH,MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS, USER_IN_DEF_DKIM_WL autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 8F106C433EF for ; Wed, 22 Sep 2021 04:37:52 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 61308611B0 for ; Wed, 22 Sep 2021 04:37:52 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S231366AbhIVEjU (ORCPT ); Wed, 22 Sep 2021 00:39:20 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:60600 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229495AbhIVEjS (ORCPT ); Wed, 22 Sep 2021 00:39:18 -0400 Received: from mail-pg1-x533.google.com (mail-pg1-x533.google.com [IPv6:2607:f8b0:4864:20::533]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id BE93AC061574 for ; Tue, 21 Sep 2021 21:37:48 -0700 (PDT) Received: by mail-pg1-x533.google.com with SMTP id r2so1332354pgl.10 for ; Tue, 21 Sep 2021 21:37:48 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20210112; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=m6kldB4rj/R0WAkH0imKrXheJ5V9FdbCmll402cf/IA=; b=ICo+Qb1xVrXPRSI7NnKutgqIbTVSjHefGoL4UTvWcdLzlKBJcZSOAiv37rrnqZo5Rz jNJVc+p3x+rdeAuuHjiG9hFl/ETgaYUUm8MAeg5KJi1Bk02G1yoSkpeyIWxy2d9wZAtw i8YENzp7fDKb9uovIf0iENUaGw66di9BTbJjh/Zo1wJKNAkzSX4udgaHcybfT3HuiKiF bYitJiAyGbWz/wX/nZy8zut9LS8vOk9eL2QPKDL1xRWvGwwX0T2+e/i/jly2XCsK1cpU sdeeADNM/l3nMY1rLbHvXxjuGnmm38PHeJOlwbdjlTi4jwxwQUNHex7hqfXsEoKMrZH6 MkhA== 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; bh=m6kldB4rj/R0WAkH0imKrXheJ5V9FdbCmll402cf/IA=; b=UIqYsYDm/ORQbjf4zSYZSOa34XE/S/bVRDTXvFF325X6imKLiQMv6cY+bcGaPYdHHR cf9svPpqCJxzyLkCBG3Jagkwmsbm5K0KrmFqYDTh+liwE2hSzMUTWSZojrgsQ5PCpqeI baGjROjUzSaDJXHy9L25RGj2nMGReq+7r6W/EFDawZPXZAGQmzkXSoGqN4Qqx5ARi5e7 P/gwKndBhHjctz1VZiG76Uj9mCQ6Z/du4u842ymZPJ7mblWmVFkIV6FWAtJ8U70/QEEx TI05raPiUMQ8twldCN5X+m+hIvATwPj/UkjtIHSOqrSRThRHMCDC6l4YE2UBCnLg8zuB 618w== X-Gm-Message-State: AOAM531uSgQLLqeaIWp1F2nyS25JnzLCFE7n1+3ohBYAdibWY6YlnceC qfrQTYyb4x+WehZqDClurkpS2kNnfyrlNq4P1TfsTw== X-Google-Smtp-Source: ABdhPJxyLcmXtJzqXIty5HMyLdZx4wO2ii7Xm2RUsWDJqS7lv9IkDE3r3XN2xqczjqfgfhUPLESfVblUgyopY3T/WXI= X-Received: by 2002:a63:f80a:: with SMTP id n10mr30957910pgh.303.1632285467778; Tue, 21 Sep 2021 21:37:47 -0700 (PDT) MIME-Version: 1.0 References: <20210916181510.963449-1-oupton@google.com> <20210916181510.963449-3-oupton@google.com> In-Reply-To: <20210916181510.963449-3-oupton@google.com> From: Reiji Watanabe Date: Tue, 21 Sep 2021 21:37:31 -0700 Message-ID: Subject: Re: [PATCH v8 2/8] KVM: arm64: Separate guest/host counter offset values To: Oliver Upton Cc: kvm@vger.kernel.org, kvmarm@lists.cs.columbia.edu, Catalin Marinas , Will Deacon , Marc Zyngier , Peter Shier , Sean Christopherson , David Matlack , Paolo Bonzini , linux-arm-kernel@lists.infradead.org, Jim Mattson Content-Type: text/plain; charset="UTF-8" Precedence: bulk List-ID: X-Mailing-List: kvm@vger.kernel.org Hi Oliver, On Thu, Sep 16, 2021 at 11:15 AM Oliver Upton wrote: > > In some instances, a VMM may want to update the guest's counter-timer > offset in a transparent manner, meaning that changes to the hardware > value do not affect the synthetic register presented to the guest or the > VMM through said guest's architectural state. Lay the groundwork to > separate guest offset register writes from the hardware values utilized > by KVM. > > Signed-off-by: Oliver Upton > Reviewed-by: Andrew Jones > --- > arch/arm64/kvm/arch_timer.c | 42 +++++++++++++++++++++++++++--------- > include/kvm/arm_arch_timer.h | 3 +++ > 2 files changed, 35 insertions(+), 10 deletions(-) > > diff --git a/arch/arm64/kvm/arch_timer.c b/arch/arm64/kvm/arch_timer.c > index c0101db75ad4..cf2f4a034dbe 100644 > --- a/arch/arm64/kvm/arch_timer.c > +++ b/arch/arm64/kvm/arch_timer.c > @@ -84,11 +84,9 @@ u64 timer_get_cval(struct arch_timer_context *ctxt) > > static u64 timer_get_offset(struct arch_timer_context *ctxt) > { > - struct kvm_vcpu *vcpu = ctxt->vcpu; > - > switch(arch_timer_ctx_index(ctxt)) { > case TIMER_VTIMER: > - return __vcpu_sys_reg(vcpu, CNTVOFF_EL2); > + return ctxt->host_offset; > default: > return 0; > } > @@ -128,17 +126,33 @@ static void timer_set_cval(struct arch_timer_context *ctxt, u64 cval) > > static void timer_set_offset(struct arch_timer_context *ctxt, u64 offset) > { > - struct kvm_vcpu *vcpu = ctxt->vcpu; > - > switch(arch_timer_ctx_index(ctxt)) { > case TIMER_VTIMER: > - __vcpu_sys_reg(vcpu, CNTVOFF_EL2) = offset; > + ctxt->host_offset = offset; > break; > default: > WARN(offset, "timer %ld\n", arch_timer_ctx_index(ctxt)); > } > } > > +static void timer_set_guest_offset(struct arch_timer_context *ctxt, u64 offset) > +{ > + struct kvm_vcpu *vcpu = ctxt->vcpu; > + > + switch (arch_timer_ctx_index(ctxt)) { > + case TIMER_VTIMER: { > + u64 host_offset = timer_get_offset(ctxt); > + > + host_offset += offset - __vcpu_sys_reg(vcpu, CNTVOFF_EL2); > + __vcpu_sys_reg(vcpu, CNTVOFF_EL2) = offset; > + timer_set_offset(ctxt, host_offset); > + break; > + } > + default: > + WARN_ONCE(offset, "timer %ld\n", arch_timer_ctx_index(ctxt)); > + } > +} > + > u64 kvm_phys_timer_read(void) > { > return timecounter->cc->read(timecounter->cc); > @@ -749,7 +763,8 @@ int kvm_timer_vcpu_reset(struct kvm_vcpu *vcpu) > > /* Make offset updates for all timer contexts atomic */ > static void update_timer_offset(struct kvm_vcpu *vcpu, > - enum kvm_arch_timers timer, u64 offset) > + enum kvm_arch_timers timer, u64 offset, > + bool guest_visible) The name 'guest_visible' looks confusing to me because it also affects the type of the 'offset' that its caller needs to specify. (The 'offset' must be an offset from the guest's physical counter if 'guest_visible' == true, and an offset from the host's virtual counter otherwise.) Having said that, I don't have a good alternative name for it though... IMHO, 'is_host_offset' would be less confusing because it indicates what the caller needs to specify. > { > int i; > struct kvm *kvm = vcpu->kvm; > @@ -758,13 +773,20 @@ static void update_timer_offset(struct kvm_vcpu *vcpu, > lockdep_assert_held(&kvm->lock); > > kvm_for_each_vcpu(i, tmp, kvm) > - timer_set_offset(vcpu_get_timer(tmp, timer), offset); > + if (guest_visible) > + timer_set_guest_offset(vcpu_get_timer(tmp, timer), > + offset); > + else > + timer_set_offset(vcpu_get_timer(tmp, timer), offset); > > /* > * When called from the vcpu create path, the CPU being created is not > * included in the loop above, so we just set it here as well. > */ > - timer_set_offset(vcpu_get_timer(vcpu, timer), offset); > + if (guest_visible) > + timer_set_guest_offset(vcpu_get_timer(vcpu, timer), offset); > + else > + timer_set_offset(vcpu_get_timer(vcpu, timer), offset); > } > > static void update_vtimer_cntvoff(struct kvm_vcpu *vcpu, u64 cntvoff) > @@ -772,7 +794,7 @@ static void update_vtimer_cntvoff(struct kvm_vcpu *vcpu, u64 cntvoff) > struct kvm *kvm = vcpu->kvm; > > mutex_lock(&kvm->lock); > - update_timer_offset(vcpu, TIMER_VTIMER, cntvoff); > + update_timer_offset(vcpu, TIMER_VTIMER, cntvoff, true); > mutex_unlock(&kvm->lock); > } > > diff --git a/include/kvm/arm_arch_timer.h b/include/kvm/arm_arch_timer.h > index 51c19381108c..9d65d4a29f81 100644 > --- a/include/kvm/arm_arch_timer.h > +++ b/include/kvm/arm_arch_timer.h > @@ -42,6 +42,9 @@ struct arch_timer_context { > /* Duplicated state from arch_timer.c for convenience */ > u32 host_timer_irq; > u32 host_timer_irq_flags; > + > + /* offset relative to the host's physical counter-timer */ > + u64 host_offset; > }; Just out of curiosity, have you considered having 'host_offset' in one place (in arch_timer_cpu or somewhere ?) as physical offset rather than having them separately for each arch_timer_context ? I would think that could simplify the offset calculation code and update_ptimer_cntpoff() doesn't need to call update_timer_offset() for TIMER_VTIMER. It would require extra memory accesses for timer_get_offset(TIMER_VTIMER) though... Otherwise, Reviewed-by: Reiji Watanabe Thanks, Reiji 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 X-Spam-Level: X-Spam-Status: No, score=-13.5 required=3.0 tests=BAYES_00, DKIM_ADSP_CUSTOM_MED,DKIM_INVALID,DKIM_SIGNED,HEADER_FROM_DIFFERENT_DOMAINS, INCLUDES_CR_TRAILER,INCLUDES_PATCH,MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS autolearn=unavailable autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 3FAA3C433FE for ; Wed, 22 Sep 2021 04:37:53 +0000 (UTC) Received: from mm01.cs.columbia.edu (mm01.cs.columbia.edu [128.59.11.253]) by mail.kernel.org (Postfix) with ESMTP id AFD3261206 for ; Wed, 22 Sep 2021 04:37:52 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.4.1 mail.kernel.org AFD3261206 Authentication-Results: mail.kernel.org; dmarc=fail (p=reject dis=none) header.from=google.com Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=lists.cs.columbia.edu Received: from localhost (localhost [127.0.0.1]) by mm01.cs.columbia.edu (Postfix) with ESMTP id 283EF4A173; Wed, 22 Sep 2021 00:37:52 -0400 (EDT) X-Virus-Scanned: at lists.cs.columbia.edu Authentication-Results: mm01.cs.columbia.edu (amavisd-new); dkim=softfail (fail, message has been altered) header.i=@google.com Received: from mm01.cs.columbia.edu ([127.0.0.1]) by localhost (mm01.cs.columbia.edu [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id 8lGlICOaKVj0; Wed, 22 Sep 2021 00:37:50 -0400 (EDT) Received: from mm01.cs.columbia.edu (localhost [127.0.0.1]) by mm01.cs.columbia.edu (Postfix) with ESMTP id CD0834A193; Wed, 22 Sep 2021 00:37:50 -0400 (EDT) Received: from localhost (localhost [127.0.0.1]) by mm01.cs.columbia.edu (Postfix) with ESMTP id 536A34A00B for ; Wed, 22 Sep 2021 00:37:50 -0400 (EDT) X-Virus-Scanned: at lists.cs.columbia.edu Received: from mm01.cs.columbia.edu ([127.0.0.1]) by localhost (mm01.cs.columbia.edu [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id OiCYt-Uaebvt for ; Wed, 22 Sep 2021 00:37:49 -0400 (EDT) Received: from mail-pg1-f178.google.com (mail-pg1-f178.google.com [209.85.215.178]) by mm01.cs.columbia.edu (Postfix) with ESMTPS id 2244749F6C for ; Wed, 22 Sep 2021 00:37:49 -0400 (EDT) Received: by mail-pg1-f178.google.com with SMTP id q68so1341073pga.9 for ; Tue, 21 Sep 2021 21:37:49 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20210112; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=m6kldB4rj/R0WAkH0imKrXheJ5V9FdbCmll402cf/IA=; b=ICo+Qb1xVrXPRSI7NnKutgqIbTVSjHefGoL4UTvWcdLzlKBJcZSOAiv37rrnqZo5Rz jNJVc+p3x+rdeAuuHjiG9hFl/ETgaYUUm8MAeg5KJi1Bk02G1yoSkpeyIWxy2d9wZAtw i8YENzp7fDKb9uovIf0iENUaGw66di9BTbJjh/Zo1wJKNAkzSX4udgaHcybfT3HuiKiF bYitJiAyGbWz/wX/nZy8zut9LS8vOk9eL2QPKDL1xRWvGwwX0T2+e/i/jly2XCsK1cpU sdeeADNM/l3nMY1rLbHvXxjuGnmm38PHeJOlwbdjlTi4jwxwQUNHex7hqfXsEoKMrZH6 MkhA== 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; bh=m6kldB4rj/R0WAkH0imKrXheJ5V9FdbCmll402cf/IA=; b=jNKULsaLCD4VVAXfoPwYume/35GSAf+uoM6WhKBZn02K9CGdZKMlTJrx/KfBi2pd1v 4SG1POXbYxsVsXPM3d3DAixuOyX1g7WcwFJZxTOeSn6qsWC50Advq9LMKJ+pptyPXgtn YFyBYkwgCrBW94QWM4GHP/MVBRC+HbA8vHElbvt0GO/cmrDE76Ugu4W8e5U1L0U7WLbx hSAHrjp3gf6cHpfgcn4nouxXtvKCs2r8ogk+uVgYlWp9xupLv0zUzqllG6hK6mZZ7GkJ dLBX7C1CGVT5yoSHRQPZnFJa5XXYORChJs17V32Ukw3aJEzyOYlXZqq6YpT52f7dRU0x +N8w== X-Gm-Message-State: AOAM531sidwYa1GxJlfKAZppJKRUZGNfj1bs5kDDgcHn+S67qm0v7TKT Zu2V9ijWets8+Zrd6CwkwXzAuwspEAN+5z7nyjdY1Q== X-Google-Smtp-Source: ABdhPJxyLcmXtJzqXIty5HMyLdZx4wO2ii7Xm2RUsWDJqS7lv9IkDE3r3XN2xqczjqfgfhUPLESfVblUgyopY3T/WXI= X-Received: by 2002:a63:f80a:: with SMTP id n10mr30957910pgh.303.1632285467778; Tue, 21 Sep 2021 21:37:47 -0700 (PDT) MIME-Version: 1.0 References: <20210916181510.963449-1-oupton@google.com> <20210916181510.963449-3-oupton@google.com> In-Reply-To: <20210916181510.963449-3-oupton@google.com> From: Reiji Watanabe Date: Tue, 21 Sep 2021 21:37:31 -0700 Message-ID: Subject: Re: [PATCH v8 2/8] KVM: arm64: Separate guest/host counter offset values To: Oliver Upton Cc: kvm@vger.kernel.org, Catalin Marinas , Sean Christopherson , Peter Shier , Marc Zyngier , David Matlack , Paolo Bonzini , Will Deacon , kvmarm@lists.cs.columbia.edu, linux-arm-kernel@lists.infradead.org, Jim Mattson X-BeenThere: kvmarm@lists.cs.columbia.edu X-Mailman-Version: 2.1.14 Precedence: list List-Id: Where KVM/ARM decisions are made List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Errors-To: kvmarm-bounces@lists.cs.columbia.edu Sender: kvmarm-bounces@lists.cs.columbia.edu Hi Oliver, On Thu, Sep 16, 2021 at 11:15 AM Oliver Upton wrote: > > In some instances, a VMM may want to update the guest's counter-timer > offset in a transparent manner, meaning that changes to the hardware > value do not affect the synthetic register presented to the guest or the > VMM through said guest's architectural state. Lay the groundwork to > separate guest offset register writes from the hardware values utilized > by KVM. > > Signed-off-by: Oliver Upton > Reviewed-by: Andrew Jones > --- > arch/arm64/kvm/arch_timer.c | 42 +++++++++++++++++++++++++++--------- > include/kvm/arm_arch_timer.h | 3 +++ > 2 files changed, 35 insertions(+), 10 deletions(-) > > diff --git a/arch/arm64/kvm/arch_timer.c b/arch/arm64/kvm/arch_timer.c > index c0101db75ad4..cf2f4a034dbe 100644 > --- a/arch/arm64/kvm/arch_timer.c > +++ b/arch/arm64/kvm/arch_timer.c > @@ -84,11 +84,9 @@ u64 timer_get_cval(struct arch_timer_context *ctxt) > > static u64 timer_get_offset(struct arch_timer_context *ctxt) > { > - struct kvm_vcpu *vcpu = ctxt->vcpu; > - > switch(arch_timer_ctx_index(ctxt)) { > case TIMER_VTIMER: > - return __vcpu_sys_reg(vcpu, CNTVOFF_EL2); > + return ctxt->host_offset; > default: > return 0; > } > @@ -128,17 +126,33 @@ static void timer_set_cval(struct arch_timer_context *ctxt, u64 cval) > > static void timer_set_offset(struct arch_timer_context *ctxt, u64 offset) > { > - struct kvm_vcpu *vcpu = ctxt->vcpu; > - > switch(arch_timer_ctx_index(ctxt)) { > case TIMER_VTIMER: > - __vcpu_sys_reg(vcpu, CNTVOFF_EL2) = offset; > + ctxt->host_offset = offset; > break; > default: > WARN(offset, "timer %ld\n", arch_timer_ctx_index(ctxt)); > } > } > > +static void timer_set_guest_offset(struct arch_timer_context *ctxt, u64 offset) > +{ > + struct kvm_vcpu *vcpu = ctxt->vcpu; > + > + switch (arch_timer_ctx_index(ctxt)) { > + case TIMER_VTIMER: { > + u64 host_offset = timer_get_offset(ctxt); > + > + host_offset += offset - __vcpu_sys_reg(vcpu, CNTVOFF_EL2); > + __vcpu_sys_reg(vcpu, CNTVOFF_EL2) = offset; > + timer_set_offset(ctxt, host_offset); > + break; > + } > + default: > + WARN_ONCE(offset, "timer %ld\n", arch_timer_ctx_index(ctxt)); > + } > +} > + > u64 kvm_phys_timer_read(void) > { > return timecounter->cc->read(timecounter->cc); > @@ -749,7 +763,8 @@ int kvm_timer_vcpu_reset(struct kvm_vcpu *vcpu) > > /* Make offset updates for all timer contexts atomic */ > static void update_timer_offset(struct kvm_vcpu *vcpu, > - enum kvm_arch_timers timer, u64 offset) > + enum kvm_arch_timers timer, u64 offset, > + bool guest_visible) The name 'guest_visible' looks confusing to me because it also affects the type of the 'offset' that its caller needs to specify. (The 'offset' must be an offset from the guest's physical counter if 'guest_visible' == true, and an offset from the host's virtual counter otherwise.) Having said that, I don't have a good alternative name for it though... IMHO, 'is_host_offset' would be less confusing because it indicates what the caller needs to specify. > { > int i; > struct kvm *kvm = vcpu->kvm; > @@ -758,13 +773,20 @@ static void update_timer_offset(struct kvm_vcpu *vcpu, > lockdep_assert_held(&kvm->lock); > > kvm_for_each_vcpu(i, tmp, kvm) > - timer_set_offset(vcpu_get_timer(tmp, timer), offset); > + if (guest_visible) > + timer_set_guest_offset(vcpu_get_timer(tmp, timer), > + offset); > + else > + timer_set_offset(vcpu_get_timer(tmp, timer), offset); > > /* > * When called from the vcpu create path, the CPU being created is not > * included in the loop above, so we just set it here as well. > */ > - timer_set_offset(vcpu_get_timer(vcpu, timer), offset); > + if (guest_visible) > + timer_set_guest_offset(vcpu_get_timer(vcpu, timer), offset); > + else > + timer_set_offset(vcpu_get_timer(vcpu, timer), offset); > } > > static void update_vtimer_cntvoff(struct kvm_vcpu *vcpu, u64 cntvoff) > @@ -772,7 +794,7 @@ static void update_vtimer_cntvoff(struct kvm_vcpu *vcpu, u64 cntvoff) > struct kvm *kvm = vcpu->kvm; > > mutex_lock(&kvm->lock); > - update_timer_offset(vcpu, TIMER_VTIMER, cntvoff); > + update_timer_offset(vcpu, TIMER_VTIMER, cntvoff, true); > mutex_unlock(&kvm->lock); > } > > diff --git a/include/kvm/arm_arch_timer.h b/include/kvm/arm_arch_timer.h > index 51c19381108c..9d65d4a29f81 100644 > --- a/include/kvm/arm_arch_timer.h > +++ b/include/kvm/arm_arch_timer.h > @@ -42,6 +42,9 @@ struct arch_timer_context { > /* Duplicated state from arch_timer.c for convenience */ > u32 host_timer_irq; > u32 host_timer_irq_flags; > + > + /* offset relative to the host's physical counter-timer */ > + u64 host_offset; > }; Just out of curiosity, have you considered having 'host_offset' in one place (in arch_timer_cpu or somewhere ?) as physical offset rather than having them separately for each arch_timer_context ? I would think that could simplify the offset calculation code and update_ptimer_cntpoff() doesn't need to call update_timer_offset() for TIMER_VTIMER. It would require extra memory accesses for timer_get_offset(TIMER_VTIMER) though... Otherwise, Reviewed-by: Reiji Watanabe Thanks, Reiji _______________________________________________ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm 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 X-Spam-Level: X-Spam-Status: No, score=-15.2 required=3.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_ADSP_CUSTOM_MED,DKIM_SIGNED,DKIM_VALID,HEADER_FROM_DIFFERENT_DOMAINS, INCLUDES_CR_TRAILER,INCLUDES_PATCH,MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS autolearn=unavailable autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 7DBE7C433EF for ; Wed, 22 Sep 2021 04:39:43 +0000 (UTC) Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 3229561156 for ; Wed, 22 Sep 2021 04:39:43 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.4.1 mail.kernel.org 3229561156 Authentication-Results: mail.kernel.org; dmarc=fail (p=reject dis=none) header.from=google.com Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=lists.infradead.org DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender: Content-Transfer-Encoding:Content-Type:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:Cc:To:Subject:Message-ID:Date:From: In-Reply-To:References:MIME-Version:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=azgax6H9Mw+bZJ73rOmjLpUzf8wlkKxUnMdIiIsvJPA=; b=v2gCNloi7kVrrc 0O1FmVofve3RY3ZNOZ9hNQbCG8T8fd8UqwsXhkw97xQnvDwygHphsIRO5YL9cFY/64M7oFKXj+HYa bb0XbMdTQ3qt/j9gsiG2yiRAejWIIp9pu2UpG7yJaXHD2AFj2haI3jWARxlNbY1PywU61Eg4/Eyxr v9/zuZpOS5ZZdSlaSmb4bHKLTbUwmSociZEyevDNdukuCwqBoYjC9X9VnSu+JxZJFa3KlCqnJJIkc jG3FyqW7HSm6k0UPvFU9nlaBLSBG/Avz7KbEvLu8JaF0Iv+jJo/6oBhTtMkwTQQpsJ9VgbET+3dBZ HDKn54UqxbxOkzQ83QQA==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.94.2 #2 (Red Hat Linux)) id 1mSu16-006tsJ-BS; Wed, 22 Sep 2021 04:37:56 +0000 Received: from mail-pg1-x532.google.com ([2607:f8b0:4864:20::532]) by bombadil.infradead.org with esmtps (Exim 4.94.2 #2 (Red Hat Linux)) id 1mSu12-006trc-Gs for linux-arm-kernel@lists.infradead.org; Wed, 22 Sep 2021 04:37:54 +0000 Received: by mail-pg1-x532.google.com with SMTP id g184so1362623pgc.6 for ; Tue, 21 Sep 2021 21:37:48 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20210112; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=m6kldB4rj/R0WAkH0imKrXheJ5V9FdbCmll402cf/IA=; b=ICo+Qb1xVrXPRSI7NnKutgqIbTVSjHefGoL4UTvWcdLzlKBJcZSOAiv37rrnqZo5Rz jNJVc+p3x+rdeAuuHjiG9hFl/ETgaYUUm8MAeg5KJi1Bk02G1yoSkpeyIWxy2d9wZAtw i8YENzp7fDKb9uovIf0iENUaGw66di9BTbJjh/Zo1wJKNAkzSX4udgaHcybfT3HuiKiF bYitJiAyGbWz/wX/nZy8zut9LS8vOk9eL2QPKDL1xRWvGwwX0T2+e/i/jly2XCsK1cpU sdeeADNM/l3nMY1rLbHvXxjuGnmm38PHeJOlwbdjlTi4jwxwQUNHex7hqfXsEoKMrZH6 MkhA== 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; bh=m6kldB4rj/R0WAkH0imKrXheJ5V9FdbCmll402cf/IA=; b=fnBjAeku4Uts46cjAk426BRVQUxume5O0bdLTPOaNdr81tjZnbwoZmb7fwtqi7pnyU rUp5luUsN+lBT2wcLFtAtgsqwCVws7vfW2tcB49gNk1YioJocogb+zvaoHlu5bfQPCPR hxC7MUKzEKwwu6pgd3b/CXm7p/5uMjXlbFsQI/DSRb+et6c7hhOIwYaMJbrh2wphbtto +UGwRg/KVK/GtyvFGEPIvSeh/o0f6OZNsKsKRFb1k7KcVFMkvoUtB29TECZW77dwujad +MowHWoOauFuzrkIUtaRE0TAaTCaprugdYzPJbap+BI+G+yifQ3FiIf7f2ihTeS8OOxu eAYw== X-Gm-Message-State: AOAM533B6FQxYP8LPIkJ3j49qnNxCN3BE8R0Nu9KJg/fPNh608mBkfsG VCaijGuJT1IQXuEcUhCP6zJu3r04o7TScKLl6/OY2w== X-Google-Smtp-Source: ABdhPJxyLcmXtJzqXIty5HMyLdZx4wO2ii7Xm2RUsWDJqS7lv9IkDE3r3XN2xqczjqfgfhUPLESfVblUgyopY3T/WXI= X-Received: by 2002:a63:f80a:: with SMTP id n10mr30957910pgh.303.1632285467778; Tue, 21 Sep 2021 21:37:47 -0700 (PDT) MIME-Version: 1.0 References: <20210916181510.963449-1-oupton@google.com> <20210916181510.963449-3-oupton@google.com> In-Reply-To: <20210916181510.963449-3-oupton@google.com> From: Reiji Watanabe Date: Tue, 21 Sep 2021 21:37:31 -0700 Message-ID: Subject: Re: [PATCH v8 2/8] KVM: arm64: Separate guest/host counter offset values To: Oliver Upton Cc: kvm@vger.kernel.org, kvmarm@lists.cs.columbia.edu, Catalin Marinas , Will Deacon , Marc Zyngier , Peter Shier , Sean Christopherson , David Matlack , Paolo Bonzini , linux-arm-kernel@lists.infradead.org, Jim Mattson X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20210921_213752_626127_55180C0B X-CRM114-Status: GOOD ( 28.80 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org Hi Oliver, On Thu, Sep 16, 2021 at 11:15 AM Oliver Upton wrote: > > In some instances, a VMM may want to update the guest's counter-timer > offset in a transparent manner, meaning that changes to the hardware > value do not affect the synthetic register presented to the guest or the > VMM through said guest's architectural state. Lay the groundwork to > separate guest offset register writes from the hardware values utilized > by KVM. > > Signed-off-by: Oliver Upton > Reviewed-by: Andrew Jones > --- > arch/arm64/kvm/arch_timer.c | 42 +++++++++++++++++++++++++++--------- > include/kvm/arm_arch_timer.h | 3 +++ > 2 files changed, 35 insertions(+), 10 deletions(-) > > diff --git a/arch/arm64/kvm/arch_timer.c b/arch/arm64/kvm/arch_timer.c > index c0101db75ad4..cf2f4a034dbe 100644 > --- a/arch/arm64/kvm/arch_timer.c > +++ b/arch/arm64/kvm/arch_timer.c > @@ -84,11 +84,9 @@ u64 timer_get_cval(struct arch_timer_context *ctxt) > > static u64 timer_get_offset(struct arch_timer_context *ctxt) > { > - struct kvm_vcpu *vcpu = ctxt->vcpu; > - > switch(arch_timer_ctx_index(ctxt)) { > case TIMER_VTIMER: > - return __vcpu_sys_reg(vcpu, CNTVOFF_EL2); > + return ctxt->host_offset; > default: > return 0; > } > @@ -128,17 +126,33 @@ static void timer_set_cval(struct arch_timer_context *ctxt, u64 cval) > > static void timer_set_offset(struct arch_timer_context *ctxt, u64 offset) > { > - struct kvm_vcpu *vcpu = ctxt->vcpu; > - > switch(arch_timer_ctx_index(ctxt)) { > case TIMER_VTIMER: > - __vcpu_sys_reg(vcpu, CNTVOFF_EL2) = offset; > + ctxt->host_offset = offset; > break; > default: > WARN(offset, "timer %ld\n", arch_timer_ctx_index(ctxt)); > } > } > > +static void timer_set_guest_offset(struct arch_timer_context *ctxt, u64 offset) > +{ > + struct kvm_vcpu *vcpu = ctxt->vcpu; > + > + switch (arch_timer_ctx_index(ctxt)) { > + case TIMER_VTIMER: { > + u64 host_offset = timer_get_offset(ctxt); > + > + host_offset += offset - __vcpu_sys_reg(vcpu, CNTVOFF_EL2); > + __vcpu_sys_reg(vcpu, CNTVOFF_EL2) = offset; > + timer_set_offset(ctxt, host_offset); > + break; > + } > + default: > + WARN_ONCE(offset, "timer %ld\n", arch_timer_ctx_index(ctxt)); > + } > +} > + > u64 kvm_phys_timer_read(void) > { > return timecounter->cc->read(timecounter->cc); > @@ -749,7 +763,8 @@ int kvm_timer_vcpu_reset(struct kvm_vcpu *vcpu) > > /* Make offset updates for all timer contexts atomic */ > static void update_timer_offset(struct kvm_vcpu *vcpu, > - enum kvm_arch_timers timer, u64 offset) > + enum kvm_arch_timers timer, u64 offset, > + bool guest_visible) The name 'guest_visible' looks confusing to me because it also affects the type of the 'offset' that its caller needs to specify. (The 'offset' must be an offset from the guest's physical counter if 'guest_visible' == true, and an offset from the host's virtual counter otherwise.) Having said that, I don't have a good alternative name for it though... IMHO, 'is_host_offset' would be less confusing because it indicates what the caller needs to specify. > { > int i; > struct kvm *kvm = vcpu->kvm; > @@ -758,13 +773,20 @@ static void update_timer_offset(struct kvm_vcpu *vcpu, > lockdep_assert_held(&kvm->lock); > > kvm_for_each_vcpu(i, tmp, kvm) > - timer_set_offset(vcpu_get_timer(tmp, timer), offset); > + if (guest_visible) > + timer_set_guest_offset(vcpu_get_timer(tmp, timer), > + offset); > + else > + timer_set_offset(vcpu_get_timer(tmp, timer), offset); > > /* > * When called from the vcpu create path, the CPU being created is not > * included in the loop above, so we just set it here as well. > */ > - timer_set_offset(vcpu_get_timer(vcpu, timer), offset); > + if (guest_visible) > + timer_set_guest_offset(vcpu_get_timer(vcpu, timer), offset); > + else > + timer_set_offset(vcpu_get_timer(vcpu, timer), offset); > } > > static void update_vtimer_cntvoff(struct kvm_vcpu *vcpu, u64 cntvoff) > @@ -772,7 +794,7 @@ static void update_vtimer_cntvoff(struct kvm_vcpu *vcpu, u64 cntvoff) > struct kvm *kvm = vcpu->kvm; > > mutex_lock(&kvm->lock); > - update_timer_offset(vcpu, TIMER_VTIMER, cntvoff); > + update_timer_offset(vcpu, TIMER_VTIMER, cntvoff, true); > mutex_unlock(&kvm->lock); > } > > diff --git a/include/kvm/arm_arch_timer.h b/include/kvm/arm_arch_timer.h > index 51c19381108c..9d65d4a29f81 100644 > --- a/include/kvm/arm_arch_timer.h > +++ b/include/kvm/arm_arch_timer.h > @@ -42,6 +42,9 @@ struct arch_timer_context { > /* Duplicated state from arch_timer.c for convenience */ > u32 host_timer_irq; > u32 host_timer_irq_flags; > + > + /* offset relative to the host's physical counter-timer */ > + u64 host_offset; > }; Just out of curiosity, have you considered having 'host_offset' in one place (in arch_timer_cpu or somewhere ?) as physical offset rather than having them separately for each arch_timer_context ? I would think that could simplify the offset calculation code and update_ptimer_cntpoff() doesn't need to call update_timer_offset() for TIMER_VTIMER. It would require extra memory accesses for timer_get_offset(TIMER_VTIMER) though... Otherwise, Reviewed-by: Reiji Watanabe Thanks, Reiji _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel