Received-SPF: pass (google.com: domain of linux-kernel+bounces-180543-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Message-ID: <4ba18e4e-5971-4683-82eb-63c985e98e6b@intel.com>
Date: Thu, 16 May 2024 13:21:40 +1200
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH 08/16] KVM: x86/mmu: Bug the VM if kvm_zap_gfn_range() is
 called for TDX
To: Isaku Yamahata <isaku.yamahata@intel.com>
CC: Sean Christopherson <seanjc@google.com>, Rick Edgecombe
	<rick.p.edgecombe@intel.com>, <pbonzini@redhat.com>, <kvm@vger.kernel.org>,
	<linux-kernel@vger.kernel.org>, <isaku.yamahata@gmail.com>,
	<erdemaktas@google.com>, <sagis@google.com>, <yan.y.zhao@intel.com>,
	<dmatlack@google.com>, <isaku.yamahata@linux.intel.com>
References: <20240515005952.3410568-1-rick.p.edgecombe@intel.com>
 <20240515005952.3410568-9-rick.p.edgecombe@intel.com>
 <ZkTWDfuYD-ThdYe6@google.com> <20240515162240.GC168153@ls.amr.corp.intel.com>
 <eab9201e-702e-46bc-9782-d6dfe3da2127@intel.com>
 <20240516001530.GG168153@ls.amr.corp.intel.com>
