linux-kernel - Re: [PATCH v6 2/7] KVM: VMX: Add proper cache tracking for PKRS

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <YkTAzCPZ3zXYDBLj@google.com>
Date:   Wed, 30 Mar 2022 20:42:52 +0000
From:   Sean Christopherson <seanjc@...gle.com>
To:     Chenyi Qiang <chenyi.qiang@...el.com>
Cc:     Paolo Bonzini <pbonzini@...hat.com>,
        Vitaly Kuznetsov <vkuznets@...hat.com>,
        Wanpeng Li <wanpengli@...cent.com>,
        Jim Mattson <jmattson@...gle.com>,
        Joerg Roedel <joro@...tes.org>,
        Xiaoyao Li <xiaoyao.li@...el.com>, kvm@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v6 2/7] KVM: VMX: Add proper cache tracking for PKRS

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@...el.com>
> ---

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