Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <6b425468-3aac-0123-c690-df8d750ce29e@suse.com>
Date:   Mon, 17 Jul 2023 10:02:01 +0300
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.11.0
Subject: Re: [PATCH 08/10] x86/tdx: Unify TDX_HYPERCALL and TDX_MODULE_CALL
 assembly
Content-Language: en-US
To:     "Huang, Kai" <kai.huang@intel.com>,
        "kirill.shutemov@linux.intel.com" <kirill.shutemov@linux.intel.com>,
        "peterz@infradead.org" <peterz@infradead.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Cc:     "Hansen, Dave" <dave.hansen@intel.com>,
        "Christopherson,, Sean" <seanjc@google.com>,
        "bp@alien8.de" <bp@alien8.de>, "x86@kernel.org" <x86@kernel.org>,
        "hpa@zytor.com" <hpa@zytor.com>,
        "mingo@redhat.com" <mingo@redhat.com>,
        "tglx@linutronix.de" <tglx@linutronix.de>,
        "pbonzini@redhat.com" <pbonzini@redhat.com>,
        "kvm@vger.kernel.org" <kvm@vger.kernel.org>,
        "Yamahata, Isaku" <isaku.yamahata@intel.com>,
        "sathyanarayanan.kuppuswamy@linux.intel.com" 
        <sathyanarayanan.kuppuswamy@linux.intel.com>
References: <cover.1689151537.git.kai.huang@intel.com>
 <c22a4697cfe90ab4e1de18d27aa48ea2152dbcfa.1689151537.git.kai.huang@intel.com>
 <b95c4169-88c8-219e-87b7-6c4e058c246a@suse.com>
 <71396c8928a5596be70482a467e9ba612286d659.camel@intel.com>
From:   Nikolay Borisov <nik.borisov@suse.com>
In-Reply-To: <71396c8928a5596be70482a467e9ba612286d659.camel@intel.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?Yng0T1k0cGZOZUtwMXptQ3NBRmYvbkY3dXRNTGswbWhjMWFENUkweGpuUUhV?=
 =?utf-8?B?a3M5RHFqS0l3NWNLcmFBaEdxaVRYSUcvTDVVZ08vN3JWaTdnNGtPbUxvbWVw?=
 =?utf-8?B?TzJ5VGQ2cmhnZytvMlRvcFdrZU8xdHdUWkd5clFibGJTZ05tZ1ViaDM5KzZy?=
 =?utf-8?B?dHVia0FYU1FxeEhlTGRWTHpYWjVGbHNLckF5c2wxYnRwQnI5Y1liUU13QXo2?=
 =?utf-8?B?M3BYSGsyNGtvNG5uQ1hmeklvYVBJZ1FYeUk5U01pZ0xqdnBxdkRSMHYzL1NT?=
 =?utf-8?B?Z3hYblBJTHFkbWZMc1hncEx3UXJZRWxiNm5JYm5yOTFyRTdnSzVtODVybEFu?=
 =?utf-8?B?c0VoR2xEQ1lRUWNsVi9IUGUwd3JDTDNJdWUrbVhxS2FMNW9mM1BiVkRuTzVx?=
 =?utf-8?B?MDFNV3JaajNMcjdCcVFQYmo5cFQwK3RVQWhsbVcxRUE2RW0vRmFEVWw4SmNG?=
 =?utf-8?B?dkwwc2FweGwrZE54THNVUC9KTEZwNE9SYlduU0U0R0x2cmdlOVBsQ1hqWk5y?=
 =?utf-8?B?OC9BOFYyZm5VRGZWY0FWbDZhOHVJREtDYk1EV3J0a01JVHNWQVplT0NZSXdk?=
 =?utf-8?B?ZzBxbXJ5WmM3UmxnUjl4cjFWS3lHTnE0M1VUVHBXdEVQRmVhNXQrNlQvTXNy?=
 =?utf-8?B?dDR4bXV4azlwUk13MVRWM0JmTm1mcWQ5eTJqdHJKdXpJTHpGS29ycndCbXRy?=
 =?utf-8?B?RVB4WmxFM21paHp2UkV5TnpMNjdGd0grTnRNdGNBT1ZxR1Y4YkNITjE3SzZX?=
 =?utf-8?B?ZVNTUEwrdGdHdC95NnBiNWhqaUtMQVgwRUJyay9aTWs2WUFHYkVBNDlCSFRY?=
 =?utf-8?B?MDVmdU9jT2VFc3VKTzV0aW5TUXZBODRib0lmQ3g1SmNQbWRjUlh1djdvUmpD?=
 =?utf-8?B?cFkydDZzZ0pHekNEV3V6aUU2bzFWcUdqajZ5aTRuMkJtMlJBK3FldTZLbEtQ?=
 =?utf-8?B?S3JsVkp5aUVmOWR0NXRZZ2k5Y3p2QWZkbDZnWG05MWZrR1JTSHpEOUhJTk00?=
 =?utf-8?B?ejR5cEkxcEdzWkMyUXdNejljeUV4clBmTGJCZ0lYdmsra3JLUWNkZTRhMlNJ?=
 =?utf-8?B?cGEwaXFrSE1ic3lMbHpRb01rZHBtM3hRdURrZ2s2cVJ6RHlWRC9lR3RqSkJs?=
 =?utf-8?B?TG5uci9uQlVBTEV3VFFWVnlqdklaa0VndnZFbGZwWHVQVDVtYVErNytGbG1h?=
 =?utf-8?B?YXh1bEhKVnFObncyZFdlbmI0Y00yNWxGbDFaNGtBRitIeGRUQ3BqbzJQSERZ?=
 =?utf-8?B?aXlvaHArS0swN1ZBSzl1R1hQejFaZWJtZ2NrL3JvSnBPbi8zeEE4K3BqMzcr?=
 =?utf-8?B?RCtqOWM0TjQxSGhETUh3dzMyZkd6Rm4zc0hYbzVhVGtuRHI4STdQZS9MbDg2?=
 =?utf-8?B?REt3U3NLbi9EV2VHWTNEVDhzN1cyT1pFeUQ0MFk3ZkhaRUZjUUk2aFo2MTEz?=
 =?utf-8?B?aTZweUJDVytYNlZWWUJhZmgyaGdSZ21EY0lwQlhoOE5jTXl2L1cyUEc0S2Jx?=
 =?utf-8?B?MFFxYjcrblB5aU53SUJpK245bE9jNS8rMEZEUnhHZXJndEZIRWFJeGkzalVn?=
 =?utf-8?B?TDRIdU9FZ0FDclRLWEdTYmgwTVd3b00zK0c4WmxDQnFuVHBydlEzL0ZNV1hv?=
 =?utf-8?B?cndHdWlESjFSR0cxTCtxUGFGYkZ3S0hGUkhMWDVKOHFwKzVncERXTUtLZmNK?=
 =?utf-8?B?Q0IwRFhMbkdTNDVwRmo3Tzl5dkMwRStQMGttN2lMUUtwcHBFZ25ZYUpNNFIr?=
 =?utf-8?B?WitpTGhJdEt0NlV0cmtOb21EckxRNG1Ub3R6aUVTVSs3UER6ekUxT1F2b0Mz?=
 =?utf-8?B?Z2sxMDE5VFpiLzFTay90cVo5dm9IcGRXM0dzZmpRcEJxSUMyc0tUQ0dRNEto?=
 =?utf-8?B?NGpYVFNEUk1MTWt2UzduSVphM0dzVHVYRmVKMVBpWWRsT2gvS0syOGNDRUkz?=
 =?utf-8?B?VWpLcmZUR3RsQXFzVUZkZ3B6QlZGekc2T1JaUVVPbXF5UDgySWhJZnFIU05x?=
 =?utf-8?B?bjhzc290S0h4U3BDV1ZLWk9ZdjREZXVUTitjQUphZUZGRjFaNjcrYTB6d0ZP?=
 =?utf-8?B?TC9PTUpLenMwSldNZy9lSHlnTzk1SEFUdU1qTkF3b0NrdllEMHJxTy9kY2ly?=
 =?utf-8?B?UmZLTFA5SFdTZmxRS0c3Tm1DaUNPZVdMTXhORzVBS2N0eUZhYjhCUVJZb045?=
 =?utf-8?Q?RyAG35qjh3iuHL5l4XbcwVhyf9eMDQoxHh2rVlZD1Lpd?=
