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=-18.3 required=3.0 tests=BAYES_00,DKIMWL_WL_MED, DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,FSL_HELO_FAKE, HEADER_FROM_DIFFERENT_DOMAINS,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 80EA3C433EF for ; Wed, 22 Sep 2021 14:44:33 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 63B89611CA for ; Wed, 22 Sep 2021 14:44:33 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S236199AbhIVOqC (ORCPT ); Wed, 22 Sep 2021 10:46:02 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:33224 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S236164AbhIVOqB (ORCPT ); Wed, 22 Sep 2021 10:46:01 -0400 Received: from mail-pj1-x1034.google.com (mail-pj1-x1034.google.com [IPv6:2607:f8b0:4864:20::1034]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id CFDD4C061574 for ; Wed, 22 Sep 2021 07:44:31 -0700 (PDT) Received: by mail-pj1-x1034.google.com with SMTP id nn5-20020a17090b38c500b0019af1c4b31fso2471829pjb.3 for ; Wed, 22 Sep 2021 07:44:31 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20210112; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to; bh=k92i1CTT4UfLIvqoj9NDLUAGbjRSVGtjE0AGo1vSZmQ=; b=f4lwilm8F32Il0kw2q+o2S5gn8bX330s+QqjMwYgJsMXJx2nLKL8EyWd3+MfL2K9jX 0tGZwNvTXYGj36wKXNRWv7f2/8WtblkGM8adydd7zCEXJGRgzSafRw0/O5bJe3dDxQQ7 U6i6z0ifislGQqzGxuWjmB4KG9fM9SillTMfXqaBzcnu4xfTxwQwb9DhTuwjk8oShc1N jmvJCl5gNVv2BNfFOnt3eTgLyXM+NbWeomDRALyfRFD5e79waLuLKw5bLUrghLsJW9+c 416hVoGg/lJKH5l5wbmxWkJQPlcGZB1pD/Mw8bTjlh40CsuIN/vh+c8yxxUJ2SyTVGzx o6KQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to; bh=k92i1CTT4UfLIvqoj9NDLUAGbjRSVGtjE0AGo1vSZmQ=; b=SRkQktL0mrNPaqG3lLBteX7BKfMOp11VL+4s9Wp8IQycA5m4Te0j/w+5pcWNQyfEg/ FolXforh3aACk9LsaJQFifChFOjGIyvhAbjitFMexykW1F30/FLWy/pyqhn32L4tZRci pg2qL+4LDn2t0FrKx2JHiiitNzuxcEGa7vhrp5NGiGmEwaGAc4MKwsQo6j65NjD/jC+C BikPiNmvn8ai6eu18iV7rU6hdu3aG0r+i6etXhL82hWDFsOH+XkibfzOe+WU59H1bQPO IPw+DepHuUp5d2mpXSbwNLQISgStyJwg6RH2EHbPivoRvT0C809RoVzA9xBiBpFI0At6 X5/g== X-Gm-Message-State: AOAM530UbesfufzKXBN+v2LAL8CUSnQ+BqYDXarZInMrOwPUdF6gm3OJ E7Csl9eofaYMJQl9+5o17Gn/IQ== X-Google-Smtp-Source: ABdhPJy6Cm8J5yFpuY0b3Q26cuBfzP0jR3SXJy8Sit4o8X0PGq0Nl30V3OZC+YaU8p9jHy4oX17FmA== X-Received: by 2002:a17:902:8606:b029:12c:2625:76cf with SMTP id f6-20020a1709028606b029012c262576cfmr223350plo.17.1632321871110; Wed, 22 Sep 2021 07:44:31 -0700 (PDT) Received: from google.com (157.214.185.35.bc.googleusercontent.com. [35.185.214.157]) by smtp.gmail.com with ESMTPSA id d14sm2901977pfq.61.2021.09.22.07.44.30 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 22 Sep 2021 07:44:30 -0700 (PDT) Date: Wed, 22 Sep 2021 14:44:26 +0000 From: Sean Christopherson To: Reiji Watanabe Cc: Oliver Upton , kvm@vger.kernel.org, kvmarm@lists.cs.columbia.edu, Catalin Marinas , Will Deacon , Marc Zyngier , Peter Shier , David Matlack , Paolo Bonzini , linux-arm-kernel@lists.infradead.org, Jim Mattson Subject: Re: [PATCH v8 2/8] KVM: arm64: Separate guest/host counter offset values Message-ID: References: <20210916181510.963449-1-oupton@google.com> <20210916181510.963449-3-oupton@google.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: Precedence: bulk List-ID: X-Mailing-List: kvm@vger.kernel.org On Tue, Sep 21, 2021, Reiji Watanabe wrote: > Hi Oliver, > > On Thu, Sep 16, 2021 at 11:15 AM Oliver Upton wrote: > > +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); Really getting into nitpicking territory, but it maybe name this timer_set_virtual_offset() (assuming v=virtual and p=physical). Based on the name, I expected this to set a variable literally named guest_offset, but it reads and writes host_offset. Maintaining the virtual vs. physical terminology all the way down avoids having direct host vs. guest naming conflicts, i.e. virtual and host aren't generally though of as mutually exclusive. > > + 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. I'd say ditch the param altogether and just have two separate helpers. Even in the final code, the callers all pass explicit 'true' or 'false', i.e. the callers can just as easily call a different function. Despite the near-identical code, smushing guest and host into the same function doesn't actually save much, just the function prototype and the local variables. That'd also avoid having to document/comment what 'true' and 'false' means at the call sites. > > { > > 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) This needs braces if you keep it as is. > > - timer_set_offset(vcpu_get_timer(tmp, timer), offset); > > + if (guest_visible) > > + timer_set_guest_offset(vcpu_get_timer(tmp, timer), > > + offset); Let this poke out, 84 chars isn't the end of the world. > > + 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. > > */ Any reason this can't be called from kvm_arch_vcpu_postcreate()? That'd eliminate the need for the extra handling. The vCPU is technically visible to userspace, but userspace would have to very intentionally do the wrong thing to cause problems, and I don't see any obviosu danger to the host. > > - 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); > > } > > 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=-10.2 required=3.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_ADSP_CUSTOM_MED,DKIM_SIGNED,DKIM_VALID,FSL_HELO_FAKE, HEADER_FROM_DIFFERENT_DOMAINS,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 8D096C433F5 for ; Wed, 22 Sep 2021 14:46:53 +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 4D1E461050 for ; Wed, 22 Sep 2021 14:46:53 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.4.1 mail.kernel.org 4D1E461050 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:In-Reply-To:MIME-Version:References: Message-ID:Subject:Cc:To:From:Date:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=XKcc3dMpPsBNL8xJDG5qWm8cig7unGuG8srAi7R5WvQ=; b=bJtQO6RVU4cZ+S E14PRWWEveP6Yz6bsboMnqy0kChL/GDcYyZgZ3oudTuS95j/FTWd/gZjx4AJUu2w64QCKwq8n7e/g 4xbsvJ25Ffua3dqvTnVdMJg9iDtiQ1rZ4y1d7XX5vr2iqJtr5qrdt+5hxuBiwSdqnli0ZkoYN3tie Rf3DMH7pFRjwRfydc76VT+CSUk3bOPo2BURY/otahfuYuU8pXIt088PJ3glzm8LY5T38eemv6aVtc FAKg9dMl1FMCCpqlIUlmvZjpq6a//asxaut9pveua0+vFMUccU/NcmO9IjbXp2RYVUUKEatyyaqLD QuoQSw3B4bTDBBknFxcg==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.94.2 #2 (Red Hat Linux)) id 1mT3UE-008riS-AT; Wed, 22 Sep 2021 14:44:38 +0000 Received: from mail-pj1-x1035.google.com ([2607:f8b0:4864:20::1035]) by bombadil.infradead.org with esmtps (Exim 4.94.2 #2 (Red Hat Linux)) id 1mT3UA-008ri5-Ny for linux-arm-kernel@lists.infradead.org; Wed, 22 Sep 2021 14:44:36 +0000 Received: by mail-pj1-x1035.google.com with SMTP id r7so1665119pjo.3 for ; Wed, 22 Sep 2021 07:44:31 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20210112; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to; bh=k92i1CTT4UfLIvqoj9NDLUAGbjRSVGtjE0AGo1vSZmQ=; b=f4lwilm8F32Il0kw2q+o2S5gn8bX330s+QqjMwYgJsMXJx2nLKL8EyWd3+MfL2K9jX 0tGZwNvTXYGj36wKXNRWv7f2/8WtblkGM8adydd7zCEXJGRgzSafRw0/O5bJe3dDxQQ7 U6i6z0ifislGQqzGxuWjmB4KG9fM9SillTMfXqaBzcnu4xfTxwQwb9DhTuwjk8oShc1N jmvJCl5gNVv2BNfFOnt3eTgLyXM+NbWeomDRALyfRFD5e79waLuLKw5bLUrghLsJW9+c 416hVoGg/lJKH5l5wbmxWkJQPlcGZB1pD/Mw8bTjlh40CsuIN/vh+c8yxxUJ2SyTVGzx o6KQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to; bh=k92i1CTT4UfLIvqoj9NDLUAGbjRSVGtjE0AGo1vSZmQ=; b=htwTm3qFrBzD/NP5ZMKWT9uFQYJlpkkEaaKUtR1jup2XZMzslc+uHd7M0Gn9NCpt4z W3SxEbj4tfYtuMp3SWvTDMhDw24yLWGen32A+c+kxmHKEji2fYxPrS38nED1kAA7ckoX 7lKfbcRS2e2QFEcxX8+eB0BtR19ZfviUjnedqiaQKLjoD5ANRuyR83RQR05Gh+hO8Jxf 5M5QN7opPApyi70o4zKRXkMVNZzxSDFSOaJeVUQRNXdvM0ATp9YAaksmMkMWAt/39bL3 0Yx19qkV0VBHevdk/Mt3t7PqIoItFckJ+XVZbUr2zRcvZOQRzuySKFfGPwOznZA1pz2E APQw== X-Gm-Message-State: AOAM532uu/4mCuFzQoRQzQdTlNj2+Ssqaw2a7+2hA+VLlfP6BoNIX27i h+DbPmccYEh05ZOF6TjEzhaxCA== X-Google-Smtp-Source: ABdhPJy6Cm8J5yFpuY0b3Q26cuBfzP0jR3SXJy8Sit4o8X0PGq0Nl30V3OZC+YaU8p9jHy4oX17FmA== X-Received: by 2002:a17:902:8606:b029:12c:2625:76cf with SMTP id f6-20020a1709028606b029012c262576cfmr223350plo.17.1632321871110; Wed, 22 Sep 2021 07:44:31 -0700 (PDT) Received: from google.com (157.214.185.35.bc.googleusercontent.com. [35.185.214.157]) by smtp.gmail.com with ESMTPSA id d14sm2901977pfq.61.2021.09.22.07.44.30 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 22 Sep 2021 07:44:30 -0700 (PDT) Date: Wed, 22 Sep 2021 14:44:26 +0000 From: Sean Christopherson To: Reiji Watanabe Cc: Oliver Upton , kvm@vger.kernel.org, kvmarm@lists.cs.columbia.edu, Catalin Marinas , Will Deacon , Marc Zyngier , Peter Shier , David Matlack , Paolo Bonzini , linux-arm-kernel@lists.infradead.org, Jim Mattson Subject: Re: [PATCH v8 2/8] KVM: arm64: Separate guest/host counter offset values Message-ID: References: <20210916181510.963449-1-oupton@google.com> <20210916181510.963449-3-oupton@google.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20210922_074434_833627_0E0FC628 X-CRM114-Status: GOOD ( 26.47 ) 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 On Tue, Sep 21, 2021, Reiji Watanabe wrote: > Hi Oliver, > > On Thu, Sep 16, 2021 at 11:15 AM Oliver Upton wrote: > > +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); Really getting into nitpicking territory, but it maybe name this timer_set_virtual_offset() (assuming v=virtual and p=physical). Based on the name, I expected this to set a variable literally named guest_offset, but it reads and writes host_offset. Maintaining the virtual vs. physical terminology all the way down avoids having direct host vs. guest naming conflicts, i.e. virtual and host aren't generally though of as mutually exclusive. > > + 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. I'd say ditch the param altogether and just have two separate helpers. Even in the final code, the callers all pass explicit 'true' or 'false', i.e. the callers can just as easily call a different function. Despite the near-identical code, smushing guest and host into the same function doesn't actually save much, just the function prototype and the local variables. That'd also avoid having to document/comment what 'true' and 'false' means at the call sites. > > { > > 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) This needs braces if you keep it as is. > > - timer_set_offset(vcpu_get_timer(tmp, timer), offset); > > + if (guest_visible) > > + timer_set_guest_offset(vcpu_get_timer(tmp, timer), > > + offset); Let this poke out, 84 chars isn't the end of the world. > > + 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. > > */ Any reason this can't be called from kvm_arch_vcpu_postcreate()? That'd eliminate the need for the extra handling. The vCPU is technically visible to userspace, but userspace would have to very intentionally do the wrong thing to cause problems, and I don't see any obviosu danger to the host. > > - 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); > > } > > _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel 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=-8.5 required=3.0 tests=BAYES_00,DKIM_ADSP_CUSTOM_MED, DKIM_INVALID,DKIM_SIGNED,FSL_HELO_FAKE,HEADER_FROM_DIFFERENT_DOMAINS, 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 87614C433EF for ; Wed, 22 Sep 2021 18:11:51 +0000 (UTC) Received: from mm01.cs.columbia.edu (mm01.cs.columbia.edu [128.59.11.253]) by mail.kernel.org (Postfix) with ESMTP id EAAB3610A1 for ; Wed, 22 Sep 2021 18:11:50 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.4.1 mail.kernel.org EAAB3610A1 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 52FCA40291; Wed, 22 Sep 2021 14:11:50 -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 9lRhsL-8yqOo; Wed, 22 Sep 2021 14:11:49 -0400 (EDT) Received: from mm01.cs.columbia.edu (localhost [127.0.0.1]) by mm01.cs.columbia.edu (Postfix) with ESMTP id 267524B0A0; Wed, 22 Sep 2021 14:11:49 -0400 (EDT) Received: from localhost (localhost [127.0.0.1]) by mm01.cs.columbia.edu (Postfix) with ESMTP id 984ED4079A for ; Wed, 22 Sep 2021 10:44:33 -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 8WK3PGiC7INT for ; Wed, 22 Sep 2021 10:44:32 -0400 (EDT) Received: from mail-pj1-f49.google.com (mail-pj1-f49.google.com [209.85.216.49]) by mm01.cs.columbia.edu (Postfix) with ESMTPS id 55F4640159 for ; Wed, 22 Sep 2021 10:44:32 -0400 (EDT) Received: by mail-pj1-f49.google.com with SMTP id lb1-20020a17090b4a4100b001993f863df2so2456923pjb.5 for ; Wed, 22 Sep 2021 07:44:32 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20210112; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to; bh=k92i1CTT4UfLIvqoj9NDLUAGbjRSVGtjE0AGo1vSZmQ=; b=f4lwilm8F32Il0kw2q+o2S5gn8bX330s+QqjMwYgJsMXJx2nLKL8EyWd3+MfL2K9jX 0tGZwNvTXYGj36wKXNRWv7f2/8WtblkGM8adydd7zCEXJGRgzSafRw0/O5bJe3dDxQQ7 U6i6z0ifislGQqzGxuWjmB4KG9fM9SillTMfXqaBzcnu4xfTxwQwb9DhTuwjk8oShc1N jmvJCl5gNVv2BNfFOnt3eTgLyXM+NbWeomDRALyfRFD5e79waLuLKw5bLUrghLsJW9+c 416hVoGg/lJKH5l5wbmxWkJQPlcGZB1pD/Mw8bTjlh40CsuIN/vh+c8yxxUJ2SyTVGzx o6KQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to; bh=k92i1CTT4UfLIvqoj9NDLUAGbjRSVGtjE0AGo1vSZmQ=; b=Y0+AyY/Ar+MY2vJ1uQXXsVMa5oW7R7hhId6X5rsEr3EEzQhSxrQ7IS/e/WYjGWmsjz X/JorvffKuq1CgKzPUWMPKe/B8xUErzjlXHisURSfX2faLGvfClGwGAlUQUS8YD2ecIA 3zgNB4t/hWa7aSNXphzY3lnx+EhIx1tMiDvlNSn1ZxUck8q+4/qAHA013KFZLRi5FoG/ hGPB41pah6/6hX0qr0YhcNI7bpMhvlMgfV01AqyZZNCC6JLuSnfvS+3R4nmYS5ST5nNZ JlDixf54tffykULVEiwANavmGr4VPsxmwU6ghOp8Cx7iecDxmHCZ6XB/GORqd4kJ0WaH 0BEw== X-Gm-Message-State: AOAM531mYFDBoi0cKZfR9/DWavq0j+GszGoZ11VKUATYuua2TGwqB2Zx ZP78sPjdymNEeDjdqtePdMLFmw== X-Google-Smtp-Source: ABdhPJy6Cm8J5yFpuY0b3Q26cuBfzP0jR3SXJy8Sit4o8X0PGq0Nl30V3OZC+YaU8p9jHy4oX17FmA== X-Received: by 2002:a17:902:8606:b029:12c:2625:76cf with SMTP id f6-20020a1709028606b029012c262576cfmr223350plo.17.1632321871110; Wed, 22 Sep 2021 07:44:31 -0700 (PDT) Received: from google.com (157.214.185.35.bc.googleusercontent.com. [35.185.214.157]) by smtp.gmail.com with ESMTPSA id d14sm2901977pfq.61.2021.09.22.07.44.30 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 22 Sep 2021 07:44:30 -0700 (PDT) Date: Wed, 22 Sep 2021 14:44:26 +0000 From: Sean Christopherson To: Reiji Watanabe Subject: Re: [PATCH v8 2/8] KVM: arm64: Separate guest/host counter offset values Message-ID: References: <20210916181510.963449-1-oupton@google.com> <20210916181510.963449-3-oupton@google.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: X-Mailman-Approved-At: Wed, 22 Sep 2021 14:11:48 -0400 Cc: kvm@vger.kernel.org, Catalin Marinas , 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 On Tue, Sep 21, 2021, Reiji Watanabe wrote: > Hi Oliver, > > On Thu, Sep 16, 2021 at 11:15 AM Oliver Upton wrote: > > +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); Really getting into nitpicking territory, but it maybe name this timer_set_virtual_offset() (assuming v=virtual and p=physical). Based on the name, I expected this to set a variable literally named guest_offset, but it reads and writes host_offset. Maintaining the virtual vs. physical terminology all the way down avoids having direct host vs. guest naming conflicts, i.e. virtual and host aren't generally though of as mutually exclusive. > > + 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. I'd say ditch the param altogether and just have two separate helpers. Even in the final code, the callers all pass explicit 'true' or 'false', i.e. the callers can just as easily call a different function. Despite the near-identical code, smushing guest and host into the same function doesn't actually save much, just the function prototype and the local variables. That'd also avoid having to document/comment what 'true' and 'false' means at the call sites. > > { > > 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) This needs braces if you keep it as is. > > - timer_set_offset(vcpu_get_timer(tmp, timer), offset); > > + if (guest_visible) > > + timer_set_guest_offset(vcpu_get_timer(tmp, timer), > > + offset); Let this poke out, 84 chars isn't the end of the world. > > + 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. > > */ Any reason this can't be called from kvm_arch_vcpu_postcreate()? That'd eliminate the need for the extra handling. The vCPU is technically visible to userspace, but userspace would have to very intentionally do the wrong thing to cause problems, and I don't see any obviosu danger to the host. > > - 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); > > } > > _______________________________________________ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm