Re: [PATCH] KVM: x86/mmu: Embed direct bits into gpa for KVM_PRE_FAULT_MEMORY

[Sean Christopherson <seanjc@google.com>]

On Thu, Jun 12, 2025, Rick P Edgecombe wrote:
> On Thu, 2025-06-12 at 20:20 +0800, Yan Zhao wrote:
> > What about passing is is_private instead?  
> > 
> > static inline bool kvm_is_mirror_fault(struct kvm *kvm, bool is_private)
> > {
> >  	return kvm_has_mirrored_tdp(kvm) && is_private;
> > }
> > 
> > tdp_mmu_get_root_for_fault() and kvm_tdp_mmu_gpa_is_mapped() can pass in
> > faul->is_private or is_private directly, leaving the parsing of error_code &
> > PFERR_PRIVATE_ACCESS only in kvm_mmu_do_page_fault().
> 
> General question about the existing code...
> 
> Why do we have the error code bits separated out into bools in struct
> kvm_page_fault? It transitions between:
> 1. Native exit info (exit qualification, AMD error code, etc)

This step should be obvious :-)

> 2. Synthetic error codes
> 3. struct kvm_page_fault bools *and* synthetic error code.

A few reasons.

 a. The error_code is used in other paths, e.g. see the PFERR_IMPLICIT_ACCESS
    usage in emulator_write_std(), and the @access parameter from FNAME(gva_to_gpa)
    to FNAME(walk_addr_generic) (which is why FNAME(walk_addr) takes a sanitized
    "access", a.k.a. error code, instead of e.g. kvm_page_fault.
 b. Keeping the entire error code allowed adding kvm_page_fault without having
    to churn *everything*.
 c. Preserving the entire error code simplifies the handoff to async #PF.
 d. Unpacking error_code into bools makes downstream code much cleaner, e.g.
    page_fault_can_be_fast() is a good example.
 e. Waiting until kvm_mmu_do_page_fault() to fill kvm_page_fault deduplicates a
    _lot_ of boilerplate, and allows for many fields to be "const".
 f. I really, really want to make (most of) kvm_page_fault a structure that's
    common to all architectures, at which point tracking e.g. exec, read, write,
    etc. using bool is pretty much the only sane option.

> Why don't we go right to struct kvm_page_fault bools? Or just leave the
> synthetic error code in struct kvm_page_fault and refer to it? Having both in
> struct kvm_page_fault seems wrong, at least.

I actually like it.  It's like having both the raw and decoded information for
CPUID or RDMSR output.  All of the relevant fields are "const", so there's very
little chance of the state becoming out of sync.

I suppose an alternative would be to create union+bitfield overlay, but that
wouldn't work if/when pieces of kvm_page_fault are shared with other architectures,
and even without that angle in play, I think I actually prefer manually filling
bools.