Received-SPF: pass (google.com: domain of linux-kernel+bounces-160284-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Date: Fri, 26 Apr 2024 08:28:23 -0700
In-Reply-To: <970c8891af05d0cb3ccb6eab2d67a7def3d45f74.camel@intel.com>
Precedence: bulk
Mime-Version: 1.0
References: <cover.1708933498.git.isaku.yamahata@intel.com>
 <e324ff5e47e07505648c0092a5370ac9ddd72f0b.1708933498.git.isaku.yamahata@intel.com>
 <2daf03ae-6b5a-44ae-806e-76d09fb5273b@linux.intel.com> <20240313171428.GK935089@ls.amr.corp.intel.com>
 <52bc2c174c06f94a44e3b8b455c0830be9965cdf.camel@intel.com>
 <1d1da229d4bd56acabafd2087a5fabca9f48c6fc.camel@intel.com>
 <20240319215015.GA1994522@ls.amr.corp.intel.com> <CA+EHjTxFZ3kzcMCeqgCv6+UsetAUUH4uSY_V02J1TqakM=HKKQ@mail.gmail.com>
 <970c8891af05d0cb3ccb6eab2d67a7def3d45f74.camel@intel.com>
Message-ID: <ZivIF9vjKcuGie3s@google.com>
Subject: Re: [PATCH v19 011/130] KVM: Add new members to struct kvm_gfn_range
 to operate on
From: Sean Christopherson <seanjc@google.com>
To: Rick P Edgecombe <rick.p.edgecombe@intel.com>
Cc: "tabba@google.com" <tabba@google.com>, Isaku Yamahata <isaku.yamahata@intel.com>, 
	Tina Zhang <tina.zhang@intel.com>, Kai Huang <kai.huang@intel.com>, 
	"binbin.wu@linux.intel.com" <binbin.wu@linux.intel.com>, Bo2 Chen <chen.bo@intel.com>, 
	"sagis@google.com" <sagis@google.com>, 
	"isaku.yamahata@linux.intel.com" <isaku.yamahata@linux.intel.com>, 
	"isaku.yamahata@gmail.com" <isaku.yamahata@gmail.com>, Erdem Aktas <erdemaktas@google.com>, 
	"kvm@vger.kernel.org" <kvm@vger.kernel.org>, "pbonzini@redhat.com" <pbonzini@redhat.com>, 
	Hang Yuan <hang.yuan@intel.com>, 
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="us-ascii"

On Fri, Apr 26, 2024, Rick P Edgecombe wrote:
> On Fri, 2024-04-26 at 08:39 +0100, Fuad Tabba wrote:
> > > I'm fine with those names. Anyway, I'm fine with wither way, two bools or
> > > enum.
> > 
> > I don't have a strong opinion, but I'd brought it up in a previous
> > patch series. I think that having two bools to encode three states is
> > less intuitive and potentially more bug prone, more so than the naming
> > itself (i.e., _only):

Hmm, yeah, I buy that argument.  We could even harded further by poisoning '0'
to force KVM to explicitly.  Aha!  And maybe use a bitmap?

	enum {
		BUGGY_KVM_INVALIDATION		= 0,
		PROCESS_SHARED			= BIT(0),
		PROCESS_PRIVATE			= BIT(1),
		PROCESS_PRIVATE_AND_SHARED	= PROCESS_SHARED | PROCESS_PRIVATE,
	};

> > https://lore.kernel.org/all/ZUO1Giju0GkUdF0o@google.com/
> 
> Currently in our internal branch we switched to:
> exclude_private
> exclude_shared
> 
> It came together bettter in the code that uses it.

If the choice is between an enum and exclude_*, I would strongly prefer the enum.
Using exclude_* results in inverted polarity for the code that triggers invalidations.

> But I started to wonder if we actually really need exclude_shared. For TDX
> zapping private memory has to be done with more care, because it cannot be re-
> populated without guest coordination. But for shared memory if we are zapping a
> range that includes both private and shared memory, I don't think it should hurt
> to zap the shared memory.

Hell no, I am not risking taking on more baggage in KVM where userspace or some
other subsystem comes to rely on KVM spuriously zapping SPTEs in response to an
unrelated userspace action.