mbox series

[RFC,00/10] KVM: x86/mmu: simplify argument to kvm page fault handler

Message ID cover.1618914692.git.isaku.yamahata@intel.com (mailing list archive)
Headers show
Series KVM: x86/mmu: simplify argument to kvm page fault handler | expand

Message

Isaku Yamahata April 20, 2021, 10:39 a.m. UTC
This is a preliminary clean up for TDX which complicates KVM page fault
execution path. simplify those execution path as preparation.

The current kvm page fault handlers passes around many arguments to the
functions.
To simplify those arguments and local variables, introduce data structure,
struct kvm_page_fault,  to hold those arguments, variables, and passes around the pointer
to struct kvm_page_fault.

struct kvm_page_fault is allocated on stack on the caller of kvm fault handler,
kvm_mmu_do_page_fault().
And then  push down the pointer to inner functions step by step.
The conversion order is as follows
. kvm_mmu_do_page_fault() with introducing struct kvm_page_fault
. kvm_mmu.page_fault(): kvm_tdp_page_fault(), nonpaging_page_fault(), FNAME(page_fault)
. direct_page_fault()
. try_async_pf()
. page_fault_handle_page_track()
. handle_abnormal_pfn()
. fast_page_fault()
. __direct_map()
. kvm_tdp_mmu_map()
. FNAME(fetch)

Probably more functions should be converted. or some should not converted.
Only code refactoring and no functional change is intended.

Isaku Yamahata (10):
  KVM: x86/mmu: make kvm_mmu_do_page_fault() receive single argument
  KVM: x86/mmu: make kvm_mmu:page_fault receive single argument
  KVM: x86/mmu: make direct_page_fault() receive single argument
  KVM: x86/mmu: make try_async_pf() receive single argument
  KVM: x86/mmu: make page_fault_handle_page_track() receive single
    argument
  KVM: x86/mmu: make handle_abnormal_pfn() receive single argument
  KVM: x86/mmu: make fast_page_fault() receive single argument
  KVM: x86/mmu: make __direct_map() receive single argument
  KVM: x86/mmu: make kvm_tdp_mmu_map() receive single argument
  KVM: x86/mmu: make FNAME(fetch) receive single argument

 arch/x86/include/asm/kvm_host.h |   4 +-
 arch/x86/kvm/mmu.h              |  49 ++++++++++--
 arch/x86/kvm/mmu/mmu.c          | 130 +++++++++++++++++---------------
 arch/x86/kvm/mmu/paging_tmpl.h  |  60 +++++++--------
 arch/x86/kvm/mmu/tdp_mmu.c      |  21 +++---
 arch/x86/kvm/mmu/tdp_mmu.h      |   4 +-
 arch/x86/kvm/x86.c              |   4 +-
 7 files changed, 156 insertions(+), 116 deletions(-)

Comments

Sean Christopherson May 26, 2021, 9:10 p.m. UTC | #1
On Tue, Apr 20, 2021, Isaku Yamahata wrote:
> This is a preliminary clean up for TDX which complicates KVM page fault
> execution path.

Ooh, a series to complicate the page fault path!  ;-)

Grammatical snarkiness aside, I'm all in favor of adding a struct to collect the
page fault collateral.  Overarching feedback:

  - Have kvm_mmu_do_page_fault() handle initialization of the struct.  That
    will allow making most of the fields const, and will avoid the rather painful
    kvm_page_fault_init().

  - Pass @vcpu separately.  Yes, it's associated with the fault, but literally
    the first line in every consumer is "struct kvm_vcpu *vcpu = kpf->vcpu;".

  - Use "fault" instead of "kpf", mostly because it reads better for people that
    aren't intimately familiar with the code, but also to avoid having to refactor
    a huge amount of code if we decide to rename kvm_page_fault, e.g. if we decide
    to use that name to return fault information to userspace.

  - Snapshot anything that is computed in multiple places, even if it is
    derivative of existing info.  E.g. it probably makes sense to grab
    write/fetch (or exec).


E.g. I'm thinking something like

struct kvm_page_fault {
	const gpa_t cr2_or_gpa;
	const u32 error_code;
	const bool write;
	const bool read;
	const bool fetch;
	const bool prefault;
	const bool is_tdp;

	gfn_t gfn;
	hva_t hva;
	int max_level;

	kvm_pfn_t pfn;
	bool map_writable;
};

int kvm_tdp_page_fault(struct kvm_vcpu *vcpu, struct kvm_page_fault *fault);

static inline int kvm_mmu_do_page_fault(struct kvm_vcpu *vcpu, gpa_t cr2_or_gpa,
					u32 err, bool prefault)
{
	struct kvm_page_fault fault = {
		.cr2_or_gpa = cr2_or_gpa,
		.error_code = err,
		.write	    = err & PFERR_WRITE_MASK,
		.fetch	    = err & PFERR_FETCH_MASK,
		.perm	    = ...
		.rsvd	    = err & PFERR_RSVD_MASK,

		.is_tdp	    = vcpu->arch.mmu->page_fault == kvm_tdp_page_fault,

		...
	};

#ifdef CONFIG_RETPOLINE
	if (likely(fault.is_tdp))
		return kvm_tdp_page_fault(vcpu, &fault);
#endif
	return vcpu->arch.mmu->page_fault(vcpu, &fault);
}
Paolo Bonzini June 10, 2021, 12:45 p.m. UTC | #2
On 26/05/21 23:10, Sean Christopherson wrote:
>    - Have kvm_mmu_do_page_fault() handle initialization of the struct.  That
>      will allow making most of the fields const, and will avoid the rather painful
>      kvm_page_fault_init().
> 
>    - Pass @vcpu separately.  Yes, it's associated with the fault, but literally
>      the first line in every consumer is "struct kvm_vcpu *vcpu = kpf->vcpu;".
> 
>    - Use "fault" instead of "kpf", mostly because it reads better for people that
>      aren't intimately familiar with the code, but also to avoid having to refactor
>      a huge amount of code if we decide to rename kvm_page_fault, e.g. if we decide
>      to use that name to return fault information to userspace.
> 
>    - Snapshot anything that is computed in multiple places, even if it is
>      derivative of existing info.  E.g. it probably makes sense to grab

I agree with all of these (especially it was a bit weird not to see vcpu 
in the prototypes).  Thanks Sean for the review!

Paolo
Isaku Yamahata June 10, 2021, 10 p.m. UTC | #3
Thanks for feedback. Let me respin it.

On Thu, Jun 10, 2021 at 02:45:55PM +0200,
Paolo Bonzini <pbonzini@redhat.com> wrote:

> On 26/05/21 23:10, Sean Christopherson wrote:
> >    - Have kvm_mmu_do_page_fault() handle initialization of the struct.  That
> >      will allow making most of the fields const, and will avoid the rather painful
> >      kvm_page_fault_init().
> > 
> >    - Pass @vcpu separately.  Yes, it's associated with the fault, but literally
> >      the first line in every consumer is "struct kvm_vcpu *vcpu = kpf->vcpu;".
> > 
> >    - Use "fault" instead of "kpf", mostly because it reads better for people that
> >      aren't intimately familiar with the code, but also to avoid having to refactor
> >      a huge amount of code if we decide to rename kvm_page_fault, e.g. if we decide
> >      to use that name to return fault information to userspace.
> > 
> >    - Snapshot anything that is computed in multiple places, even if it is
> >      derivative of existing info.  E.g. it probably makes sense to grab
> 
> I agree with all of these (especially it was a bit weird not to see vcpu in
> the prototypes).  Thanks Sean for the review!
> 
> Paolo
>