Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:18 as permitted sender) client-ip=2620:137:e000::1:18;
Date:   Wed, 30 Mar 2022 20:42:52 +0000
From:   Sean Christopherson <seanjc@google.com>
To:     Chenyi Qiang <chenyi.qiang@intel.com>
Cc:     Paolo Bonzini <pbonzini@redhat.com>,
        Vitaly Kuznetsov <vkuznets@redhat.com>,
        Wanpeng Li <wanpengli@tencent.com>,
        Jim Mattson <jmattson@google.com>,
        Joerg Roedel <joro@8bytes.org>,
        Xiaoyao Li <xiaoyao.li@intel.com>, kvm@vger.kernel.org,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH v6 2/7] KVM: VMX: Add proper cache tracking for PKRS
Message-ID: <YkTAzCPZ3zXYDBLj@google.com>
References: <20220221080840.7369-1-chenyi.qiang@intel.com>
 <20220221080840.7369-3-chenyi.qiang@intel.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20220221080840.7369-3-chenyi.qiang@intel.com>
Precedence: bulk

On Mon, Feb 21, 2022, Chenyi Qiang wrote:
> Add PKRS caching into the standard register caching mechanism in order
> to take advantage of the availability checks provided by regs_avail.
> 
> This is because vcpu->arch.pkrs will be rarely acceesed by KVM, only in
> the case of host userspace MSR reads and GVA->GPA translation in
> following patches. It is unnecessary to keep it up-to-date at all times.

It might be worth throwing in a blurb that the potential benefits of this caching
are tenous.

Barring userspace wierdness, the MSR read is not a hot path.

permission_fault() is slightly more common, but I would be surprised if caching
actually provides meaningful performance benefit.  The PKRS checks are done only
once per virtual access, i.e. only on the final translation, so the cache will get
a hit if and only if there are multiple translations in a single round of emulation,
where a "round of emulation" ends upon entry to the guest.  With unrestricted
guest, i.e. for all intents and purposes every VM using PKRS, there aren't _that_
many scenarios where KVM will (a) emulate in the first place and (b) emulate enough
accesses for the caching to be meaningful.

That said, this is basically "free", so I've no objection to adding it.  But I do
think it's worth documenting that it's nice-to-have so that we don't hesitate to
rip it out in the future if there's a strong reason to drop the caching.

> Signed-off-by: Chenyi Qiang <chenyi.qiang@intel.com>
> ---

Reviewed-by: Sean Christopherson <seanjc@google.com>