[PATCH 0/4] KVM: SVM: SEV-ES groundwork

[PATCH 0/4] KVM: SVM: SEV-ES groundwork

[Joerg Roedel]

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

[PATCH 1/4] KVM: SVM: nested: Don't allocate VMCB structures on stack

[Joerg Roedel]

From: Joerg Roedel <jroedel@suse.de>

Do not allocate a vmcb_control_area and a vmcb_save_area on the stack,
as these structures will become larger with future extenstions of
SVM and thus the svm_set_nested_state() function will become a too large
stack frame.

Signed-off-by: Joerg Roedel <jroedel@suse.de>
---
 arch/x86/kvm/svm/nested.c | 44 ++++++++++++++++++++++++++-------------
 1 file changed, 30 insertions(+), 14 deletions(-)

diff --git a/arch/x86/kvm/svm/nested.c b/arch/x86/kvm/svm/nested.c
index 61378a3c2ce4..f3c3c4e1ca7f 100644
--- a/arch/x86/kvm/svm/nested.c
+++ b/arch/x86/kvm/svm/nested.c
@@ -1061,8 +1061,9 @@ static int svm_set_nested_state(struct kvm_vcpu *vcpu,
 	struct vmcb *hsave = svm->nested.hsave;
 	struct vmcb __user *user_vmcb = (struct vmcb __user *)
 		&user_kvm_nested_state->data.svm[0];
-	struct vmcb_control_area ctl;
-	struct vmcb_save_area save;
+	struct vmcb_control_area *ctl;
+	struct vmcb_save_area *save;
+	int ret;
 	u32 cr0;
 
 	if (kvm_state->format != KVM_STATE_NESTED_FORMAT_SVM)
@@ -1096,13 +1097,22 @@ static int svm_set_nested_state(struct kvm_vcpu *vcpu,
 		return -EINVAL;
 	if (kvm_state->size < sizeof(*kvm_state) + KVM_STATE_NESTED_SVM_VMCB_SIZE)
 		return -EINVAL;
-	if (copy_from_user(&ctl, &user_vmcb->control, sizeof(ctl)))
-		return -EFAULT;
-	if (copy_from_user(&save, &user_vmcb->save, sizeof(save)))
-		return -EFAULT;
 
-	if (!nested_vmcb_check_controls(&ctl))
-		return -EINVAL;
+	ret  = -ENOMEM;
+	ctl  = kzalloc(sizeof(*ctl),  GFP_KERNEL);
+	save = kzalloc(sizeof(*save), GFP_KERNEL);
+	if (!ctl || !save)
+		goto out_free;
+
+	ret = -EFAULT;
+	if (copy_from_user(ctl, &user_vmcb->control, sizeof(ctl)))
+		goto out_free;
+	if (copy_from_user(save, &user_vmcb->save, sizeof(save)))
+		goto out_free;
+
+	ret = -EINVAL;
+	if (!nested_vmcb_check_controls(ctl))
+		goto out_free;
 
 	/*
 	 * Processor state contains L2 state.  Check that it is
@@ -1110,15 +1120,15 @@ static int svm_set_nested_state(struct kvm_vcpu *vcpu,
 	 */
 	cr0 = kvm_read_cr0(vcpu);
         if (((cr0 & X86_CR0_CD) == 0) && (cr0 & X86_CR0_NW))
-                return -EINVAL;
+                goto out_free;
 
 	/*
 	 * Validate host state saved from before VMRUN (see
 	 * nested_svm_check_permissions).
 	 * TODO: validate reserved bits for all saved state.
 	 */
-	if (!(save.cr0 & X86_CR0_PG))
-		return -EINVAL;
+	if (!(save->cr0 & X86_CR0_PG))
+		goto out_free;
 
 	/*
 	 * All checks done, we can enter guest mode.  L1 control fields
@@ -1127,15 +1137,21 @@ static int svm_set_nested_state(struct kvm_vcpu *vcpu,
 	 * contains saved L1 state.
 	 */
 	copy_vmcb_control_area(&hsave->control, &svm->vmcb->control);
-	hsave->save = save;
+	hsave->save = *save;
 
 	svm->nested.vmcb = kvm_state->hdr.svm.vmcb_pa;
-	load_nested_vmcb_control(svm, &ctl);
+	load_nested_vmcb_control(svm, ctl);
 	nested_prepare_vmcb_control(svm);
 
 out_set_gif:
 	svm_set_gif(svm, !!(kvm_state->flags & KVM_STATE_NESTED_GIF_SET));
-	return 0;
+
+	ret = 0;
+out_free:
+	kfree(save);
+	kfree(ctl);
+
+	return ret;
 }
 
 struct kvm_x86_nested_ops svm_nested_ops = {
-- 
2.17.1

Re: [PATCH 1/4] KVM: SVM: nested: Don't allocate VMCB structures on stack

[Sean Christopherson]

On Wed, Jul 29, 2020 at 03:22:31PM +0200, Joerg Roedel wrote:
> From: Joerg Roedel <jroedel@suse.de>
> 
> Do not allocate a vmcb_control_area and a vmcb_save_area on the stack,
> as these structures will become larger with future extenstions of
> SVM and thus the svm_set_nested_state() function will become a too large
> stack frame.

Speaking of too large, would it be overly paranoid to add:

  BUILD_BUG_ON(sizeof(struct vmcb_control_area) + sizeof(struct vmcb_save_area) <
	       KVM_STATE_NESTED_SVM_VMCB_SIZE)

More so for documentation than for any real concern that the SVM architecture
will do something silly, e.g. to make it obvious that patch 2 in this series
won't break backwards compatibility.
 
> Signed-off-by: Joerg Roedel <jroedel@suse.de>
> ---
>  arch/x86/kvm/svm/nested.c | 44 ++++++++++++++++++++++++++-------------
>  1 file changed, 30 insertions(+), 14 deletions(-)
> 
> diff --git a/arch/x86/kvm/svm/nested.c b/arch/x86/kvm/svm/nested.c
> index 61378a3c2ce4..f3c3c4e1ca7f 100644
> --- a/arch/x86/kvm/svm/nested.c
> +++ b/arch/x86/kvm/svm/nested.c
> @@ -1061,8 +1061,9 @@ static int svm_set_nested_state(struct kvm_vcpu *vcpu,
>  	struct vmcb *hsave = svm->nested.hsave;
>  	struct vmcb __user *user_vmcb = (struct vmcb __user *)
>  		&user_kvm_nested_state->data.svm[0];
> -	struct vmcb_control_area ctl;
> -	struct vmcb_save_area save;
> +	struct vmcb_control_area *ctl;
> +	struct vmcb_save_area *save;
> +	int ret;
>  	u32 cr0;
>  
>  	if (kvm_state->format != KVM_STATE_NESTED_FORMAT_SVM)
> @@ -1096,13 +1097,22 @@ static int svm_set_nested_state(struct kvm_vcpu *vcpu,
>  		return -EINVAL;
>  	if (kvm_state->size < sizeof(*kvm_state) + KVM_STATE_NESTED_SVM_VMCB_SIZE)
>  		return -EINVAL;
> -	if (copy_from_user(&ctl, &user_vmcb->control, sizeof(ctl)))
> -		return -EFAULT;
> -	if (copy_from_user(&save, &user_vmcb->save, sizeof(save)))
> -		return -EFAULT;
>  
> -	if (!nested_vmcb_check_controls(&ctl))
> -		return -EINVAL;
> +	ret  = -ENOMEM;
> +	ctl  = kzalloc(sizeof(*ctl),  GFP_KERNEL);
> +	save = kzalloc(sizeof(*save), GFP_KERNEL);
> +	if (!ctl || !save)
> +		goto out_free;
> +
> +	ret = -EFAULT;
> +	if (copy_from_user(ctl, &user_vmcb->control, sizeof(ctl)))

The sizeof() calc is wrong, this is now calculating the size of the pointer,
not the size of the struct.  It'd need to be sizeof(*ctl).

> +		goto out_free;
> +	if (copy_from_user(save, &user_vmcb->save, sizeof(save)))

Same bug here.

> +		goto out_free;
> +
> +	ret = -EINVAL;
> +	if (!nested_vmcb_check_controls(ctl))
> +		goto out_free;
>  
>  	/*
>  	 * Processor state contains L2 state.  Check that it is
> @@ -1110,15 +1120,15 @@ static int svm_set_nested_state(struct kvm_vcpu *vcpu,
>  	 */
>  	cr0 = kvm_read_cr0(vcpu);
>          if (((cr0 & X86_CR0_CD) == 0) && (cr0 & X86_CR0_NW))
> -                return -EINVAL;
> +                goto out_free;
>  
>  	/*
>  	 * Validate host state saved from before VMRUN (see
>  	 * nested_svm_check_permissions).
>  	 * TODO: validate reserved bits for all saved state.
>  	 */
> -	if (!(save.cr0 & X86_CR0_PG))
> -		return -EINVAL;
> +	if (!(save->cr0 & X86_CR0_PG))
> +		goto out_free;
>  
>  	/*
>  	 * All checks done, we can enter guest mode.  L1 control fields
> @@ -1127,15 +1137,21 @@ static int svm_set_nested_state(struct kvm_vcpu *vcpu,
>  	 * contains saved L1 state.
>  	 */
>  	copy_vmcb_control_area(&hsave->control, &svm->vmcb->control);
> -	hsave->save = save;
> +	hsave->save = *save;
>  
>  	svm->nested.vmcb = kvm_state->hdr.svm.vmcb_pa;
> -	load_nested_vmcb_control(svm, &ctl);
> +	load_nested_vmcb_control(svm, ctl);
>  	nested_prepare_vmcb_control(svm);
>  
>  out_set_gif:
>  	svm_set_gif(svm, !!(kvm_state->flags & KVM_STATE_NESTED_GIF_SET));
> -	return 0;
> +
> +	ret = 0;
> +out_free:
> +	kfree(save);
> +	kfree(ctl);
> +
> +	return ret;
>  }
>  
>  struct kvm_x86_nested_ops svm_nested_ops = {
> -- 
> 2.17.1
> 

Re: [PATCH 1/4] KVM: SVM: nested: Don't allocate VMCB structures on stack

[Joerg Roedel]

Hi Sean,

thanks for your review!

On Wed, Jul 29, 2020 at 08:14:55AM -0700, Sean Christopherson wrote:
> On Wed, Jul 29, 2020 at 03:22:31PM +0200, Joerg Roedel wrote:
> Speaking of too large, would it be overly paranoid to add:
> 
>   BUILD_BUG_ON(sizeof(struct vmcb_control_area) + sizeof(struct vmcb_save_area) <
> 	       KVM_STATE_NESTED_SVM_VMCB_SIZE)
> 
> More so for documentation than for any real concern that the SVM architecture
> will do something silly, e.g. to make it obvious that patch 2 in this series
> won't break backwards compatibility.

The check should actually be '>', but then it makes sense. The control-
and save-area together are still way smaller than 4k. I will add the
check for '>' to this patch.

> > +	ret = -EFAULT;
> > +	if (copy_from_user(ctl, &user_vmcb->control, sizeof(ctl)))
> 
> The sizeof() calc is wrong, this is now calculating the size of the pointer,
> not the size of the struct.  It'd need to be sizeof(*ctl).
> 
> > +		goto out_free;
> > +	if (copy_from_user(save, &user_vmcb->save, sizeof(save)))
> 
> Same bug here.

Thanks, fixed that.

Regards,

	Joerg

[PATCH 2/4] KVM: SVM: Add GHCB definitions

[Joerg Roedel]

From: Tom Lendacky <thomas.lendacky@amd.com>

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 <thomas.lendacky@amd.com>
Signed-off-by: Joerg Roedel <jroedel@suse.de>
---
 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

[PATCH 3/4] KVM: SVM: Add GHCB Accessor functions

[Joerg Roedel]

From: Joerg Roedel <jroedel@suse.de>

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 <jroedel@suse.de>
---
 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

Re: [PATCH 3/4] KVM: SVM: Add GHCB Accessor functions

[Sean Christopherson]

On Wed, Jul 29, 2020 at 03:22:33PM +0200, Joerg Roedel wrote:
> From: Joerg Roedel <jroedel@suse.de>
> 
> 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 <jroedel@suse.de>
> ---
>  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
> 

Re: [PATCH 3/4] KVM: SVM: Add GHCB Accessor functions

[Joerg Roedel]

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
> > 

[PATCH 4/4] KVM: SVM: Use __packed shorthand

[Joerg Roedel]

From: Borislav Petkov <bp@alien8.de>

Use the shorthand to make it more readable.

No functional changes.

Signed-off-by: Joerg Roedel <jroedel@suse.de>
---
 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