Hi Paolo,
here are some groundwork patches for the upcoming SEV-ES support in the
Linux kernel. They are part of both the client patch-set and of the KVM
hypervisor patches (under development).
Patch 1 necesary to fix a compile warning about a stack-frame getting
too large. The other 3 patches are currently posted as part of the
SEV-ES client patch-set.
It would be great if you could consider them for v5.9, so that the
client and the hypervisor patch-sets can be developed more independently
of each other.
Please let me know what you think.
Regards,
Joerg
Borislav Petkov (1):
KVM: SVM: Use __packed shorthand
Joerg Roedel (2):
KVM: SVM: nested: Don't allocate VMCB structures on stack
KVM: SVM: Add GHCB Accessor functions
Tom Lendacky (1):
KVM: SVM: Add GHCB definitions
arch/x86/include/asm/svm.h | 118 ++++++++++++++++++++++++++++++++++---
arch/x86/kvm/svm/nested.c | 44 +++++++++-----
arch/x86/kvm/svm/svm.c | 2 +
3 files changed, 143 insertions(+), 21 deletions(-)
--
2.17.1
From: Joerg Roedel <[email protected]>
Building a correct GHCB for the hypervisor requires setting valid bits
in the GHCB. Simplify that process by providing accessor functions to
set values and to update the valid bitmap.
Signed-off-by: Joerg Roedel <[email protected]>
---
arch/x86/include/asm/svm.h | 61 ++++++++++++++++++++++++++++++++++++++
1 file changed, 61 insertions(+)
diff --git a/arch/x86/include/asm/svm.h b/arch/x86/include/asm/svm.h
index 9a3e0b802716..0420250b008b 100644
--- a/arch/x86/include/asm/svm.h
+++ b/arch/x86/include/asm/svm.h
@@ -341,4 +341,65 @@ struct __attribute__ ((__packed__)) vmcb {
#define SVM_CR0_SELECTIVE_MASK (X86_CR0_TS | X86_CR0_MP)
+/* GHCB Accessor functions */
+
+#define DEFINE_GHCB_INDICES(field) \
+ u16 idx = offsetof(struct vmcb_save_area, field) / 8; \
+ u16 byte_idx = idx / 8; \
+ u16 bit_idx = idx % 8; \
+ BUILD_BUG_ON(byte_idx > ARRAY_SIZE(ghcb->save.valid_bitmap));
+
+#define GHCB_SET_VALID(ghcb, field) \
+ { \
+ DEFINE_GHCB_INDICES(field) \
+ (ghcb)->save.valid_bitmap[byte_idx] |= BIT(bit_idx); \
+ }
+
+#define DEFINE_GHCB_SETTER(field) \
+ static inline void \
+ ghcb_set_##field(struct ghcb *ghcb, u64 value) \
+ { \
+ GHCB_SET_VALID(ghcb, field) \
+ (ghcb)->save.field = value; \
+ }
+
+#define DEFINE_GHCB_ACCESSORS(field) \
+ static inline bool ghcb_is_valid_##field(const struct ghcb *ghcb) \
+ { \
+ DEFINE_GHCB_INDICES(field) \
+ return !!((ghcb)->save.valid_bitmap[byte_idx] \
+ & BIT(bit_idx)); \
+ } \
+ \
+ static inline void \
+ ghcb_set_##field(struct ghcb *ghcb, u64 value) \
+ { \
+ GHCB_SET_VALID(ghcb, field) \
+ (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)
+
#endif
--
2.17.1
From: Borislav Petkov <[email protected]>
Use the shorthand to make it more readable.
No functional changes.
Signed-off-by: Joerg Roedel <[email protected]>
---
arch/x86/include/asm/svm.h | 16 ++++++++--------
1 file changed, 8 insertions(+), 8 deletions(-)
diff --git a/arch/x86/include/asm/svm.h b/arch/x86/include/asm/svm.h
index 0420250b008b..af91ced0f370 100644
--- a/arch/x86/include/asm/svm.h
+++ b/arch/x86/include/asm/svm.h
@@ -150,14 +150,14 @@ struct __attribute__ ((__packed__)) vmcb_control_area {
#define SVM_NESTED_CTL_NP_ENABLE BIT(0)
#define SVM_NESTED_CTL_SEV_ENABLE BIT(1)
-struct __attribute__ ((__packed__)) vmcb_seg {
+struct vmcb_seg {
u16 selector;
u16 attrib;
u32 limit;
u64 base;
-};
+} __packed;
-struct __attribute__ ((__packed__)) vmcb_save_area {
+struct vmcb_save_area {
struct vmcb_seg es;
struct vmcb_seg cs;
struct vmcb_seg ss;
@@ -231,9 +231,9 @@ struct __attribute__ ((__packed__)) vmcb_save_area {
u64 xcr0;
u8 valid_bitmap[16];
u64 x87_state_gpa;
-};
+} __packed;
-struct __attribute__ ((__packed__)) ghcb {
+struct ghcb {
struct vmcb_save_area save;
u8 reserved_save[2048 - sizeof(struct vmcb_save_area)];
@@ -242,7 +242,7 @@ struct __attribute__ ((__packed__)) ghcb {
u8 reserved_1[10];
u16 protocol_version; /* negotiated SEV-ES/GHCB protocol version */
u32 ghcb_usage;
-};
+} __packed;
static inline void __unused_size_checks(void)
@@ -252,11 +252,11 @@ static inline void __unused_size_checks(void)
BUILD_BUG_ON(sizeof(struct ghcb) != 4096);
}
-struct __attribute__ ((__packed__)) vmcb {
+struct vmcb {
struct vmcb_control_area control;
u8 reserved_control[1024 - sizeof(struct vmcb_control_area)];
struct vmcb_save_area save;
-};
+} __packed;
#define SVM_CPUID_FUNC 0x8000000a
--
2.17.1
From: Tom Lendacky <[email protected]>
Extend the vmcb_safe_area with SEV-ES fields and add a new
'struct ghcb' which will be used for guest-hypervisor communication.
Signed-off-by: Tom Lendacky <[email protected]>
Signed-off-by: Joerg Roedel <[email protected]>
---
arch/x86/include/asm/svm.h | 45 +++++++++++++++++++++++++++++++++++++-
arch/x86/kvm/svm/svm.c | 2 ++
2 files changed, 46 insertions(+), 1 deletion(-)
diff --git a/arch/x86/include/asm/svm.h b/arch/x86/include/asm/svm.h
index 8a1f5382a4ea..9a3e0b802716 100644
--- a/arch/x86/include/asm/svm.h
+++ b/arch/x86/include/asm/svm.h
@@ -200,13 +200,56 @@ struct __attribute__ ((__packed__)) vmcb_save_area {
u64 br_to;
u64 last_excp_from;
u64 last_excp_to;
+
+ /*
+ * The following part of the save area is valid only for
+ * SEV-ES guests when referenced through the GHCB.
+ */
+ u8 reserved_7[104];
+ 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__)) 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;
};
static inline void __unused_size_checks(void)
{
- BUILD_BUG_ON(sizeof(struct vmcb_save_area) != 0x298);
+ BUILD_BUG_ON(sizeof(struct vmcb_save_area) != 1032);
BUILD_BUG_ON(sizeof(struct vmcb_control_area) != 256);
+ BUILD_BUG_ON(sizeof(struct ghcb) != 4096);
}
struct __attribute__ ((__packed__)) vmcb {
diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
index 783330d0e7b8..953cf947f022 100644
--- a/arch/x86/kvm/svm/svm.c
+++ b/arch/x86/kvm/svm/svm.c
@@ -4161,6 +4161,8 @@ static struct kvm_x86_init_ops svm_init_ops __initdata = {
static int __init svm_init(void)
{
+ __unused_size_checks();
+
return kvm_init(&svm_init_ops, sizeof(struct vcpu_svm),
__alignof__(struct vcpu_svm), THIS_MODULE);
}
--
2.17.1
On Wed, Jul 29, 2020 at 03:22:33PM +0200, Joerg Roedel wrote:
> From: Joerg Roedel <[email protected]>
>
> Building a correct GHCB for the hypervisor requires setting valid bits
> in the GHCB. Simplify that process by providing accessor functions to
> set values and to update the valid bitmap.
>
> Signed-off-by: Joerg Roedel <[email protected]>
> ---
> arch/x86/include/asm/svm.h | 61 ++++++++++++++++++++++++++++++++++++++
> 1 file changed, 61 insertions(+)
>
> diff --git a/arch/x86/include/asm/svm.h b/arch/x86/include/asm/svm.h
> index 9a3e0b802716..0420250b008b 100644
> --- a/arch/x86/include/asm/svm.h
> +++ b/arch/x86/include/asm/svm.h
> @@ -341,4 +341,65 @@ struct __attribute__ ((__packed__)) vmcb {
>
> #define SVM_CR0_SELECTIVE_MASK (X86_CR0_TS | X86_CR0_MP)
>
> +/* GHCB Accessor functions */
> +
> +#define DEFINE_GHCB_INDICES(field) \
> + u16 idx = offsetof(struct vmcb_save_area, field) / 8; \
Using sizeof(u64) instead of '8' would be helpful here.
> + u16 byte_idx = idx / 8; \
> + u16 bit_idx = idx % 8; \
Oof. I love macro frameworks as much as the next person, but declaring
local variables in a macro like this is nasty.
> + BUILD_BUG_ON(byte_idx > ARRAY_SIZE(ghcb->save.valid_bitmap));
> +
> +#define GHCB_SET_VALID(ghcb, field) \
> + { \
> + DEFINE_GHCB_INDICES(field) \
> + (ghcb)->save.valid_bitmap[byte_idx] |= BIT(bit_idx); \
Rather than manually calculate the byte/bit indices just use __set_bit()
and test_bit(). That will also solve the variable declaration issue.
E.g.
#define GHB_BITMAP_IDX(field) \
(offsetof(struct vmcb_save_area, (field)) / sizeof(u64))
#define GHCB_SET_VALID(ghcb, field) \
__set_bit(GHCB_BITMAP_IDX(field), (unsigned long *)&ghcb->save.valid_bitmap)
Or alternatively drop GHCB_SET_VALID() and just open code the two users.
> + }
> +
> +#define DEFINE_GHCB_SETTER(field) \
> + static inline void \
> + ghcb_set_##field(struct ghcb *ghcb, u64 value) \
> + { \
> + GHCB_SET_VALID(ghcb, field) \
> + (ghcb)->save.field = value; \
The ghcb doesn't need to be wrapped in (), it's a parameter to a function.
Same comment for the below usage.
> + }
> +
> +#define DEFINE_GHCB_ACCESSORS(field) \
> + static inline bool ghcb_is_valid_##field(const struct ghcb *ghcb) \
I'd prefer to follow the naming of the arch reg accessors, i.e.
static inline bool ghcb_##field##_is_valid(...)
to pair with
kvm_##lname##_read
kvm_##lname##_write
And because ghcb_is_valid_rip() reads a bit weird, e.g. IMO is more likely
to be read as "does the RIP in the GHCB hold a valid (canonical) value",
versus ghcb_rip_is_valid() reading as "is RIP valid in the GHCB".
> + { \
> + DEFINE_GHCB_INDICES(field) \
> + return !!((ghcb)->save.valid_bitmap[byte_idx] \
> + & BIT(bit_idx)); \
> + } \
> + \
> + static inline void \
> + ghcb_set_##field(struct ghcb *ghcb, u64 value) \
> + { \
> + GHCB_SET_VALID(ghcb, field) \
> + (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)
> +
> #endif
> --
> 2.17.1
>
On Wed, Jul 29, 2020 at 08:43:28AM -0700, Sean Christopherson wrote:
> Rather than manually calculate the byte/bit indices just use __set_bit()
> and test_bit(). That will also solve the variable declaration issue.
>
> E.g.
>
> #define GHB_BITMAP_IDX(field) \
> (offsetof(struct vmcb_save_area, (field)) / sizeof(u64))
>
> #define GHCB_SET_VALID(ghcb, field) \
> __set_bit(GHCB_BITMAP_IDX(field), (unsigned long *)&ghcb->save.valid_bitmap)
>
> Or alternatively drop GHCB_SET_VALID() and just open code the two users.
Thanks for your suggestions, I updated the patch and will do some
testing before re-posting.
Regards,
Joerg
>
> > + }
> > +
> > +#define DEFINE_GHCB_SETTER(field) \
> > + static inline void \
> > + ghcb_set_##field(struct ghcb *ghcb, u64 value) \
> > + { \
> > + GHCB_SET_VALID(ghcb, field) \
> > + (ghcb)->save.field = value; \
>
>
> The ghcb doesn't need to be wrapped in (), it's a parameter to a function.
> Same comment for the below usage.
>
> > + }
> > +
> > +#define DEFINE_GHCB_ACCESSORS(field) \
> > + static inline bool ghcb_is_valid_##field(const struct ghcb *ghcb) \
>
> I'd prefer to follow the naming of the arch reg accessors, i.e.
>
> static inline bool ghcb_##field##_is_valid(...)
>
> to pair with
>
> kvm_##lname##_read
> kvm_##lname##_write
>
> And because ghcb_is_valid_rip() reads a bit weird, e.g. IMO is more likely
> to be read as "does the RIP in the GHCB hold a valid (canonical) value",
> versus ghcb_rip_is_valid() reading as "is RIP valid in the GHCB".
>
> > + { \
> > + DEFINE_GHCB_INDICES(field) \
> > + return !!((ghcb)->save.valid_bitmap[byte_idx] \
> > + & BIT(bit_idx)); \
> > + } \
> > + \
> > + static inline void \
> > + ghcb_set_##field(struct ghcb *ghcb, u64 value) \
> > + { \
> > + GHCB_SET_VALID(ghcb, field) \
> > + (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)
> > +
> > #endif
> > --
> > 2.17.1
> >