Message-ID: <9c9773d1-c494-2dfe-cd2a-95e3cfdfa09f@amd.com>
Date:   Fri, 26 Mar 2021 10:42:56 -0500
From:   Brijesh Singh <brijesh.singh@....com>
To:     Borislav Petkov <bp@...en8.de>
Cc:     brijesh.singh@....com, linux-kernel@...r.kernel.org,
        x86@...nel.org, kvm@...r.kernel.org, ak@...ux.intel.com,
        Thomas Gleixner <tglx@...utronix.de>,
        Ingo Molnar <mingo@...hat.com>, Joerg Roedel <jroedel@...e.de>,
        "H. Peter Anvin" <hpa@...or.com>, Tony Luck <tony.luck@...el.com>,
        Dave Hansen <dave.hansen@...el.com>,
        "Peter Zijlstra (Intel)" <peterz@...radead.org>,
        Paolo Bonzini <pbonzini@...hat.com>,
        Tom Lendacky <thomas.lendacky@....com>,
        David Rientjes <rientjes@...gle.com>,
        Sean Christopherson <seanjc@...gle.com>
Subject: Re: [RFC Part1 PATCH 03/13] x86: add a helper routine for the
 PVALIDATE instruction


On 3/26/21 9:30 AM, Borislav Petkov wrote:
> On Wed, Mar 24, 2021 at 11:44:14AM -0500, Brijesh Singh wrote:
>>  arch/x86/include/asm/sev-snp.h | 52 ++++++++++++++++++++++++++++++++++
> Hmm, a separate header.
>
> Yeah, I know we did sev-es.h but I think it all should be in a single
> sev.h which contains all AMD-specific memory encryption declarations.
> It's not like it is going to be huge or so, by the looks of how big
> sev-es.h is.
>
> Or is there a particular need to have a separate snp header?
>
> If not, please do a pre-patch which renames sev-es.h to sev.h and then
> add the SNP stuff to it.


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.


>
>>  1 file changed, 52 insertions(+)
>>  create mode 100644 arch/x86/include/asm/sev-snp.h
>>
>> diff --git a/arch/x86/include/asm/sev-snp.h b/arch/x86/include/asm/sev-snp.h
>> new file mode 100644
>> index 000000000000..5a6d1367cab7
>> --- /dev/null
>> +++ b/arch/x86/include/asm/sev-snp.h
>> @@ -0,0 +1,52 @@
>> +/* SPDX-License-Identifier: GPL-2.0 */
>> +/*
>> + * AMD SEV Secure Nested Paging Support
>> + *
>> + * Copyright (C) 2021 Advanced Micro Devices, Inc.
>> + *
>> + * Author: Brijesh Singh <brijesh.singh@....com>
>> + */
>> +
>> +#ifndef __ASM_SECURE_NESTED_PAGING_H
>> +#define __ASM_SECURE_NESTED_PAGING_H
>> +
>> +#ifndef __ASSEMBLY__
>> +#include <asm/irqflags.h> /* native_save_fl() */
> Where is that used? Looks like leftovers.


Initially I was thinking to use the native_save_fl() to read the rFlags
but then realized that what if rFlags get changed between the call to
pvalidate instruction and native_save_fl(). I will remove this header
inclusion. Thank you for pointing.

>
>> +
>> +/* Return code of __pvalidate */
>> +#define PVALIDATE_SUCCESS		0
>> +#define PVALIDATE_FAIL_INPUT		1
>> +#define PVALIDATE_FAIL_SIZEMISMATCH	6
>> +
>> +/* RMP page size */
>> +#define RMP_PG_SIZE_2M			1
>> +#define RMP_PG_SIZE_4K			0
>> +
>> +#ifdef CONFIG_AMD_MEM_ENCRYPT
>> +static inline int __pvalidate(unsigned long vaddr, int rmp_psize, int validate,
> Why the "__" prefix?

I was trying to adhere to existing functions which uses a direct
instruction opcode. Most of those function have "__" prefix (e.g
__mwait, __tpause, ..).

Should I drop the __prefix ?

 

>
>> +			      unsigned long *rflags)
>> +{
>> +	unsigned long flags;
>> +	int rc;
>> +
>> +	asm volatile(".byte 0xF2, 0x0F, 0x01, 0xFF\n\t"
>> +		     "pushf; pop %0\n\t"
> Ewww, PUSHF is expensive.
>
>> +		     : "=rm"(flags), "=a"(rc)
>> +		     : "a"(vaddr), "c"(rmp_psize), "d"(validate)
>> +		     : "memory", "cc");
>> +
>> +	*rflags = flags;
>> +	return rc;
> Hmm, rc *and* rflags. Manual says "Upon completion, a return code is
> stored in EAX. rFLAGS bits OF, ZF, AF, PF and SF are set based on this
> return code."
>
> So what exactly does that mean and is the return code duplicated in
> rFLAGS?


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.

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.

> If so, can you return a single value which has everything you need to
> know?
>
> I see that you're using the retval only for the carry flag to check
> whether the page has already been validated so I think you could define
> a set of return value defines from that function which callers can
> check.


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 */

* Remove the rFlags parameters from the __pvalidate()

* Update the __pvalidate to check the rFlags.CF and if set then return
the new user-defined error code.


> And looking above again, you do have PVALIDATE_* defines except that
> nothing's using them. Use them please.

Actually the later patches does make use of the error codes (e.g
SIZEMISMATCH). The caller should check the error code value and can take
an action to resolve them. e.g a sizemismatch is seen then it can use
the lower page level for the validation etc.


>
> Also, for how to do condition code checks properly, see how the
> CC_SET/CC_OUT macros are used.


I will look into it. thanks.


>> +}
>> +
>> +#else	/* !CONFIG_AMD_MEM_ENCRYPT */
> This else-ifdeffery can go too if you move the ifdeffery inside the
> function:
>
> static inline int __pvalidate(unsigned long vaddr, int rmp_psize, int validate,
> {
> 	int rc = 0;
>
> #fidef CONFIG_AMD_MEM_ENCRYPT
>
> 	...
>
> #endif
>
> 	return rc;
> }


Noted. thanks


>
> Thx.
>

Powered by blists - more mailing lists