linux-kernel - Re: [PATCH Part1 RFC v2 05/20] x86/sev: Define SNP Page State Change VMGEXIT structure

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 18 May 2021 10:06:48 -0500
From:   Brijesh Singh <brijesh.singh@....com>
To:     Borislav Petkov <bp@...en8.de>
Cc:     brijesh.singh@....com, x86@...nel.org,
        linux-kernel@...r.kernel.org, kvm@...r.kernel.org,
        tglx@...utronix.de, jroedel@...e.de, thomas.lendacky@....com,
        pbonzini@...hat.com, mingo@...hat.com, dave.hansen@...el.com,
        rientjes@...gle.com, seanjc@...gle.com, peterz@...radead.org,
        hpa@...or.com, tony.luck@...el.com
Subject: Re: [PATCH Part1 RFC v2 05/20] x86/sev: Define SNP Page State Change
 VMGEXIT structure


On 5/18/21 5:41 AM, Borislav Petkov wrote:
> On Fri, Apr 30, 2021 at 07:16:01AM -0500, Brijesh Singh wrote:
>> An SNP-active guest will use the page state change NAE VMGEXIT defined in
>> the GHCB specification to ask the hypervisor to make the guest page
>> private or shared in the RMP table. In addition to the private/shared,
>> the guest can also ask the hypervisor to split or combine multiple 4K
>> validated pages as a single 2M page or vice versa.
>>
>> See GHCB specification section Page State Change for additional
>> information.
>>
>> Signed-off-by: Brijesh Singh <brijesh.singh@....com>
>> ---
>>  arch/x86/include/asm/sev-common.h | 46 +++++++++++++++++++++++++++++++
>>  arch/x86/include/uapi/asm/svm.h   |  2 ++
>>  2 files changed, 48 insertions(+)
>>
>> diff --git a/arch/x86/include/asm/sev-common.h b/arch/x86/include/asm/sev-common.h
>> index 8142e247d8da..07b8612bf182 100644
>> --- a/arch/x86/include/asm/sev-common.h
>> +++ b/arch/x86/include/asm/sev-common.h
>> @@ -67,6 +67,52 @@
>>  #define GHCB_HV_FEATURES_SNP_RESTRICTED_INJECTION_TIMER		\
>>  		(BIT_ULL(3) | GHCB_HV_FEATURES_SNP_RESTRICTED_INJECTION)
>>  
>> +/* SNP Page State Change */
>> +#define GHCB_MSR_PSC_REQ		0x014
>> +#define SNP_PAGE_STATE_PRIVATE		1
>> +#define SNP_PAGE_STATE_SHARED		2
>> +#define SNP_PAGE_STATE_PSMASH		3
>> +#define SNP_PAGE_STATE_UNSMASH		4
>> +#define GHCB_MSR_PSC_GFN_POS		12
>> +#define GHCB_MSR_PSC_GFN_MASK		0xffffffffffULL
> Why don't you use GENMASK_ULL() as I suggested? All those "f"s are
> harder to count than seeing the start and end of a mask.
>
>> +#define GHCB_MSR_PSC_OP_POS		52
> Also, having defines for 12 and 52 doesn't make it more readable,
> frankly.
>
> When I see GENMASK_ULL(51, 12) it is kinda clear what it is. With the
> defines, I have to go chase every define and replace it with the number
> mentally.

I was still adhering to the previous convention in the file to define
the values. I will submit prepatch to change them to also use GENMASK so
that its consistent with SNP updates I add in this series.


