Received-SPF: pass (google.com: domain of linux-kernel+bounces-180507-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Message-ID: <4e0968ae-11db-426a-b3a4-afbd4b8e9a49@intel.com>
Date: Thu, 16 May 2024 12:12:31 +1200
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH 04/16] KVM: x86/mmu: Add address conversion functions for
 TDX shared bit of GPA
To: "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>
CC: "sagis@google.com" <sagis@google.com>, "Aktas, Erdem"
	<erdemaktas@google.com>, "dmatlack@google.com" <dmatlack@google.com>, "Zhao,
 Yan Y" <yan.y.zhao@intel.com>, "isaku.yamahata@gmail.com"
	<isaku.yamahata@gmail.com>, "linux-kernel@vger.kernel.org"
	<linux-kernel@vger.kernel.org>
References: <20240515005952.3410568-1-rick.p.edgecombe@intel.com>
 <20240515005952.3410568-5-rick.p.edgecombe@intel.com>
 <9f315b56-7da8-428d-bc19-224e19f557e0@intel.com>
 <307f2360707e5c7377a898b544fabc7d72421572.camel@intel.com>
 <eb98d0e7-8fbd-40d2-a6b3-0bf98edb77f9@intel.com>
 <fe9687d5f17fa04e5e15fdfd7021fa6e882d5e37.camel@intel.com>
 <465b8cb0-4ce1-4c9b-8c31-64e4a503e5f2@intel.com>
 <bf1038ae56693014e62984af671af52a5f30faba.camel@intel.com>
Content-Language: en-US
From: "Huang, Kai" <kai.huang@intel.com>
In-Reply-To: <bf1038ae56693014e62984af671af52a5f30faba.camel@intel.com>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?WUJnd0NJZ3E0TXRVTzl6SkZ2R29tZ1RwelRIeGY5MzJZYlNvTVZmMEgrQW45?=
 =?utf-8?B?dFpXVlhGT2lXZzNielpWYTlwTHV2dVh6Y2pJMHpvZXBMdzYwODVuMXVpV3J0?=
 =?utf-8?B?TU5DVGs4NkxvOWVyeHRiem92NytlZVRtSzkySjRVWHVPSmRiVE1YajQ3ajZm?=
 =?utf-8?B?SHgzWGFkdVU2dlBkUStlTm9YVUx4WVBJc1NQWmw0OWZwemhTRk1XRzV2dVpQ?=
 =?utf-8?B?SE5hRlhRTHloejQvOFFyejFHa1Nwb1RSV2xuSTM4eUlqbGMwQ1BKRlRnd0lY?=
 =?utf-8?B?eUpObEtVVEd0Q1E2elN4Q1oyTmFyeVowQWduUTczVE9ySlVnMklZYWVOTjlH?=
 =?utf-8?B?cVAwalNnYjJSMWk2djlmRHZraWI3cXlQZkhhdCtzRk5WOEdmTDUvNkFSRk9W?=
 =?utf-8?B?VUJ3SHUrb2V6OVU5QkdJS29udVhFMjZpNGZGcFpHK2xjejc3OHV3cDJwTDc1?=
 =?utf-8?B?Q3luMWt4SXRtU3BnWnlzL0xSRGJaMVRBT1lmSVZqRFRjb1N5WVJoeGs1SlBL?=
 =?utf-8?B?Nk9WdXBWTS8wYzFiRmNicUlQM01ubFVuM3gwL1pJQmtkekc0L1gyeHhhRHg2?=
 =?utf-8?B?SE83OTlpdmVIanFnZkdhUUdZSWxldmx1RytDcnZva09nSlpJenlSaXhxdmhO?=
 =?utf-8?B?QUNBbllWdGRkSnZVbjh6V2tBSU9PM1kvNzlLRHRmeitoZ1k0REJUajVzeXN4?=
 =?utf-8?B?MDV3MThEZEp6N0phaUtuNU5NR1hEOEYyUXhDSTNtcnRYRENzNnlrUlhGOE9x?=
 =?utf-8?B?Q1RoOVNUNklFNjhjTEdUS21kZ3BWM1FRTTBUdm5NalNwU3QybFR6N1FZUkJX?=
 =?utf-8?B?aG1BMUo2eCt4dXA1MUx6VzU5djlzS25wdXpBMXNVbVAzNjVac2M5RnVRem92?=
 =?utf-8?B?dVdBVGpRYVNQZGtXek1UcjNITzg1Sy91b0l0cmJsVjNnYWVHVy93SWFiRXZh?=
 =?utf-8?B?dUhEVG9aOHpYc3l2YjNhR1ZxOStqZ0liZFNZK3l5cnh6eUN1ZWtKZlRJR3Nk?=
 =?utf-8?B?cTJVdG8ydUd1eWxZZXhEcERMTC9PNnBxZjFWMDgvZFVneVZDQkFJTDd3UnMz?=
 =?utf-8?B?bHc3bi95UEtmTkFYWVkveUNaMzVreDk3a29ia2pvd25ZYVN3dTZXUGhyYVZZ?=
 =?utf-8?B?TnNnTHFNZkc0NWtmUWJwOGZRTzRtRE1GbFdwVDQ2b0xlN3Z6L3dYb3R0dzcw?=
 =?utf-8?B?L09DVmQrdzFRU25vY0prV3NxZFlZLzBOTTkyK3dVY2JMeTlPMG5PUG43Nm9i?=
 =?utf-8?B?SmZPbVI3TGhuMjJza3piNHNLekR6cmZsYWo5UXpzMkJYeHJ4MVJSMzlaUjRW?=
 =?utf-8?B?TkY2a2I1RDI4L1M4SUF2ZXM4T1VVeUkzaUI5NEJpR0ZPUkpmYXVROGQ1bGhX?=
 =?utf-8?B?QUVic2N0cUJ6U1N5NCtTQnJmMEE4alJvandFTmlBUVBhK0tVUTlZOUZxRFRs?=
 =?utf-8?B?WlN5STJrK1h3NGtZdXhtY1FxQndZQ1d4eHJiNnZKNnRQU2s4YW1xbktFUWI5?=
 =?utf-8?B?L3R5MGg1WTJYVlN3U3kxUk45UkU4bk5xWGNqUHVydW5KaGNuOXdxKzhBSmxm?=
 =?utf-8?B?RGNxbUo3RWNQRnFEejExeWlSbG5GQ2NzcVArL2F3cU4vSHc4TXAwbElCQTRt?=
 =?utf-8?B?UWZQalNsRkZNZ2NEVUhHWTlRZDRlV3dNcUhwbEoyU3JsQy9BOXNuaEhKckNh?=
 =?utf-8?B?RHJOZnRKNDN6MVdWenpMcTEzdlVkMUZZRkk1Y1NPUmtZNXJoNkRIZ0Ivb2lh?=
 =?utf-8?B?emtXMEVvZEJ4QXQ1anQ4YU9EcEU1ZDRad1JtcVlTQy9MRERVdm1mNGZ2WVdy?=
 =?utf-8?B?dUlRNEVCYTJQRDBYaHBMOEd1bGxBQmJpdnVTOE1zcG43UExadkUwdWk5bW4y?=
 =?utf-8?B?UG5wQzZPcGZYdFhCS3ZXV1FaVnZyc3RjcWJRdlhZYzJSam83NlFTdmV6VFVS?=
 =?utf-8?B?bmVwRHA4TjY2ZWkrNnljQVh5MVVIMFU3bkN6RDY1ZGZkMkloUThsMGxaZGRC?=
 =?utf-8?B?V2hJblBEbTdMdzRCMjkvcC9HSUQ1ZFQ0S0t3MXdqU3ZzYlNSdU1VaEFqZUdo?=
 =?utf-8?B?SUlqMFdIZHQ5cjE5OHVsc0xob3dXeXpBMUNSUHRETW1BSTY0bnYzSVR1ekxm?=
 =?utf-8?Q?ULmvps1A5cFK5lLI1kWNB9f7H?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 2b7ac04a-ef8f-4bfc-5527-08dc753ce618
