Re: [RFC Part1 PATCH 03/13] x86: add a helper routine for the PVALIDATE instruction

From: Brijesh Singh <brijesh.singh@amd.com>
To: Borislav Petkov <bp@alien8.de>
Cc: brijesh.singh@amd.com, linux-kernel@vger.kernel.org,
	x86@kernel.org, kvm@vger.kernel.org, ak@linux.intel.com,
	Thomas Gleixner <tglx@linutronix.de>,
	Ingo Molnar <mingo@redhat.com>, Joerg Roedel <jroedel@suse.de>,
	"H. Peter Anvin" <hpa@zytor.com>, Tony Luck <tony.luck@intel.com>,
	Dave Hansen <dave.hansen@intel.com>,
	"Peter Zijlstra (Intel)" <peterz@infradead.org>,
	Paolo Bonzini <pbonzini@redhat.com>,
	Tom Lendacky <thomas.lendacky@amd.com>,
	David Rientjes <rientjes@google.com>,
	Sean Christopherson <seanjc@google.com>
Subject: Re: [RFC Part1 PATCH 03/13] x86: add a helper routine for the PVALIDATE instruction
Date: Fri, 26 Mar 2021 15:01:27 -0500	[thread overview]
Message-ID: <17a1d149-5536-0fb7-e441-cdf9cf7fa78e@amd.com> (raw)
In-Reply-To: <20210326192214.GK25229@zn.tnic>

On 3/26/21 2:22 PM, Borislav Petkov wrote:
> On Fri, Mar 26, 2021 at 10:42:56AM -0500, Brijesh Singh wrote:
>> There is no strong reason for a separate sev-snp.h. I will add a
>> pre-patch to rename sev-es.h to sev.h and add SNP stuff to it.
> Thx.
>
>> I was trying to adhere to existing functions which uses a direct
>> instruction opcode.
> That's not really always the case:
>
> arch/x86/include/asm/special_insns.h
>
> The "__" prefixed things should mean lower abstraction level helpers and
> we drop the ball on those sometimes.
>
>> It's not duplicate error code. The EAX returns an actual error code. The
>> rFlags contains additional information. We want both the codes available
>> to the caller so that it can make a proper decision.
>>
>> e.g.
>>
>> 1. A callers validate an address 0x1000. The instruction validated it
>> and return success.
> Your function returns PVALIDATE_SUCCESS.
>
>> 2. Caller asked to validate the same address again. The instruction will
>> return success but since the address was validated before hence
>> rFlags.CF will be set to indicate that PVALIDATE instruction did not
>> made any change in the RMP table.
> Your function returns PVALIDATE_VALIDATED_ALREADY or so.
>
>> You are correct that currently I am using only carry flag. So far we
>> don't need other flags. What do you think about something like this:
>>
>> * Add a new user defined error code
>>
>>  #define PVALIDATE_FAIL_NOUPDATE        255 /* The error is returned if
>> rFlags.CF set */
> Or that.
>
>> * Remove the rFlags parameters from the __pvalidate()
> Yes, it seems unnecessary at the moment. And I/O function arguments are
> always yuck.
>
>> * Update the __pvalidate to check the rFlags.CF and if set then return
>> the new user-defined error code.
> Yap, you can convert all that to pvalidate() return values, methinks,
> and then make that function simpler for callers because they should
> not have to deal with rFLAGS - only return values to denote what the
> function did.

Ack. I will made the required changes in next version.

>
> Thx.
>