On Wed, Feb 9, 2022 at 8:44 AM Varad Gautam <varad.gautam@xxxxxxxx> wrote:
>
> Origin: Linux 64222515138e43da1fcf288f0289ef1020427b87
>
> Suppress -Waddress-of-packed-member to allow taking addresses on struct
> ghcb / struct vmcb_save_area fields.
>
> Signed-off-by: Varad Gautam <varad.gautam@xxxxxxxx>
> ---
> lib/x86/amd_sev.h | 106 ++++++++++++++++++++++++++++++++++++++++++++
> lib/x86/svm.h | 37 ++++++++++++++++
> x86/Makefile.x86_64 | 1 +
> 3 files changed, 144 insertions(+)
>
> diff --git a/lib/x86/amd_sev.h b/lib/x86/amd_sev.h
> index afbacf3..ed71c18 100644
> --- a/lib/x86/amd_sev.h
> +++ b/lib/x86/amd_sev.h
> @@ -18,6 +18,49 @@
> #include "desc.h"
> #include "asm/page.h"
> #include "efi.h"
> +#include "processor.h"
> +#include "insn/insn.h"
> +#include "svm.h"
> +
> +struct __attribute__ ((__packed__)) ghcb {
> + struct vmcb_save_area save;
> + u8 reserved_save[2048 - sizeof(struct vmcb_save_area)];
> +
> + u8 shared_buffer[2032];
> +
> + u8 reserved_1[10];
> + u16 protocol_version; /* negotiated SEV-ES/GHCB protocol version */
> + u32 ghcb_usage;
> +};
> +
> +/* SEV definitions from linux's include/asm/sev.h */
nit: "include/asm/sev.h" should be "arch/x86/include/asm/sev.h".
Also, while I feel that I like verbose comments more than many, it
might be best to skip this one. Because when this code diverges from
Linux, it's just going to cause confusion.
> +#define GHCB_PROTO_OUR 0x0001UL
> +#define GHCB_PROTOCOL_MAX 1ULL
> +#define GHCB_DEFAULT_USAGE 0ULL
> +
> +#define VMGEXIT() { asm volatile("rep; vmmcall\n\r"); }
> +
> +enum es_result {
> + ES_OK, /* All good */
> + ES_UNSUPPORTED, /* Requested operation not supported */
> + ES_VMM_ERROR, /* Unexpected state from the VMM */
> + ES_DECODE_FAILED, /* Instruction decoding failed */
> + ES_EXCEPTION, /* Instruction caused exception */
> + ES_RETRY, /* Retry instruction emulation */
> +};
> +
> +struct es_fault_info {
> + unsigned long vector;
> + unsigned long error_code;
> + unsigned long cr2;
> +};
> +
> +/* ES instruction emulation context */
> +struct es_em_ctxt {
> + struct ex_regs *regs;
> + struct insn insn;
> + struct es_fault_info fi;
> +};
>
> /*
> * AMD Programmer's Manual Volume 3
> @@ -59,6 +102,69 @@ void handle_sev_es_vc(struct ex_regs *regs);
> unsigned long long get_amd_sev_c_bit_mask(void);
> unsigned long long get_amd_sev_addr_upperbound(void);
>
> +static int _test_bit(int nr, const volatile unsigned long *addr)
> +{
> + const volatile unsigned long *word = addr + BIT_WORD(nr);
> + unsigned long mask = BIT_MASK(nr);
> +
> + return (*word & mask) != 0;
> +}
This looks like it's copy/pasted from lib/arm/bitops.c? Maybe it's
worth moving this helper into a platform independent bitops library.
Alternatively, we could add an x86-specific test_bit implementation to
lib/x86/processor.h, where `set_bit()` is defined.
> +
> +/* GHCB Accessor functions from Linux's include/asm/svm.h */
> +
> +#define GHCB_BITMAP_IDX(field) \
> + (offsetof(struct vmcb_save_area, field) / sizeof(u64))
> +
> +#define DEFINE_GHCB_ACCESSORS(field) \
> + static inline bool ghcb_##field##_is_valid(const struct ghcb *ghcb) \
> + { \
> + return _test_bit(GHCB_BITMAP_IDX(field), \
> + (unsigned long *)&ghcb->save.valid_bitmap); \
> + } \
> + \
> + static inline u64 ghcb_get_##field(struct ghcb *ghcb) \
> + { \
> + return ghcb->save.field; \
> + } \
> + \
> + static inline u64 ghcb_get_##field##_if_valid(struct ghcb *ghcb) \
> + { \
> + return ghcb_##field##_is_valid(ghcb) ? ghcb->save.field : 0; \
> + } \
> + \
> + static inline void ghcb_set_##field(struct ghcb *ghcb, u64 value) \
> + { \
> + set_bit(GHCB_BITMAP_IDX(field), \
> + (u8 *)&ghcb->save.valid_bitmap); \
> + ghcb->save.field = value; \
> + }
> +
> +DEFINE_GHCB_ACCESSORS(cpl)
> +DEFINE_GHCB_ACCESSORS(rip)
> +DEFINE_GHCB_ACCESSORS(rsp)
> +DEFINE_GHCB_ACCESSORS(rax)
> +DEFINE_GHCB_ACCESSORS(rcx)
> +DEFINE_GHCB_ACCESSORS(rdx)
> +DEFINE_GHCB_ACCESSORS(rbx)
> +DEFINE_GHCB_ACCESSORS(rbp)
> +DEFINE_GHCB_ACCESSORS(rsi)
> +DEFINE_GHCB_ACCESSORS(rdi)
> +DEFINE_GHCB_ACCESSORS(r8)
> +DEFINE_GHCB_ACCESSORS(r9)
> +DEFINE_GHCB_ACCESSORS(r10)
> +DEFINE_GHCB_ACCESSORS(r11)
> +DEFINE_GHCB_ACCESSORS(r12)
> +DEFINE_GHCB_ACCESSORS(r13)
> +DEFINE_GHCB_ACCESSORS(r14)
> +DEFINE_GHCB_ACCESSORS(r15)
> +DEFINE_GHCB_ACCESSORS(sw_exit_code)
> +DEFINE_GHCB_ACCESSORS(sw_exit_info_1)
> +DEFINE_GHCB_ACCESSORS(sw_exit_info_2)
> +DEFINE_GHCB_ACCESSORS(sw_scratch)
> +DEFINE_GHCB_ACCESSORS(xcr0)
> +
> +#define MSR_AMD64_SEV_ES_GHCB 0xc0010130
Should this go in lib/x86/msr.h?
> +
> #endif /* TARGET_EFI */
>
> #endif /* _X86_AMD_SEV_H_ */
> diff --git a/lib/x86/svm.h b/lib/x86/svm.h
> index f74b13a..f046455 100644
> --- a/lib/x86/svm.h
> +++ b/lib/x86/svm.h
> @@ -197,6 +197,42 @@ struct __attribute__ ((__packed__)) vmcb_save_area {
> u64 br_to;
> u64 last_excp_from;
> u64 last_excp_to;
In upstream Linux @ 64222515138e, above the save area, there was a
change made for ES. See below. Maybe we should go ahead pull this
change from Linux while we're here adding the VMSA.
kvm-unit-tests, with this patch applied:
172 u8 reserved_3[112];
173 u64 cr4;
Linux @ 64222515138e:
245 u8 reserved_3[104];
246 u64 xss; /* Valid for SEV-ES only */
247 u64 cr4;
> +
> + /*
> + * The following part of the save area is valid only for
> + * SEV-ES guests when referenced through the GHCB or for
> + * saving to the host save area.
> + */
> + u8 reserved_7[72];
> + u32 spec_ctrl; /* Guest version of SPEC_CTRL at 0x2E0 */
> + u8 reserved_7b[4];
> + u32 pkru;
> + u8 reserved_7a[20];
> + u64 reserved_8; /* rax already available at 0x01f8 */
> + u64 rcx;
> + u64 rdx;
> + u64 rbx;
> + u64 reserved_9; /* rsp already available at 0x01d8 */
> + u64 rbp;
> + u64 rsi;
> + u64 rdi;
> + u64 r8;
> + u64 r9;
> + u64 r10;
> + u64 r11;
> + u64 r12;
> + u64 r13;
> + u64 r14;
> + u64 r15;
> + u8 reserved_10[16];
> + u64 sw_exit_code;
> + u64 sw_exit_info_1;
> + u64 sw_exit_info_2;
> + u64 sw_scratch;
> + u8 reserved_11[56];
> + u64 xcr0;
> + u8 valid_bitmap[16];
> + u64 x87_state_gpa;
> };
>
> struct __attribute__ ((__packed__)) vmcb {
> @@ -297,6 +333,7 @@ struct __attribute__ ((__packed__)) vmcb {
> #define SVM_EXIT_WRITE_DR6 0x036
> #define SVM_EXIT_WRITE_DR7 0x037
> #define SVM_EXIT_EXCP_BASE 0x040
> +#define SVM_EXIT_LAST_EXCP 0x05f
nit: There is a spacing issue here. When this patch is applied, 0x05f
is not aligned with the constants above and below.
> #define SVM_EXIT_INTR 0x060
> #define SVM_EXIT_NMI 0x061
> #define SVM_EXIT_SMI 0x062
> diff --git a/x86/Makefile.x86_64 b/x86/Makefile.x86_64
> index a3cb75a..7d3eb53 100644
> --- a/x86/Makefile.x86_64
> +++ b/x86/Makefile.x86_64
> @@ -13,6 +13,7 @@ endif
>
> fcf_protection_full := $(call cc-option, -fcf-protection=full,)
> COMMON_CFLAGS += -mno-red-zone -mno-sse -mno-sse2 $(fcf_protection_full)
> +COMMON_CFLAGS += -Wno-address-of-packed-member
>
> cflatobjs += lib/x86/setjmp64.o
> cflatobjs += lib/x86/intel-iommu.o
> --
> 2.32.0
>