Content-Language: en-US
From: "Huang, Kai" <kai.huang@intel.com>
In-Reply-To: <20240516001530.GG168153@ls.amr.corp.intel.com>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?dk9xVEh4YUg3Ym5SRFNrZ1lObndsb1hOLzY1RTNpQkFBNU5odkt6c25wbnF4?=
 =?utf-8?B?alAxL3d6ZEo4OEkyUytSeVpmd3dCeVR0QkdNbDFoZTNaSkRNYmwwQTcxM0Zx?=
 =?utf-8?B?ZHhZaTFlQ2FFdU5OckEwYkErVEVxTTltbURsMGUraDBLajVYY1FBbENrZDdN?=
 =?utf-8?B?Sm5BVVJBQ29CL3ZocEN5VWZhTUlxdVo1anVUcnc1ZmVJM24ydEFHbXFGSzYw?=
 =?utf-8?B?cC9DNG9EOUJiZ2FkcUI1bjZ2dEVjd1hBRCswRWtvSXhaaTNWME5HTkMyK2gy?=
 =?utf-8?B?clZ6V2NnOXNHaThCUGg5U29mR2w3OWRMZ0pKUE9XZmt5VGNiMTk5aUtZNHlI?=
 =?utf-8?B?ZE55YVQrUlh6MTA4amlyK1hkbUoxUGxaRURIUDRrOE8vRTF3Q29HUk1EcmVQ?=
 =?utf-8?B?RkFHaEp3QjdPb1NKKytzak5rUEpxMFpvQWs4bEkva1lxa3NjUWZ2UXZIQUNu?=
 =?utf-8?B?U2MrWjF3QXQyN0VVbGNMcGNGcXZ2eldMVEs3NlRNMXhrSGlTYlBxdERYc1Jr?=
 =?utf-8?B?NHhEaFJJajVIQVcwOFNHbnovd0h1d1JqKzNTWkFobm1wTGx0bExpUVpDaTQ4?=
 =?utf-8?B?eGM3cGlYQnRBYnZuOGRURHJwd1FSd2JZSDFrUjFoVm5Rc0RDdi9rcnRoYURa?=
 =?utf-8?B?QVJpUUlrWkRxbHpHWjUzMWV3UFNBZGVwcHNSRkpmYzhDWHNYLzh3d2JJczRE?=
 =?utf-8?B?NWgwUnZSemU0dTBZQzZha3hRL0xQMVlRdXduT1hHSlYxUEZySVRqRVYrTGZT?=
 =?utf-8?B?TU9qVjN5NnpmaGNOaHJwVURsdzV0dkZ5NkJxWk5GRlM2UjFoN2E1d1BYZ1Zz?=
 =?utf-8?B?WVZMdFRQcFFKbEJRZVZvb1RaQ3NVZDNheVhZbFVScFh4Zk5YK1pFS2tDbm9y?=
 =?utf-8?B?Y0x5T2hlclJEclV2NVB0RVNTOTMzaEFEZ3JDd2dEMEIxWUNqWjAzZTRUYWV0?=
 =?utf-8?B?V3psamJ6djl6aXhNcTh4dnh5WHpyVGhsOWVjKy9jd2VZMlIwdE1oVWRKK3dL?=
 =?utf-8?B?RW5pUkp1TTB0dVYzaUh0K3Urc2JEdFNGSW5FWlFBckVWTEY1cXpyWU0yRW5q?=
 =?utf-8?B?U2ZQWG1LVjk5WGZtOVI1SVJKWGVWOXRwMUlBeUhMdTFkRnlmVjRtczJQbzl3?=
 =?utf-8?B?TnNiSkhsTk91S01Ub1VoSFpLQVV1QjJqbnd3Wmh3ZTlXTFViS01RbTVHOFZE?=
 =?utf-8?B?YnJjUnBRTDhhbkR5OGpMOC9VbXBzWGNtQi9ML28rTnJBNjEwYlpuNHhEdnk0?=
 =?utf-8?B?cDBBOFg4dkFRYmVGZmhQaWFJOTFscFhSMjJJSk9kbDRyZXNaNEZRSzhVOVNF?=
 =?utf-8?B?Z0FuZzRlbEJSY3VPZlphMGw4cDJvdHRTNW5lTlpFQWxnU2hFcElYSXhtUDFl?=
 =?utf-8?B?T2ZJbE5DTEVpZXhFa2FwQjNLK3YyVmxXRFlNbUEwSCtLNXlsUVpLb2U5Zi9E?=
 =?utf-8?B?Kzg1THN6ZGZ4d2dnZkFaeE5MQ0dvalBlVm1YU25yY25zazFiOEZERXZSRTUz?=
 =?utf-8?B?TVdydHRXbW1pQVU0YVN5bHpIUlYyUGlnLytERVRkZWhGZy91cXcyeWJCS1pw?=
 =?utf-8?B?a3o4QVdIbmVrWTkvMzB3WXBwM3VpSitEd1JFRjhGL3BGT29iNWFyaitIbm1E?=
 =?utf-8?B?S3dwUmxLU002N0xsK2gzck5MUVdsRkhOYnZzNnhUYWtsVUdhL3N1OEIvODJi?=
 =?utf-8?B?QWpBUDd5TWhOamFNQzlFcHRFa0ZNM1FMeWdla1NQcUhYVlloRVVuZmNLMFVu?=
 =?utf-8?B?U3ZxNFBHSlRVMTl2aUZKbjdJSE9hMlBjOEp3VEhlMjg1bm5LUlRNL2dZUEhK?=
 =?utf-8?B?dytQSVplK2FoK1VMTVRBT2VMRmZkaUVEMnhKZS9iUXdZcjFxYVFFcnErbEZZ?=
 =?utf-8?B?TWRBUE5IN2YwU09xYzRqN3BQVzlpdVE1OFRBQ093aDgyTG5rTmZHU3RRNmNi?=
 =?utf-8?B?OW5oU0FON3pQRXh4djkxOFpDMkJtOXgxUkJxaWlHZmNhWlVYWXFzdWQ1N3lD?=
 =?utf-8?B?WEU4Y0xQbjUrVWtGc0JMMFU4by80Si9sUXRZQlNNbU1GbHJ3NzRia3Z2MDU3?=
 =?utf-8?B?bXp1dGFSaEhNL1NMWVZlVENCUHBHZ2JjNXgyb0dHaXE0aWtDbWl6U2FMb0dM?=
 =?utf-8?Q?YQJDFQd2txEOsaPR+V8i5Bo3T?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 135fef85-d8cf-423f-bcd7-08dc75468fb0
