Received-SPF: pass (google.com: domain of linux-kernel+bounces-111053-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Date: Fri, 22 Mar 2024 15:18:39 +0800
From: Chao Gao <chao.gao@intel.com>
To: Isaku Yamahata <isaku.yamahata@intel.com>
CC: "Edgecombe, Rick P" <rick.p.edgecombe@intel.com>, "kvm@vger.kernel.org"
	<kvm@vger.kernel.org>, "linux-kernel@vger.kernel.org"
	<linux-kernel@vger.kernel.org>, "Zhang, Tina" <tina.zhang@intel.com>,
	"seanjc@google.com" <seanjc@google.com>, "Yuan, Hang" <hang.yuan@intel.com>,
	"Huang, Kai" <kai.huang@intel.com>, "Chen, Bo2" <chen.bo@intel.com>,
	"sagis@google.com" <sagis@google.com>, "isaku.yamahata@gmail.com"
	<isaku.yamahata@gmail.com>, "Aktas, Erdem" <erdemaktas@google.com>,
	"pbonzini@redhat.com" <pbonzini@redhat.com>, <isaku.yamahata@linux.intel.com>
Subject: Re: [PATCH v19 056/130] KVM: x86/tdp_mmu: Init role member of struct
 kvm_mmu_page at allocation
Message-ID: <Zf0wz82nQoL0VsAd@chao-email>
References: <cover.1708933498.git.isaku.yamahata@intel.com>
 <5d2307efb227b927cc9fa3e18787fde8e1cb13e2.1708933498.git.isaku.yamahata@intel.com>
 <9c58ad553facc17296019a8dad6a262bbf1118bd.camel@intel.com>
 <20240321212412.GR1994522@ls.amr.corp.intel.com>
Content-Type: text/plain; charset="utf-8"
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <20240321212412.GR1994522@ls.amr.corp.intel.com>
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?YXlXQ1Q4RzczNGZaRWo2WTJmdTk5cmV3bGFIYXhwdVRtUU43dTQyL09DVzRD?=
 =?utf-8?B?ZnRPUEhZaGlPOU5pSjNhZ2tvQW1UL3Z1eG1zME13NmRLR2RxMVVCbHljRGww?=
 =?utf-8?B?Ykw2blNTSWZGamlWSmVBUkhWRnFqTHh5ZVlKNTBWNjJ1SHlNVmdNWTBuanR3?=
 =?utf-8?B?RlkxRXJ2MFN0YkdIWWZ6dWFlenhuTUorVFJQSGJlR1ZmZThJTmlHMHRsL1dX?=
 =?utf-8?B?aUo0eEo4OXpsNG1zcXVTU0YwSXhxbysrOCtobGxld0JrbVZ4dzB4Z2V1V2JQ?=
 =?utf-8?B?dHhYaElsY3VnSXNyNHpDeGJ0N21FeHcrYnJORDdsb3RSQlVBUVdMS3JzRGhp?=
 =?utf-8?B?VStLUjN5R25hWkcxZVdNUE9SYXJNNUVGVVQxdnVvRGVkdEN5ZVlXeGFwU2RC?=
 =?utf-8?B?YVJsVnJhYmJmQkI0ZHhJSkkxOWxYNWp5MUlpdkRLK3NxaUZGTzZiSVJFZmpS?=
 =?utf-8?B?SXpFNmZ3TUd6OURJVnE2Y0kwekM4elFRL1Zmd1QvV2dQUE1FWE5Ba2dzZ1J5?=
 =?utf-8?B?MXhMUzJYSHRESml3RlB3cGFPS01RVWdmRFl4UEk2MGxITUVIRktkQ2Y0MTYx?=
 =?utf-8?B?SFYybEFMdnlMWUsyajdQTUdwU0hmYUMxNlBFQ2wvakRyUEdCcUJtOGU0YkZQ?=
 =?utf-8?B?eHhPYnczckw4OUVyVFpQMGlFWkE5bUhaWGdLZUo5SHJ0aFBmaG9RUW1sWWxH?=
 =?utf-8?B?OGpzWjA1WExLK1VRMkp4VlhNV0lPNWFoZ0hsL2o3blBGTE5ZaW5YN1RaZ3ZG?=
 =?utf-8?B?aUxDU2o5NHNTcVdXV3FTU3lobDBYMjF2d3NybEF6bTdUUUJIb2VManBzRllW?=
 =?utf-8?B?ZjE0dDFSS0JrZXI0MGdnd3pBb2NlUDBiSWE0RkFzU2g2cG9CeFc4d3BVRTJy?=
 =?utf-8?B?R2dNVm01T3M0M3M4ZXJwd1pQTGJRaXVrYWZsbTZQUlo1ZVQ2V0wrYW9sY0lL?=
 =?utf-8?B?VWZqVFRQQkZnV3VQYTkyajNxK0VmeVlLaEl4MlhjUzRaS2d6ak1yMFBXZUFW?=
 =?utf-8?B?OG1vdkVPVkpyNHhYcmEvVDFVMlpGOHhBekxobmd2c25NcU53U3BPRTk0bG9w?=
 =?utf-8?B?YUFWSkxJS3RyaW40bmtyNmhsZWlBVmZ0bVIzOVJ0RTI2UVdTWDVNWS9ZSitC?=
 =?utf-8?B?MWJRYlB0Wll1L29LMjhMazJQRkd1T0VySkg2Unk2M0pON0tGekJ4N3pkbzVo?=
 =?utf-8?B?WUl0cWJjenNEYmZSb0NPUW5UV0IvbitHNnJkbTgwcCtIdGx4TlFuTHZsSE1H?=
 =?utf-8?B?MjhmS1ArR2tPblpJd0loOFl1ZW5RclhJYnFTSEVCZ2dsL3ptSjlJWm82OEJu?=
 =?utf-8?B?VENvTjE4ZWNRbmJBbWJiOHlmVzVMUWxoalE5cXJWK3hpSDdaVUt6QVNLd0Zi?=
 =?utf-8?B?eDYyVjZMbFROTHQyVlRaaVRvQjZ5ZmNXbU9mOG1OUWU1ZSt0RHNQWVJlOU1I?=
 =?utf-8?B?c1NOQmdLeG9JTGo5dEo0ZEZnaGpDYVRFQ3FmMlFGeWVDcHUvUGNmcHkvZy9W?=
 =?utf-8?B?Y2hoTEJrSU91NklJMHJvZkxYUkZwVjJGcjRYT1VzUXhhWUtNeXhBSU52Qi9j?=
 =?utf-8?B?ZkR6Wm9vOXhuUlJkbzZaN1dST092dXlXeXlPSnNzRHFMTzR3K3VOdkdzeGhm?=
 =?utf-8?B?RVdTTGJibnZkZk9FcTNxaEx6ZU0zRkdZM1ViVjR0bGpJWVZUQVhUMmh0Rmhh?=
 =?utf-8?B?WWpxOXVBUmlSWU81MWNnTFJDS3FJU0N2bmFvNkt3MWd2aDhZTWZmMVUyTXd1?=
 =?utf-8?B?eFBxQjc1amRja2pNY3paR3NWT1RVTHNod2lCVldhUFkzYXZIdm1ZbzBGSDcr?=
 =?utf-8?B?b01jNDBuMzNPWjF0RW95SnNGb0xjVm9wUW1NN1N2aDNGZnRmSG1PWFdab3gy?=
 =?utf-8?B?SkJXTDNiVW9XajFDMDdGeTRWUlkyL3FTcUdTeGsrT0pQVmg5K21JR3BXWnRn?=
 =?utf-8?B?QTlxdmFuN2FvSFo2Q2FjU21VS2tyV3BpckVpSC9YOERtVHlPTW1VQWVSenBz?=
 =?utf-8?B?Rm1XRi9OUEZFR1pYN2dGK1l1WU9XeENhNXFwZXpaNGdOeFpyS3BSaDJGNmM4?=
 =?utf-8?B?eGpCc3JzdytrakY0UXBxaC9DVFFpeVlDUUNsUVFWdFpkaS9UdGVFR3JMdEls?=
 =?utf-8?Q?f9ite0SSc2YqofQwn0aDKAh3l?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 82e23e5d-8e22-4bd2-d867-08dc4a405280
X-MS-Exchange-CrossTenant-AuthSource: CH3PR11MB8660.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 22 Mar 2024 07:18:50.6101
 (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: 1JmCIwZBROc8DL9p/c+IT8n2TZfQ3bG/241Ui4FMGRsoqOAG9MGWaxrOV5+JujZxOj2sTLrx3+FuNxNULJHhWw==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SA2PR11MB5177
X-OriginatorOrg: intel.com

On Thu, Mar 21, 2024 at 02:24:12PM -0700, Isaku Yamahata wrote:
>On Thu, Mar 21, 2024 at 12:11:11AM +0000,
>"Edgecombe, Rick P" <rick.p.edgecombe@intel.com> wrote:
>
>> On Mon, 2024-02-26 at 00:25 -0800, isaku.yamahata@intel.com wrote:
>> > To handle private page tables, argument of is_private needs to be
>> > passed
>> > down.  Given that already page level is passed down, it would be
>> > cumbersome
>> > to add one more parameter about sp. Instead replace the level
>> > argument with
>> > union kvm_mmu_page_role.  Thus the number of argument won't be
>> > increased
>> > and more info about sp can be passed down.
>> > 
>> > For private sp, secure page table will be also allocated in addition
>> > to
>> > struct kvm_mmu_page and page table (spt member).  The allocation
>> > functions
>> > (tdp_mmu_alloc_sp() and __tdp_mmu_alloc_sp_for_split()) need to know
>> > if the
>> > allocation is for the conventional page table or private page table. 
>> > Pass
>> > union kvm_mmu_role to those functions and initialize role member of
>> > struct
>> > kvm_mmu_page.
>> 
>> tdp_mmu_alloc_sp() is only called in two places. One for the root, and
>> one for the mid-level tables.
>> 
>> In later patches when the kvm_mmu_alloc_private_spt() part is added,
>> the root case doesn't need anything done. So the code has to take
>> special care in tdp_mmu_alloc_sp() to avoid doing anything for the
>> root.
>> 
>> It only needs to do the special private spt allocation in non-root
>> case. If we open code that case, I think maybe we could drop this
>> patch, like the below.
>> 
>> The benefits are to drop this patch (which looks to already be part of
>> Paolo's series), and simplify "KVM: x86/mmu: Add a private pointer to
>> struct kvm_mmu_page". I'm not sure though, what do you think? Only
>> build tested.
>
>Makes sense.  Until v18, it had config to disable private mmu part at
>compile time.  Those functions have #ifdef in mmu_internal.h.  v19
>dropped the config for the feedback.
>  https://lore.kernel.org/kvm/Zcrarct88veirZx7@google.com/
>
>After looking at mmu_internal.h, I think the following three function could be
>open coded.
>kvm_mmu_private_spt(), kvm_mmu_init_private_spt(), kvm_mmu_alloc_private_spt(),
>and kvm_mmu_free_private_spt().

It took me a few minutes to figure out why the mirror root page doesn't need
a private_spt.

Per TDX module spec:

  Secure EPT’s root page (EPML4 or EPML5, depending on whether the host VMM uses
  4-level or 5-level EPT) does not need to be explicitly added. It is created
  during TD initialization (TDH.MNG.INIT) and is stored as part of TDCS.

I suggest adding the above as a comment somewhere even if we decide to open-code
kvm_mmu_alloc_private_spt().

IMO, some TDX details bleed into KVM MMU regardless of whether we open-code
kvm_mmu_alloc_private_spt() or not. This isn't good though I cannot think of
a better solution.