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,HEADER_FROM_DIFFERENT_DOMAINS, INCLUDES_PATCH,MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS,USER_IN_DEF_DKIM_WL 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 D2E1BC433B4 for ; Wed, 19 May 2021 15:40:23 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id ACA1C611AE for ; Wed, 19 May 2021 15:40:23 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1354939AbhESPlk (ORCPT ); Wed, 19 May 2021 11:41:40 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:57996 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S242076AbhESPlf (ORCPT ); Wed, 19 May 2021 11:41:35 -0400 Received: from mail-pl1-x62d.google.com (mail-pl1-x62d.google.com [IPv6:2607:f8b0:4864:20::62d]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id EBD2FC061760 for ; Wed, 19 May 2021 08:40:14 -0700 (PDT) Received: by mail-pl1-x62d.google.com with SMTP id v13so7245901ple.9 for ; Wed, 19 May 2021 08:40:14 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20161025; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to; bh=5bEuxX2X8bBUVqC8o4ZD8af+cxfwPYrie2CqQyfE1eg=; b=qRETapL+guGrCRWWx8ib5azlaovGzEPYrmFu8ScRF6zwzGS2UvHzB6A+78Bc9WOqMk 5PZf+y5DdzbsF/6aJ/EnoKVI6P2fSFypZV5Xj7ldUSA0YCNHcEetvdfVjjqNWvfvjzcU i7gUDbjdg0HsEzBNE1LDFaPyh7c8IAxpusJdwH3WzvMtO5e5hsuPayfq510JlllOKjnJ 9mrnsIS0LICp0CZjeJdV1RK9lB4sfHFNUumZvRFT1i2nJE81MyPL63u1EibjDpXpz2CY r4h6cJYviHXpX83OC9OGoyszfc/cwRtDFdQ1gOd376N6Dsfhr22asd2BvpA9lwG2kq70 XmVA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to; bh=5bEuxX2X8bBUVqC8o4ZD8af+cxfwPYrie2CqQyfE1eg=; b=RwgSlFqdkqqPRgnu3pOV79r3YxF1Utcfrzekyihq1Hnrc7JbvNyEx+Cges2GTCXedK Z5qLAkzjSRb32vEGYv4JX01ioEX0rEEAhmsH/Bnss0fslfNjC+6YvHHkI/6khOpvLwnE qXbCOSFkRsgymumr5ZAMen/Z+fCT0Hunj1K9H3faylf6b8Mux7inaOjO5ukMvdLlHz1F marBNcmZICZAJnal13qdwTH94JJine9zY4jHJND3TY/5D+PmKHynvK3zTO3yWxQItz6F D8MnvEmDUEKPmjDfEmIv9b4OEQvRAXYcs6xgkucXzkYaf5rEB6U5G/ry/LN8MzMyEhB5 KB3A== X-Gm-Message-State: AOAM532wsa63McfggkEPqUPRMiM4macHNn6Kir+MWU4+wU9ucX4Aq2uU NqCXxTTZUtcwYeEzUPIwltQsyw== X-Google-Smtp-Source: ABdhPJxhN0z+Kc4wpNsbbohN9nkWE0iRXUWhsGpnxbGaJuuoAihs6TJvhqtMh2V9Dpv4p6HEvC9WRA== X-Received: by 2002:a17:90a:a386:: with SMTP id x6mr12059847pjp.193.1621438814276; Wed, 19 May 2021 08:40:14 -0700 (PDT) Received: from google.com (240.111.247.35.bc.googleusercontent.com. [35.247.111.240]) by smtp.gmail.com with ESMTPSA id u19sm8076836pfn.158.2021.05.19.08.40.13 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 19 May 2021 08:40:13 -0700 (PDT) Date: Wed, 19 May 2021 15:40:10 +0000 From: Sean Christopherson To: "Stamatis, Ilias" Cc: "kvm@vger.kernel.org" , "linux-kernel@vger.kernel.org" , "jmattson@google.com" , "Woodhouse, David" , "vkuznets@redhat.com" , "joro@8bytes.org" , "mtosatti@redhat.com" , "zamsden@gmail.com" , "pbonzini@redhat.com" , "mlevitsk@redhat.com" , "wanpengli@tencent.com" Subject: Re: [PATCH v2 03/10] KVM: X86: Add kvm_scale_tsc_l1() and kvm_compute_tsc_offset_l1() Message-ID: References: <20210512150945.4591-1-ilstam@amazon.com> <20210512150945.4591-4-ilstam@amazon.com> <772e232c27d180f876a5b49d7f188c0c3acd7560.camel@amazon.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <772e232c27d180f876a5b49d7f188c0c3acd7560.camel@amazon.com> Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Wed, May 19, 2021, Stamatis, Ilias wrote: > On Tue, 2021-05-18 at 23:04 +0000, Sean Christopherson wrote: > > On Wed, May 12, 2021, Ilias Stamatis wrote: > > > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c > > > index 07cf5d7ece38..84af1af7a2cc 100644 > > > --- a/arch/x86/kvm/x86.c > > > +++ b/arch/x86/kvm/x86.c > > > @@ -2319,18 +2319,30 @@ u64 kvm_scale_tsc(struct kvm_vcpu *vcpu, u64 tsc) > > > } > > > EXPORT_SYMBOL_GPL(kvm_scale_tsc); > > > > > > -static u64 kvm_compute_tsc_offset(struct kvm_vcpu *vcpu, u64 target_tsc) > > > +u64 kvm_scale_tsc_l1(struct kvm_vcpu *vcpu, u64 tsc) > > > +{ > > > + u64 _tsc = tsc; > > > + u64 ratio = vcpu->arch.l1_tsc_scaling_ratio; > > > + > > > + if (ratio != kvm_default_tsc_scaling_ratio) > > > + _tsc = __scale_tsc(ratio, tsc); > > > + > > > + return _tsc; > > > +} > > > > Just make the ratio a param. This is complete copy+paste of kvm_scale_tsc(), > > with 3 characters added. And all of the callers are already in an L1-specific > > function or have L1 vs. L2 awareness. IMO, that makes the code less magical, too, > > as I don't have to dive into a helper to see that it reads l1_tsc_scaling_ratio > > versus tsc_scaling_ratio. > > > > That's how I did it initially but changed it into a separate function after > receiving feedback on v1. I'm neutral, I don't mind changing it back. Ah, I see the conundrum. The vendor code isn't straightforward because of all the enabling checks against vmcs12 controls. Given that, I don't terribly mind the callbacks, but I do think the connection between the computation and the VMWRITE needs to be more explicit. Poking around the code, the other thing that would help would be to get rid of the awful decache_tsc_multiplier(). That helper was added to paper over the completely broken logic of commit ff2c3a180377 ("KVM: VMX: Setup TSC scaling ratio when a vcpu is loaded"). Its use in vmx_vcpu_load_vmcs() is basically "write the VMCS if we forgot to earlier", which is all kinds of wrong. If we get rid of that stupidity as prep work at the beginning of this series, and have the "setters" return the computed value, the nested VMX code can consume the value directly instead of having the subtle dependency on the helpers. vmcs_write64(TSC_OFFSET, kvm_calc_l2_tsc_offset(vcpu)); if (kvm_has_tsc_control) vmcs_write64(TSC_MULTIPLIER, kvm_calc_l2_tsc_multiplier(vcpu)); Side topic, the checks against the vmcs12 controls are wrong. Specifically, when checking a secondary execution control, KVM needs to first check that the secondary control is enabled in the primary control. But, we helpers for that. The primary control should use its helper, too. And while you're at it, drop the local variable in the getter. I.e.: diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c index 3c4eb14a1e86..8735f2d71e17 100644 --- a/arch/x86/kvm/vmx/vmx.c +++ b/arch/x86/kvm/vmx/vmx.c @@ -1801,13 +1801,12 @@ static u64 vmx_get_l2_tsc_offset(struct kvm_vcpu *vcpu) static u64 vmx_get_l2_tsc_multiplier(struct kvm_vcpu *vcpu) { struct vmcs12 *vmcs12 = get_vmcs12(vcpu); - u64 multiplier = kvm_default_tsc_scaling_ratio; - if (vmcs12->cpu_based_vm_exec_control & CPU_BASED_USE_TSC_OFFSETTING && - vmcs12->secondary_vm_exec_control & SECONDARY_EXEC_TSC_SCALING) - multiplier = vmcs12->tsc_multiplier; + if (nested_cpu_has(vmcs12, CPU_BASED_USE_TSC_OFFSETTING) && + nested_cpu_has2(vmcs12, SECONDARY_EXEC_TSC_SCALING)) + return vmcs12->tsc_multiplier; - return multiplier; + return kvm_default_tsc_scaling_ratio; } Side topic #2: I now see why the x86.c helpers skip the math if the multiplier is kvm_default_tsc_scaling_ratio.