Received-SPF: pass (google.com: domain of linux-kernel+bounces-184221-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Message-ID: <5e8119c0-31f5-4aa9-a496-4ae10bd745a3@intel.com>
Date: Tue, 21 May 2024 10:34:54 +1200
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH 10/16] KVM: x86/tdp_mmu: Support TDX private mapping for
 TDP MMU
To: Isaku Yamahata <isaku.yamahata@intel.com>
CC: "Edgecombe, Rick P" <rick.p.edgecombe@intel.com>, "dmatlack@google.com"
	<dmatlack@google.com>, "isaku.yamahata@gmail.com" <isaku.yamahata@gmail.com>,
	"seanjc@google.com" <seanjc@google.com>, "sagis@google.com"
	<sagis@google.com>, "isaku.yamahata@linux.intel.com"
	<isaku.yamahata@linux.intel.com>, "Aktas, Erdem" <erdemaktas@google.com>,
	"Zhao, Yan Y" <yan.y.zhao@intel.com>, "kvm@vger.kernel.org"
	<kvm@vger.kernel.org>, "pbonzini@redhat.com" <pbonzini@redhat.com>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
References: <a08779dc-056c-421c-a573-f0b1ba9da8ad@intel.com>
 <588d801796415df61136ce457156d9ff3f2a2661.camel@intel.com>
 <021e8ee11c87bfac90e886e78795d825ddab32ee.camel@intel.com>
 <eb7417cccf1065b9ac5762c4215195150c114ef8.camel@intel.com>
 <20240516194209.GL168153@ls.amr.corp.intel.com>
 <ffd24fa5-b573-4334-95c6-42429fd9ee38@intel.com>
 <20240517081440.GM168153@ls.amr.corp.intel.com>
 <b6ca3e0a18d7a472d89eeb48aaa22f5b019a769c.camel@intel.com>
 <0d48522f37d75d63f09d2a5091e3fa91913531ee.camel@intel.com>
 <791ab3de8170d90909f3e053bf91485784d36c61.camel@intel.com>
 <20240520185817.GA22775@ls.amr.corp.intel.com>
