Received-SPF: pass (google.com: domain of linux-kernel+bounces-134157-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Date: Sun, 7 Apr 2024 11:50:04 +0800
From: Chao Gao <chao.gao@intel.com>
To: Isaku Yamahata <isaku.yamahata@intel.com>
CC: Sean Christopherson <seanjc@google.com>, <kvm@vger.kernel.org>,
	<linux-kernel@vger.kernel.org>, <isaku.yamahata@gmail.com>, Paolo Bonzini
	<pbonzini@redhat.com>, <erdemaktas@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>
Subject: Re: [PATCH v19 108/130] KVM: TDX: Handle TDX PV HLT hypercall
Message-ID: <ZhIX7K0WK+gYtcan@chao-email>
References: <cover.1708933498.git.isaku.yamahata@intel.com>
 <c083430632ba9e80abd09bccd5609fb3cd9d9c63.1708933498.git.isaku.yamahata@intel.com>
 <ZgzMH3944ZaBx8B3@chao-email>
 <Zg1seIaTmM94IyR8@google.com>
 <20240404232537.GV2444378@ls.amr.corp.intel.com>
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <20240404232537.GV2444378@ls.amr.corp.intel.com>
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?LOOiQCvZwJbY4CTEqsOwo8nW6G1dj4FpQ48fNJaEAlVzUZgea5UWFH8cgRjl?=
 =?us-ascii?Q?EflERt+uviHfJL31RXzPPzhFNLIey5Ii/ipg2H4EDfIKef6tY75l24nrhI2j?=
 =?us-ascii?Q?MVNyaSg3nch5CW7nd4nttQSa7SuNpgpCc1UYO/cTAoaPUkQyVSSoAA1pKjIS?=
 =?us-ascii?Q?FeLuaxN+QESx5WMMBoIgRi6QoOmro6+Z0oygW5tFIpmdtRK56hs2oYu9YVUx?=
 =?us-ascii?Q?R5AplsxNtHGnmnbdP9eeWH2T9Of2BkYg2HfNjxw5jeDlw27u8a6qhWKQtCGr?=
 =?us-ascii?Q?akGTmmmVMKVNvL413O5PtDKu8G14Y7t11Gm1GRIEgNxDRwEdiNZQPljrfVFS?=
 =?us-ascii?Q?ozCWuw+tNFMFG6KlN+e1Vhn4IBr35rm1tnxm0tE4lFBDQ1RpcL3oOXFGF3MK?=
 =?us-ascii?Q?/JHJEtzazjEIiBoFxhU6Mv7Irw/kk6Xx5QV9slg1SRQcYJN801is7mprctNk?=
 =?us-ascii?Q?jT5NaJGsEe09hbCVhU2jfZhUs+oFtqnPR6bgGvtGjJMJ71d4gFufJbBJgH6N?=
 =?us-ascii?Q?6rA96r4/E2yB/mNVsb6aUQCpsfWq+IFGoAw72T/goGWvfPgWZ/H27mWKppF7?=
 =?us-ascii?Q?g2djo5/+p9lq2nnNJbJk6a3VcYMNwh0GnubYRL12XUGfcXtFJfJJwHPq6qQK?=
 =?us-ascii?Q?Fs/fgQ1+qOSepgI75jFSRY+zJuwvDmrONgPdJlN/I1+Sdu/nmSEQDro5jnRD?=
 =?us-ascii?Q?j6gpPKnAhSR1/JEbBJjG6Bvot4Cu6tr0Elwm17i1ZV2B9aRDJB0BcWC9jAwu?=
 =?us-ascii?Q?B307GLWEdUCrqmWUeBcADh7kRLD5NZW0r2Hnx1R3IzTlIMqCBK7aSkh8UEt5?=
 =?us-ascii?Q?4agLsg9zSpeSjW+2vXc7RJW5S5CwjSITuBtFJNyGPJVR5nMnrJsLPD1pgVJv?=
 =?us-ascii?Q?t1wSDGoiyv84O9iznxqUrVXtoveiFeuzSkCCgZvI0BZhS3Hscwcz9SEiYXNt?=
 =?us-ascii?Q?1Yn+3c2ZATtm8BBJ06o/Sp7E5BrSMyPsw/HivsnFOooKoYEk653PZHLNCbpS?=
 =?us-ascii?Q?jqdYNa2NJWbQFKJ+S/ibCAsUhH7IgdwU0TEsDvNvBqKBwT5ENoH0zRRXNZa8?=
 =?us-ascii?Q?wleYdjHe+F3MDUgBzJMALRW5KooP2WkDvvn/RpHBEhUgUpT7oIC0XvDQU2U8?=
 =?us-ascii?Q?O8EL0xiTWF5Ma4m2fSdyzEXpSbdk7zfaFzatJKnRxACcTbZd+G/FiKE9Z1kK?=
 =?us-ascii?Q?i4Ucyr9XTffxeHQRUqau35Uu6LvEbjWebZHPrQ5uZk0kqn8nkXSf59MuZBt4?=
 =?us-ascii?Q?+SvNNLBPHY4IqcLbt0VNwNcSARUxJbAsV3IYNiouk393kOcG4JOtKiAWbPoE?=
 =?us-ascii?Q?jqF0q/8VjONGCc4aB/Cw7gFp6+0xMG5GoavPgtDC2jXmCqs6Om+O3TpVHCIE?=
 =?us-ascii?Q?dsSvXH8fvI3jHyhMwz5p8BfjeivD5fUdgoTggJPr2Q+MOcuOKJCFskxriGlC?=
 =?us-ascii?Q?Y8rR2PE2B1b2OjkBrzLXWv53N31qLgrha5jpRVXtcb5g5LN14QH+S0nLmsnh?=
 =?us-ascii?Q?dbJlrBX3MqG3+4Zy5MM7wppsrjVojfVzNgKTnsC1ZjgwSXndFZbYbwjCxXRR?=
 =?us-ascii?Q?B80DmRJmrnaPZxXph9GZSOSu9dW6+tE1kwPVO8VV?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 56998a8f-c4bb-4c30-8047-08dc56b5d52a
X-MS-Exchange-CrossTenant-AuthSource: CH3PR11MB8660.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 07 Apr 2024 03:50:14.6610
 (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: SjzuwJfzOh2CXoc5PfA414kG41pqso4ww3atyKSdjvTKNDasOZmXS/seNa+aM3GtBKbv6MV1D095Ugn4GAqoHQ==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BY1PR11MB7982
X-OriginatorOrg: intel.com

>> > >+	union tdx_vcpu_state_details details;
>> > >+	struct vcpu_tdx *tdx = to_tdx(vcpu);
>> > >+
>> > >+	if (ret || vcpu->arch.mp_state != KVM_MP_STATE_HALTED)
>> > >+		return true;
>> > 
>> > Question: why mp_state matters here?
>> > >+
>> > >+	if (tdx->interrupt_disabled_hlt)
>> > >+		return false;
>> > 
>> > Shouldn't we move this into vt_interrupt_allowed()? VMX calls the function to
>> > check if interrupt is disabled.
>
>Chao, are you suggesting to implement tdx_interrupt_allowed() as
>"EXIT_REASON_HLT && a0" instead of "return true"?
>I don't think it makes sense because it's rare case and we can't avoid spurious
>wakeup for TDX case.

Yes. KVM differeniates "interrupt allowed" from "has interrupt", e.g.,

static inline bool kvm_vcpu_has_events(struct kvm_vcpu *vcpu)
..

	if (kvm_arch_interrupt_allowed(vcpu) &&
	    (kvm_cpu_has_interrupt(vcpu) ||
	    kvm_guest_apic_has_interrupt(vcpu)))
		return true;


I think tdx_protected_apic_has_interrupt() mixes them together, which isn't
good.

Probably it is a minor thing; if no one else thinks it is better to move the
"interrupt allowed" check to tdx_interrupt_allowed(), I am also fine with not
doing that.