X-OriginatorOrg: suse.com
X-MS-Exchange-CrossTenant-Network-Message-Id: e30fecfe-db12-4f42-2789-08db8693bb7b
X-MS-Exchange-CrossTenant-AuthSource: PA4PR04MB7790.eurprd04.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 17 Jul 2023 07:02:07.0334
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: f7a17af6-1c5c-4a36-aa8b-f5be247aa4ba
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: lCZygl5rikC64YQxB9q3BTaFzAo6U5nMhxVZPsiM3p5k6CBJVElGZZ6+9R16cVlI+LjZe2KKHGz0mU2f2kWiSQ==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: AM7PR04MB6808
X-Spam-Status: No, score=-2.2 required=5.0 tests=BAYES_00,DKIM_SIGNED,
        DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,NICE_REPLY_A,
        RCVD_IN_DNSWL_BLOCKED,RCVD_IN_MSPIKE_H2,SPF_HELO_PASS,SPF_PASS,
        T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org


On 17.07.23 г. 9:35 ч., Huang, Kai wrote:
> 
>>> +/* Called from __tdx_hypercall() for unrecoverable failure */
>>> +static noinstr void __tdx_hypercall_failed(void)
>>> +{
>>> +	instrumentation_begin();
>>> +	panic("TDVMCALL failed. TDX module bug?");
>>> +}
>>
>> So what's the deal with this instrumentation here. The instruction is
>> noinstr, so you want to make just the panic call itself instrumentable?,
>> if so where's the instrumentation_end() cal;?No instrumentation_end()
>> call. Actually is this complexity really worth it for the failure case?
>>
>> AFAICS there is a single call site for __tdx_hypercall_failed so why
>> noot call panic() directly ?
> 
> W/o this patch, the __tdx_hypercall_failed() is called from the TDX_HYPERCALL
> assembly, which is in .noinstr.text, and 'instrumentation_begin()' was needed to
> avoid the build warning I suppose.
> 
> However now with this patch __tdx_hypercall_failed() is called from
> __tdx_hypercall() which is a C function w/o 'noinstr' annotation, thus I believe
> instrumentation_begin() and 'noinstr' annotation are not needed anymore.
> 
> I didn't notice this while moving this function around and my kernel build test
> didn't warn me about this.  I'll change in next version.
> 
> In fact, perhaps this patch perhaps is too big for review.  I will also try to
> split it to smaller ones.

Can't you simply call panic() directly? Less going around the code while 
someone is reading it?

<snip>