Received-SPF: pass (google.com: domain of linux-kernel+bounces-180497-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Message-ID: <465b8cb0-4ce1-4c9b-8c31-64e4a503e5f2@intel.com>
Date: Thu, 16 May 2024 11:44: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: "Zhao, Yan Y" <yan.y.zhao@intel.com>, "sagis@google.com"
	<sagis@google.com>, "dmatlack@google.com" <dmatlack@google.com>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	"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>
Content-Language: en-US
From: "Huang, Kai" <kai.huang@intel.com>
In-Reply-To: <fe9687d5f17fa04e5e15fdfd7021fa6e882d5e37.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?OFIxeUlOSEM4WVg1QkxSWVhMN0xXOVVidFEwSTM1d3NmSWtxNFN2dzlaVzhw?=
 =?utf-8?B?V09Ud0t6ZDg5RGJXOVdTakx5RWZwdU1ZNFBRbU1OSGRqWVZDdll4VnBLNUxa?=
 =?utf-8?B?WGpnZzhwanBkeGpVQi9RWlROTU9wS1BnZERXYm9XM1huVVJzcklQYjNHSC9J?=
 =?utf-8?B?REZncndyZ2RNd0FlWVc5VkdsVGYxdm4vVVo1TDdZL3NPNkhwOW9NVlpQNXdL?=
 =?utf-8?B?Nm5JKzZYeHhSY2EyTG8reU4yVWM5Q0dCT1k0N0w2MG44clJvOFJYUWgraDFn?=
 =?utf-8?B?dFl1c1pwSkJHOUdwVXJLS0N2bCtpbUprTTVxTmxXM1lMSzUwWFhRTkJZL2Fa?=
 =?utf-8?B?SmZoSXpmWmpCL1Y4UVViQzgwUDYvTmpGOStqeGxrelB4UldlMlhzVVluU3hR?=
 =?utf-8?B?eFhMZTdzL0pBT1RPM3VmUWE4VlpHWmZtVFhma0h4ZG8xOFkyd05PSWpOTGZr?=
 =?utf-8?B?QXdENC93VVFLVUYzLzVtSmFjeHhlNFBBYVovS1BtNnR5cUFPWStwYnFFMUY2?=
 =?utf-8?B?VkJtOFk2NW9EbVJMdUFITTFyWTQxTjMwdFEwUldoSHFPbHlLY1k5ZjREWHBm?=
 =?utf-8?B?ZG1GYUhrSkNxOGU5MTVSNVZZY0ptVndhd1V5VnFiS2NyLzNFZ0pkT2t4RWVR?=
 =?utf-8?B?bEMyTFdVS08zVkUxam5HbW1mN1ZYdmV6NnNTK1o4NDkvRUZaVllpcGJhelpO?=
 =?utf-8?B?MmU4WVY1b0JTTEZGL1NIN3FkU3NxUzRnaUU3RG1xVGN3MEN6TjU1N1d3UjdZ?=
 =?utf-8?B?MUo3cWYyWjR4ZEUvcXI2clFsNjFHVUcvTjF0Q045QnZLSUYwR3JiZ0t6aE13?=
 =?utf-8?B?V3VOYk9TOWV2WmJiTndlM09wM3J3clNvc0hpRXdjOUFCOWUrRVhUeEFUUFpr?=
 =?utf-8?B?WGM0Z2Nuemp1dkJhL1NMQWRQcGNmQWJxQ2Q2SEJrUld2NWpkaTVxS0VLcFox?=
 =?utf-8?B?cURRUk83UkJpWDVLUGthVUxyWkNseVdjZXl6T0ZuSS9nRUJ0RTd4eTg0a2py?=
 =?utf-8?B?Y2RhVGdGOWt5dUh3dXFUeVFZai96QW5MS3Znc2s1MUVVQngwOG5FWG1CSktw?=
 =?utf-8?B?VjJpVkkwVEpvRWY2UDJ0MDJEQ3NZMW0zdTd3d2VRaXowdG5ZWnY4TUNFTXM2?=
 =?utf-8?B?WDBPektCcGJ0TzFzbWlBK1NmenJjdDNsMjZhU0liQWs1azFyS09BelluL05u?=
 =?utf-8?B?ZGp3N0psS1ZUa0hOVGpaTno3V1d2T2xkNEpVd2prWW1rVTFZMSttdDhyUmI2?=
 =?utf-8?B?VG5XZVZmWXYzUmlnZmdSZGc3Q2dUR21CY2plLzZjeUlzMkNKa2lhckxOSHM0?=
 =?utf-8?B?RGVYbW84NHBia25UTmtLSFB3czFBSlFnK1paL1JGU21GbnMwOGlleG50OVlB?=
 =?utf-8?B?YUk3bXZVSU51Z1VVdUlhN1JyYTdjdEdhWUhqTytiQmxKVEs3d1plRnpSRDRj?=
 =?utf-8?B?YnRHZEZGQ09xSTR6RXlkNUlvdGJobm5LaHo5TklUYjBJQ050cGgyMFF3SVll?=
 =?utf-8?B?TGZHeWdNOGgxUENqQmVZNGhvUzdvSUxMb2V2dFJNR2xhUXd6Ung5WkRKLytj?=
 =?utf-8?B?a2NNUjJ6a3F5OW4rV0FCdTA3L1B2am9VNVRoa0hIZDEwSWVuNVBPYjFaRjRj?=
 =?utf-8?B?cE9kWUZLSGtXaDhJUnREKzBKRGlJY2ZoMndmRlBBU3NMdDc0Y242eUx4TGlp?=
 =?utf-8?B?cVMySHJrZEh5SGt1aWxCRHZXQjIwRndxaG9FL1ZzV0tPbkNmUk9IajN6TTIw?=
 =?utf-8?B?RXQ3OG5ocVNDNVpCRFBiQVhxa1NnN1BLVWRjNFBVRzNwWC9LaUxxb0p3WXRq?=
 =?utf-8?B?dHIzaExVV3NXOUNmOXhHbzlmR1dJVkhUc2lJYlVDdWJhM0o5RHJCN1RwdHRv?=
 =?utf-8?B?Nkd2NGZQRFlzczdZNGRzd2JQRXI5b212T2JleGhoaGROdUpiUGt6K3Fid3RG?=
 =?utf-8?B?d2pRYlR5QmdFUjJnZUc4VWdLcXRlVnBYRFRZSnpZZm5vTE14ZkcxWlFadUwz?=
 =?utf-8?B?VkZ2b2JLTjBmdkkwRDNxdTh3QWp4YTExR0UyM25XRVhSL2tjQlk0dlRQQS9J?=
 =?utf-8?B?TzFpV2tycWFCSFpIMjUxSE1zRUN1S0U4bmlDUlBxTm9ZT1ZBak1EOFgvVjF4?=
 =?utf-8?Q?l13viGtOjim6+kLsVWkhEyAzl?=
X-MS-Exchange-CrossTenant-Network-Message-Id: e77786b9-4d6d-4a5d-b3ce-08dc7538fc93
X-MS-Exchange-CrossTenant-AuthSource: BL1PR11MB5978.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 15 May 2024 23:44:39.7518
 (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: DACsyQqQV0osiL2ZhF8t98/rVSidSAeiB5/jVY8TeobBYs7kBr6UeXxqTe2DdzhX7rkfWy1wLRRhr7Cmt7bPqg==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: IA1PR11MB7342
X-OriginatorOrg: intel.com


On 16/05/2024 11:38 am, Edgecombe, Rick P wrote:
> On Thu, 2024-05-16 at 11:31 +1200, Huang, Kai wrote:
>>
>>
>> On 16/05/2024 11:21 am, Edgecombe, Rick P wrote:
>>> On Thu, 2024-05-16 at 10:34 +1200, Huang, Kai wrote:
>>>>
>>>>
>>>> On 15/05/2024 12:59 pm, Rick Edgecombe wrote:
>>>>> From: Isaku Yamahata <isaku.yamahata@intel.com>
>>>>>
>>>>> Introduce a "gfn_shared_mask" field in the kvm_arch structure to record
>>>>> GPA
>>>>> shared bit and provide address conversion helpers for TDX shared bit of
>>>>> GPA.
>>>>>
>>>>> TDX designates a specific GPA bit as the shared bit, which can be either
>>>>> bit 51 or bit 47 based on configuration.
>>>>>
>>>>> This GPA shared bit indicates whether the corresponding physical page is
>>>>> shared (if shared bit set) or private (if shared bit cleared).
>>>>>
>>>>> - GPAs with shared bit set will be mapped by VMM into conventional EPT,
>>>>>        which is pointed by shared EPTP in TDVMCS, resides in host VMM
>>>>> memory
>>>>>        and is managed by VMM.
>>>>> - GPAs with shared bit cleared will be mapped by VMM firstly into a
>>>>>        mirrored EPT, which resides in host VMM memory. Changes of the
>>>>> mirrored
>>>>>        EPT are then propagated into a private EPT, which resides outside
>>>>> of
>>>>> host
>>>>>        VMM memory and is managed by TDX module.
>>>>>
>>>>> Add the "gfn_shared_mask" field to the kvm_arch structure for each VM
>>>>> with
>>>>> a default value of 0. It will be set to the position of the GPA shared
>>>>> bit
>>>>> in GFN through TD specific initialization code.
>>>>>
>>>>> Provide helpers to utilize the gfn_shared_mask to determine whether a
>>>>> GPA
>>>>> is shared or private, retrieve the GPA shared bit value, and
>>>>> insert/strip
>>>>> shared bit to/from a GPA.
>>>>
>>>> I am seriously thinking whether we should just abandon this whole
>>>> kvm_gfn_shared_mask() thing.
>>>>
>>>> We already have enough mechanisms around private memory and the mapping
>>>> of it:
>>>>
>>>> 1) Xarray to query whether a given GFN is private or shared;
>>>> 2) fault->is_private to indicate whether a faulting address is private
>>>> or shared;
>>>> 3) sp->is_private to indicate whether a "page table" is only for private
>>>> mapping;
>>>
>>> You mean drop the helpers, or the struct kvm member? I think we still need
>>> the
>>> shared bit position stored somewhere. memslots, Xarray, etc need to operate
>>> on
>>> the GFN without the shared it.
>>
>> The struct member, and the whole thing.  The shared bit is only included
>> in the faulting address, and we can strip that away upon
>> handle_ept_violation().
>>
>> One thing I can think of is we still need to append the shared bit to
>> the actual GFN when we setup the shared page table mapping.  For that I
>> am thinking whether we can do in TDX specific code.
>>
>> Anyway, I don't think the 'gfn_shared_mask' is necessarily good at this
>> stage.
> 
> 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.

> 
> 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).