Received-SPF: pass (google.com: domain of linux-kernel+bounces-180661-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Date: Thu, 16 May 2024 13:52:19 +0800
From: Yan Zhao <yan.y.zhao@intel.com>
To: "Huang, Kai" <kai.huang@intel.com>
CC: "Edgecombe, Rick P" <rick.p.edgecombe@intel.com>, "kvm@vger.kernel.org"
	<kvm@vger.kernel.org>, "pbonzini@redhat.com" <pbonzini@redhat.com>,
	"seanjc@google.com" <seanjc@google.com>, "sagis@google.com"
	<sagis@google.com>, "linux-kernel@vger.kernel.org"
	<linux-kernel@vger.kernel.org>, "Aktas, Erdem" <erdemaktas@google.com>,
	"isaku.yamahata@gmail.com" <isaku.yamahata@gmail.com>, "dmatlack@google.com"
	<dmatlack@google.com>
Subject: Re: [PATCH 04/16] KVM: x86/mmu: Add address conversion functions for
 TDX shared bit of GPA
Message-ID: <ZkWfE2KSpPgv6RND@yzhao56-desk.sh.intel.com>
Reply-To: Yan Zhao <yan.y.zhao@intel.com>
References: <fe9687d5f17fa04e5e15fdfd7021fa6e882d5e37.camel@intel.com>
 <465b8cb0-4ce1-4c9b-8c31-64e4a503e5f2@intel.com>
 <bf1038ae56693014e62984af671af52a5f30faba.camel@intel.com>
 <4e0968ae-11db-426a-b3a4-afbd4b8e9a49@intel.com>
 <0a168cbcd8e500452f3b6603cc53d088b5073535.camel@intel.com>
 <6df62046-aa3b-42bd-b5d6-e44349332c73@intel.com>
 <1270d9237ba8891098fb52f7abe61b0f5d02c8ad.camel@intel.com>
 <d5c37fcc-e457-43b0-8905-c6e230cf7dda@intel.com>
 <0a1afee57c955775bef99d6cf34fd70a18edb869.camel@intel.com>
 <d8ff5e19-85a7-46bf-9dad-7221b54d8502@intel.com>
Content-Type: text/plain; charset="iso-8859-1"
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <d8ff5e19-85a7-46bf-9dad-7221b54d8502@intel.com>
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?iso-8859-1?Q?zD5jUxnDPYxZB6ezqZ7D0l2LvuSyWrBvEuSroiAI9K5VPTgRu8m28ejlAZ?=
 =?iso-8859-1?Q?uHD+zaCbbVFPCbM3yX25dmWpfRqz5E5O48pDCiOj1ZDFMtRCpvuN8cA/yf?=
 =?iso-8859-1?Q?ZokdQl4UVvhBi29zwgAXJNpoSa5lfMvIT7qW7epHJhrMh8NI3QSA1Ri31p?=
 =?iso-8859-1?Q?LyQ8sXUuiZSNUuuKEZCtNSH56yp1fQ0DRfK9X/fGdZpVtqb3BwtsKZ3/56?=
 =?iso-8859-1?Q?mquSrs/oRDH8eE7KEBbgyg3U6py5y2vH2V/IzKQZ1FG0Nk4ik0B2vwTgst?=
 =?iso-8859-1?Q?szRAdd/xUGiVkdMPSkrmdLL6xVNHH3/60Z3MGFkUw+LuXxH6/dEzX/KFD0?=
 =?iso-8859-1?Q?wCXGnrceOf8kTS49HudhDnZNnHBceY6pqqnh6jlHonmTDLRK7PvauyxAMn?=
 =?iso-8859-1?Q?+rFzb0zzpY1qPkY8Lo0hn7WNf3yK3O2RIffEtu7gPvSFU6siPcwJqcA04b?=
 =?iso-8859-1?Q?rJbLwewWpryD/YL1XHSYccoCi4doscpVxXqcyjoJ/yj9WB1YeGaq45P/sX?=
 =?iso-8859-1?Q?NSs//hRK9+H7HO7YbluvHqXr5Ef0qwo0GduoY/2HWJsKkZKt1dz4V0qsqn?=
 =?iso-8859-1?Q?3YMPMVMgZ6spOr8oMpPvQ10nicZaJO8JQbn0a9jsfzgcOKf8GfDSMlE/xo?=
 =?iso-8859-1?Q?1+gROcx9MiG8yFt6jpEUG1mwOt+tx7vsJGCWi2bxbIw30tFjyBtZShFret?=
 =?iso-8859-1?Q?Cuyymv4miSNZuzHk9Lhk9rmrX8DTdz0NJ96yea8hp8HfUX5CsAQI+t8kDJ?=
 =?iso-8859-1?Q?/Boo+LlYIEJjMniZePhgnnM2Nfimqn1pw6qlBotWUySYP/GBpg7NkCrGz/?=
 =?iso-8859-1?Q?rZKH8BH4cZZDGj+2depePEUOT+7TDGoTEEwxm0YtWYH5SjPEyWza+LAtXK?=
 =?iso-8859-1?Q?aYPfCnYMBK+pEAtNfTz3ie0u0T00J1Lq+MFZ+3QoEF9IYCGQSVDNZo8OJQ?=
 =?iso-8859-1?Q?ngRb+MJ5TDGOs5uKo96ivxdgrxYIBByP9zD5BCoLy4oDsCRBSSjup7qrd5?=
 =?iso-8859-1?Q?jHapuC8cb1vfTxNwaLeVp6u+42yIiAEK4ifZfR+Pwo/+x3jo2A2ROjrP5P?=
 =?iso-8859-1?Q?D4iCQZke1pVADt0k6CssMmNdGiDOFigx/XOtZCc5vtQtIW+vkJounK9fvb?=
 =?iso-8859-1?Q?0CJ77l5btMqHFM9GkUKiGPRLk11HinKDo1wwQp+THYZfyhVT025w6RVUnt?=
 =?iso-8859-1?Q?KpR5QfUxwSEZZ7r/+d8VpSPx+fGEriITsf/ND5ed3/mIHdx2qNLELov7IV?=
 =?iso-8859-1?Q?5OOXIcl8ivwIymzBSvkPowXsKlrR+WwDEdgE6s7LqxQNjzsy2CnHYCLdiU?=
 =?iso-8859-1?Q?hR54DyOdq4be88EFwhrwDKEwfQVy3Pd/aBoyipidZ5hJIDKlQvEhvWW5/x?=
 =?iso-8859-1?Q?HNJFwpePqgc6uEi/HXLtFEdrtCTTuqtVIFXhY53uWCwu3+he9GGH0sABpZ?=
 =?iso-8859-1?Q?8znoPtGrdiRZW4HKu0qj2s3UYyrmtzhaiEPtCAiJX02B4h9ThyRvC826QX?=
 =?iso-8859-1?Q?4TqHAOEtL/IbqBgdezVyarPMVFN6zV9mg7mg+YjeKQmeNnnmlwaEFJZqkL?=
 =?iso-8859-1?Q?VqeKgUO04h/oidxCXU6PAgUu3u38kSJV0xSYu+CPyTl5d245JgzacMyqMz?=
 =?iso-8859-1?Q?6sSnDJ/y4ymFlomsIW9D40o7h/QQHwR5es?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 83b5403d-8506-4250-5158-08dc756c7502
X-MS-Exchange-CrossTenant-AuthSource: DS7PR11MB5966.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 16 May 2024 05:53:06.0936
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: AO8bOhk0r3sudzKYyZmrcjUBdLQZo09zltpL4NVlsNW6qa6C8g36p51g3dp7r8Wqp1/V7lgvn4FnM2CqEm12jw==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SN7PR11MB7089
X-OriginatorOrg: intel.com

On Thu, May 16, 2024 at 01:40:41PM +1200, Huang, Kai wrote:
> 
> 
> On 16/05/2024 1:20 pm, Edgecombe, Rick P wrote:
> > On Thu, 2024-05-16 at 13:04 +1200, Huang, Kai wrote:
> > > 
> > > I really don't see difference between ...
> > > 
> > >  ????????is_private_mem(gpa)
> > > 
> > > ... and
> > > 
> > >  ????????is_private_gpa(gpa)
> > > 
> > > If it confuses me, it can confuses other people.
> > 
> > Again, point taken. I'll try to think of a better name. Please share if you do.
> > 
> > > 
> > > The point is there's really no need to distinguish the two.? The GPA is
> > > only meaningful when it refers to the memory that it points to.
> > > 
> > > So far I am not convinced we need this helper, because such info we can
> > > already get from:
> > > 
> > >  ?? 1) fault->is_private;
> > >  ?? 2) Xarray which records memtype for given GFN.
> > > 
> > > So we should just get rid of it.
> > 
> > Kai, can you got look through the dev branch a bit more before making the same
> > point on every patch?
> > 
> > kvm_is_private_gpa() is used to set PFERR_PRIVATE_ACCESS, which in turn sets
> > fault->is_private. So you are saying we can use these other things that are
> > dependent on it. Look at the other callers too.
> 
> Well, I think I didn't make myself clear.
> 
> I don't object to have this helper.  If it helps, then we can have it.
> 
> My objection is the current implementation of it, because it is
> *conceptually* wrong for SEV-SNP.
> 
> Btw, I just look at the dev branch.
> 
> For the common code, it is used in kvm_tdp_mmu_map() and
> kvm_tdp_mmu_fast_pf_get_last_sptep() to get whether a GPA is private.
> 
> As said above, I don't see why we need a helper with the "current
> implementation" (which consults kvm_shared_gfn_mask()) for them.  We can
> just use fault->gfn + fault->is_private for such purpose.
What about a name like kvm_is_private_and_mirrored_gpa()?
Only TDX's private memory is mirrored and the common code needs a way to
tell that.


> It is also used in the TDX code like TDX variant handle_ept_violation() and
> tdx_vcpu_init_mem_region().  For them to be honest I don't quite care
> whether a helper is used.  We can have a helper if we have multiple callers,
> but this helper should be in TDX code, but not common MMU code.
>