Received-SPF: pass (google.com: domain of linux-kernel+bounces-181729-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Message-ID: <e31f0793-c939-4018-9c3b-041aa8800515@intel.com>
Date: Fri, 17 May 2024 16:26:51 +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>, "dmatlack@google.com"
	<dmatlack@google.com>, "linux-kernel@vger.kernel.org"
	<linux-kernel@vger.kernel.org>, "Zhao, Yan Y" <yan.y.zhao@intel.com>,
	"isaku.yamahata@gmail.com" <isaku.yamahata@gmail.com>, "Aktas, Erdem"
	<erdemaktas@google.com>
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>
 <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>
 <9556a9a426af155ee12533166ed3b8ee86fa88fe.camel@intel.com>
 <d9c2a9b4-a6b8-4d29-9c22-ebdce77f3606@intel.com>
 <240f61b5da5015a8205de414a87c3c433b1c09a0.camel@intel.com>
Content-Language: en-US
From: "Huang, Kai" <kai.huang@intel.com>
In-Reply-To: <240f61b5da5015a8205de414a87c3c433b1c09a0.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?WHk1NUoyd3lRaElraFZiSXBPd1hDQU5iMWhTR2JPL3ZDY2RwY3FtOGQxMU1B?=
 =?utf-8?B?Tm8za0ZvRFd6T0wwaUlNQjB4cWtSY09iTTlCdUxQM0tYWHc5MlE2QVpleVVs?=
 =?utf-8?B?WHIrdExDMWY3aXV5QzR6VUhBY2NOckl4cHFGdEdJc1N3bGs1VEtGdlFURDJF?=
 =?utf-8?B?K0RMSDBMWjlMSHFBS3lkd0I3ZzZwKyt2cWNJbXpuSjRXbnEvdkM3VkNhZ0hm?=
 =?utf-8?B?Z1U3YmpKTysrRUxEa2xtaVY0Y2pkdC9xZ2VMNnNtMFFhc25zUnpJMUhCVGs1?=
 =?utf-8?B?VStNUmJGVmpoM0pKaVJsRTlEaVVyeFI1d1o5U044UjEyTWd0RHoxUVVLckhx?=
 =?utf-8?B?TEpXeVBOaXRucFBvMVhzZi96RE52N1NHeXhuT3gra3d3dSt0OGtMSFR1cXh3?=
 =?utf-8?B?OXQ2djE2dUhzenpubFQ5aUZHQTA3b3NFUGZGNllxWVVxV0ZOUm84dHlvM1JF?=
 =?utf-8?B?bG44NERuUmpwSnJaWCtVVkYxUUhRaEJ1WHY3eVNUUGNraTNhMGNZRlYyUXlt?=
 =?utf-8?B?VDkvc2haeUU0bCtQMDZWT1Z5ejR0VW1UWDBKKzdDZkh6NVpoZ09yM0Ftbjkw?=
 =?utf-8?B?Q1BuSnRpTk51OWpHUEFDQWozb2JvZ2hZVkYyanR2VnFpL1ZidVdkdjJQSmls?=
 =?utf-8?B?VHBsVFRabDJKZUhRNmhOckNhRjdQZS9RVmtLYUNNTGZiV2xvaWdFZGxSM1VB?=
 =?utf-8?B?QWM5Wk14SFVMYnR6VTFDc2NQbC9SVU9ESGVGdldMajJITUw1NGFlcWFxejRS?=
 =?utf-8?B?M095dHpUNHlSTTJUNTJuT3FLQThWY0crSGM1MFdHaHlxVEZsd2dZZWxJWUlz?=
 =?utf-8?B?YThVN1BkaEQ0SkhueHFOdm9qekxQRlRGTTlOSVdCQW1FSzc3ajV6bDdsUXh2?=
 =?utf-8?B?TVlndTNuRHpRSFNOMWwxT0tXVmtDSktVV255N2F2VlpoTEU0WkF5QitHcjhD?=
 =?utf-8?B?U3lHbXRpeDZpZnUxNGlRZkZtOVE1eHhoV1drdW1XbW1qZkpWWmxCOWJxL3BY?=
 =?utf-8?B?K1lFUUxZSWNia3hpdDYxYkpWaFZzV3F6MFJ6V3RlSHpjY2xrOVE3bFRzMmVr?=
 =?utf-8?B?VmY2WnRXLzhUaTJGazBuWXJUSnJSajNNcitwMnFoaTBoQ2dYZ3UvTkZCUHFp?=
 =?utf-8?B?cTlMTkxyQWMxQkJPVDlRVTluUGxtVTVmYUF5SWRJSkJwOFpWbW80TUFDZ2pZ?=
 =?utf-8?B?WDExTXd6d0ZxRnZxVDFjMDVaZEg3MjdWV2RlZWNQa1YxR1VnYjdxcG9XSUMr?=
 =?utf-8?B?WGhSTENZbTFxOFNLanlUeXVKdXdXNkIwVEJieStCSGNXMXNxakFjYTlTVjdx?=
 =?utf-8?B?cUxCSEIzVlZkQXJqc05xNm5XaWNhVDJOeTI4VCt5UllFYkw3aDZWeUFhOHla?=
 =?utf-8?B?RWZJTlBtRkdXbGlUaVJxSFJIcUVhMmJxQ1VmbktWYVpZNmtFREFVR2pmZUUz?=
 =?utf-8?B?OHBNcTVmc09pd241dUNRQnFtOGloSFFoTWtxdC9WOXVKcXJCUmxqSVVKSXZY?=
 =?utf-8?B?ZDIwRFZ4Mld1Ymo0TzFUcHFGOWF0STQyeDE1NGV6elF0eXZPL3o5N015N0Zi?=
 =?utf-8?B?OXJoV3ZJSFJTYldJUXJlTXBiK3U0bWVhMHdYN3ZGbmxOY3hCVXlENXBtb2ZL?=
 =?utf-8?B?T0ZUY3NxYVJ4bldNcStkWk44eGRFMWFCYnM3aW5oT042RE00dWNaSUhGQXR2?=
 =?utf-8?B?VEFseGtoNVRtK3hybFE1dWJycmh2cEF6UEtVSzdIaWZJcURGK2lORndCQ2ZT?=
 =?utf-8?B?NmR5YTRRb2xadUppMHF1dDErd0JKOG5ZWUlvenpTYnVHeVRPdjhkTXY5Zzcw?=
 =?utf-8?B?SGdxdXpzS014SCtjUlY2Q1M1WEFVK0gyeVFBdmoyRlErT01GeXBvRkVZeEJE?=
 =?utf-8?B?NkxMdDA5MnR5UVV5T0tLSGZkYkF2UWprY2hSZUMrR3FDN2lGZ3RJc1dQcDZZ?=
 =?utf-8?B?ajVDQkJUczk3RTVsOWx1d2ZLQVFpRVZyMVBCaVFNWUlDdC9rSVpvMklVSmp5?=
 =?utf-8?B?TGhGbWY4MHRlN0ZQd2JtcUhoU09VeGlYbkY0RGN2NzQzYXgrbG12VjlpT2FK?=
 =?utf-8?B?Znp6SXFOTXowMzBEUFlRalEySzFobmVhZncyb0JsejZoZ3M3U1ZkblNHV25G?=
 =?utf-8?Q?yp9YEfsIr89LoRFNlYFMyTPfQ?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 0919bd50-c268-4dfd-bbd3-08dc76299cbc
X-MS-Exchange-CrossTenant-AuthSource: BL1PR11MB5978.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 17 May 2024 04:27:07.6268
 (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: rbRyvmY/QJ4xveG6/8xYFi1rH/t49SWfw4biFw7zVuBGJ0IUCoQJuH9O99miXmY9UY5lkMK1BfUetEqxx0jdKA==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DS7PR11MB7783
X-OriginatorOrg: intel.com


>> E.g,  why we cannot do:
>>
>>          static bool kvm_use_private_root(struct kvm *kvm)
>>          {
>>                  return kvm->arch.vm_type == VM_TYPE_TDX;
>>          }
>>
>> Or,
>>          static bool kvm_use_private_root(struct kvm *kvm)
>>          {
>>                  return kvm->arch.use_private_root;
>>          }
>>
>> Or, assuming we would love to keep the kvm_gfn_shared_mask():
>>
>>          static bool kvm_use_private_root(struct kvm *kvm)
>>          {
>>                  return !!kvm_gfn_shared_mask(kvm);
>>          }
>>
>> And then:
>>
>> In fault handler:
>>
>>          if (fault->is_private && kvm_use_private_root(kvm))
>>                  // use private root
>>          else
>>                  // use shared/normal root
>>
>> When you zap:
>>
>>          bool private_gpa = kvm_mem_is_private(kvm, gfn);
>>          
>>          if (private_gpa && kvm_use_private_root(kvm))
>>                  // zap private root
>>          else
>>                  // zap shared/normal root.
>>
> 
> I think you are trying to say not to abuse kvm_gfn_shared_mask() as is currently
> done in this logic. But we already agreed on this. So not sure.

To be clear:  We agreed on this in general, but not on this 
kvm_on_private_root().

It's obvious that you still want to "use kvm_gfn_shared_mask() to 
determine whether a GPA is private" for this helper but I don't like it. 
  In fact I don't see why we even need this helper.

I think I am just too obsessed on avoiding using kvm_gfn_shared_mask() 
so I'll stop commenting/replying on this.

[...]

> 
> I don't think we can get rid of the shared mask. Even if we relied on
> kvm_mem_is_private() to determine if a GPA is private or shared, at absolute
> minimum we need to add the shared bit when we are zapping a GFN or mapping it.

No we cannot, but we can avoid using it here.

> 
> Let's table the discussion until we have some code to look again.

100% agreed.