From: "Kuppuswamy, Sathyanarayanan" <sathyanarayanan.kuppuswamy@linux.intel.com>
To: Dave Hansen <dave.hansen@intel.com>,
Peter Zijlstra <peterz@infradead.org>,
Andy Lutomirski <luto@kernel.org>
Cc: Andi Kleen <ak@linux.intel.com>,
Kirill Shutemov <kirill.shutemov@linux.intel.com>,
Kuppuswamy Sathyanarayanan <knsathya@kernel.org>,
Dan Williams <dan.j.williams@intel.com>,
Raj Ashok <ashok.raj@intel.com>,
Sean Christopherson <seanjc@google.com>,
linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2 1/1] x86/tdx: Add __tdcall() and __tdvmcall() helper functions
Date: Thu, 22 Apr 2021 18:09:38 -0700 [thread overview]
Message-ID: <9161efc0-fd25-d239-32b7-5d2c726579b0@linux.intel.com> (raw)
In-Reply-To: <d99941db-6ee6-267e-dece-6220af0ea305@intel.com>
On 4/20/21 4:42 PM, Dave Hansen wrote:
> On 4/20/21 4:12 PM, Kuppuswamy, Sathyanarayanan wrote:
>> On 4/20/21 12:59 PM, Dave Hansen wrote:
>>> On 4/20/21 12:20 PM, Kuppuswamy, Sathyanarayanan wrote:
>>>>>> approach is, it adds a few extra instructions for every
>>>>>> TDCALL use case when compared to distributed checks. Although
>>>>>> it's a bit less efficient, it's worth it to make the code more
>>>>>> readable.
>>>>>
>>>>> What's a "distributed check"?
>>>>
>>>> It should be "distributed TDVMCALL/TDCALL inline assembly calls"
>>>
>>> It's still not clear to what that refers.
>>
>> I am just comparing the performance cost of using generic
>> TDCALL()/TDVMCALL() function implementation with "usage specific"
>> (GetQuote,MapGPA, HLT,etc) custom TDCALL()/TDVMCALL() inline assembly
>> implementation.
>
> So, I actually had an idea what you were talking about, but I have
> *ZERO* idea what "distributed" means in this context.
>
> I think you are trying to say something along the lines of:
>
> Just like syscalls, not all TDVMCALL/TDCALLs use the same set
> of argument registers. The implementation here picks the
> current worst-case scenario for TDCALL (4 registers). For
> TDCALLs with fewer than 4 arguments, there will end up being
> a few superfluous (cheap) instructions. But, this approach
> maximizes code reuse.
>
Yes, you are correct. I will word it better in my next version.
>
>>>>> This also doesn't talk at all about why this approach was
>>>>> chosen versus inline assembly. You're going to be asked "why
>>>>> not use inline asm?"
>>>> "To make the core more readable and less error prone." I have
>>>> added this info in above paragraph. Do you think we need more
>>>> argument to justify our approach?
>>>
>>> Yes, you need much more justification. That's pretty generic and
>>> non-specific.
>> readability is one of the main motivation for not choosing inline
>
> I'm curious. Is there a reason you are not choosing to use
> capitalization in your replies? I personally use capitalization as a
> visual clue for where a reply starts.
>
> I'm not sure whether this indicates that your keyboard is not
> functioning properly, or that these replies are simply not important
> enough to warrant the use of the Shift key. Or, is it simply an
> oversight? Or, maybe I'm just being overly picky because I've been
> working on these exact things with my third-grader a bit too much lately.
>
> Either way, I personally would appreciate your attention to detail in
> crafting writing that is easy to parse, since I'm the one that's going
> to have to parse it. Details show that you care about the content you
> produce. Even if you don't mean it, a lack of attention to detail (even
> capital letters) can be perceived to mean that you do not care about
> what you write. If you don't care about it, why should the reader?
>
>> assembly. Since number of lines of instructions (with comments) are
>> over 70, using inline assembly made it hard to read. Another reason
>> is, since we
>> are using many registers (R8-R15, R[A-D]X)) in TDVMCAL/TDCALL
>> operation, we are not sure whether some older compiler can follow
>> our specified inline assembly constraints.
>
> As for the justification, that's much improved. Please include that,
> along with some careful review of the grammar.
It's an oversight from my end. I will keep it in mind in my future
replies.
>
>>>>>> + movl $TDVMCALL_EXPOSE_REGS_MASK, %ecx
>>>
>>> You've introduced two concepts here, without differentiating them. You
>>> need to work to differentiate these two kinds of failure somewhere. You
>>> can't simply refer to both as "failure".
>> will clarify it. I have assumed that once the user reads the spec, its
>> easier
>> to understand.
>
> Your code should be 100% self-supporting without the spec. The spec can
> be there in a supportive role to help resolve ambiguity or add fine
> detail. But, I think this is a major, repeated problem with this patch
> set: it relies too much on reviewers spending quality time with the spec.
>
I will review the patch set again and add necessary comments to fix this gap.
>>>>> Also, do you *REALLY* need to do this from assembly? Can't it be done
>>>>> in the C wrapper?
>>>> Its common for all use cases of TDVMCALL (vendor specific, in/out, etc).
>>>> so added
>>>> it here.
>>>
>>> That's not a good reason. You could just as easily have a C wrapper
>>> which all uses of TDVMCALL go through.
>> Any reason for not preferring it in assembly code?
>
> Assembly is a last resort. It should only be used for things that
> simply can't be written in C or are horrific to understand and manage
> when written in C. A single statement like:
>
> BUG_ON(something);
>
> does not qualify in my book as something that's horrific to write in C.
>
>> Also, using wrapper will add more complication for in/out instruction
>> substitution use case. please check the use case in following patch.
>> https://github.com/intel/tdx/commit/1b73f60aa5bb93554f3b15cd786a9b10b53c1543
>
> I'm seeing a repeated theme here. The approach in this patch series,
> and in this email thread in general appears to be one where the patch
> submitter is doing as little work as possible and trying to make the
> reviewer do as much work as possible.
>
> This is a 300-line diff with all kinds of stuff going on in it. I'm not
> sure to what you are referring. You haven't made it easy to figure out.
I have pointed that patch to give reference to how in/out instructions
are substituted with tdvmcalls(). Specific implementation is spread across
multiple lines/files in that patch. So I did not include specific line
numbers.
But let me try to explain it here. What I meant by complication is,
for in/out instruction, we use alternative_io() to substitute in/out
instructions with tdg_in()/tdg_out() assembly calls. So we have to ensure
that we don't corrupt registers or stack from the substituted instructions
If you check the implementation of tdg_in()/tdg_out(), you will notice
that we have added code to preserve the caller registers. So, if we use
C wrapper for this use case, there is a chance that it might mess
the caller registers or stack.
alternative_io("in" #bwl " %w2, %" #bw "0", \
"call tdg_in" #bwl, X86_FEATURE_TDX_GUEST, \
"=a"(value), "d"(port))
>
> It would make it a lot easier if you pointed to a specific line, or
> copied-and-pasted the code to which you refer. I would really encourage
> you to try to make your content easier for reviewers to digest:
> Capitalize the start of sentences. Make unambiguous references to code.
> Don't blindly cite the spec. Fully express your thoughts.
>
> You'll end up with happier reviewers instead of grumpy ones.
Got it. I will try to keep your suggestion in mind for future
communications.
>
> ...
>>>> More warnings at-least show that we are working
>>>> with malicious VMM.
>>>
>> In our case, we will get WARN() output only if guest triggers
>> TDCALL()/TDVMCALL()
>> right? So getting WARN() message for failure of guest triggered call is
>> justifiable right?
>
> The output of these calls and thus the error code comes from another
> piece of software, either the SEAM module or the VMM.
>
> The error can be from one of several reasons:
> 1. Guest error/bug where the guest provides a bad value. This is
> probably the most likely scenario. But, it's impossible to
> differentiate from the other cases because it's a guest bug.
> 2. SEAM error/bug. If the spec says "SEAM will not do this", then you
> can probably justify a WARN_ON_ONCE(). If the call is security-
> sensitve, like part of attestation, then you can't meaningfully
> recover from it and it probably deserves a BUG_ON().
> 3. VMM error/bug/malice. Again, you might be able to justify a
> WARN_ON_ONCE(). We do that for userspace that might be attacking
> the kernel. These are *NEVER* fatal and should be rate-limited.
>
> I don't see *ANYWHERE* in this list where an unbounded, unratelimited
> WARN() is appropriate. But, that's just my $0.02.
WARN_ON_ONCE() will not work for our use case. Since tdvmcall()/tdcall()
can be triggered for multiple use cases. So we can't print errors only
once.
I will go with pr_warn_ratelimited() for this use case.
>
--
Sathyanarayanan Kuppuswamy
Linux Kernel Developer
next prev parent reply other threads:[~2021-04-23 1:09 UTC|newest]
Thread overview: 161+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-02-06 3:02 Test Email sathyanarayanan.kuppuswamy
2021-02-05 23:38 ` [RFC v1 00/26] Add TDX Guest Support Kuppuswamy Sathyanarayanan
2021-02-05 23:38 ` [RFC v1 01/26] x86/paravirt: Introduce CONFIG_PARAVIRT_XL Kuppuswamy Sathyanarayanan
2021-02-05 23:38 ` [RFC v1 02/26] x86/cpufeatures: Add TDX Guest CPU feature Kuppuswamy Sathyanarayanan
2021-02-05 23:38 ` [RFC v1 03/26] x86/cpufeatures: Add is_tdx_guest() interface Kuppuswamy Sathyanarayanan
2021-04-01 21:08 ` Dave Hansen
2021-04-01 21:15 ` Kuppuswamy, Sathyanarayanan
2021-04-01 21:19 ` Dave Hansen
2021-04-01 22:25 ` Kuppuswamy, Sathyanarayanan
2021-02-05 23:38 ` [RFC v1 04/26] x86/tdx: Get TD execution environment information via TDINFO Kuppuswamy Sathyanarayanan
2021-02-08 10:00 ` Peter Zijlstra
2021-02-08 19:10 ` Kuppuswamy, Sathyanarayanan
2021-02-05 23:38 ` [RFC v1 05/26] x86/traps: Add #VE support for TDX guest Kuppuswamy Sathyanarayanan
2021-02-08 10:20 ` Peter Zijlstra
2021-02-08 16:23 ` Andi Kleen
2021-02-08 16:33 ` Peter Zijlstra
2021-02-08 16:46 ` Sean Christopherson
2021-02-08 16:59 ` Peter Zijlstra
2021-02-08 19:05 ` Kuppuswamy, Sathyanarayanan
2021-02-08 16:46 ` Andi Kleen
2021-02-12 19:20 ` Dave Hansen
2021-02-12 19:47 ` Andy Lutomirski
2021-02-12 20:06 ` Sean Christopherson
2021-02-12 20:17 ` Dave Hansen
2021-02-12 20:37 ` Sean Christopherson
2021-02-12 20:46 ` Dave Hansen
2021-02-12 20:54 ` Sean Christopherson
2021-02-12 21:06 ` Dave Hansen
2021-02-12 21:37 ` Sean Christopherson
2021-02-12 21:47 ` Andy Lutomirski
2021-02-12 21:48 ` Dave Hansen
2021-02-14 19:33 ` Andi Kleen
2021-02-14 19:54 ` Andy Lutomirski
2021-02-12 20:20 ` Andy Lutomirski
2021-02-12 20:44 ` Sean Christopherson
2021-02-05 23:38 ` [RFC v1 06/26] x86/tdx: Add HLT " Kuppuswamy Sathyanarayanan
2021-02-05 23:38 ` [RFC v1 07/26] x86/tdx: Wire up KVM hypercalls Kuppuswamy Sathyanarayanan
2021-02-05 23:38 ` [RFC v1 08/26] x86/tdx: Add MSR support for TDX guest Kuppuswamy Sathyanarayanan
2021-02-05 23:38 ` [RFC v1 09/26] x86/tdx: Handle CPUID via #VE Kuppuswamy Sathyanarayanan
2021-02-05 23:42 ` Andy Lutomirski
2021-02-07 14:13 ` Kirill A. Shutemov
2021-02-07 16:01 ` Dave Hansen
2021-02-07 20:29 ` Kirill A. Shutemov
2021-02-07 22:31 ` Dave Hansen
2021-02-07 22:45 ` Andy Lutomirski
2021-02-08 17:10 ` Sean Christopherson
2021-02-08 17:35 ` Andy Lutomirski
2021-02-08 17:47 ` Sean Christopherson
2021-03-18 21:30 ` [PATCH v1 1/1] x86/tdx: Add tdcall() and tdvmcall() helper functions Kuppuswamy Sathyanarayanan
2021-03-19 16:55 ` Sean Christopherson
2021-03-19 17:42 ` Kuppuswamy, Sathyanarayanan
2021-03-19 18:22 ` Dave Hansen
2021-03-19 19:58 ` Kuppuswamy, Sathyanarayanan
2021-03-26 23:38 ` [PATCH v2 1/1] x86/tdx: Add __tdcall() and __tdvmcall() " Kuppuswamy Sathyanarayanan
2021-04-20 17:36 ` Dave Hansen
2021-04-20 19:20 ` Kuppuswamy, Sathyanarayanan
2021-04-20 19:59 ` Dave Hansen
2021-04-20 23:12 ` Kuppuswamy, Sathyanarayanan
2021-04-20 23:42 ` Dave Hansen
2021-04-23 1:09 ` Kuppuswamy, Sathyanarayanan [this message]
2021-04-23 1:21 ` Dave Hansen
2021-04-23 1:35 ` Andi Kleen
2021-04-23 15:15 ` Sean Christopherson
2021-04-23 15:28 ` Dan Williams
2021-04-23 15:38 ` Andi Kleen
2021-04-23 15:50 ` Sean Christopherson
2021-04-23 15:47 ` Andi Kleen
2021-04-23 18:18 ` Kuppuswamy, Sathyanarayanan
2021-04-20 23:53 ` Dan Williams
2021-04-20 23:59 ` Kuppuswamy, Sathyanarayanan
2021-02-05 23:38 ` [RFC v1 10/26] x86/io: Allow to override inX() and outX() implementation Kuppuswamy Sathyanarayanan
2021-02-05 23:38 ` [RFC v1 11/26] x86/tdx: Handle port I/O Kuppuswamy Sathyanarayanan
2021-02-05 23:38 ` [RFC v1 12/26] x86/tdx: Handle in-kernel MMIO Kuppuswamy Sathyanarayanan
2021-04-01 19:56 ` Dave Hansen
2021-04-01 22:26 ` Sean Christopherson
2021-04-01 22:53 ` Dave Hansen
2021-02-05 23:38 ` [RFC v1 13/26] x86/tdx: Handle MWAIT, MONITOR and WBINVD Kuppuswamy Sathyanarayanan
2021-02-05 23:43 ` Andy Lutomirski
2021-02-05 23:54 ` Kuppuswamy, Sathyanarayanan
2021-02-06 1:05 ` Andy Lutomirski
2021-03-27 0:18 ` [PATCH v1 1/1] " Kuppuswamy Sathyanarayanan
2021-03-27 2:40 ` Andy Lutomirski
2021-03-27 3:40 ` Kuppuswamy, Sathyanarayanan
2021-03-27 16:03 ` Andy Lutomirski
2021-03-27 22:54 ` [PATCH v2 " Kuppuswamy Sathyanarayanan
2021-03-29 17:14 ` Dave Hansen
2021-03-29 21:55 ` Kuppuswamy, Sathyanarayanan
2021-03-29 22:02 ` Dave Hansen
2021-03-29 22:09 ` Kuppuswamy, Sathyanarayanan
2021-03-29 22:12 ` Dave Hansen
2021-03-29 22:42 ` Kuppuswamy, Sathyanarayanan
2021-03-29 23:16 ` [PATCH v3 " Kuppuswamy Sathyanarayanan
2021-03-29 23:23 ` Andy Lutomirski
2021-03-29 23:37 ` Kuppuswamy, Sathyanarayanan
2021-03-29 23:42 ` Sean Christopherson
2021-03-29 23:58 ` Andy Lutomirski
2021-03-30 2:04 ` Andi Kleen
2021-03-30 2:58 ` Andy Lutomirski
2021-03-30 15:14 ` Sean Christopherson
2021-03-30 16:37 ` Andy Lutomirski
2021-03-30 16:57 ` Sean Christopherson
2021-04-07 15:24 ` Andi Kleen
2021-03-31 21:09 ` [PATCH v4 " Kuppuswamy Sathyanarayanan
2021-03-31 21:49 ` Dave Hansen
2021-03-31 22:29 ` Kuppuswamy, Sathyanarayanan
2021-03-31 21:53 ` Sean Christopherson
2021-03-31 22:00 ` Dave Hansen
2021-03-31 22:06 ` Sean Christopherson
2021-03-31 22:11 ` Dave Hansen
2021-03-31 22:28 ` Kuppuswamy, Sathyanarayanan
2021-03-31 22:32 ` Sean Christopherson
2021-03-31 22:34 ` Dave Hansen
2021-04-01 3:28 ` Andi Kleen
2021-04-01 3:46 ` Dave Hansen
2021-04-01 4:24 ` Andi Kleen
2021-04-01 4:51 ` [PATCH v5 " Kuppuswamy Sathyanarayanan
2021-03-29 23:39 ` [PATCH v3 " Sean Christopherson
2021-03-29 23:38 ` Dave Hansen
2021-03-30 4:56 ` [PATCH v1 " Xiaoyao Li
2021-03-30 15:00 ` Andi Kleen
2021-03-30 15:10 ` Dave Hansen
2021-03-30 17:02 ` Kuppuswamy, Sathyanarayanan
2021-02-05 23:38 ` [RFC v1 14/26] ACPI: tables: Add multiprocessor wake-up support Kuppuswamy Sathyanarayanan
2021-02-05 23:38 ` [RFC v1 15/26] x86/boot: Add a trampoline for APs booting in 64-bit mode Kuppuswamy Sathyanarayanan
2021-02-05 23:38 ` [RFC v1 16/26] x86/boot: Avoid #VE during compressed boot for TDX platforms Kuppuswamy Sathyanarayanan
2021-02-05 23:38 ` [RFC v1 17/26] x86/boot: Avoid unnecessary #VE during boot process Kuppuswamy Sathyanarayanan
2021-02-05 23:38 ` [RFC v1 18/26] x86/topology: Disable CPU hotplug support for TDX platforms Kuppuswamy Sathyanarayanan
2021-02-05 23:38 ` [RFC v1 19/26] x86/tdx: Forcefully disable legacy PIC for TDX guests Kuppuswamy Sathyanarayanan
2021-02-05 23:38 ` [RFC v1 20/26] x86/tdx: Introduce INTEL_TDX_GUEST config option Kuppuswamy Sathyanarayanan
2021-02-05 23:38 ` [RFC v1 21/26] x86/mm: Move force_dma_unencrypted() to common code Kuppuswamy Sathyanarayanan
2021-04-01 20:06 ` Dave Hansen
2021-04-06 15:37 ` Kirill A. Shutemov
2021-04-06 16:11 ` Dave Hansen
2021-04-06 16:37 ` Kirill A. Shutemov
2021-02-05 23:38 ` [RFC v1 22/26] x86/tdx: Exclude Shared bit from __PHYSICAL_MASK Kuppuswamy Sathyanarayanan
2021-04-01 20:13 ` Dave Hansen
2021-04-06 15:54 ` Kirill A. Shutemov
2021-04-06 16:12 ` Dave Hansen
2021-02-05 23:38 ` [RFC v1 23/26] x86/tdx: Make pages shared in ioremap() Kuppuswamy Sathyanarayanan
2021-04-01 20:26 ` Dave Hansen
2021-04-06 16:00 ` Kirill A. Shutemov
2021-04-06 16:14 ` Dave Hansen
2021-02-05 23:38 ` [RFC v1 24/26] x86/tdx: Add helper to do MapGPA TDVMALL Kuppuswamy Sathyanarayanan
2021-02-05 23:38 ` [RFC v1 25/26] x86/tdx: Make DMA pages shared Kuppuswamy Sathyanarayanan
2021-04-01 21:01 ` Dave Hansen
2021-04-06 16:31 ` Kirill A. Shutemov
2021-04-06 16:38 ` Dave Hansen
2021-04-06 17:16 ` Sean Christopherson
2021-02-05 23:38 ` [RFC v1 26/26] x86/kvm: Use bounce buffers for TD guest Kuppuswamy Sathyanarayanan
2021-04-01 21:17 ` Dave Hansen
2021-02-06 3:04 ` Test Email sathyanarayanan.kuppuswamy
2021-02-06 6:24 ` [RFC v1 00/26] Add TDX Guest Support sathyanarayanan.kuppuswamy
2021-03-31 21:38 ` Kuppuswamy, Sathyanarayanan
2021-04-02 0:02 ` Dave Hansen
2021-04-02 2:48 ` Andi Kleen
2021-04-02 15:27 ` Dave Hansen
2021-04-02 21:32 ` Andi Kleen
2021-04-03 16:26 ` Dave Hansen
2021-04-03 17:28 ` Andi Kleen
2021-04-04 15:02 ` Dave Hansen
2021-04-12 17:24 ` Dan Williams
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=9161efc0-fd25-d239-32b7-5d2c726579b0@linux.intel.com \
--to=sathyanarayanan.kuppuswamy@linux.intel.com \
--cc=ak@linux.intel.com \
--cc=ashok.raj@intel.com \
--cc=dan.j.williams@intel.com \
--cc=dave.hansen@intel.com \
--cc=kirill.shutemov@linux.intel.com \
--cc=knsathya@kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=luto@kernel.org \
--cc=peterz@infradead.org \
--cc=seanjc@google.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).