Content-Language: en-US
From: "Huang, Kai" <kai.huang@intel.com>
In-Reply-To: <20240520185817.GA22775@ls.amr.corp.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?TVhXU1hoSHlLa25Ebld2a2Z2RFlmTUk4Vi90QUhsb3ZXYXpoc0o2WDdYdnlW?=
 =?utf-8?B?Y3VLNGxPd1daMWNRYm9YaE9iMjlUMGFSWDZzTVdoVDRubzF2NlNEdWNQdmZ1?=
 =?utf-8?B?QzUyejlKZGZVQTVuK0cyMTYwelZGTmtlNWFPY1piREtoN01ZNmthbmN6b3BE?=
 =?utf-8?B?bGJ5M0Fmb1hRWDc1ZENWTkUvYnZ3b0pHREdid1Nnem4zd0ZyY2VVc3J6VXZJ?=
 =?utf-8?B?WVBTZ0pTcEQxUm4wRW50LzB6YUdiNlNwTDRZWXNNbmtFRi9ka01kcXBVTkVt?=
 =?utf-8?B?QXJVNG9zQjhndHRURmJyUzIvcTRDVEJFaU9kNnJyNUVDZGNMem1tcDhZVHkz?=
 =?utf-8?B?enJCdHVyQW5EamtkV053SkQvcm9HbTR0Uk5XNXBkMGZMUnM4KzhtV2VScDFy?=
 =?utf-8?B?TnRIOThocDhCTlBUWDhIeVhGd1hlN0RnQnNiUFFGeFhnVExWcElTODA2dDB6?=
 =?utf-8?B?eFI0NFNQYm5ZUXFYMGgzWmZvd0J4V09BSjRHSEJLNVFkVlAvUm05aHhPTk5t?=
 =?utf-8?B?VUwwbUNWTHZEc2dwWUhwelZmdjJOUWVTeVFTeGtQNzlFZkFyK0NGN08zQmhq?=
 =?utf-8?B?dnorQW1razdQTllUVk91L1A3Z2JteUhlQnY3Qm5Yb0ZKa3g4MjJ4SFNsb2ZE?=
 =?utf-8?B?Wkh2aHFtUSttTlBPR09hbTRreTE4YkN5ZEF1OVdVRkE3RjlLcjBvV1BSN3li?=
 =?utf-8?B?SE5Ga0NyRm1UWHU4VkJXcC9VVXNvTlN5SDBvZ3d3bTFaN1FsTHp4ZjN0dGFX?=
 =?utf-8?B?bFNweVRYMkdOYVh1U0pZelpZenNzWXB0NU5LODFhRzJJSWpVLzgrdzE0QnJr?=
 =?utf-8?B?ZEpyUmhnVUNUd0ZKeDM0enN0Y1FuVndGVFJ0N0g1YkhIUEVRcUcvZUFsOGIz?=
 =?utf-8?B?UjNpbEpGTG9taDJpS0JOTkQ0RFRvZGxoeU54bGx6RktNRXgrdG1EYkRzOVgr?=
 =?utf-8?B?RzFZWXg3eG1nbnc4UXBMM2xHYWhKN09yVERzZjVjQU5KdWlWbDcrOE42RXR3?=
 =?utf-8?B?R0J3NXErR1Z4T0YvTTNGbTJXUkpkM05PSlRlVDhqRjdObzlYYlppUFU5Y1VK?=
 =?utf-8?B?bEdBeXlpUW1OQ25Za2hFRXB0eWJGbmtCM0liRmc2TE8yenlnQjFPb3cyYkRI?=
 =?utf-8?B?L250czEwbWh1a2laVE15VGhNZzJIRG15NW01YWtWUTBla2piWnFxWklZcHd2?=
 =?utf-8?B?eHozaVFsaG5wREdoZ2kyaStmc3JsYjQwRVFXYXJ5NmNVSDJqTDl0NWQ3V0N1?=
 =?utf-8?B?aWFESjNlZ0lBYWh0Y2s3ZGRGOWc4WHUrdmREZnhCVnp3eWx4eVh2L29veUk2?=
 =?utf-8?B?d3RKT0Zjc3I2T1J5VFJrMGlEUkRqVnBNZWZObW1uUExqc1BuczVVcnVoZ2tE?=
 =?utf-8?B?cTkzQzFvZGVkdGJ5cFR0dXRrNXhRd09CRE43aStsQ1EwTmgybXltNm51eTBG?=
 =?utf-8?B?R1l1ZytGdXZ0cVlkb3BUc1IrU1lCQzFUUnJMRm45VVB4TXFTdkNKRTBnSnJo?=
 =?utf-8?B?dGkrdGtkRVpjOWxOd3QrK1hmYlUzZmJ4YkJ5R21WYklKRGtPOGtkelFVSkhC?=
 =?utf-8?B?NS92alFQUWF1anB0RFlpaDFQdGVoMTRLSktYcEJPNWtQRmMzN0FEQmlvR1Bi?=
 =?utf-8?B?WmxpcnZWQkV4WDN2T2o5bUtTZ1ZtaWxCWmtCVC9wOTNoVG04QllZa01YcUtw?=
 =?utf-8?B?NE5KRmUzK2UxaERjUlhYekdtbkU3Um5nZGwyeXBqNDNiejlJWGtUNEw2ekV0?=
 =?utf-8?B?ekZ0OVNvL2FRdEVpcU81Y2RxUTRvRzFoNFNMOS9LL2hFa1JDRjdEeWJZbDZs?=
 =?utf-8?B?Z2FmakdwZ20vY09wTm1mdTFvdldxV0pTQkthcDl4dlFXZDQwOS9Od3grYVFh?=
 =?utf-8?B?bjhvaXFTcVFEcjUwbjVCc3ExM3dBb1o4bmJzaWxPd0FaUHEyblJ6ZHZTdk5u?=
 =?utf-8?B?NVNaRzhtTktFNDdybjNYTkJRS20rRm0vV3l1VlhYYU5qaGtsN2V3VTdJMVpK?=
 =?utf-8?B?R0dUZjdpRWpqUi9oMHM1aVhkZDVVeFpEMmdnWXdRTW1xNm0yOG1YVlBWYzd1?=
 =?utf-8?B?VHhJUldKVUxsUjhaWjNRVnNKVmZaV2tMV3BubVFCZkJOQ3dBWnNrWjJmUkor?=
 =?utf-8?Q?czRv50WZnBrbSoCrd2slKsEX0?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 8dfa1db9-9b32-4366-a90a-08dc791d1712