X-MS-Exchange-CrossTenant-AuthSource: BL1PR11MB5978.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 16 May 2024 00:12:40.0227
 (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: kfdROxOMUZig0lWGutuuS16i3HCV2TUtta3Hx+N5pCkNC6cxLnCN6fYNMe6OUw0ts2111R9il/U8W9mqiFqT+g==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: PH0PR11MB7493
X-OriginatorOrg: intel.com


On 16/05/2024 11:59 am, Edgecombe, Rick P wrote:
> On Thu, 2024-05-16 at 11:44 +1200, Huang, Kai wrote:
>>>
>>> Sorry, still not clear. We need to strip the bit away, so we need to know
>>> what
>>> bit it is. The proposal is to not remember it on struct kvm, so where do we
>>> get
>>> it?
>>
>> The TDX specific code can get it when TDX guest is created.
> 
> The TDX specific code sets it. It knows GPAW/shared bit location.
> 
>>
>>>
>>> Actually, we used to allow it to be selected (via GPAW), but now we could
>>> determine it based on EPT level and MAXPA. So we could possibly recalculate
>>> it
>>> in some helper...
>>>
>>> But it seems you are suggesting to do away with the concept of knowing what
>>> the
>>> shared bit is.
>>
>> What I am suggesting is essentially to replace this
>> kvm_gfn_shared_mask() with some kvm_x86_ops callback (which can just
>> return the shared bit), assuming the common code somehow still need it
>> (e.g., setting up the SPTE for shared mapping, which must include the
>> shared bit to the GPA).
>>
>> The advantage of this we can get rid of the concept of 'gfn_shared_mask'
>> in the MMU common code.  All GFNs referenced in the common code is the
>> actual GFN (w/o the shared bit).
> 
> When it is actually being used as the shared bit instead of as a way to check if
> a guest is a TD, what is the problem? I think the shared_mask serves a real
> (small) purpose, but it is misused for a bunch of other stuff. If we move that
> other stuff to new helpers, the shared mask will still be needed for it's
> original job.
> 
> What is the benefit of the x86_ops over a static inline?

I don't have strong objection if the use of kvm_gfn_shared_mask() is 
contained in smaller areas that truly need it.  Let's discuss in 
relevant patch(es).

However I do think the helpers like below makes no sense (for SEV-SNP):

+static inline bool kvm_is_private_gpa(const struct kvm *kvm, gpa_t gpa)
+{
+	gfn_t mask = kvm_gfn_shared_mask(kvm);
+
+	return mask && !(gpa_to_gfn(gpa) & mask);
+}