From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-6.6 required=3.0 tests=BAYES_00, HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,NICE_REPLY_A,SPF_HELO_NONE, SPF_PASS,USER_AGENT_SANE_1 autolearn=no autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 6C1D2C4338F for ; Tue, 24 Aug 2021 18:18:55 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 3644E611AF for ; Tue, 24 Aug 2021 18:18:55 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S232930AbhHXSTh (ORCPT ); Tue, 24 Aug 2021 14:19:37 -0400 Received: from mga02.intel.com ([134.134.136.20]:17853 "EHLO mga02.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229548AbhHXSTg (ORCPT ); Tue, 24 Aug 2021 14:19:36 -0400 X-IronPort-AV: E=McAfee;i="6200,9189,10086"; a="204570173" X-IronPort-AV: E=Sophos;i="5.84,347,1620716400"; d="scan'208";a="204570173" Received: from fmsmga001.fm.intel.com ([10.253.24.23]) by orsmga101.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 24 Aug 2021 11:18:51 -0700 X-IronPort-AV: E=Sophos;i="5.84,347,1620716400"; d="scan'208";a="597672338" Received: from ydevadig-mobl.amr.corp.intel.com (HELO skuppusw-mobl5.amr.corp.intel.com) ([10.209.137.104]) by fmsmga001-auth.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 24 Aug 2021 11:18:50 -0700 Subject: Re: [PATCH v5 08/12] x86/tdx: Add HLT support for TDX guest To: Sean Christopherson , Borislav Petkov Cc: Thomas Gleixner , Ingo Molnar , Peter Zijlstra , Andy Lutomirski , Peter H Anvin , Dave Hansen , Tony Luck , Dan Williams , Andi Kleen , Kirill Shutemov , Kuppuswamy Sathyanarayanan , x86@kernel.org, linux-kernel@vger.kernel.org References: <20210804181329.2899708-1-sathyanarayanan.kuppuswamy@linux.intel.com> <20210804181329.2899708-9-sathyanarayanan.kuppuswamy@linux.intel.com> From: "Kuppuswamy, Sathyanarayanan" Message-ID: Date: Tue, 24 Aug 2021 11:18:48 -0700 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Firefox/78.0 Thunderbird/78.11.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 8/24/21 10:06 AM, Sean Christopherson wrote: > On Tue, Aug 24, 2021, Borislav Petkov wrote: >> On Wed, Aug 04, 2021 at 11:13:25AM -0700, Kuppuswamy Sathyanarayanan wrote: >>> +static __cpuidle void tdg_safe_halt(void) >>> +{ >>> + u64 ret; >>> + >>> + /* >>> + * Enable interrupts next to the TDVMCALL to avoid >>> + * performance degradation. >> >> That comment needs some more love to say exactly what the problem is. > > LOL, I guess hanging the vCPU counts as degraded performance. But this comment > can and should go away entirely... > >>> + */ >>> + local_irq_enable(); > > ...because this is broken. It's also disturbing because it suggests that these > patches are not being tested. Sorry, some how we missed this issue before our submission. We do usual boot test before submission. Since this fix does not block the boot process, it did not get caught. But we already found this in full functional testing and also fixed it in github tree. I will remove this in next submission. > > The STI _must_ immediately precede TDCALL, and it _must_ execute with interrupts > disabled. The whole point of the STI blocking shadow is to ensure interrupts are > blocked until _after_ the HLT completes so that a wake event is not recongized > before the HLT, in which case the vCPU will get stuck in HLT because its wake > event alreadyfired. Enabling IRQs well before the TDCALL defeats the purpose of > the STI dance in __tdx_hypercall(). > > There's even a massive comment in __tdx_hypercall() explaining all this... > >>> + >>> + /* IRQ is enabled, So set R12 as 0 */ > > It would be helpful to use local variables to document what's up, e.g. > > const bool irqs_enabled = true; > const bool do_sti = true; > > ret = _tdx_hypercall(EXIT_REASON_HLT, irqs_enabled0, 0, 0, do_sti, NULL); Ok. I can follow your suggestion in next submission. > >>> + ret = _tdx_hypercall(EXIT_REASON_HLT, 0, 0, 0, 1, NULL); >>> + >>> + /* It should never fail */ >>> + BUG_ON(ret); >>> +} >> >> -- >> Regards/Gruss, >> Boris. >> >> https://people.kernel.org/tglx/notes-about-netiquette -- Sathyanarayanan Kuppuswamy Linux Kernel Developer