On 2/12/22 8:09 PM, Marc Orr wrote:
> 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.
>
Ack, dropping the comment.
>> +#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.
>
lib/x86/processor.h sounds like a decent place for both test_bit() and
lower_bits() later.
>> +
>> +/* 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.
>
I'll update this in v3.
> 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.
>
Ack.
>> #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
>>
>
