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