Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Fri, 26 Mar 2021 20:22:14 +0100
From:   Borislav Petkov <bp@alien8.de>
To:     Brijesh Singh <brijesh.singh@amd.com>
Cc:     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
Message-ID: <20210326192214.GK25229@zn.tnic>
References: <20210324164424.28124-1-brijesh.singh@amd.com>
 <20210324164424.28124-4-brijesh.singh@amd.com>
 <20210326143026.GB27507@zn.tnic>
 <9c9773d1-c494-2dfe-cd2a-95e3cfdfa09f@amd.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <9c9773d1-c494-2dfe-cd2a-95e3cfdfa09f@amd.com>
Precedence: bulk

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.

Thx.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette