Re: [PATCH 6/8] kvm/x86: Add mem fault exit on EPT violations

From: Peter Xu <peterx@redhat.com>
To: Sean Christopherson <seanjc@google.com>
Cc: Anish Moorthy <amoorthy@google.com>,
	Paolo Bonzini <pbonzini@redhat.com>,
	Marc Zyngier <maz@kernel.org>,
	Oliver Upton <oliver.upton@linux.dev>,
	James Houghton <jthoughton@google.com>,
	Ben Gardon <bgardon@google.com>,
	David Matlack <dmatlack@google.com>,
	Ricardo Koller <ricarkol@google.com>,
	Chao Peng <chao.p.peng@linux.intel.com>,
	Axel Rasmussen <axelrasmussen@google.com>,
	kvm@vger.kernel.org, kvmarm@lists.linux.dev
Subject: Re: [PATCH 6/8] kvm/x86: Add mem fault exit on EPT violations
Date: Thu, 16 Feb 2023 17:55:48 -0500	[thread overview]
Message-ID: <Y+60dIGWIXEuHpwW@x1n> (raw)
In-Reply-To: <Y+0VK6vZpMqAQ2Dc@google.com>

Hi, Sean,

On Wed, Feb 15, 2023 at 09:23:55AM -0800, Sean Christopherson wrote:
> > @@ -4230,9 +4231,25 @@ static int __kvm_faultin_pfn(struct kvm_vcpu *vcpu, struct kvm_page_fault *fault
> >  	}
> >  
> >  	async = false;
> > -	fault->pfn = __gfn_to_pfn_memslot(slot, fault->gfn, false, false, &async,
> > -					  fault->write, &fault->map_writable,
> > -					  &fault->hva);
> > +	mem_fault_nowait = memory_faults_enabled(vcpu->kvm);
> > +
> > +	fault->pfn = __gfn_to_pfn_memslot(
> > +		slot, fault->gfn,
> > +		mem_fault_nowait,
> 
> Hrm, as prep work for this series, I think we should first clean up the pile o'
> bools.  This came up before when the "interruptible" flag was added[*].  We punted
> then, but I think it's time to bite the bullet, especially since "nowait" and
> "async" need to be mutually exclusive.
> 
> [*] https://lore.kernel.org/all/YrR9i3yHzh5ftOxB@google.com

IIUC at that time we didn't reach a consensus on how to rework it, and the
suggestion was using a bool and separate the cleanup (while the flag
cleanup got NACKed..), which makes sense to me.

Personally I like your other patch a lot:

https://lore.kernel.org/all/YrTNGVpT8Cw2yrnr@google.com/

The question is, after we merge the bools into a flag, do we still need a
struct* anyway?  If we can reach a consensus, I'll be happy to continue
working on the cleanup (by picking up your patch first).  Or if Anish would
like to take it over in any form that'll be also perfect.

Thanks,

-- 
Peter Xu