>> +#define GHCB_MSR_PSC_OP_MASK		0xf
>> +#define GHCB_MSR_PSC_REQ_GFN(gfn, op) 	\
>> +	(((unsigned long)((op) & GHCB_MSR_PSC_OP_MASK) << GHCB_MSR_PSC_OP_POS) | \
>> +	(((gfn) << GHCB_MSR_PSC_GFN_POS) & GHCB_MSR_PSC_GFN_MASK) | GHCB_MSR_PSC_REQ)
>> +
>> +#define GHCB_MSR_PSC_RESP		0x015
>> +#define GHCB_MSR_PSC_ERROR_POS		32
>> +#define GHCB_MSR_PSC_ERROR_MASK		0xffffffffULL
>> +#define GHCB_MSR_PSC_RSVD_POS		12
>> +#define GHCB_MSR_PSC_RSVD_MASK		0xfffffULL
>> +#define GHCB_MSR_PSC_RESP_VAL(val)	((val) >> GHCB_MSR_PSC_ERROR_POS)
>> +
>> +/* SNP Page State Change NAE event */
>> +#define VMGEXIT_PSC_MAX_ENTRY		253
>> +#define VMGEXIT_PSC_INVALID_HEADER	0x100000001
>> +#define VMGEXIT_PSC_INVALID_ENTRY	0x100000002
>> +#define VMGEXIT_PSC_FIRMWARE_ERROR(x)	((x & 0xffffffffULL) | 0x200000000)
>> +
>> +struct __packed snp_page_state_header {
>> +	u16 cur_entry;
>> +	u16 end_entry;
>> +	u32 reserved;
>> +};
>> +
>> +struct __packed snp_page_state_entry {
>> +	u64 cur_page:12;
>> +	u64 gfn:40;
>> +	u64 operation:4;
>> +	u64 pagesize:1;
>> +	u64 reserved:7;
> Please write it like this:
>
> 	u64 cur_page 	: 12,
> 	    gfn		: 40,
> 	    ...
>
> to show that all those fields are part of the same u64.
>
> struct perf_event_attr in include/uapi/linux/perf_event.h is a good
> example.
>
Ah, thanks for pointing. I will switch to using it.


>> +};
>> +
>> +struct __packed snp_page_state_change {
>> +	struct snp_page_state_header header;
>> +	struct snp_page_state_entry entry[VMGEXIT_PSC_MAX_ENTRY];
>> +};
>> +
>>  #define GHCB_MSR_TERM_REQ		0x100
>>  #define GHCB_MSR_TERM_REASON_SET_POS	12
>>  #define GHCB_MSR_TERM_REASON_SET_MASK	0xf
>> diff --git a/arch/x86/include/uapi/asm/svm.h b/arch/x86/include/uapi/asm/svm.h
>> index 7fbc311e2de1..f7bf12cad58c 100644
>> --- a/arch/x86/include/uapi/asm/svm.h
>> +++ b/arch/x86/include/uapi/asm/svm.h
>> @@ -108,6 +108,7 @@
>>  #define SVM_VMGEXIT_AP_JUMP_TABLE		0x80000005
>>  #define SVM_VMGEXIT_SET_AP_JUMP_TABLE		0
>>  #define SVM_VMGEXIT_GET_AP_JUMP_TABLE		1
>> +#define SVM_VMGEXIT_SNP_PAGE_STATE_CHANGE	0x80000010
> SVM_VMGEXIT_SNP_PSC

Noted.


>
>>  #define SVM_VMGEXIT_HYPERVISOR_FEATURES		0x8000fffd
>>  #define SVM_VMGEXIT_UNSUPPORTED_EVENT		0x8000ffff
>>  
>> @@ -216,6 +217,7 @@
>>  	{ SVM_VMGEXIT_NMI_COMPLETE,	"vmgexit_nmi_complete" }, \
>>  	{ SVM_VMGEXIT_AP_HLT_LOOP,	"vmgexit_ap_hlt_loop" }, \
>>  	{ SVM_VMGEXIT_AP_JUMP_TABLE,	"vmgexit_ap_jump_table" }, \
>> +	{ SVM_VMGEXIT_SNP_PAGE_STATE_CHANGE,	"vmgexit_page_state_change" }, \
> Ditto.

These strings are printed in the trace so I thought giving them a full
name makes sense


>
> Thx.
>