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.3 required=3.0 tests=BAYES_00,DKIMWL_WL_MED, DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS, 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 27ED0C4338F for ; Fri, 30 Jul 2021 15:22:19 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 0D90960F46 for ; Fri, 30 Jul 2021 15:22:19 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S239581AbhG3PWW (ORCPT ); Fri, 30 Jul 2021 11:22:22 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:55186 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S239558AbhG3PWU (ORCPT ); Fri, 30 Jul 2021 11:22:20 -0400 Received: from mail-lf1-x12c.google.com (mail-lf1-x12c.google.com [IPv6:2a00:1450:4864:20::12c]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id AE94EC06175F for ; Fri, 30 Jul 2021 08:22:14 -0700 (PDT) Received: by mail-lf1-x12c.google.com with SMTP id h2so18622204lfu.4 for ; Fri, 30 Jul 2021 08:22:14 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20161025; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=/1o7+QKd97cIwS/kb/Ro1mV06vaGWcTs0D3fJHtTeso=; b=nVqXEBrlXTGcTUwaUH5Wm8FEDfko7xv0Xdqrq5ntdbi6gth09eo2rp2GDwyHrYvBtt qcCaJ8HQgf4tyQdQ6aro6IyWLEpR5ja2MABBOxIHdNHEOIYAAx4P6J0nwbLx1/VjwT3z MnxgpQjuj/UbcnSfMYqagxbISEsp7GkQYhqGVNHjVMqv0GR0qtz9XXiVfUvngr69Llko Zlutqz6DBFg76VF5mD1QAh2yDxoxr7I/kjDc3v6eRfwv/YBPGLIJ1G6ckwEqO9WL8UT4 LTjGh2P4o7HCw6SZOwcuTIAbcdkvvScewj+4RG5cfRjkKrck+bbowZ88vu01qTxghj3R HVcQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=/1o7+QKd97cIwS/kb/Ro1mV06vaGWcTs0D3fJHtTeso=; b=skE5X0Pt/L7yJnGB5NFVJPseL/hh+yTsHST59ud03vYRb8K7mhxKMD4KvcnRYQHq7+ pDzuh7RD12rTpz6Fkaf0QVsXIVU0Xa6dGrVleoVNUUbfbtoKlQNIabYSH2y6rG2e9Min 4mjV/IIQPZe0J2Y4jzCLz61TyCX2701rRvEdDNaF0aXz5GRosV6Pd8TwxNZzvPnEKd7l 4/hdyzpf0tNUudSuWrihkcsC+KnBGJGv8R9ITZA5ySNbNDifegLat3dlo4l8TcJCR6ws 1XvijAgf0kcDFb5oK36YxvRmEwyTX/ZUUZYx0Soh+TrXP3TiNMCbeNAoomVJWjdF9w1V owDA== X-Gm-Message-State: AOAM532XTIR0bBY6t0D72IynqdaXXuW6469sO6BMvd/Wf0D3JRfGBw7a VhuJvfueFvi6SLKHaCj5mVfPDrAR0E6I1jfEZMwdEA== X-Google-Smtp-Source: ABdhPJy3MzOJEL7JUXn3pv247OvpOyrn4DFM65cNNeDejFTsyvt+MsdJW5ifOeeaLmsG2MxQDXZ+nQuM/comVP/br64= X-Received: by 2002:a05:6512:3d26:: with SMTP id d38mr2131778lfv.411.1627658532665; Fri, 30 Jul 2021 08:22:12 -0700 (PDT) MIME-Version: 1.0 References: <20210729173300.181775-1-oupton@google.com> <20210729173300.181775-12-oupton@google.com> <875yws2h5w.wl-maz@kernel.org> In-Reply-To: <875yws2h5w.wl-maz@kernel.org> From: Oliver Upton Date: Fri, 30 Jul 2021 08:22:01 -0700 Message-ID: Subject: Re: [PATCH v5 11/13] KVM: arm64: Provide userspace access to the physical counter offset To: Marc Zyngier Cc: kvm@vger.kernel.org, kvmarm@lists.cs.columbia.edu, Paolo Bonzini , Sean Christopherson , 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 Content-Type: text/plain; charset="UTF-8" Precedence: bulk List-ID: X-Mailing-List: kvm@vger.kernel.org On Fri, Jul 30, 2021 at 4:08 AM Marc Zyngier wrote: > > FEAT_ECV provides a guest physical counter-timer offset register > > (CNTPOFF_EL2), but ECV-enabled hardware is nonexistent at the time of > > writing so support for it was elided for the sake of the author :) > > You seem to have done most the work for that, and there are SW models > out there that implement ECV. If anything, the ECV support should come > first, and its emulation only as a poor man's workaround. > > It is also good to show silicon vendors that they suck at providing > useful features, and pointing them to the code that would use these > features *is* an incentive. Lol, then so be it. I'll add ECV support to this series. What can I test with, though? I don't recall QEMU supporting ECV last I checked.. > I really dislike the fallback to !vhe here. I'd rather you > special-case the emulated ptimer in the VHE case: > > if (has_vhe()) { > map->direct_vtimer = vcpu_vtimer(vcpu); > if (!timer_get_offset(vcpu_ptimer(vcpu))) > map->direct_ptimer = vcpu_ptimer(vcpu); > map->emul_ptimer = NULL; > } else { > map->direct_ptimer = NULL; > map->emul_ptimer = vcpu_ptimer(vcpu); > } > } else { Ack. > and you can drop the timer_emulation_required() helper which doesn't > serve any purpose. Especially if I add ECV to this series, I'd prefer to carry it than open-code the check for CNTPOFF && !ECV in get_timer_map(). Do you concur? I can tighten it to ptimer_emulation_required() and avoid taking an arch_timer context if you prefer. > > +static inline bool __hyp_handle_counter(struct kvm_vcpu *vcpu) > > +{ > > + u32 sysreg = esr_sys64_to_sysreg(kvm_vcpu_get_esr(vcpu)); > > + int rt = kvm_vcpu_sys_get_rt(vcpu); > > + u64 rv; > > + > > + if (sysreg != SYS_CNTPCT_EL0) > > That's feels really optimistic. You don't check for the exception > class, and pray that the bits will align? ;-) Doh! Missed the EC check. > Please don't add more small includes like this. It makes things hard > to read and hides bugs Ack. > the counter read can (and will) be speculated, > so you definitely need an ISB before the access. Please also look at > __arch_counter_get_cntpct() and arch_counter_enforce_ordering(). Wouldn't it be up to the guest to decide if it wants the counter to be speculated or not? I debated this a bit myself in the implementation, as we would trap both ordered and un-ordered reads. Well, no way to detect it :) > > > > -/* > > - * Should only be called on non-VHE systems. > > - * VHE systems use EL2 timers and configure EL1 timers in kvm_timer_init_vhe(). > > - */ > > This comment still applies. this function *is* nVHE specific. > Ack. > > You now pay the price of reprogramming CNTHCTL_EL2 on every entry for > VHE, and that's not right. On VHE, it should be enough to perform the > programming on vcpu_load() only. > True. I'll rework the programming in the next series. > Overall, this patch needs a bit of splitting up (userspace interface, > HV rework), re-optimise VHE, and it *definitely* wants the ECV support > for the physical timer. > Sure thing, thanks for the review Marc! -- Best, Oliver 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=-4.5 required=3.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_ADSP_CUSTOM_MED,DKIM_SIGNED,DKIM_VALID,HEADER_FROM_DIFFERENT_DOMAINS, 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 55509C4338F for ; Fri, 30 Jul 2021 15:24:35 +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 1E03960F4B for ; Fri, 30 Jul 2021 15:24:35 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.4.1 mail.kernel.org 1E03960F4B 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=VCe+p2SzmQRY6mqG/MP/T8gLgxRpBm6t4qRDF/q56gY=; b=mnFOdZog7PEhXu sCRAcypa019RuFIL+gzHC/l4diw4iUbNCzRkLUqWH5x/3lXq/TJPijo7WvjFG0bTvDGcjZ43BR2Nw gA4ymA6pop+8jjUz5vua6uXHQIc05jMixqY8M/L+twT7OmUyelEmWCKMRB+WmhME30ZOlSNKPDQzG QrQH1TmEk3jLHqm5Kq9oDkTXB5cnrCgqxrtnIArwwXl/XA3pis0Bo6vl0jFHobfr/2hF9KL3SSe0K YPKpzfri1a3DuFfP47IftuVBxIS91OjBNtQhild9ZAF6PSOfNIyEMt/EnK7+ZWzrlBYKgWJhJStOM Jva28uhU5166fNU1r5jA==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.94.2 #2 (Red Hat Linux)) id 1m9ULW-009GyW-8P; Fri, 30 Jul 2021 15:22:47 +0000 Received: from mail-lf1-x134.google.com ([2a00:1450:4864:20::134]) by bombadil.infradead.org with esmtps (Exim 4.94.2 #2 (Red Hat Linux)) id 1m9UL2-009Gnu-3k for linux-arm-kernel@lists.infradead.org; Fri, 30 Jul 2021 15:22:17 +0000 Received: by mail-lf1-x134.google.com with SMTP id t9so5604138lfc.6 for ; Fri, 30 Jul 2021 08:22:15 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20161025; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=/1o7+QKd97cIwS/kb/Ro1mV06vaGWcTs0D3fJHtTeso=; b=nVqXEBrlXTGcTUwaUH5Wm8FEDfko7xv0Xdqrq5ntdbi6gth09eo2rp2GDwyHrYvBtt qcCaJ8HQgf4tyQdQ6aro6IyWLEpR5ja2MABBOxIHdNHEOIYAAx4P6J0nwbLx1/VjwT3z MnxgpQjuj/UbcnSfMYqagxbISEsp7GkQYhqGVNHjVMqv0GR0qtz9XXiVfUvngr69Llko Zlutqz6DBFg76VF5mD1QAh2yDxoxr7I/kjDc3v6eRfwv/YBPGLIJ1G6ckwEqO9WL8UT4 LTjGh2P4o7HCw6SZOwcuTIAbcdkvvScewj+4RG5cfRjkKrck+bbowZ88vu01qTxghj3R HVcQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=/1o7+QKd97cIwS/kb/Ro1mV06vaGWcTs0D3fJHtTeso=; b=sHBiBblXmAQYv3A8c6tuTrPD7C5ziXASixIw1jlIvvf/ZTGMvo+bYQan4B4Sc/svm4 U6URl7WNAcruYeXqOvQoXZj1yIczAr7dhlwx/LAG4vEhhmLO7RkhTpyL9BwrbeudkWOZ TQyIdgeypFde9IthFo6We0g6GV7288dluvFsvPY6/A2CS+QdgE2L+HnjCJ+LE/H8Lz3D RDD1mDjpxT68WokSpZAbe+VXhdj0WZ8wYJurD03y+72gWH/BBi9dNzqZzOwQLJurWJ1F 4E7rIkgT6gjJQ1878HEQBZSCP29FNaLtrj1wDkqAlQBVmmNmceqgeKo9QlKa4326jibF ovXQ== X-Gm-Message-State: AOAM530lhe97xqZ35ssmMh1+nIxrewVhWWHjMCWnQiHI6d9JHGUXuFcR vEO3nM/NIp7UZY3By9ghUqRhbbizIPHkAHqrYD0m3A== X-Google-Smtp-Source: ABdhPJy3MzOJEL7JUXn3pv247OvpOyrn4DFM65cNNeDejFTsyvt+MsdJW5ifOeeaLmsG2MxQDXZ+nQuM/comVP/br64= X-Received: by 2002:a05:6512:3d26:: with SMTP id d38mr2131778lfv.411.1627658532665; Fri, 30 Jul 2021 08:22:12 -0700 (PDT) MIME-Version: 1.0 References: <20210729173300.181775-1-oupton@google.com> <20210729173300.181775-12-oupton@google.com> <875yws2h5w.wl-maz@kernel.org> In-Reply-To: <875yws2h5w.wl-maz@kernel.org> From: Oliver Upton Date: Fri, 30 Jul 2021 08:22:01 -0700 Message-ID: Subject: Re: [PATCH v5 11/13] KVM: arm64: Provide userspace access to the physical counter offset To: Marc Zyngier Cc: kvm@vger.kernel.org, kvmarm@lists.cs.columbia.edu, Paolo Bonzini , Sean Christopherson , 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 X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20210730_082216_237647_44B585B7 X-CRM114-Status: GOOD ( 33.97 ) 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 Fri, Jul 30, 2021 at 4:08 AM Marc Zyngier wrote: > > FEAT_ECV provides a guest physical counter-timer offset register > > (CNTPOFF_EL2), but ECV-enabled hardware is nonexistent at the time of > > writing so support for it was elided for the sake of the author :) > > You seem to have done most the work for that, and there are SW models > out there that implement ECV. If anything, the ECV support should come > first, and its emulation only as a poor man's workaround. > > It is also good to show silicon vendors that they suck at providing > useful features, and pointing them to the code that would use these > features *is* an incentive. Lol, then so be it. I'll add ECV support to this series. What can I test with, though? I don't recall QEMU supporting ECV last I checked.. > I really dislike the fallback to !vhe here. I'd rather you > special-case the emulated ptimer in the VHE case: > > if (has_vhe()) { > map->direct_vtimer = vcpu_vtimer(vcpu); > if (!timer_get_offset(vcpu_ptimer(vcpu))) > map->direct_ptimer = vcpu_ptimer(vcpu); > map->emul_ptimer = NULL; > } else { > map->direct_ptimer = NULL; > map->emul_ptimer = vcpu_ptimer(vcpu); > } > } else { Ack. > and you can drop the timer_emulation_required() helper which doesn't > serve any purpose. Especially if I add ECV to this series, I'd prefer to carry it than open-code the check for CNTPOFF && !ECV in get_timer_map(). Do you concur? I can tighten it to ptimer_emulation_required() and avoid taking an arch_timer context if you prefer. > > +static inline bool __hyp_handle_counter(struct kvm_vcpu *vcpu) > > +{ > > + u32 sysreg = esr_sys64_to_sysreg(kvm_vcpu_get_esr(vcpu)); > > + int rt = kvm_vcpu_sys_get_rt(vcpu); > > + u64 rv; > > + > > + if (sysreg != SYS_CNTPCT_EL0) > > That's feels really optimistic. You don't check for the exception > class, and pray that the bits will align? ;-) Doh! Missed the EC check. > Please don't add more small includes like this. It makes things hard > to read and hides bugs Ack. > the counter read can (and will) be speculated, > so you definitely need an ISB before the access. Please also look at > __arch_counter_get_cntpct() and arch_counter_enforce_ordering(). Wouldn't it be up to the guest to decide if it wants the counter to be speculated or not? I debated this a bit myself in the implementation, as we would trap both ordered and un-ordered reads. Well, no way to detect it :) > > > > -/* > > - * Should only be called on non-VHE systems. > > - * VHE systems use EL2 timers and configure EL1 timers in kvm_timer_init_vhe(). > > - */ > > This comment still applies. this function *is* nVHE specific. > Ack. > > You now pay the price of reprogramming CNTHCTL_EL2 on every entry for > VHE, and that's not right. On VHE, it should be enough to perform the > programming on vcpu_load() only. > True. I'll rework the programming in the next series. > Overall, this patch needs a bit of splitting up (userspace interface, > HV rework), re-optimise VHE, and it *definitely* wants the ECV support > for the physical timer. > Sure thing, thanks for the review Marc! -- Best, Oliver _______________________________________________ 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=-3.6 required=3.0 tests=BAYES_00,DKIM_ADSP_CUSTOM_MED, DKIM_INVALID,DKIM_SIGNED,HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI, SPF_HELO_NONE,SPF_PASS 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 995B4C4320A for ; Fri, 30 Jul 2021 15:22:19 +0000 (UTC) Received: from mm01.cs.columbia.edu (mm01.cs.columbia.edu [128.59.11.253]) by mail.kernel.org (Postfix) with ESMTP id 1C56E61059 for ; Fri, 30 Jul 2021 15:22:19 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.4.1 mail.kernel.org 1C56E61059 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 8B8EB4B0C9; Fri, 30 Jul 2021 11:22:18 -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 MX63yujUh0n8; Fri, 30 Jul 2021 11:22:17 -0400 (EDT) Received: from mm01.cs.columbia.edu (localhost [127.0.0.1]) by mm01.cs.columbia.edu (Postfix) with ESMTP id 73FAA4B0A3; Fri, 30 Jul 2021 11:22:17 -0400 (EDT) Received: from localhost (localhost [127.0.0.1]) by mm01.cs.columbia.edu (Postfix) with ESMTP id EAC644B0A3 for ; Fri, 30 Jul 2021 11:22:15 -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 mZsZDRc6cr+m for ; Fri, 30 Jul 2021 11:22:14 -0400 (EDT) Received: from mail-lf1-f43.google.com (mail-lf1-f43.google.com [209.85.167.43]) by mm01.cs.columbia.edu (Postfix) with ESMTPS id 84B2D4B09A for ; Fri, 30 Jul 2021 11:22:14 -0400 (EDT) Received: by mail-lf1-f43.google.com with SMTP id p38so3679920lfa.0 for ; Fri, 30 Jul 2021 08:22:14 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20161025; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=/1o7+QKd97cIwS/kb/Ro1mV06vaGWcTs0D3fJHtTeso=; b=nVqXEBrlXTGcTUwaUH5Wm8FEDfko7xv0Xdqrq5ntdbi6gth09eo2rp2GDwyHrYvBtt qcCaJ8HQgf4tyQdQ6aro6IyWLEpR5ja2MABBOxIHdNHEOIYAAx4P6J0nwbLx1/VjwT3z MnxgpQjuj/UbcnSfMYqagxbISEsp7GkQYhqGVNHjVMqv0GR0qtz9XXiVfUvngr69Llko Zlutqz6DBFg76VF5mD1QAh2yDxoxr7I/kjDc3v6eRfwv/YBPGLIJ1G6ckwEqO9WL8UT4 LTjGh2P4o7HCw6SZOwcuTIAbcdkvvScewj+4RG5cfRjkKrck+bbowZ88vu01qTxghj3R HVcQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=/1o7+QKd97cIwS/kb/Ro1mV06vaGWcTs0D3fJHtTeso=; b=FQfbvs5X4BzAeYxE9+T8SjQ5JJsR2pjNjvOwiuKWjCzYe08YO6keSb4TejO1oK8/80 AsOr8fAsi+g6vdGIAcSjcE2mTuk/8ZIc1eQcq9DdU2uPRm6PWt/O6Hur2PNOW3cPjDMv fGI2q1bsk9MEUMlSgW4oVlXwiFxpGD5E+1ChjOwoWMHemI2Zjdha1D6jd7ANITztUsRK 3vgx4wkOLMn+BX5lCFv8JbqVmFSHQ4bB3CKPOI9npwqSsJcyqffXW2bqYoZxyRocSosw T1+6tskD4TqlVEI9d/9NIOlmKKIj5IbEQvRi8E/DgNf5MGxfLp5O5r0t5vQkpSKi/ctq eWuQ== X-Gm-Message-State: AOAM533lD+MYCDA2+i3mgF4yeErrwuZQUVdNI2Xu9/UWJVdTs5LRgIa6 DNHRdOf+3Y8pii5O9aZN9LFbwdL5mb3eky0Pg83ftw== X-Google-Smtp-Source: ABdhPJy3MzOJEL7JUXn3pv247OvpOyrn4DFM65cNNeDejFTsyvt+MsdJW5ifOeeaLmsG2MxQDXZ+nQuM/comVP/br64= X-Received: by 2002:a05:6512:3d26:: with SMTP id d38mr2131778lfv.411.1627658532665; Fri, 30 Jul 2021 08:22:12 -0700 (PDT) MIME-Version: 1.0 References: <20210729173300.181775-1-oupton@google.com> <20210729173300.181775-12-oupton@google.com> <875yws2h5w.wl-maz@kernel.org> In-Reply-To: <875yws2h5w.wl-maz@kernel.org> From: Oliver Upton Date: Fri, 30 Jul 2021 08:22:01 -0700 Message-ID: Subject: Re: [PATCH v5 11/13] KVM: arm64: Provide userspace access to the physical counter offset To: Marc Zyngier Cc: kvm@vger.kernel.org, Sean Christopherson , 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 Fri, Jul 30, 2021 at 4:08 AM Marc Zyngier wrote: > > FEAT_ECV provides a guest physical counter-timer offset register > > (CNTPOFF_EL2), but ECV-enabled hardware is nonexistent at the time of > > writing so support for it was elided for the sake of the author :) > > You seem to have done most the work for that, and there are SW models > out there that implement ECV. If anything, the ECV support should come > first, and its emulation only as a poor man's workaround. > > It is also good to show silicon vendors that they suck at providing > useful features, and pointing them to the code that would use these > features *is* an incentive. Lol, then so be it. I'll add ECV support to this series. What can I test with, though? I don't recall QEMU supporting ECV last I checked.. > I really dislike the fallback to !vhe here. I'd rather you > special-case the emulated ptimer in the VHE case: > > if (has_vhe()) { > map->direct_vtimer = vcpu_vtimer(vcpu); > if (!timer_get_offset(vcpu_ptimer(vcpu))) > map->direct_ptimer = vcpu_ptimer(vcpu); > map->emul_ptimer = NULL; > } else { > map->direct_ptimer = NULL; > map->emul_ptimer = vcpu_ptimer(vcpu); > } > } else { Ack. > and you can drop the timer_emulation_required() helper which doesn't > serve any purpose. Especially if I add ECV to this series, I'd prefer to carry it than open-code the check for CNTPOFF && !ECV in get_timer_map(). Do you concur? I can tighten it to ptimer_emulation_required() and avoid taking an arch_timer context if you prefer. > > +static inline bool __hyp_handle_counter(struct kvm_vcpu *vcpu) > > +{ > > + u32 sysreg = esr_sys64_to_sysreg(kvm_vcpu_get_esr(vcpu)); > > + int rt = kvm_vcpu_sys_get_rt(vcpu); > > + u64 rv; > > + > > + if (sysreg != SYS_CNTPCT_EL0) > > That's feels really optimistic. You don't check for the exception > class, and pray that the bits will align? ;-) Doh! Missed the EC check. > Please don't add more small includes like this. It makes things hard > to read and hides bugs Ack. > the counter read can (and will) be speculated, > so you definitely need an ISB before the access. Please also look at > __arch_counter_get_cntpct() and arch_counter_enforce_ordering(). Wouldn't it be up to the guest to decide if it wants the counter to be speculated or not? I debated this a bit myself in the implementation, as we would trap both ordered and un-ordered reads. Well, no way to detect it :) > > > > -/* > > - * Should only be called on non-VHE systems. > > - * VHE systems use EL2 timers and configure EL1 timers in kvm_timer_init_vhe(). > > - */ > > This comment still applies. this function *is* nVHE specific. > Ack. > > You now pay the price of reprogramming CNTHCTL_EL2 on every entry for > VHE, and that's not right. On VHE, it should be enough to perform the > programming on vcpu_load() only. > True. I'll rework the programming in the next series. > Overall, this patch needs a bit of splitting up (userspace interface, > HV rework), re-optimise VHE, and it *definitely* wants the ECV support > for the physical timer. > Sure thing, thanks for the review Marc! -- Best, Oliver _______________________________________________ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm