linux-kernel - Re: [PATCH v8 05/15] KVM: pfncache: remove KVM_GUEST_USES

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]

Date:   Tue, 21 Nov 2023 22:24:08 +0000
From:   David Woodhouse <dwmw2@...radead.org>
To:     Paul Durrant <paul@....org>,
        Sean Christopherson <seanjc@...gle.com>,
        Paolo Bonzini <pbonzini@...hat.com>,
        Thomas Gleixner <tglx@...utronix.de>,
        Ingo Molnar <mingo@...hat.com>, Borislav Petkov <bp@...en8.de>,
        Dave Hansen <dave.hansen@...ux.intel.com>, x86@...nel.org,
        "H. Peter Anvin" <hpa@...or.com>, kvm@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v8 05/15] KVM: pfncache: remove KVM_GUEST_USES_PFN usage

On Tue, 2023-11-21 at 18:02 +0000, Paul Durrant wrote:
> From: Paul Durrant <pdurrant@...zon.com>
> 
> As noted in [1] the KVM_GUEST_USES_PFN usage flag is never set by any
> callers of kvm_gpc_init(), which also makes the 'vcpu' argument redundant.
> Moreover, all existing callers specify KVM_HOST_USES_PFN so the usage
> check in hva_to_pfn_retry() and hence the 'usage' argument to
> kvm_gpc_init() are also redundant.
> Remove the pfn_cache_usage enumeration and remove the redundant arguments,
> fields of struct gfn_to_hva_cache, and all the related code.
> 
> [1] https://lore.kernel.org/all/ZQiR8IpqOZrOpzHC@google.com/
> 
> Signed-off-by: Paul Durrant <pdurrant@...zon.com>

I think it's https://lore.kernel.org/all/ZBEEQtmtNPaEqU1i@google.com/
which is the key reference. I'm not sure I'm 100% on board, but I never
got round to replying to Sean's email because it was one of those "put
up or shut up situations" and I didn't have the bandwidth to actually
write the code to prove my point.

I think it *is* important to support non-pinned pages. There's a reason
we even made the vapic page migratable. We want to support memory
hotplug, we want to cope with machine checks telling us to move certain
pages (which I suppose is memory hotplug). See commit 38b9917350cb
("kvm: vmx: Implement set_apic_access_page_addr") for example.

I agree that in the first round of the nVMX code there were bugs. And
sure, of *course* it isn't sufficient to wire up the invalidation
without either a KVM_REQ_SOMETHIMG to put it back, or just a *check* on
the corresponding gpc on the way back into the guest. We'd have worked
that out.

And yes, the gpc has had bugs as we implemented it, but the point was
that we got to something which *is* working, and forms a usable
building block.

So I'm not really sold on the idea of ditching KVM_GUEST_USES_PFN. I
think we could get it working, and I think it's worth it. But my
opinion is worth very little unless I express it in 'diff -up' form
instead of prose, and reverting this particular patch is the least of
my barriers to doing so, so reluctantly...

Reviewed-by: David Woodhouse <dwmw@...zon.co.uk>

Download attachment "smime.p7s" of type "application/pkcs7-signature" (5965 bytes)