Received-SPF: pass (google.com: domain of linux-kernel+bounces-165610-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1;
Message-ID: <399cec29-ddf4-4dd5-a34b-ffec72cbfa26@intel.com>
Date: Wed, 1 May 2024 09:54:07 -0700
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v19 101/130] KVM: TDX: handle ept violation/misconfig exit
To: Isaku Yamahata <isaku.yamahata@intel.com>
CC: Chao Gao <chao.gao@intel.com>, <kvm@vger.kernel.org>,
	<linux-kernel@vger.kernel.org>, <isaku.yamahata@gmail.com>, Paolo Bonzini
	<pbonzini@redhat.com>, <erdemaktas@google.com>, Sean Christopherson
	<seanjc@google.com>, Sagi Shahar <sagis@google.com>, Kai Huang
	<kai.huang@intel.com>, <chen.bo@intel.com>, <hang.yuan@intel.com>,
	<tina.zhang@intel.com>, <isaku.yamahata@linux.intel.com>
References: <cover.1708933498.git.isaku.yamahata@intel.com>
 <f05b978021522d70a259472337e0b53658d47636.1708933498.git.isaku.yamahata@intel.com>
 <Zgoz0sizgEZhnQ98@chao-email>
 <20240403184216.GJ2444378@ls.amr.corp.intel.com>
 <43cbaf90-7af3-4742-97b7-2ea587b16174@intel.com>
 <20240501155620.GA13783@ls.amr.corp.intel.com>
