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 868D6C47080 for ; Mon, 24 May 2021 04:58:27 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 685DF61132 for ; Mon, 24 May 2021 04:58:27 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S232211AbhEXE7S (ORCPT ); Mon, 24 May 2021 00:59:18 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:44918 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229824AbhEXE7Q (ORCPT ); Mon, 24 May 2021 00:59:16 -0400 Received: from mail-pf1-x430.google.com (mail-pf1-x430.google.com [IPv6:2607:f8b0:4864:20::430]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 39E6BC061756 for ; Sun, 23 May 2021 21:57:48 -0700 (PDT) Received: by mail-pf1-x430.google.com with SMTP id g18so18181453pfr.2 for ; Sun, 23 May 2021 21:57:48 -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=5elIPDTXRjLLPudcp8o02c7J6ZLjvAHG6TdyshBpqHM=; b=GWDeEevlJHCXyEL1BH5HMf5da6qvir0//wvvQybCoUH2K+Q2NqDqUCJp7MH7jOQI5g QKF/OOjUn+h+YmGjjHES0dGefuvap12Tr8lrjCxE8s9e2bId60fT490TEHBs1vtN0a+r ix9YhH1hTViU72dRKmc8cZl6nidX1fM/fhPBfsHWblsJywSPXlJUzOrdRS6tMMbvAVKK O2yLR1XwJth/8XHsuLJ+Rz2NUGlU1Q5IXK3Ttw3+cve/Uxqf7CqhvPJkSpNwfnn+KSbA c20a31xQuZGDFCMpLBACa4GOfpEz6kHEUw2PWJeJ/kmKJSPTXWADzF2OOQHpc+G0p1s1 UOfg== 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=5elIPDTXRjLLPudcp8o02c7J6ZLjvAHG6TdyshBpqHM=; b=RcD7Tx7UmuUVe2tk6Ratn5ywqjzPoDaRxStfMqTxKebcx7AirRiINqqYoMPgLGkyxN kZD91xLBVO/SjK7wvRdPy+n50SlgOKN2KWeSCUGUbirpXfJ0i2UwZ/Kw6JUlkV6bHMFM FbKgPXYiQFyM0a3XY8yey327GbhiXvDscO+tHuqcDMBalukhj33a1Pq4Zp7vaa7jsqbw 4udTgvrLNaMC7vJA0EFEtgqfTiE7J2R7K47ibC5aUWw3rLio0Pkqw/Kg6U41wrckIZ/A OuBmybIC3o4Mg6P7wSLtgZ3zeb8YSOh9InY9xmr2TK7w1kO0IcM+DFSalMu7Qs/okIH7 LQtw== X-Gm-Message-State: AOAM5322vpuoF/Ruvev7pgyTF455l5KF0zNDVDtbxg9xug7mFkzH8GKI N6y5hqhgWfO2MSaLD+ofYACz3PYn7G5s0mXcRV7VTw== X-Google-Smtp-Source: ABdhPJz2s/6JAGw+NL3QrTHg+jeS1T5q4PDbtAvgk6IlTF0h9XB36MnFhjRoAg+6LOvjbpKDPjYCubpOO5MD1FRK2x0= X-Received: by 2002:a63:1e4f:: with SMTP id p15mr11633324pgm.40.1621832267459; Sun, 23 May 2021 21:57:47 -0700 (PDT) MIME-Version: 1.0 References: <20210424004645.3950558-1-seanjc@google.com> <20210424004645.3950558-7-seanjc@google.com> In-Reply-To: From: Reiji Watanabe Date: Sun, 23 May 2021 21:57:31 -0700 Message-ID: Subject: Re: [PATCH 06/43] KVM: x86: Properly reset MMU context at vCPU RESET/INIT To: Sean Christopherson Cc: Paolo Bonzini , Vitaly Kuznetsov , Wanpeng Li , Jim Mattson , Joerg Roedel , kvm@vger.kernel.org, linux-kernel@vger.kernel.org Content-Type: text/plain; charset="UTF-8" Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Wed, May 19, 2021 at 10:16 AM Sean Christopherson wrote: > > On Tue, May 18, 2021, Reiji Watanabe wrote: > > > > > + if (kvm_cr0_mmu_role_changed(old_cr0, kvm_read_cr0(vcpu)) || > > > > > + kvm_cr4_mmu_role_changed(old_cr4, kvm_read_cr4(vcpu))) > > > > > + kvm_mmu_reset_context(vcpu); > > > > > } > > > > > > > > I'm wondering if kvm_vcpu_reset() should call kvm_mmu_reset_context() > > > > for a change in EFER.NX as well. > > > > > > Oooh. So there _should_ be no need. Paging has to be enabled for EFER.NX to > > > be relevant, and INIT toggles CR0.PG 1=>0 if paging was enabled and so is > > > guaranteed to trigger a context reset. And we do want to skip the context reset, > > > e.g. INIT-SIPI-SIPI when the vCPU has paging disabled should continue using the > > > same MMU. > > > > > > But, kvm_calc_mmu_role_common() neglects to ignore NX if CR0.PG=0, and so the > > > MMU role will be stale if INIT clears EFER.NX without forcing a context reset. > > > However, that's benign from a functionality perspective because the context > > > itself correctly incorporates CR0.PG, it's only the role that's borked. I.e. > > > KVM will fail to reuse a page/context due to the spurious role.nxe, but the > > > permission checks are always be correct. > > > > > > I'll add a comment here and send a patch to fix the role calculation. > > > > Thank you so much for the explanation ! > > I understand your intention and why it would be benign. > > > > Then, I'm wondering if kvm_cr4_mmu_role_changed() needs to be > > called here. Looking at the Intel SDM, in my understanding, > > all the bits kvm_cr4_mmu_role_changed() checks are relevant > > only if paging is enabled. (Or is my understanding incorrect ??) > > Duh, yes. And it goes even beyond that, CR0.WP is only relevant if CR0.PG=1, > i.e. INIT with CR0.PG=0 and CR0.WP=1 will incorrectly trigger a MMU reset with > the current logic. > > Sadly, simply omitting the CR4 check puts us in an awkward situation where, due > to the MMU role CR4 calculations not accounting for CR0.PG=0, KVM will run with > a stale role. > > The other consideration is that kvm_post_set_cr4() and kvm_post_set_cr0() should > also skip kvm_mmu_reset_context() if CR0.PG=0, but again that requires fixing > the role calculations first (or at the same time). > > I think I'll throw in those cleanups to the beginning of this series. The result > is going to be disgustingly long, but I really don't want to introduce code that > knowingly leaves KVM in an inconsistent state, nor do I want to add useless > checks on CR4 and EFER. Yes, I would think having the cleanups would be better. Thank you ! Reiji