| Message ID | 20210324164424.28124-5-brijesh.singh@amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add AMD Secure Nested Paging (SEV-SNP) Guest Support | expand |
On Wed, Mar 24, 2021 at 11:44:15AM -0500, Brijesh Singh wrote: > An SNP-active guest will use the page state change VNAE MGEXIT defined in I guess this was supposed to mean "NAE VMGEXIT" but pls write "NAE" out at least once so that reader can find its way around the spec. > the GHCB specification section 4.1.6 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. > > Cc: Thomas Gleixner <tglx@linutronix.de> > Cc: Ingo Molnar <mingo@redhat.com> > Cc: Borislav Petkov <bp@alien8.de> > Cc: Joerg Roedel <jroedel@suse.de> > Cc: "H. Peter Anvin" <hpa@zytor.com> > Cc: Tony Luck <tony.luck@intel.com> > Cc: Dave Hansen <dave.hansen@intel.com> > Cc: "Peter Zijlstra (Intel)" <peterz@infradead.org> > Cc: Paolo Bonzini <pbonzini@redhat.com> > Cc: Tom Lendacky <thomas.lendacky@amd.com> > Cc: David Rientjes <rientjes@google.com> > Cc: Sean Christopherson <seanjc@google.com> > Cc: x86@kernel.org > Cc: kvm@vger.kernel.org > Signed-off-by: Brijesh Singh <brijesh.singh@amd.com> > --- > arch/x86/include/asm/sev-snp.h | 34 +++++++++++++++++++++++++++++++++ > arch/x86/include/uapi/asm/svm.h | 1 + > 2 files changed, 35 insertions(+) > > diff --git a/arch/x86/include/asm/sev-snp.h b/arch/x86/include/asm/sev-snp.h > index 5a6d1367cab7..f514dad276f2 100644 > --- a/arch/x86/include/asm/sev-snp.h > +++ b/arch/x86/include/asm/sev-snp.h > @@ -22,6 +22,40 @@ > #define RMP_PG_SIZE_2M 1 > #define RMP_PG_SIZE_4K 0 > > +/* Page State Change MSR Protocol */ > +#define GHCB_SNP_PAGE_STATE_CHANGE_REQ 0x0014 > +#define GHCB_SNP_PAGE_STATE_REQ_GFN(v, o) (GHCB_SNP_PAGE_STATE_CHANGE_REQ | \ > + ((unsigned long)((o) & 0xf) << 52) | \ > + (((v) << 12) & 0xffffffffffffff)) This macro needs to be more readable and I'm not sure the masking is correct. IOW, something like this perhaps: #define GHCB_SNP_PAGE_STATE_REQ_GFN(va, operation) \ ((((operation) & 0xf) << 52) | ((va) & GENMASK_ULL(51, 12)) | GHCB_SNP_PAGE_STATE_CHANGE_REQ) where you have each GHCBData element at the proper place, msb to lsb. Now, GHCB spec says: "GHCBData[51:12] – Guest physical frame number" and I'm not clear as to what this macro takes: a virtual address or a pfn. If it is a pfn, then you need to do: (((pfn) << 12) & GENMASK_ULL(51, 0)) but if it is a virtual address you need to do what I have above. Since you do "<< 12" then it must be a pfn already but then you should call the argument "pfn" so that it is clear what it takes. Your mask above covers [55:0] but [55:52] is the page operation so the high bit in that mask needs to be 51. AFAICT, ofc. > +#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_SNP_PAGE_STATE_CHANGE_RESP 0x0015 > +#define GHCB_SNP_PAGE_STATE_RESP_VAL(val) ((val) >> 32) ^^^^^^^^^^^^ Some stray tabs here and above, pls pay attention to vertical alignment too. > + > +/* Page State Change NAE event */ > +#define SNP_PAGE_STATE_CHANGE_MAX_ENTRY 253 > +struct __packed snp_page_state_header { > + uint16_t cur_entry; > + uint16_t end_entry; > + uint32_t reserved; > +}; > + > +struct __packed snp_page_state_entry { > + uint64_t cur_page:12; > + uint64_t gfn:40; > + uint64_t operation:4; > + uint64_t pagesize:1; > + uint64_t reserved:7; > +}; Any particular reason for the uint<width>_t types or can you use our shorter u<width> types? > + > +struct __packed snp_page_state_change { > + struct snp_page_state_header header; > + struct snp_page_state_entry entry[SNP_PAGE_STATE_CHANGE_MAX_ENTRY]; Also, looking further into the patchset, I wonder if it would make sense to do: s/PAGE_STATE_CHANGE/PSC/g to avoid breaking lines of huge statements: + if ((GHCB_SEV_GHCB_RESP_CODE(val) != GHCB_SNP_PAGE_STATE_CHANGE_RESP) || + (GHCB_SNP_PAGE_STATE_RESP_VAL(val) != 0)) + sev_es_terminate(GHCB_SEV_ES_REASON_GENERAL_REQUEST); and turn them into something more readable + if ((GHCB_SEV_GHCB_RESP_CODE(val) != GHCB_SNP_PSC_RESP) || + (GHCB_SNP_PSC_RESP_VAL(val))) + sev_es_terminate(GHCB_SEV_ES_REASON_GENERAL_REQUEST); Also, you have GHCB_SEV and GHCB_SNP prefixes and I wonder whether we even need them and have everything be prefixed simply GHCB_ because that is the protocol, after all. Which will then turn the above into: + if ((GHCB_RESP_CODE(val) != GHCB_PSC_RESP) || (GHCB_PSC_RESP_VAL(val))) + sev_es_terminate(GHCB_REASON_GENERAL_REQUEST); Oh yeah baby! :-) Thx.
On 4/1/21 5:32 AM, Borislav Petkov wrote: > On Wed, Mar 24, 2021 at 11:44:15AM -0500, Brijesh Singh wrote: >> An SNP-active guest will use the page state change VNAE MGEXIT defined in > I guess this was supposed to mean "NAE VMGEXIT" but pls write "NAE" out > at least once so that reader can find its way around the spec. Noted. I will fix in next rev. >> the GHCB specification section 4.1.6 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. >> >> Cc: Thomas Gleixner <tglx@linutronix.de> >> Cc: Ingo Molnar <mingo@redhat.com> >> Cc: Borislav Petkov <bp@alien8.de> >> Cc: Joerg Roedel <jroedel@suse.de> >> Cc: "H. Peter Anvin" <hpa@zytor.com> >> Cc: Tony Luck <tony.luck@intel.com> >> Cc: Dave Hansen <dave.hansen@intel.com> >> Cc: "Peter Zijlstra (Intel)" <peterz@infradead.org> >> Cc: Paolo Bonzini <pbonzini@redhat.com> >> Cc: Tom Lendacky <thomas.lendacky@amd.com> >> Cc: David Rientjes <rientjes@google.com> >> Cc: Sean Christopherson <seanjc@google.com> >> Cc: x86@kernel.org >> Cc: kvm@vger.kernel.org >> Signed-off-by: Brijesh Singh <brijesh.singh@amd.com> >> --- >> arch/x86/include/asm/sev-snp.h | 34 +++++++++++++++++++++++++++++++++ >> arch/x86/include/uapi/asm/svm.h | 1 + >> 2 files changed, 35 insertions(+) >> >> diff --git a/arch/x86/include/asm/sev-snp.h b/arch/x86/include/asm/sev-snp.h >> index 5a6d1367cab7..f514dad276f2 100644 >> --- a/arch/x86/include/asm/sev-snp.h >> +++ b/arch/x86/include/asm/sev-snp.h >> @@ -22,6 +22,40 @@ >> #define RMP_PG_SIZE_2M 1 >> #define RMP_PG_SIZE_4K 0 >> >> +/* Page State Change MSR Protocol */ >> +#define GHCB_SNP_PAGE_STATE_CHANGE_REQ 0x0014 >> +#define GHCB_SNP_PAGE_STATE_REQ_GFN(v, o) (GHCB_SNP_PAGE_STATE_CHANGE_REQ | \ >> + ((unsigned long)((o) & 0xf) << 52) | \ >> + (((v) << 12) & 0xffffffffffffff)) > This macro needs to be more readable and I'm not sure the masking is > correct. IOW, something like this perhaps: > > #define GHCB_SNP_PAGE_STATE_REQ_GFN(va, operation) \ > ((((operation) & 0xf) << 52) | ((va) & GENMASK_ULL(51, 12)) | GHCB_SNP_PAGE_STATE_CHANGE_REQ) I guess I was trying to keep it in consistent with sev-es.h macro definitions in which the command is used before the fields. In next version, I will use the msb to lsb ordering. > > where you have each GHCBData element at the proper place, msb to lsb. > Now, GHCB spec says: > > "GHCBData[51:12] – Guest physical frame number" > > and I'm not clear as to what this macro takes: a virtual address or a > pfn. If it is a pfn, then you need to do: > > (((pfn) << 12) & GENMASK_ULL(51, 0)) > > but if it is a virtual address you need to do what I have above. Since > you do "<< 12" then it must be a pfn already but then you should call > the argument "pfn" so that it is clear what it takes. Yes, the macro takes the pfn. > Your mask above covers [55:0] but [55:52] is the page operation so the > high bit in that mask needs to be 51. Ack. I will limit the mask to 51 so that we don't go outside the pfn field width. thank you for pointing it. > AFAICT, ofc. > >> +#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_SNP_PAGE_STATE_CHANGE_RESP 0x0015 >> +#define GHCB_SNP_PAGE_STATE_RESP_VAL(val) ((val) >> 32) > ^^^^^^^^^^^^ > > Some stray tabs here and above, pls pay attention to vertical alignment too. I noticed that sev-es.h uses tab when defining the macro to build command. Another example where I tried to keep a bit consistentency with sev-es.h. I will drop it in next rev. > >> + >> +/* Page State Change NAE event */ >> +#define SNP_PAGE_STATE_CHANGE_MAX_ENTRY 253 >> +struct __packed snp_page_state_header { >> + uint16_t cur_entry; >> + uint16_t end_entry; >> + uint32_t reserved; >> +}; >> + >> +struct __packed snp_page_state_entry { >> + uint64_t cur_page:12; >> + uint64_t gfn:40; >> + uint64_t operation:4; >> + uint64_t pagesize:1; >> + uint64_t reserved:7; >> +}; > Any particular reason for the uint<width>_t types or can you use our > shorter u<width> types? IIRC, the spec structure has uint<width>_t, so I used it as-is. No strong reason for using it. I will switch to u64 type in the next version. > >> + >> +struct __packed snp_page_state_change { >> + struct snp_page_state_header header; >> + struct snp_page_state_entry entry[SNP_PAGE_STATE_CHANGE_MAX_ENTRY]; > Also, looking further into the patchset, I wonder if it would make sense > to do: > > s/PAGE_STATE_CHANGE/PSC/g > > to avoid breaking lines of huge statements: > > + if ((GHCB_SEV_GHCB_RESP_CODE(val) != GHCB_SNP_PAGE_STATE_CHANGE_RESP) || > + (GHCB_SNP_PAGE_STATE_RESP_VAL(val) != 0)) > + sev_es_terminate(GHCB_SEV_ES_REASON_GENERAL_REQUEST); > > and turn them into something more readable > > + if ((GHCB_SEV_GHCB_RESP_CODE(val) != GHCB_SNP_PSC_RESP) || > + (GHCB_SNP_PSC_RESP_VAL(val))) > + sev_es_terminate(GHCB_SEV_ES_REASON_GENERAL_REQUEST); > > Also, you have GHCB_SEV and GHCB_SNP prefixes and I wonder whether we > even need them and have everything be prefixed simply GHCB_ because that > is the protocol, after all. > > Which will then turn the above into: > > + if ((GHCB_RESP_CODE(val) != GHCB_PSC_RESP) || (GHCB_PSC_RESP_VAL(val))) > + sev_es_terminate(GHCB_REASON_GENERAL_REQUEST); > > Oh yeah baby! :-) It looks much better :-). I am good with dropping GHCB_{SEV,SNP} prefix, and also rename the PAGE_STATE_CHANGE to PSC. Thanks. > > Thx. >
On Thu, Apr 01, 2021 at 09:11:34AM -0500, Brijesh Singh wrote: > I guess I was trying to keep it in consistent with sev-es.h macro > definitions in which the command is used before the fields. In next > version, I will use the msb to lsb ordering. Yes pls. And then you could fix the sev-es.h macro too, in a prepatch maybe or in the same one, to do the same so that when reading the GHCB doc, it maps directly to the macros. > IIRC, the spec structure has uint<width>_t, so I used it as-is. No > strong reason for using it. I will switch to u64 type in the next version. Yeah, the uint* things are in the C spec but we don't need this definition outside the kernel, right? Thx.
diff --git a/arch/x86/include/asm/sev-snp.h b/arch/x86/include/asm/sev-snp.h index 5a6d1367cab7..f514dad276f2 100644 --- a/arch/x86/include/asm/sev-snp.h +++ b/arch/x86/include/asm/sev-snp.h @@ -22,6 +22,40 @@ #define RMP_PG_SIZE_2M 1 #define RMP_PG_SIZE_4K 0 +/* Page State Change MSR Protocol */ +#define GHCB_SNP_PAGE_STATE_CHANGE_REQ 0x0014 +#define GHCB_SNP_PAGE_STATE_REQ_GFN(v, o) (GHCB_SNP_PAGE_STATE_CHANGE_REQ | \ + ((unsigned long)((o) & 0xf) << 52) | \ + (((v) << 12) & 0xffffffffffffff)) +#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_SNP_PAGE_STATE_CHANGE_RESP 0x0015 +#define GHCB_SNP_PAGE_STATE_RESP_VAL(val) ((val) >> 32) + +/* Page State Change NAE event */ +#define SNP_PAGE_STATE_CHANGE_MAX_ENTRY 253 +struct __packed snp_page_state_header { + uint16_t cur_entry; + uint16_t end_entry; + uint32_t reserved; +}; + +struct __packed snp_page_state_entry { + uint64_t cur_page:12; + uint64_t gfn:40; + uint64_t operation:4; + uint64_t pagesize:1; + uint64_t reserved:7; +}; + +struct __packed snp_page_state_change { + struct snp_page_state_header header; + struct snp_page_state_entry entry[SNP_PAGE_STATE_CHANGE_MAX_ENTRY]; +}; + #ifdef CONFIG_AMD_MEM_ENCRYPT static inline int __pvalidate(unsigned long vaddr, int rmp_psize, int validate, unsigned long *rflags) diff --git a/arch/x86/include/uapi/asm/svm.h b/arch/x86/include/uapi/asm/svm.h index 554f75fe013c..751867aa432f 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_PAGE_STATE_CHANGE 0x80000010 #define SVM_VMGEXIT_UNSUPPORTED_EVENT 0x8000ffff #define SVM_EXIT_ERR -1
An SNP-active guest will use the page state change VNAE MGEXIT defined in the GHCB specification section 4.1.6 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. Cc: Thomas Gleixner <tglx@linutronix.de> Cc: Ingo Molnar <mingo@redhat.com> Cc: Borislav Petkov <bp@alien8.de> Cc: Joerg Roedel <jroedel@suse.de> Cc: "H. Peter Anvin" <hpa@zytor.com> Cc: Tony Luck <tony.luck@intel.com> Cc: Dave Hansen <dave.hansen@intel.com> Cc: "Peter Zijlstra (Intel)" <peterz@infradead.org> Cc: Paolo Bonzini <pbonzini@redhat.com> Cc: Tom Lendacky <thomas.lendacky@amd.com> Cc: David Rientjes <rientjes@google.com> Cc: Sean Christopherson <seanjc@google.com> Cc: x86@kernel.org Cc: kvm@vger.kernel.org Signed-off-by: Brijesh Singh <brijesh.singh@amd.com> --- arch/x86/include/asm/sev-snp.h | 34 +++++++++++++++++++++++++++++++++ arch/x86/include/uapi/asm/svm.h | 1 + 2 files changed, 35 insertions(+)