X-MS-Exchange-CrossTenant-AuthSource: BL1PR11MB5978.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 16 May 2024 01:21:50.0550
 (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: lGVQx/DIh7omUZqVJEDXNSP5Jpu1WyHgH7OsmL9ziZKFCulr+/Mmc9Exex6XRZIQYuHKshWbmKiTTRxFOmxcPg==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BL1PR11MB6028
X-OriginatorOrg: intel.com


On 16/05/2024 12:15 pm, Isaku Yamahata wrote:
> On Thu, May 16, 2024 at 10:17:50AM +1200,
> "Huang, Kai" <kai.huang@intel.com> wrote:
> 
>> On 16/05/2024 4:22 am, Isaku Yamahata wrote:
>>> On Wed, May 15, 2024 at 08:34:37AM -0700,
>>> Sean Christopherson <seanjc@google.com> wrote:
>>>
>>>>> diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c
>>>>> index d5cf5b15a10e..808805b3478d 100644
>>>>> --- a/arch/x86/kvm/mmu/mmu.c
>>>>> +++ b/arch/x86/kvm/mmu/mmu.c
>>>>> @@ -6528,8 +6528,17 @@ void kvm_zap_gfn_range(struct kvm *kvm, gfn_t gfn_start, gfn_t gfn_end)
>>>>>    	flush = kvm_rmap_zap_gfn_range(kvm, gfn_start, gfn_end);
>>>>> -	if (tdp_mmu_enabled)
>>>>> +	if (tdp_mmu_enabled) {
>>>>> +		/*
>>>>> +		 * kvm_zap_gfn_range() is used when MTRR or PAT memory
>>>>> +		 * type was changed.  TDX can't handle zapping the private
>>>>> +		 * mapping, but it's ok because KVM doesn't support either of
>>>>> +		 * those features for TDX. In case a new caller appears, BUG
>>>>> +		 * the VM if it's called for solutions with private aliases.
>>>>> +		 */
>>>>> +		KVM_BUG_ON(kvm_gfn_shared_mask(kvm), kvm);
>>>>
>>>> Please stop using kvm_gfn_shared_mask() as a proxy for "is this TDX".  Using a
>>>> generic name quite obviously doesn't prevent TDX details for bleeding into common
>>>> code, and dancing around things just makes it all unnecessarily confusing.
>>>>
>>>> If we can't avoid bleeding TDX details into common code, my vote is to bite the
>>>> bullet and simply check vm_type.
>>>
>>> TDX has several aspects related to the TDP MMU.
>>> 1) Based on the faulting GPA, determine which KVM page table to walk.
>>>      (private-vs-shared)
>>> 2) Need to call TDX SEAMCALL to operate on Secure-EPT instead of direct memory
>>>      load/store.  TDP MMU needs hooks for it.
>>> 3) The tables must be zapped from the leaf. not the root or the middle.
>>>
>>> For 1) and 2), what about something like this?  TDX backend code will set
>>> kvm->arch.has_mirrored_pt = true; I think we will use kvm_gfn_shared_mask() only
>>> for address conversion (shared<->private).
>>>
>>> For 1), maybe we can add struct kvm_page_fault.walk_mirrored_pt
>>>           (or whatever preferable name)?
>>>
>>> For 3), flag of memslot handles it.
>>>
>>> ---
>>>    arch/x86/include/asm/kvm_host.h | 3 +++
>>>    1 file changed, 3 insertions(+)
>>>
>>> diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
>>> index aabf1648a56a..218b575d24bd 100644
>>> --- a/arch/x86/include/asm/kvm_host.h
>>> +++ b/arch/x86/include/asm/kvm_host.h
>>> @@ -1289,6 +1289,7 @@ struct kvm_arch {
>>>    	u8 vm_type;
>>>    	bool has_private_mem;
>>>    	bool has_protected_state;
>>> +	bool has_mirrored_pt;
>>>    	struct hlist_head mmu_page_hash[KVM_NUM_MMU_PAGES];
>>>    	struct list_head active_mmu_pages;
>>>    	struct list_head zapped_obsolete_pages;
>>> @@ -2171,8 +2172,10 @@ void kvm_configure_mmu(bool enable_tdp, int tdp_forced_root_level,
>>>    #ifdef CONFIG_KVM_PRIVATE_MEM
>>>    #define kvm_arch_has_private_mem(kvm) ((kvm)->arch.has_private_mem)
>>> +#define kvm_arch_has_mirrored_pt(kvm) ((kvm)->arch.has_mirrored_pt)
>>>    #else
>>>    #define kvm_arch_has_private_mem(kvm) false
>>> +#define kvm_arch_has_mirrored_pt(kvm) false
>>>    #endif
>>>    static inline u16 kvm_read_ldt(void)
>>
>> I think this 'has_mirrored_pt' (or a better name) is better, because it
>> clearly conveys it is for the "page table", but not the actual page that any
>> page table entry maps to.
>>
>> AFAICT we need to split the concept of "private page table itself" and the
>> "memory type of the actual GFN".
>>
>> E.g., both SEV-SNP and TDX has concept of "private memory" (obviously), but
>> I was told only TDX uses a dedicated private page table which isn't directly
>> accessible for KVV.  SEV-SNP on the other hand just uses normal page table +
>> additional HW managed table to make sure the security.
> 
> kvm_mmu_page_role.is_private is not good name now. Probably is_mirrored_pt or
> need_callback or whatever makes sense.
> 
> 
>> In other words, I think we should decide whether to invoke TDP MMU callback
>> for private mapping (the page table itself may just be normal one) depending
>> on the fault->is_private, but not whether the page table is private:
>>
>> 	if (fault->is_private && kvm_x86_ops->set_private_spte)
>> 		kvm_x86_set_private_spte(...);
>> 	else
>> 		tdp_mmu_set_spte_atomic(...);
> 
> This doesn't work for two reasons.
> 
> - We need to pass down struct kvm_page_fault fault deep only for this.
>    We could change the code in such way.
> 
> - We don't have struct kvm_page_fault fault for zapping case.
>    We could create a dummy one and pass it around.

For both above, we don't necessarily need the whole 'kvm_page_fault', we 
just need:

  1) GFN
  2) Whether it is private (points to private memory to be precise)
  3) use a separate private page table.