X-MS-Exchange-CrossTenant-AuthSource: BL1PR11MB5978.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 20 May 2024 22:35:02.9786
 (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: 1mbDUXVY2N8zNJmR++/6nHd6jwWJz4XRw0zgg0I2Qnp/AzavQ7W0iHYAn2GjqguTrOwk6DerM/byG2TSEJwJEA==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BL1PR11MB5255
X-OriginatorOrg: intel.com


On 21/05/2024 6:58 am, Isaku Yamahata wrote:
> On Mon, May 20, 2024 at 10:38:58AM +0000,
> "Huang, Kai" <kai.huang@intel.com> wrote:
> 
>> On Sat, 2024-05-18 at 15:41 +0000, Edgecombe, Rick P wrote:
>>> On Sat, 2024-05-18 at 05:42 +0000, Huang, Kai wrote:
>>>>
>>>> No.  I meant "using kvm_mmu_page.role.mirrored_pt to determine whether to
>>>> invoke kvm_x86_ops::xx_private_spt()" is not correct.
>>>
>>> I agree this looks wrong.
>>>
>>>>    Instead, we should
>>>> use fault->is_private to determine:
>>>>
>>>>          if (fault->is_private && kvm_x86_ops::xx_private_spt())
>>>>                  kvm_x86_ops::xx_private_spte();
>>>>          else
>>>>                  // normal TDP MMU operation
>>>>
>>>> The reason is this pattern works not just for TDX, but also for SNP (and
>>>> SW_PROTECTED_VM) if they ever need specific page table ops.
> 
> Do you want to split the concept from invoking hooks from mirrored PT
> and to allow invoking hooks even for shared PT (probably without
> mirrored PT)?  So far I tied the mirrored PT to invoking the hooks as
> those hooks are to reflect the changes on mirrored PT to private PT.
> 
> Is there any use case to allow hook for shared PT?

To be clear, my intention is to allow hook, if available, for "private 
GPA".  The point here is for "private GPA", but not "shared PT".

> 
> - SEV_SNP
>    Although I can't speak for SNP folks, I guess they don't need hooks.
>    I guess they want to stay away from directly modifying the TDP MMU
>    (to add TDP MMU hooks).  Instead, They added hooks to guest_memfd.
>    RMP (Reverse mapping table) doesn't have to be consistent with NPT.
> 
>    Anyway, I'll reply to
>    https://lore.kernel.org/lkml/20240501085210.2213060-1-michael.roth@amd.com/T/#m8ca554a6d4bad7fa94dedefcf5914df19c9b8051

For SNP _ONLY_ I completely understand.  The point is, TDX needs to 
modify anyway.  So if SNP can use hooks for TDX, and if in that case we 
can avoid guest_memfd hooks, then I think it's better?

But I can certainly be, and probably am, wrong, because that 
gmem_memfd() hooks have been there for long time.

>   
> TDX
>    I don't see immediate need to allow hooks for shared PT. >
> SW_PROTECTED (today)
>    It uses only shared PT and don't need hooks.
> 
> SW_PROTECTED (with mirrored pt with shared mask in future in theory)
>    This would be similar to TDX, we wouldn't need hooks for shared PT.
> 
> SW_PROTECTED (shared PT only without mirrored pt in future in theory)
>    I don't see necessity hooks for shared PT.
>    (Or I don't see value of this SW_PROTECTED case.)
> 

I don't think SW_PROTECTED VM will ever need to have any TDP MMU hook, 
because there's no hardware feature backing behind it.

My intention is for SNP.  Even if SNP doesn't need any TDP MMU hook 
today, I think invoking hook depending on "private GPA", but not 
"private page table" provides more flexibility.  And this also works for 
TDX, regardless whether SNP wants to implement any TDP MMU hook.

So conceptually speaking, I don't see any disadvantage of my proposal, 
regardless whether SNP chooses to use any TDP MMU hook or not.  On the 
other hand, if we choose to "invoke hooks depending on page table type", 
then this code will indeed be only for TDX.