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.4 required=3.0 tests=BAYES_00,DKIMWL_WL_MED, DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,FSL_HELO_FAKE, HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_CR_TRAILER,MAILING_LIST_MULTI, SPF_HELO_NONE,SPF_PASS,USER_IN_DEF_DKIM_WL autolearn=no 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 1D4A7C4338F for ; Fri, 30 Jul 2021 18:08:35 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id DF7C660EC0 for ; Fri, 30 Jul 2021 18:08:34 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S230119AbhG3SIf (ORCPT ); Fri, 30 Jul 2021 14:08:35 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:42710 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229773AbhG3SIf (ORCPT ); Fri, 30 Jul 2021 14:08:35 -0400 Received: from mail-pj1-x102d.google.com (mail-pj1-x102d.google.com [IPv6:2607:f8b0:4864:20::102d]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 45783C06175F for ; Fri, 30 Jul 2021 11:08:30 -0700 (PDT) Received: by mail-pj1-x102d.google.com with SMTP id a4-20020a17090aa504b0290176a0d2b67aso21989191pjq.2 for ; Fri, 30 Jul 2021 11:08:30 -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=U22IQhYTRPCkpysTSxxrY3OXIOUsjrcBM5qAQtaDVGA=; b=BJssnUAjT5z70MZ1iYT6dX4sF9p17EqqxIOWql+F9Uk8seuT1Mmsl7bvn2fUo1Pxfs C0mI9NqkKrBsVLH21YczNMMZWnEb00G/LeWP+XWxMl9sekNLaTj2vC3Vnm74nx/G0dMn eNiQ1lqpoxIHiIUv6WBxrWXpe/VXZLNqKGHiwmR5kARWiynI3rNGZ1js4fLeXV5ynjeg 6EPIuYKuAoyH8YJ7gJSSc90S35Rxgbrix/L5TDPDQWBMyyFfmmrNWLdpJnEQDHoj1xaA PzOZepFRIe5aFLECjxaqVYxxkEyfC6P5u5zunGvK2WGla6dONwBDm1nEg60N/MesbBpP 35pw== 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=U22IQhYTRPCkpysTSxxrY3OXIOUsjrcBM5qAQtaDVGA=; b=M2vu/9OG3HKbrnus5rVWBWWExlixQWD+jL9VT+RK9MOwM/L973BUxYgxyQnohaQLEA GvMudy7aw1TVFgVWgW+MCfdKh+1O6GEiXxATWO69trgONRoI5k1jI0GN+EotuV0Svspj vyFSzzGZnMU1ZT2kqcjXqLneO3km6rt61pEIpF+HrgH1JPpE4cOVxyq04aXkmBkryDHM jEIU1qHrGYMg2uKdsziuJfuNmDR4zcrfP0H1igHYP0IOeRA+/SG4h+R3MgfW57P59eQ7 SpnDulooS8JcZI4xrWNTx3EpAWU42rIgkyLL9aTrd3kMhKhnLEKAEQqB6LFjZ+IXXDAm PCiA== X-Gm-Message-State: AOAM531zECqotETBXv9i4oxSFSNSHnaTkb6zdr7Y0hX/tg0lxmsJIvIK TbaCgrP2yk8y9BtSwo7WYQMXSA== X-Google-Smtp-Source: ABdhPJzMaCyOmywV/AHhhMPn1KKKBoyFNH+eqA4FYc+b1MRy4AW0ZMGXtjZhMOgw+LQ8/a9GL5IIXQ== X-Received: by 2002:a17:90b:250f:: with SMTP id ns15mr4291311pjb.26.1627668509611; Fri, 30 Jul 2021 11:08:29 -0700 (PDT) Received: from google.com (157.214.185.35.bc.googleusercontent.com. [35.185.214.157]) by smtp.gmail.com with ESMTPSA id h7sm1368044pjs.38.2021.07.30.11.08.28 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Fri, 30 Jul 2021 11:08:29 -0700 (PDT) Date: Fri, 30 Jul 2021 18:08:25 +0000 From: Sean Christopherson To: Oliver Upton Cc: kvm@vger.kernel.org, kvmarm@lists.cs.columbia.edu, Paolo Bonzini , Marc Zyngier , Peter Shier , Jim Mattson , David Matlack , Ricardo Koller , Jing Zhang , Raghavendra Rao Anata , James Morse , Alexandru Elisei , Suzuki K Poulose , linux-arm-kernel@lists.infradead.org, Andrew Jones Subject: Re: [PATCH v5 02/13] KVM: x86: Refactor tsc synchronization code Message-ID: References: <20210729173300.181775-1-oupton@google.com> <20210729173300.181775-3-oupton@google.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20210729173300.181775-3-oupton@google.com> Precedence: bulk List-ID: X-Mailing-List: kvm@vger.kernel.org On Thu, Jul 29, 2021, Oliver Upton wrote: > Refactor kvm_synchronize_tsc to make a new function that allows callers > to specify TSC parameters (offset, value, nanoseconds, etc.) explicitly > for the sake of participating in TSC synchronization. > > This changes the locking semantics around TSC writes. "refactor" and "changes the locking semantics" are somewhat contradictory. The correct way to do this is to first change the locking semantics, then extract the helper you want. That makes review and archaeology easier, and isolates the locking change in case it isn't so safe after all. > Writes to the TSC will now take the pvclock gtod lock while holding the tsc > write lock, whereas before these locks were disjoint. > > Reviewed-by: David Matlack > Signed-off-by: Oliver Upton > --- > +/* > + * Infers attempts to synchronize the guest's tsc from host writes. Sets the > + * offset for the vcpu and tracks the TSC matching generation that the vcpu > + * participates in. > + * > + * Must hold kvm->arch.tsc_write_lock to call this function. Drop this blurb, lockdep assertions exist for a reason :-) > + */ > +static void __kvm_synchronize_tsc(struct kvm_vcpu *vcpu, u64 offset, u64 tsc, > + u64 ns, bool matched) > +{ > + struct kvm *kvm = vcpu->kvm; > + bool already_matched; Eww, not your code, but "matched" and "already_matched" are not helpful names, e.g. they don't provide a clue as to _what_ matched, and thus don't explain why there are two separate variables. And I would expect an "already" variant to come in from the caller, not the other way 'round. matched => freq_matched already_matched => gen_matched > + unsigned long flags; > + > + lockdep_assert_held(&kvm->arch.tsc_write_lock); > + > + already_matched = > + (vcpu->arch.this_tsc_generation == kvm->arch.cur_tsc_generation); > + > + /* > + * We track the most recent recorded KHZ, write and time to > + * allow the matching interval to be extended at each write. > + */ > + kvm->arch.last_tsc_nsec = ns; > + kvm->arch.last_tsc_write = tsc; > + kvm->arch.last_tsc_khz = vcpu->arch.virtual_tsc_khz; > + > + vcpu->arch.last_guest_tsc = tsc; > + > + /* Keep track of which generation this VCPU has synchronized to */ > + vcpu->arch.this_tsc_generation = kvm->arch.cur_tsc_generation; > + vcpu->arch.this_tsc_nsec = kvm->arch.cur_tsc_nsec; > + vcpu->arch.this_tsc_write = kvm->arch.cur_tsc_write; > + > + kvm_vcpu_write_tsc_offset(vcpu, offset); > + > + spin_lock_irqsave(&kvm->arch.pvclock_gtod_sync_lock, flags); I believe this can be spin_lock(), since AFAICT the caller _must_ disable IRQs when taking tsc_write_lock, i.e. we know IRQs are disabled at this point. > + if (!matched) { > + /* > + * We split periods of matched TSC writes into generations. > + * For each generation, we track the original measured > + * nanosecond time, offset, and write, so if TSCs are in > + * sync, we can match exact offset, and if not, we can match > + * exact software computation in compute_guest_tsc() > + * > + * These values are tracked in kvm->arch.cur_xxx variables. > + */ > + kvm->arch.nr_vcpus_matched_tsc = 0; > + kvm->arch.cur_tsc_generation++; > + kvm->arch.cur_tsc_nsec = ns; > + kvm->arch.cur_tsc_write = tsc; > + kvm->arch.cur_tsc_offset = offset; IMO, adjusting kvm->arch.cur_tsc_* belongs outside of pvclock_gtod_sync_lock. Based on the existing code, it is protected by tsc_write_lock. I don't care about the extra work while holding pvclock_gtod_sync_lock, but it's very confusing to see code that reads variables outside of a lock, then take a lock and write those same variables without first rechecking. > + matched = false; What's the point of clearing "matched"? It's already false... > + } else if (!already_matched) { > + kvm->arch.nr_vcpus_matched_tsc++; > + } > + > + kvm_track_tsc_matching(vcpu); > + spin_unlock_irqrestore(&kvm->arch.pvclock_gtod_sync_lock, flags); > +} > + 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=-6.6 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_CR_TRAILER,MAILING_LIST_MULTI, SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED autolearn=no 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 B51FBC4338F for ; Fri, 30 Jul 2021 18:10:25 +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 7EECC60EC0 for ; Fri, 30 Jul 2021 18:10:25 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.4.1 mail.kernel.org 7EECC60EC0 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=bwkhY79nmlGHnzzwL5+iHIfNckZ8vB/K+djs5fh0NsA=; b=oACOeFOHmqV+6v y85KI0pkT2JaPm9Yetrx6FQGYmfHsoVtqTE+EFPSisQDpalUqmx6A8eHEuCtw4KPfVST4c/wNiWiF 3fgBjkyIauQANnYiPdM4GmGNLnhqgLBrzA0NQWijUpQcyL6uzKSJMLdcdHvsdpdI+z1u4x3DmuQgz DzIoWOGeuN0vwGp2mizVwLFB/+I5Ks3aNWl1vrtJ9czSHs9oC4+t6zNuglWKq8dUsp0d00gj9hHKx HtClYQSJDJRLEjw4cmL99dyOKfTXdsU5YuH5Yern8+NN8H8j2uy4n/CAUz3JEeA0ppCuah9zdJzKK ebTXi5NZoXkr85gBht8A==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.94.2 #2 (Red Hat Linux)) id 1m9Ww4-009t6G-V6; Fri, 30 Jul 2021 18:08:41 +0000 Received: from mail-pl1-x62b.google.com ([2607:f8b0:4864:20::62b]) by bombadil.infradead.org with esmtps (Exim 4.94.2 #2 (Red Hat Linux)) id 1m9Ww0-009sxr-Ft for linux-arm-kernel@lists.infradead.org; Fri, 30 Jul 2021 18:08:38 +0000 Received: by mail-pl1-x62b.google.com with SMTP id e5so12017826pld.6 for ; Fri, 30 Jul 2021 11:08:30 -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=U22IQhYTRPCkpysTSxxrY3OXIOUsjrcBM5qAQtaDVGA=; b=BJssnUAjT5z70MZ1iYT6dX4sF9p17EqqxIOWql+F9Uk8seuT1Mmsl7bvn2fUo1Pxfs C0mI9NqkKrBsVLH21YczNMMZWnEb00G/LeWP+XWxMl9sekNLaTj2vC3Vnm74nx/G0dMn eNiQ1lqpoxIHiIUv6WBxrWXpe/VXZLNqKGHiwmR5kARWiynI3rNGZ1js4fLeXV5ynjeg 6EPIuYKuAoyH8YJ7gJSSc90S35Rxgbrix/L5TDPDQWBMyyFfmmrNWLdpJnEQDHoj1xaA PzOZepFRIe5aFLECjxaqVYxxkEyfC6P5u5zunGvK2WGla6dONwBDm1nEg60N/MesbBpP 35pw== 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=U22IQhYTRPCkpysTSxxrY3OXIOUsjrcBM5qAQtaDVGA=; b=LfJhGluN/lSxqwcgW/yM/N2WbZJ+kVl1MNuUk8rdFUOMTAMIp45mYTXhMvJv5LIr5+ JiuVrneD/fk5dxl/I+WzgXStjLNFlS8HfhyvhTKE0t/Dsh36STww/VU5P00D9qobYEoq Sw23hv5lkAgVp9VG8iyWvKggIJozZb6kW5ZAuvywdF4bovQzmoR4JUUQt4FF6c3YiHRC pulhEQx03K7s8Pb5nDbyp9CiiStd5p2fz+sQzAw58NA1vonGxvb7imVFHt95ht956g3I /71Dzk1pHE/qQwFjbAf6+bmB+sHWNQH3tamL6waxYXiMGEq2I6uoKMrjfG3QUGmoA70m bWUA== X-Gm-Message-State: AOAM533xZEmDJPiOdStZ8Qlp8sh0bK6q7mjAxiy7zlpMlZRib8PSkBN+ +rFknDWEa1+ot/qLhP8YuIjbrw== X-Google-Smtp-Source: ABdhPJzMaCyOmywV/AHhhMPn1KKKBoyFNH+eqA4FYc+b1MRy4AW0ZMGXtjZhMOgw+LQ8/a9GL5IIXQ== X-Received: by 2002:a17:90b:250f:: with SMTP id ns15mr4291311pjb.26.1627668509611; Fri, 30 Jul 2021 11:08:29 -0700 (PDT) Received: from google.com (157.214.185.35.bc.googleusercontent.com. [35.185.214.157]) by smtp.gmail.com with ESMTPSA id h7sm1368044pjs.38.2021.07.30.11.08.28 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Fri, 30 Jul 2021 11:08:29 -0700 (PDT) Date: Fri, 30 Jul 2021 18:08:25 +0000 From: Sean Christopherson To: Oliver Upton Cc: kvm@vger.kernel.org, kvmarm@lists.cs.columbia.edu, Paolo Bonzini , Marc Zyngier , Peter Shier , Jim Mattson , David Matlack , Ricardo Koller , Jing Zhang , Raghavendra Rao Anata , James Morse , Alexandru Elisei , Suzuki K Poulose , linux-arm-kernel@lists.infradead.org, Andrew Jones Subject: Re: [PATCH v5 02/13] KVM: x86: Refactor tsc synchronization code Message-ID: References: <20210729173300.181775-1-oupton@google.com> <20210729173300.181775-3-oupton@google.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20210729173300.181775-3-oupton@google.com> X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20210730_110836_603124_E5277A59 X-CRM114-Status: GOOD ( 23.30 ) 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 Thu, Jul 29, 2021, Oliver Upton wrote: > Refactor kvm_synchronize_tsc to make a new function that allows callers > to specify TSC parameters (offset, value, nanoseconds, etc.) explicitly > for the sake of participating in TSC synchronization. > > This changes the locking semantics around TSC writes. "refactor" and "changes the locking semantics" are somewhat contradictory. The correct way to do this is to first change the locking semantics, then extract the helper you want. That makes review and archaeology easier, and isolates the locking change in case it isn't so safe after all. > Writes to the TSC will now take the pvclock gtod lock while holding the tsc > write lock, whereas before these locks were disjoint. > > Reviewed-by: David Matlack > Signed-off-by: Oliver Upton > --- > +/* > + * Infers attempts to synchronize the guest's tsc from host writes. Sets the > + * offset for the vcpu and tracks the TSC matching generation that the vcpu > + * participates in. > + * > + * Must hold kvm->arch.tsc_write_lock to call this function. Drop this blurb, lockdep assertions exist for a reason :-) > + */ > +static void __kvm_synchronize_tsc(struct kvm_vcpu *vcpu, u64 offset, u64 tsc, > + u64 ns, bool matched) > +{ > + struct kvm *kvm = vcpu->kvm; > + bool already_matched; Eww, not your code, but "matched" and "already_matched" are not helpful names, e.g. they don't provide a clue as to _what_ matched, and thus don't explain why there are two separate variables. And I would expect an "already" variant to come in from the caller, not the other way 'round. matched => freq_matched already_matched => gen_matched > + unsigned long flags; > + > + lockdep_assert_held(&kvm->arch.tsc_write_lock); > + > + already_matched = > + (vcpu->arch.this_tsc_generation == kvm->arch.cur_tsc_generation); > + > + /* > + * We track the most recent recorded KHZ, write and time to > + * allow the matching interval to be extended at each write. > + */ > + kvm->arch.last_tsc_nsec = ns; > + kvm->arch.last_tsc_write = tsc; > + kvm->arch.last_tsc_khz = vcpu->arch.virtual_tsc_khz; > + > + vcpu->arch.last_guest_tsc = tsc; > + > + /* Keep track of which generation this VCPU has synchronized to */ > + vcpu->arch.this_tsc_generation = kvm->arch.cur_tsc_generation; > + vcpu->arch.this_tsc_nsec = kvm->arch.cur_tsc_nsec; > + vcpu->arch.this_tsc_write = kvm->arch.cur_tsc_write; > + > + kvm_vcpu_write_tsc_offset(vcpu, offset); > + > + spin_lock_irqsave(&kvm->arch.pvclock_gtod_sync_lock, flags); I believe this can be spin_lock(), since AFAICT the caller _must_ disable IRQs when taking tsc_write_lock, i.e. we know IRQs are disabled at this point. > + if (!matched) { > + /* > + * We split periods of matched TSC writes into generations. > + * For each generation, we track the original measured > + * nanosecond time, offset, and write, so if TSCs are in > + * sync, we can match exact offset, and if not, we can match > + * exact software computation in compute_guest_tsc() > + * > + * These values are tracked in kvm->arch.cur_xxx variables. > + */ > + kvm->arch.nr_vcpus_matched_tsc = 0; > + kvm->arch.cur_tsc_generation++; > + kvm->arch.cur_tsc_nsec = ns; > + kvm->arch.cur_tsc_write = tsc; > + kvm->arch.cur_tsc_offset = offset; IMO, adjusting kvm->arch.cur_tsc_* belongs outside of pvclock_gtod_sync_lock. Based on the existing code, it is protected by tsc_write_lock. I don't care about the extra work while holding pvclock_gtod_sync_lock, but it's very confusing to see code that reads variables outside of a lock, then take a lock and write those same variables without first rechecking. > + matched = false; What's the point of clearing "matched"? It's already false... > + } else if (!already_matched) { > + kvm->arch.nr_vcpus_matched_tsc++; > + } > + > + kvm_track_tsc_matching(vcpu); > + spin_unlock_irqrestore(&kvm->arch.pvclock_gtod_sync_lock, flags); > +} > + _______________________________________________ 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=-5.6 required=3.0 tests=BAYES_00,DKIM_ADSP_CUSTOM_MED, DKIM_INVALID,DKIM_SIGNED,FSL_HELO_FAKE,HEADER_FROM_DIFFERENT_DOMAINS, INCLUDES_CR_TRAILER,MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED autolearn=no 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 63E36C4338F for ; Sat, 31 Jul 2021 09:33:58 +0000 (UTC) Received: from mm01.cs.columbia.edu (mm01.cs.columbia.edu [128.59.11.253]) by mail.kernel.org (Postfix) with ESMTP id F350F61040 for ; Sat, 31 Jul 2021 09:33:57 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.4.1 mail.kernel.org F350F61040 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 A62704A4FC; Sat, 31 Jul 2021 05:33:57 -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 wwkHlFwsLpxD; Sat, 31 Jul 2021 05:33:56 -0400 (EDT) Received: from mm01.cs.columbia.edu (localhost [127.0.0.1]) by mm01.cs.columbia.edu (Postfix) with ESMTP id 38FF14B0B3; Sat, 31 Jul 2021 05:33:53 -0400 (EDT) Received: from localhost (localhost [127.0.0.1]) by mm01.cs.columbia.edu (Postfix) with ESMTP id 3FD944B0E6 for ; Fri, 30 Jul 2021 14:08:35 -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 k3-EwQADc72G for ; Fri, 30 Jul 2021 14:08:31 -0400 (EDT) Received: from mail-pj1-f50.google.com (mail-pj1-f50.google.com [209.85.216.50]) by mm01.cs.columbia.edu (Postfix) with ESMTPS id E70B14B0E5 for ; Fri, 30 Jul 2021 14:08:30 -0400 (EDT) Received: by mail-pj1-f50.google.com with SMTP id e2-20020a17090a4a02b029016f3020d867so15512502pjh.3 for ; Fri, 30 Jul 2021 11:08:30 -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=U22IQhYTRPCkpysTSxxrY3OXIOUsjrcBM5qAQtaDVGA=; b=BJssnUAjT5z70MZ1iYT6dX4sF9p17EqqxIOWql+F9Uk8seuT1Mmsl7bvn2fUo1Pxfs C0mI9NqkKrBsVLH21YczNMMZWnEb00G/LeWP+XWxMl9sekNLaTj2vC3Vnm74nx/G0dMn eNiQ1lqpoxIHiIUv6WBxrWXpe/VXZLNqKGHiwmR5kARWiynI3rNGZ1js4fLeXV5ynjeg 6EPIuYKuAoyH8YJ7gJSSc90S35Rxgbrix/L5TDPDQWBMyyFfmmrNWLdpJnEQDHoj1xaA PzOZepFRIe5aFLECjxaqVYxxkEyfC6P5u5zunGvK2WGla6dONwBDm1nEg60N/MesbBpP 35pw== 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=U22IQhYTRPCkpysTSxxrY3OXIOUsjrcBM5qAQtaDVGA=; b=BRLEXmYLDmqX2fc3u3juzMO/lLEp3HXPmfgB/1yaK7W1ZEN5hRkSpoC6VL52VvbYfh KYe3L/hdQpmQvvlRVfVLVqswb42PRfyARjH0/3+vxW2eB7u/fsoNq1jQs3dfH+vLCOGX HIs0AGs574TegHPuG4FdqKHsyuQ1YltPSuYNKqUa8ZVnQ1Jvuir2w/pLddPIyUFsc2WE 5TiFP92MtLSibkgKSYUGbdt/6fjVRiObKjic9x2HhVfMLN+jPddY2uMLb70i81aPKl2x qX66aR0DLSfKiLja1YkcqeUYTb5gO7dHFw9ROrmvtGiOAF9zz57HyX9h/NrOU4cU/DM3 f5Xg== X-Gm-Message-State: AOAM533sCtUJODXzaVFyjrGSHaTmc9+9V8FNC/Rh7IA2QMrUNmxRS+ek spQpS3oa7q2L7TmGNFlbZCOD/g== X-Google-Smtp-Source: ABdhPJzMaCyOmywV/AHhhMPn1KKKBoyFNH+eqA4FYc+b1MRy4AW0ZMGXtjZhMOgw+LQ8/a9GL5IIXQ== X-Received: by 2002:a17:90b:250f:: with SMTP id ns15mr4291311pjb.26.1627668509611; Fri, 30 Jul 2021 11:08:29 -0700 (PDT) Received: from google.com (157.214.185.35.bc.googleusercontent.com. [35.185.214.157]) by smtp.gmail.com with ESMTPSA id h7sm1368044pjs.38.2021.07.30.11.08.28 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Fri, 30 Jul 2021 11:08:29 -0700 (PDT) Date: Fri, 30 Jul 2021 18:08:25 +0000 From: Sean Christopherson To: Oliver Upton Subject: Re: [PATCH v5 02/13] KVM: x86: Refactor tsc synchronization code Message-ID: References: <20210729173300.181775-1-oupton@google.com> <20210729173300.181775-3-oupton@google.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20210729173300.181775-3-oupton@google.com> X-Mailman-Approved-At: Sat, 31 Jul 2021 05:33:51 -0400 Cc: kvm@vger.kernel.org, Marc Zyngier , Peter Shier , Raghavendra Rao Anata , David Matlack , Paolo Bonzini , 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 Thu, Jul 29, 2021, Oliver Upton wrote: > Refactor kvm_synchronize_tsc to make a new function that allows callers > to specify TSC parameters (offset, value, nanoseconds, etc.) explicitly > for the sake of participating in TSC synchronization. > > This changes the locking semantics around TSC writes. "refactor" and "changes the locking semantics" are somewhat contradictory. The correct way to do this is to first change the locking semantics, then extract the helper you want. That makes review and archaeology easier, and isolates the locking change in case it isn't so safe after all. > Writes to the TSC will now take the pvclock gtod lock while holding the tsc > write lock, whereas before these locks were disjoint. > > Reviewed-by: David Matlack > Signed-off-by: Oliver Upton > --- > +/* > + * Infers attempts to synchronize the guest's tsc from host writes. Sets the > + * offset for the vcpu and tracks the TSC matching generation that the vcpu > + * participates in. > + * > + * Must hold kvm->arch.tsc_write_lock to call this function. Drop this blurb, lockdep assertions exist for a reason :-) > + */ > +static void __kvm_synchronize_tsc(struct kvm_vcpu *vcpu, u64 offset, u64 tsc, > + u64 ns, bool matched) > +{ > + struct kvm *kvm = vcpu->kvm; > + bool already_matched; Eww, not your code, but "matched" and "already_matched" are not helpful names, e.g. they don't provide a clue as to _what_ matched, and thus don't explain why there are two separate variables. And I would expect an "already" variant to come in from the caller, not the other way 'round. matched => freq_matched already_matched => gen_matched > + unsigned long flags; > + > + lockdep_assert_held(&kvm->arch.tsc_write_lock); > + > + already_matched = > + (vcpu->arch.this_tsc_generation == kvm->arch.cur_tsc_generation); > + > + /* > + * We track the most recent recorded KHZ, write and time to > + * allow the matching interval to be extended at each write. > + */ > + kvm->arch.last_tsc_nsec = ns; > + kvm->arch.last_tsc_write = tsc; > + kvm->arch.last_tsc_khz = vcpu->arch.virtual_tsc_khz; > + > + vcpu->arch.last_guest_tsc = tsc; > + > + /* Keep track of which generation this VCPU has synchronized to */ > + vcpu->arch.this_tsc_generation = kvm->arch.cur_tsc_generation; > + vcpu->arch.this_tsc_nsec = kvm->arch.cur_tsc_nsec; > + vcpu->arch.this_tsc_write = kvm->arch.cur_tsc_write; > + > + kvm_vcpu_write_tsc_offset(vcpu, offset); > + > + spin_lock_irqsave(&kvm->arch.pvclock_gtod_sync_lock, flags); I believe this can be spin_lock(), since AFAICT the caller _must_ disable IRQs when taking tsc_write_lock, i.e. we know IRQs are disabled at this point. > + if (!matched) { > + /* > + * We split periods of matched TSC writes into generations. > + * For each generation, we track the original measured > + * nanosecond time, offset, and write, so if TSCs are in > + * sync, we can match exact offset, and if not, we can match > + * exact software computation in compute_guest_tsc() > + * > + * These values are tracked in kvm->arch.cur_xxx variables. > + */ > + kvm->arch.nr_vcpus_matched_tsc = 0; > + kvm->arch.cur_tsc_generation++; > + kvm->arch.cur_tsc_nsec = ns; > + kvm->arch.cur_tsc_write = tsc; > + kvm->arch.cur_tsc_offset = offset; IMO, adjusting kvm->arch.cur_tsc_* belongs outside of pvclock_gtod_sync_lock. Based on the existing code, it is protected by tsc_write_lock. I don't care about the extra work while holding pvclock_gtod_sync_lock, but it's very confusing to see code that reads variables outside of a lock, then take a lock and write those same variables without first rechecking. > + matched = false; What's the point of clearing "matched"? It's already false... > + } else if (!already_matched) { > + kvm->arch.nr_vcpus_matched_tsc++; > + } > + > + kvm_track_tsc_matching(vcpu); > + spin_unlock_irqrestore(&kvm->arch.pvclock_gtod_sync_lock, flags); > +} > + _______________________________________________ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm