Received-SPF: pass (google.com: domain of linux-kernel+bounces-180551-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Message-ID: <d8ff5e19-85a7-46bf-9dad-7221b54d8502@intel.com>
Date: Thu, 16 May 2024 13:40:41 +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>, "linux-kernel@vger.kernel.org"
	<linux-kernel@vger.kernel.org>, "Aktas, Erdem" <erdemaktas@google.com>,
	"isaku.yamahata@gmail.com" <isaku.yamahata@gmail.com>, "dmatlack@google.com"
	<dmatlack@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>
Content-Language: en-US
From: "Huang, Kai" <kai.huang@intel.com>
In-Reply-To: <0a1afee57c955775bef99d6cf34fd70a18edb869.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?VW05Q1YrMHBZb2FUaTZVN1dCNzI3S1lGWE5EVnpCdlp3TGNaNThLekd1OEFV?=
 =?utf-8?B?OVRqcnIrdGhPS2ZSWm8zbXo2TXNTRXFmMjR0aHRYZjFCODY0Q2J1V1NyL1p3?=
 =?utf-8?B?RENNbWxMdGdYVVFPTU4xSExXNVpmVVN5Z096VXBEK053TTJhdFloMi9vYWF2?=
 =?utf-8?B?VXhVZnVmNkJoLzlwb2hMZHkrMjExNWZzYVczNVVra1VCUXpSOUFaUWNoek5v?=
 =?utf-8?B?YUQ2aXhPNVlBMUZBdVQ3Nzh4RHgyTk5VRHBORDRrUW5CL0t4M3FXamIyQm1x?=
 =?utf-8?B?WEhNdXdQb3Jzck5Tc2U3Vm1hUXlTbHdwVFE2SmFGclFGcjIvTllYUTI4T2U3?=
 =?utf-8?B?blZOa1F1Nm9jSjY0L0RRT1VGTmMwTWdCRHZNaXpsL3I2aEtaaHRyOW1xUGY5?=
 =?utf-8?B?WFl0U1VQeUpUNkJUeUo1NDUwVnBsSFVrakR6UW5yTllLNlBqZVg1UXVibzNU?=
 =?utf-8?B?OXZuVHQ0NFg2V1E2L0syQTc2b2g2WWFLaXYzT3ltM25aTjhoTDVMNmZ5eVgx?=
 =?utf-8?B?ejlFaG54S3JEQWU3cHpwdFFWcjloZ2w0VEJuYkMvVlprUmprS3dVTkpvYjF5?=
 =?utf-8?B?aXUxaXNqYjNVS3dydVJLRU1sanBTSTBDdTZ0eU5YcjZmblFLNzJQSEp2QzBS?=
 =?utf-8?B?elBNaTUzSDM2Y2hOK0ZtbkZmaWFyQlZPZEZPQzZTTm41cDY4KzlDMm5lU1ZV?=
 =?utf-8?B?QU04Ny9GbUh0MjRCN3E5ZnVoYU5LN2p4N2JsOXFCRUJrYmFPUnp6Z0xpTFZR?=
 =?utf-8?B?VDF1ZUFnNENFVW96MzhXZXNYclZ4cjlGY3BqUUtLTm5nWjE5OXhrTzMrUVlI?=
 =?utf-8?B?YmhCZitTSVhiRUFPdWtJWmdxdlo4Wi9tSENzcHNIWVpxb0s4SjdvYlZybzdK?=
 =?utf-8?B?L2JTR3YvZy9MaUc4YXlZTzFYb0RhcnNnSnBrUEZVM2FkV3ZoUHdZZitiY29h?=
 =?utf-8?B?VU5vNjZkRlliWGxPRlV5V21POHRWMitTdlhoSythT0hnR1Awc3V0Q045K0J3?=
 =?utf-8?B?OG12SEk4ME9rekZOcjFlVWdhQlE0dUIwRWNXU2dCcExURUVUTnpTeWtjeDky?=
 =?utf-8?B?SHR4YVRWc1dKUXBjTkVxRW5wcWlCVGx5RXI3QiswTWtkSUtHdkVFUkdJRU9N?=
 =?utf-8?B?cTladU5wOFBoL0tYSHJ0aVJqdzA0d04wZEVDRG5YVERFWml5T04yaGR3V2VC?=
 =?utf-8?B?Vk9GWms4aTEwdHJKK2h0VGNjVUdkTWZaUG9SZmY4dzFCbVZmYXVzNDBSVjRM?=
 =?utf-8?B?bWk2YXhib3NpR2w1RXlWNnFBclE2V3ZwUDkwam5xelBiRHRRV0k1WTZNOFlu?=
 =?utf-8?B?WEd0ckFDd0F0bEkvdHh5VDhBN0xYVGZVcXFVWjJJcUs1TkQ1YkZETE1PYnFR?=
 =?utf-8?B?NE5JZzAyVHBqbk9TNHJXUUFQZTNIQklTS3Z0dWIvcmxBZGdsSmhnUmUrOTRG?=
 =?utf-8?B?OHE2bWpUOGswRlFkNkh1RHY1ZU1MSndDUEVsSGhLT09PK1dMMGZyZUM5YWxo?=
 =?utf-8?B?RzQ5aHZkdEtZV3BHS1BrdFhBZEwvc1MrZWVDUS9la3pSNjBHaHlCa2ZRdmZ4?=
 =?utf-8?B?Wko1ZmZ1TXBCdUdCV2QrZnJFRmphRS9BSUlrVDdiSHBrWktTbGpMbFlrNnl4?=
 =?utf-8?B?V2JIcC9lQmpseTdRNXVtNElMZWg2cWo0S3pmam9UQjdhYU5nR0orU016WU5o?=
 =?utf-8?B?bkIvUThMOXlsSStxTTFBNjZVRDZ2RzlZL0hUMFVjYTM3cUxuT0l3TERQbE5l?=
 =?utf-8?B?bitDamVRckJ2MGJLbzRTRUpzSjRqd3A4SitSMnZYY1g5eWhpbmtWSXQrTXFY?=
 =?utf-8?B?VmxIc25PSW02NlkvYWo2eStEaU5yVG1SL1M2bzFSNm1RWUxVa0xkNTlpYU91?=
 =?utf-8?B?blBtZm95aHppbHJwc05FT3FIdmVxeXlNREY0Z3JGVnVSRWx5c0sxaDJ1bFdR?=
 =?utf-8?B?MENoSXB5UUQ0cUE5K1NxdWRXd1BkZ3hOdnVibG5rN0pDb3o5a2wvR01uUVBG?=
 =?utf-8?B?UzRnNVVrcUdLK1VUWkRvUXJKbzRYL0xQd0tYSFI1WkNpWHliMFN6U1Q2UUVq?=
 =?utf-8?B?andybFlzNEdXd3pHZGxZNEt2QWdIV3dqRzZDS3FYY25mY1VTY3dxeCthL25n?=
 =?utf-8?Q?DM1cIzPbBZZdqIl49TwDZUdrC?=
X-MS-Exchange-CrossTenant-Network-Message-Id: c3fa703c-c60d-4909-c5c0-08dc75493777
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:40:50.4910
 (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: 3MVtY8pAjZD5i1uPb5POaomM0KeUntjBcmq9M9eXL/yY0Hq14IE4NaeGm/7AquEamq8unKfU5eF/sS8OIx7ERQ==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DS0PR11MB7787
X-OriginatorOrg: intel.com


On 16/05/2024 1:20 pm, Edgecombe, Rick P wrote:
> On Thu, 2024-05-16 at 13:04 +1200, Huang, Kai wrote:
>>
>> I really don't see difference between ...
>>
>>          is_private_mem(gpa)
>>
>> ... and
>>
>>          is_private_gpa(gpa)
>>
>> If it confuses me, it can confuses other people.
> 
> Again, point taken. I'll try to think of a better name. Please share if you do.
> 
>>
>> The point is there's really no need to distinguish the two.  The GPA is
>> only meaningful when it refers to the memory that it points to.
>>
>> So far I am not convinced we need this helper, because such info we can
>> already get from:
>>
>>     1) fault->is_private;
>>     2) Xarray which records memtype for given GFN.
>>
>> So we should just get rid of it.
> 
> Kai, can you got look through the dev branch a bit more before making the same
> point on every patch?
> 
> kvm_is_private_gpa() is used to set PFERR_PRIVATE_ACCESS, which in turn sets
> fault->is_private. So you are saying we can use these other things that are
> dependent on it. Look at the other callers too.

Well, I think I didn't make myself clear.

I don't object to have this helper.  If it helps, then we can have it.

My objection is the current implementation of it, because it is 
*conceptually* wrong for SEV-SNP.

Btw, I just look at the dev branch.

For the common code, it is used in kvm_tdp_mmu_map() and 
kvm_tdp_mmu_fast_pf_get_last_sptep() to get whether a GPA is private.

As said above, I don't see why we need a helper with the "current 
implementation" (which consults kvm_shared_gfn_mask()) for them.  We can 
just use fault->gfn + fault->is_private for such purpose.

It is also used in the TDX code like TDX variant handle_ept_violation() 
and tdx_vcpu_init_mem_region().  For them to be honest I don't quite 
care whether a helper is used.  We can have a helper if we have multiple 
callers, but this helper should be in TDX code, but not common MMU code.