Content-Language: en-US
From: Reinette Chatre <reinette.chatre@intel.com>
In-Reply-To: <20240501155620.GA13783@ls.amr.corp.intel.com>
Content-Type: text/plain; charset="UTF-8"
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?Z1h5aVAwNTdQbXBuY2tOczQ4Zi9WalRXemprSGlETkFBeVhYSUsxVGM4VDEx?=
 =?utf-8?B?NUpxaUk0NG9Nc1djSUp6bWVyM0hlQldsMGF4ZEx0a2tseTBDOTNqZWFnU2o3?=
 =?utf-8?B?SEJOdERzbzhKc2c4VkRxSVhQQ0VBaXhiR09LZE5lSU9aMWZYWnFWQ29Oekt1?=
 =?utf-8?B?b3RBdHZvaElmZXRJZXdSOXhnc1JtTXJjTzFqTlBnVU9RSUtoZ2Q4bXBpd0Zh?=
 =?utf-8?B?a3BybHJKSlpoZ1JBTDhrTDZBZmtRSXB6UkZ1WFFucUdiZkhpN0NvU044cWxV?=
 =?utf-8?B?Ymp4MlcwMVBVM3FLbHFDZjJGWi9SRU12UE5ubWxaVURBeFZ1OHdsN05aYlRT?=
 =?utf-8?B?bWdTWXNjaEwyZ1Z6Tlpibk5FTGlBRmxWb2luNGF2cVZqZHY5a3ZFcUZsSjZS?=
 =?utf-8?B?V2xYOGFpVEpPVmtHenF0bG5JS2J5c1NFU1NNczlnMW1HeVZIVVFoRWx5NjJy?=
 =?utf-8?B?RlI0bFY2N0k3TW9KVnFxMTJuTjZoaDBPTTRHRFovTGFpVjZOakprWlMrM1hF?=
 =?utf-8?B?MUoxa1pNdlVJWUExd0VvVnpVUFlhMGlMV2pmUkhFWUpXTXVvcFVmQmVLQktC?=
 =?utf-8?B?RWsvcHo0MUZmUExpMFU4dnNpWVMvNkJNcUtrOHE5bWVDdUdkeVplSnBoYjA1?=
 =?utf-8?B?R0lHRkU0bWVNaHM2TmVhSmhWUHBzcFJ3UDh4dDFNVnprZmRLbU14d0N2ZkNQ?=
 =?utf-8?B?K0VQR2RBTUZJWEFLVHZ1cGpMcm96NThQeUlnbUlNRUxxMGVCWmVaNlVSaWFZ?=
 =?utf-8?B?eVp5SythWW1WRHlacjZqc0UxcmhOWXFDSTA0eVk3SWZGZThQd2diNWo0Rzdr?=
 =?utf-8?B?b2hUNFpuMGQ1RmVRNndTckNPak9HYXdmbjEra1BpaEJQa2ZST1NPb3VEYWxT?=
 =?utf-8?B?NWlmQnIyUTVxMGUwZHJGVkV2Ri9ScUtYY0hERUVuamFnN2ZQS1BLTWJPRGtT?=
 =?utf-8?B?ejVDKzRFb1F3dk5MY3JKS1JuMjNUTS8xdi8xcTVzY2RHK1NKckJOQ05GUXVT?=
 =?utf-8?B?ZkY1aVFyR0k4N1BuOGdibzJpWjVpK1gvT1RQMi9ZbEdGQ2dMUk5kQ0RvaHkr?=
 =?utf-8?B?dTdTY0U4a3RTVDZvOG9jUkdMQWp3WnYrWVVicmU4UlhNOGxxVmR0bUwxNVp2?=
 =?utf-8?B?dTRFMXBSTjVNQml2ZG1paE9WZWlTOWhjbTlTdzBIamo3VnJRay92VENQNVo4?=
 =?utf-8?B?Skkxa0ZvQTBnWCtBcnFmb1hPamxuOGNDSGRZL2NWNjZvNXZuOFhZQWlmbkox?=
 =?utf-8?B?NTBhNGsxa2FOVm02NG1XcUpadkt5ZndsbHhHZkRwc0hSTVhDV3JyVlVLU2Na?=
 =?utf-8?B?blFMczRkZnFtejVzemI1TTFKRzV6NHlIVEZWNlZPelJyTWlqK0dQWHprdkFT?=
 =?utf-8?B?VC84RnBMakUwVGUzOFJ3bmMyQmxYOFJidERBTmNkOEQ3WUJ1QzBTUGhuWDJY?=
 =?utf-8?B?NzJDekpMKzFkR29veWRzWVhpUSt6WE5MOE5aR2I3bC9LRUl4RE9yWnY4Vmor?=
 =?utf-8?B?UldmMW1Pall0WEZLOTQ0andteWRyNk9mc0hrN01qTVdQcVp5ckFxMHJaY2pP?=
 =?utf-8?B?NFpzRnFoWkFGTlBydlZQaTNxRDlCeVNuMWVJTEZCbWI4WHBCbS90VGl5VlAy?=
 =?utf-8?B?YUVybEYwVE5uZjJ1TUl0RlhLT2Y2U2sxb1ovTXlXVElQUXJZd2VKMWh6a09Y?=
 =?utf-8?B?UCt0SDVBWFhPdFVtTjhsVGhMcGJPbmorTVJvSEpkYWhwVkJoOE5memVhV0xB?=
 =?utf-8?B?RXJacDNrQzhVTGhMN3RsVTBSQStLTFc5MGFMUnVHQkxxVHlnSHlxK2U0Ti8y?=
 =?utf-8?B?SWZZdmJlY2xLSUdHMFU2bVNZbkNNdW8zTWo2Q3BZWnVPdEdLNzM2aTY0aDNz?=
 =?utf-8?B?RDlrK1FuQ1VIR2llT04zcmZSMTNRZDd2WHNTdlk3L251am1zV3hDVjNXcHVH?=
 =?utf-8?B?YmhSbW5aRDZnNVorcGJLejIyVS9Ib2MrOUg0bytQOUQrVVQrdkVOMHFodGFn?=
 =?utf-8?B?eVA2Wk5sMFU5VmhxV2RUU29EcnhCQk9FVUZUU05pTURmZ09rYisva3NRc0xm?=
 =?utf-8?B?c05kdWlqKzVYMTZrOC9KZmZiNGFGazRGcEl5bGlLOWRhL2g2bVg0NFkzM1Vr?=
 =?utf-8?B?ZmpNRythM2xaREFxelNqaHQzUFJDc0VaZk1xTWNvWlpKcGN6Y3RIY0RqdGtX?=
 =?utf-8?B?Wmc9PQ==?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 7fdaa096-28e2-4003-1e4b-08dc69ff52c6