>    
> Essentially the issue is how to pass down is_private or stash the info
> somewhere or determine it somehow.  Options I think of are
> 
> - Pass around fault:
>    Con: fault isn't passed down
>    Con: Create fake fault for zapping case >
> - Stash it in struct tdp_iter and pass around iter:
>    Pro: work for zapping case
>    Con: we need to change the code to pass down tdp_iter >
> - Pass around is_private (or mirrored_pt or whatever):
>    Pro: Don't need to add member to some structure
>    Con: We need to pass it around still. >
> - Stash it in kvm_mmu_page:
>    The patch series uses kvm_mmu_page.role.
>    Pro: We don't need to pass around because we know struct kvm_mmu_page
>    Con: Need to twist root page allocation

I don't think using kvm_mmu_page.role is correct.

If kvm_mmu_page.role is private, we definitely can assume the faulting 
address is private; but otherwise the address can be both private or shared.

> 
> - Use gfn. kvm_is_private_gfn(kvm, gfn):
>    Con: The use of gfn is confusing.  It's too TDX specific.
> 
> 
>> And the 'has_mirrored_pt' should be only used to select the root of the page
>> table that we want to operate on.
> 
> We can add one more bool to struct kvm_page_fault.follow_mirrored_pt or
> something to represent it.  We can initialize it in __kvm_mmu_do_page_fault().
> 
> .follow_mirrored_pt = kvm->arch.has_mirrored_pt && kvm_is_private_gpa(gpa);
> 
> 
>> This also gives a chance that if there's anything special needs to be done
>> for page allocated for the "non-leaf" middle page table for SEV-SNP, it can
>> just fit.
> 
> Can you please elaborate on this?

I meant SEV-SNP may have it's own version of link_private_spt().

I haven't looked into it, and it may not needed from hardware's 
perspective, but providing such chance certainly doesn't hurt and is 
more flexible IMHO.