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 Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by smtp.lore.kernel.org (Postfix) with ESMTP id 62EAAEB64DA for ; Wed, 28 Jun 2023 12:58:34 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id DF69E8D0003; Wed, 28 Jun 2023 08:58:33 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id DA6DA8D0001; Wed, 28 Jun 2023 08:58:33 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id C6F388D0003; Wed, 28 Jun 2023 08:58:33 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0013.hostedemail.com [216.40.44.13]) by kanga.kvack.org (Postfix) with ESMTP id B58798D0001 for ; Wed, 28 Jun 2023 08:58:33 -0400 (EDT) Received: from smtpin23.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay09.hostedemail.com (Postfix) with ESMTP id 6DFF28094B for ; Wed, 28 Jun 2023 12:58:33 +0000 (UTC) X-FDA: 80952160506.23.E9C2849 Received: from desiato.infradead.org (desiato.infradead.org [90.155.92.199]) by imf23.hostedemail.com (Postfix) with ESMTP id 96145140028 for ; Wed, 28 Jun 2023 12:58:29 +0000 (UTC) Authentication-Results: imf23.hostedemail.com; dkim=pass header.d=infradead.org header.s=desiato.20200630 header.b="XVBu/kW3"; dmarc=none; spf=none (imf23.hostedemail.com: domain of peterz@infradead.org has no SPF policy when checking 90.155.92.199) smtp.mailfrom=peterz@infradead.org ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1687957111; h=from:from:sender:reply-to:subject:subject:date:date: message-id:message-id:to:to:cc:cc:mime-version:mime-version: content-type:content-type:content-transfer-encoding: in-reply-to:in-reply-to:references:references:dkim-signature; bh=zblRw/4j1zlPqX3Dlun6zv4SMoiOHGXrVw0l++uKd7o=; b=PfN/L1W8oxPbSO08DlHgDqQXoxlWdZRD9CO8RQ863azEPbWpu4+vNK58sOfzRbPA6ut6tl 7pqpg7/Rx9CXhEFxOZ8cIs2G1MynKDWZSCbCPR6tIjI7XiGi8ZTFp+yiJj95Ap5du8GhYx F5CgOT9Q76yThd2HvZ+pUUM3cKUb5VA= ARC-Authentication-Results: i=1; imf23.hostedemail.com; dkim=pass header.d=infradead.org header.s=desiato.20200630 header.b="XVBu/kW3"; dmarc=none; spf=none (imf23.hostedemail.com: domain of peterz@infradead.org has no SPF policy when checking 90.155.92.199) smtp.mailfrom=peterz@infradead.org ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1687957111; a=rsa-sha256; cv=none; b=Auxs/zmE5pbYi+C2a05j5JoAQSGirewYOqlKBeiSA7AaC1mW0LsQxiSQCOZyKmmeAZy5QV VMLABoNQZ03XP6IE0xDniejZsxzuWwejH20eXY2CBo7LmFovM/5U6OLRl042BJublp4AtN rJO1gP7cM7pZfb5JRxZTNxg8zurYXGU= DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=infradead.org; s=desiato.20200630; h=In-Reply-To:Content-Type:MIME-Version: References:Message-ID:Subject:Cc:To:From:Date:Sender:Reply-To: Content-Transfer-Encoding:Content-ID:Content-Description; bh=zblRw/4j1zlPqX3Dlun6zv4SMoiOHGXrVw0l++uKd7o=; b=XVBu/kW3Q71u5MMQF5W/TdFu++ oTydPtSEv1z9Oz8kaUTyYOvEdD+a9Rva9LBMBGf9XfQB6ziGwhH1FTXSdprTYWXW1kMWhJiGV3jSI l1nmyoh26nis6AOtKyG4xgefOar78evVQb7hQDKLH9ydeOwVXVwh6EkDZqQ94QCcoxGd5aIcP7dGJ 3h1dmIWjqnaEGvJQcBzuR2oKJSphBjwEAwmObHTiBMfSn851xJRTfyu/GzzdbHxEKA+jZSB4wWAmY ZuqKPwW8KKpaSxoldJ398TQqcxlGqODv/jocuFxsUoacmaG8frBdZ17RY980wzImZk+K38gC31DWJ kJWp8Sjw==; Received: from j130084.upc-j.chello.nl ([24.132.130.84] helo=noisy.programming.kicks-ass.net) by desiato.infradead.org with esmtpsa (Exim 4.96 #2 (Red Hat Linux)) id 1qEUkR-005eCg-32; Wed, 28 Jun 2023 12:58:16 +0000 Received: from hirez.programming.kicks-ass.net (hirez.programming.kicks-ass.net [192.168.1.225]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256) (Client did not present a certificate) by noisy.programming.kicks-ass.net (Postfix) with ESMTPS id 38A1030005E; Wed, 28 Jun 2023 14:58:13 +0200 (CEST) Received: by hirez.programming.kicks-ass.net (Postfix, from userid 1000) id 225F827F62B9F; Wed, 28 Jun 2023 14:58:13 +0200 (CEST) Date: Wed, 28 Jun 2023 14:58:13 +0200 From: Peter Zijlstra To: Kai Huang Cc: linux-kernel@vger.kernel.org, kvm@vger.kernel.org, linux-mm@kvack.org, x86@kernel.org, dave.hansen@intel.com, kirill.shutemov@linux.intel.com, tony.luck@intel.com, tglx@linutronix.de, bp@alien8.de, mingo@redhat.com, hpa@zytor.com, seanjc@google.com, pbonzini@redhat.com, david@redhat.com, dan.j.williams@intel.com, rafael.j.wysocki@intel.com, ashok.raj@intel.com, reinette.chatre@intel.com, len.brown@intel.com, ak@linux.intel.com, isaku.yamahata@intel.com, ying.huang@intel.com, chao.gao@intel.com, sathyanarayanan.kuppuswamy@linux.intel.com, nik.borisov@suse.com, bagasdotme@gmail.com, sagis@google.com, imammedo@redhat.com Subject: Re: [PATCH v12 05/22] x86/virt/tdx: Add SEAMCALL infrastructure Message-ID: <20230628125813.GA2438817@hirez.programming.kicks-ass.net> References: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: X-Rspamd-Server: rspam09 X-Rspamd-Queue-Id: 96145140028 X-Stat-Signature: wstgmanhzycsrkr649m9ij3knhcpgsj5 X-Rspam-User: X-HE-Tag: 1687957109-164135 X-HE-Meta: U2FsdGVkX1/Fv4jgY/L4H8xU9J6fqc2YEEDQHfiYLwvvQXq7V+aiChc+gJnMKN0FOPewDXLGNQ6zSeFILwMEz7uGFqHuHb+Dv/b8gA4JXqTYPRZ6uU9aaBBwWHIRN08D/ns+m4307FUiC2FnAzRjIlglbb9Iej3hdL8TihDEWK+r+um4bNB2zJPVUwYdq3cw0+fzXLfGr7tVSidtKIh09mFxMlkDOJzglJh9v6Tbrm4NgX3FeirYYL6fSxNnJxwRBbU7G4lDK5Ls7VU623YAmnRKSC/HZd9Hi19UEdIQlYiGDHES3naZs8DsY0sfoOA7I4oAl+dpOjL/ztieqjmSGiUM2XXKeEeLHhYhIkcK3krDU87MFo1WPYCwHbn2W/HkkrbyJ91AKC+M8MCvYGnydTGXG6o2xyZBm1GswfyoqBBMABlPlYliIl3t0/CjxBv2pM+YY6+XwPa9zn2CpX/8mT/p+sNVFgKd88lUPNTywDRTDPQwrDWG/mHZyDD794enBdKupCsU/RICYFhqgUFXwyQeMx6VZhxPPldYNVy0jDSGLG/r1ppIsLgJOtnNJO0N5JY3Fsd1Ow65Y0GOSlH1RqnIDOPhc79WDXOEY5Dx9Clg3aNz9Ua5hcdQtFVTOtGDbaXiEA1IyLIfLtU5jy6O+JvU9cajOJUcsI7YDc0O7wEBSpNl0mZlfr8rY+XqCXZrGSHE/ZXmk9o1q97BQBP2py7G5eUg1i53fWvhfMF241NT9Jg6Y2L7p+aSor+fv6mA4DrpbLORN9yCoe0ahsXDwQIyQcxZOXgGi6yz4whBNfr+qEqenAmF6tIqygX5av/314XbHGZvHeusIAq/8cgHLZvVZiDW7tn027ydADg3s3hpftSQ3Gx7jtpl1ogJ30efNFP7jkcFJNv27L+jfE0FrdED/eh2dLasbM0vPb0quV2JyCskpCoFr8cqFjng4wUxDO6Gqt2UiOu2Eypqb73 nGzER3IZ +bt8qEctOhzTkWz/4jIhTRvfbndCBDSk5LDGQR4CRDnYpbrGmikgeC8L7Vo+ebWUO8ITPDJzMQLWPHa6HPG6wBF8mHFALG01NcrVMIjhNdwNdQ6nXYW9yVSSbujVBeTnTPjZG/hK1jtPXrLM= X-Bogosity: Ham, tests=bogofilter, spamicity=0.000028, version=1.2.4 Sender: owner-linux-mm@kvack.org Precedence: bulk X-Loop: owner-majordomo@kvack.org List-ID: On Tue, Jun 27, 2023 at 02:12:35AM +1200, Kai Huang wrote: > +static int __always_unused seamcall(u64 fn, u64 rcx, u64 rdx, u64 r8, u64 r9, __always_inline perhaps? __always_unused seems wrong, worse it's still there at the end of the series: $ quilt diff --combine - | grep seamcall ... +static int __always_unused seamcall(u64 fn, u64 rcx, u64 rdx, u64 r8, u64 r9, ... + ret = seamcall(TDH_SYS_INIT, 0, 0, 0, 0, NULL, NULL); + ret = seamcall(TDH_SYS_LP_INIT, 0, 0, 0, 0, NULL, NULL); + ret = seamcall(TDH_SYS_INFO, sysinfo_pa, TDSYSINFO_STRUCT_SIZE, + ret = seamcall(TDH_SYS_CONFIG, __pa(tdmr_pa_array), + return seamcall(TDH_SYS_KEY_CONFIG, 0, 0, 0, 0, NULL, NULL); + ret = seamcall(TDH_SYS_TDMR_INIT, tdmr->base, 0, 0, 0, NULL, ... Definitely not unused. > + u64 *seamcall_ret, > + struct tdx_module_output *out) This interface is atrocious :/ Why have these two ret values? Why can't that live in a single space -- /me looks throught the callers, and finds seamcall_ret is unused :-( Worse, the input (c,d,8,9) is a strict subset of the output (c,d,8,9,10,11) so why isn't that a single thing used for both input and output. struct tdx_call { u64 rcx, rdx, r8, r9, r10, r11; }; static int __always_inline seamcall(u64 fn, struct tdx_call *regs) { } struct tdx_regs regs = { }; ret = seamcall(THD_SYS_INIT, ®s); struct tdx_regs regs = { .rcx = sysinfo_pa, .rdx = TDXSYSINFO_STRUCT_SIZE, .r8 = cmr_array_pa, .r9 = MAX_CMRS, }; ret = seamcall(THD_SYS_INFO, ®s); if (ret) return ret; print_cmrs(cmr_array, regs.r9); /me looks more at this stuff and ... WTF!?!? Can someone explain to me why __tdx_hypercall() is sane (per the above) but then we grew __tdx_module_call() as an absolute abomination and are apparently using that for seam too? > +{ > + u64 sret; > + int cpu; > + > + /* Need a stable CPU id for printing error message */ > + cpu = get_cpu(); And that's important because? Does having preemption off across the seamcall make sense? Does it still make sense when you add a loop later? > + sret = __seamcall(fn, rcx, rdx, r8, r9, out); > + put_cpu(); > + > + /* Save SEAMCALL return code if the caller wants it */ > + if (seamcall_ret) > + *seamcall_ret = sret; > + > + switch (sret) { > + case 0: > + /* SEAMCALL was successful */ > + return 0; > + case TDX_SEAMCALL_VMFAILINVALID: > + pr_err_once("module is not loaded.\n"); > + return -ENODEV; > + default: > + pr_err_once("SEAMCALL failed: CPU %d: leaf %llu, error 0x%llx.\n", > + cpu, fn, sret); > + if (out) > + pr_err_once("additional output: rcx 0x%llx, rdx 0x%llx, r8 0x%llx, r9 0x%llx, r10 0x%llx, r11 0x%llx.\n", > + out->rcx, out->rdx, out->r8, > + out->r9, out->r10, out->r11); At the very least this lacks { }, but it is quite horrendous coding style. Why switch() at all, would not: if (!rset) return 0; if (sret == TDX_SEAMCALL_VMFAILINVALID) { pr_nonsense(); return -ENODEV; } if (sret == TDX_SEAMCALL_GP) { pr_nonsense(); return -ENODEV; } if (sret == TDX_SEAMCALL_UD) { pr_nonsense(); return -EINVAL; } pr_nonsense(); return -EIO; be much clearer and have less horrific indenting issues? > + return -EIO; > + } > +}