X-MS-Exchange-CrossTenant-AuthSource: SJ2PR11MB7573.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 01 May 2024 16:54:10.7991
 (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: SFqQf1cWE/pFXc3h2k9oK1S376fsuWq4NxwEiQtNo1xXNWPeBV6UGhbrvjcFMTBxNnB13F59BbrSW4ypqYk8MGdirN7rIH7pFcMSnpCEAkk=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CH3PR11MB8239
X-OriginatorOrg: intel.com

Hi Isaku,

On 5/1/2024 8:56 AM, Isaku Yamahata wrote:
> On Tue, Apr 30, 2024 at 01:47:07PM -0700,
> Reinette Chatre <reinette.chatre@intel.com> wrote:
>> On 4/3/2024 11:42 AM, Isaku Yamahata wrote:
>>> On Mon, Apr 01, 2024 at 12:10:58PM +0800,
>>> Chao Gao <chao.gao@intel.com> wrote:
>>>
>>>>> +static int tdx_handle_ept_violation(struct kvm_vcpu *vcpu)
>>>>> +{
>>>>> +	unsigned long exit_qual;
>>>>> +
>>>>> +	if (kvm_is_private_gpa(vcpu->kvm, tdexit_gpa(vcpu))) {
>>>>> +		/*
>>>>> +		 * Always treat SEPT violations as write faults.  Ignore the
>>>>> +		 * EXIT_QUALIFICATION reported by TDX-SEAM for SEPT violations.
>>>>> +		 * TD private pages are always RWX in the SEPT tables,
>>>>> +		 * i.e. they're always mapped writable.  Just as importantly,
>>>>> +		 * treating SEPT violations as write faults is necessary to
>>>>> +		 * avoid COW allocations, which will cause TDAUGPAGE failures
>>>>> +		 * due to aliasing a single HPA to multiple GPAs.
>>>>> +		 */
>>>>> +#define TDX_SEPT_VIOLATION_EXIT_QUAL	EPT_VIOLATION_ACC_WRITE
>>>>> +		exit_qual = TDX_SEPT_VIOLATION_EXIT_QUAL;
>>>>> +	} else {
>>>>> +		exit_qual = tdexit_exit_qual(vcpu);
>>>>> +		if (exit_qual & EPT_VIOLATION_ACC_INSTR) {
>>>>
>>>> Unless the CPU has a bug, instruction fetch in TD from shared memory causes a
>>>> #PF. I think you can add a comment for this.
>>>
>>> Yes.
>>>
>>>
>>>> Maybe KVM_BUG_ON() is more appropriate as it signifies a potential bug.
>>>
>>> Bug of what component? CPU. If so, I think KVM_EXIT_INTERNAL_ERROR +
>>> KVM_INTERNAL_ERROR_UNEXPECTED_EXIT_REASON is more appropriate.
>>>
>>
>> Is below what you have in mind?
> 
> Yes. data[0] should be the raw value of exit reason if possible.
> data[2] should be exit_qual.  Hmm, I don't find document on data[] for

Did you perhaps intend to write "data[1] should be exit_qual" or would you
like to see ndata = 3? I followed existing usages, for example [1] and [2],
that have ndata = 2 with "data[1] = vcpu->arch.last_vmentry_cpu".

> KVM_INTERNAL_ERROR_UNEXPECTED_EXIT_REASON.
> Qemu doesn't assumt ndata = 2. Just report all data within ndata.

I am not sure I interpreted your response correctly so I share one possible
snippet below as I interpret it. Could you please check where I misinterpreted
you? I could also make ndata = 3 to break the existing custom and add
"data[2] = vcpu->arch.last_vmentry_cpu" to match existing pattern. What do you
think?

diff --git a/arch/x86/kvm/vmx/tdx.c b/arch/x86/kvm/vmx/tdx.c
index 499c6cd9633f..ba81e6f68c97 100644
--- a/arch/x86/kvm/vmx/tdx.c
+++ b/arch/x86/kvm/vmx/tdx.c
@@ -1305,11 +1305,20 @@ static int tdx_handle_ept_violation(struct kvm_vcpu *vcpu)
 	} else {
 		exit_qual = tdexit_exit_qual(vcpu);
 		if (exit_qual & EPT_VIOLATION_ACC_INSTR) {
+			union tdx_exit_reason exit_reason = to_tdx(vcpu)->exit_reason;
+
+			/*
+			 * Instruction fetch in TD from shared memory
+			 * causes a #PF.
+			 */
 			pr_warn("kvm: TDX instr fetch to shared GPA = 0x%lx @ RIP = 0x%lx\n",
 				tdexit_gpa(vcpu), kvm_rip_read(vcpu));
-			vcpu->run->exit_reason = KVM_EXIT_EXCEPTION;
-			vcpu->run->ex.exception = PF_VECTOR;
-			vcpu->run->ex.error_code = exit_qual;
+			vcpu->run->exit_reason = KVM_EXIT_INTERNAL_ERROR;
+			vcpu->run->internal.suberror =
+				KVM_INTERNAL_ERROR_UNEXPECTED_EXIT_REASON;
+			vcpu->run->internal.ndata = 2;
+			vcpu->run->internal.data[0] = exit_reason.full;
+			vcpu->run->internal.data[1] = exit_qual;
 			return 0;
 		}
 	}

Reinette

[1] https://github.com/kvm-x86/linux/blob/next/arch/x86/kvm/vmx/vmx.c#L6587
[2] https://github.com/kvm-x86/linux/blob/next/arch/x86/kvm/svm/svm.c#L3436