Index | Thread | Search

From:
Hans-Jörg Höxer <Hans-Joerg_Hoexer@genua.de>
Subject:
Re: vmm(4) AMD SEV
To:
Dave Voutila <dv@sisu.io>
Cc:
Alexander Bluhm <bluhm@openbsd.org>, <tech@openbsd.org>, <Hans-Joerg_Hoexer@genua.de>
Date:
Mon, 26 Aug 2024 10:55:02 +0200

Download raw body.

Thread
Hi,

thanks for the feedback!

On Fri, Aug 23, 2024 at 03:06:42PM -0400, Dave Voutila wrote:
> > +int vcpu_init_vmx(struct vcpu *, struct vm_create_params *);
> > +int vcpu_init_svm(struct vcpu *, struct vm_create_params *);
> 
> I don't see a benefit in changing the signature of vcpu_init_vmx to keep
> it similar to vcpu_init_svm. Since we're not doing anything new on VMX
> with this diff, I'd rather keep that code unchanged for now until we
> have a reason to do so.

ok.

> > +	/* Inform vmd(8) about ASID and C bit position. */
> > +	vcp->vcp_poscbit = amd64_pos_cbit;
> > +	vcp->vcp_asid[vcpu->vc_parent->vm_vcpu_ct] = vcpu->vc_vpid;
> > +
> 
> This looks odd and looking more closely at vmm, this is a future
> footgun. I'd rather the index be vcpu->vc_id, which semantically makes
> most sense.
> 
> To do this, it looks like prior to vcpu_init() is called in vm_create(),
> you need to set vcpu->vc_id, so moving the lines like:
> 
>     vcpu->vc_id = vm->vm_vcpu_ct;
>     vm->vm_vcpu_ct++;
> 
> to prior to calling vcpu_init(vcpu).

ok!

Updated diff below.
Take care!
-----------------------------------------------------------------
commit 659582f61af2e7b9fe67e83436d5bcae0295f7d8
Author: Hans-Joerg Hoexer <hshoexer@genua.de>
Date:   Thu Jul 11 16:23:00 2024 +0200

    vmm(4): SEV support
    
    This changes enables SEV support in vmm(4):
    
    - emulate cpuid 0x8000001f so the guest can discover SEV features
    - allow vmd(8) to enable SEV on VM creation
    - inform vmd(8) about the c-bit position and ASID assigned to each VCPU

diff --git a/sys/arch/amd64/amd64/identcpu.c b/sys/arch/amd64/amd64/identcpu.c
index a6e31e50255..91d267fff64 100644
--- a/sys/arch/amd64/amd64/identcpu.c
+++ b/sys/arch/amd64/amd64/identcpu.c
@@ -66,7 +66,7 @@ char cpu_model[48];
 int cpuspeed;
 
 int amd64_has_xcrypt;
-int amd64_pos_cbit;
+int amd64_pos_cbit;	/* C bit position for SEV */
 int has_rdrand;
 int has_rdseed;
 
diff --git a/sys/arch/amd64/amd64/vmm_machdep.c b/sys/arch/amd64/amd64/vmm_machdep.c
index 0df3d8b070f..eb6e9258366 100644
--- a/sys/arch/amd64/amd64/vmm_machdep.c
+++ b/sys/arch/amd64/amd64/vmm_machdep.c
@@ -82,9 +82,9 @@ int vcpu_reset_regs(struct vcpu *, struct vcpu_reg_state *);
 int vcpu_reset_regs_vmx(struct vcpu *, struct vcpu_reg_state *);
 int vcpu_reset_regs_svm(struct vcpu *, struct vcpu_reg_state *);
 int vcpu_reload_vmcs_vmx(struct vcpu *);
-int vcpu_init(struct vcpu *);
+int vcpu_init(struct vcpu *, struct vm_create_params *);
 int vcpu_init_vmx(struct vcpu *);
-int vcpu_init_svm(struct vcpu *);
+int vcpu_init_svm(struct vcpu *, struct vm_create_params *);
 int vcpu_run_vmx(struct vcpu *, struct vm_run_params *);
 int vcpu_run_svm(struct vcpu *, struct vm_run_params *);
 void vcpu_deinit(struct vcpu *);
@@ -1890,7 +1890,6 @@ vcpu_reset_regs_svm(struct vcpu *vcpu, struct vcpu_reg_state *vrs)
 {
 	struct vmcb *vmcb;
 	int ret;
-	uint16_t asid;
 
 	vmcb = (struct vmcb *)vcpu->vc_control_va;
 
@@ -1963,14 +1962,7 @@ vcpu_reset_regs_svm(struct vcpu *vcpu, struct vcpu_reg_state *vrs)
 	svm_setmsrbr(vcpu, MSR_PSTATEDEF(0));
 
 	/* Guest VCPU ASID */
-	if (vmm_alloc_vpid(&asid)) {
-		DPRINTF("%s: could not allocate asid\n", __func__);
-		ret = EINVAL;
-		goto exit;
-	}
-
-	vmcb->v_asid = asid;
-	vcpu->vc_vpid = asid;
+	vmcb->v_asid = vcpu->vc_vpid;
 
 	/* TLB Control - First time in, flush all*/
 	vmcb->v_tlb_control = SVM_TLB_CONTROL_FLUSH_ALL;
@@ -1985,9 +1977,13 @@ vcpu_reset_regs_svm(struct vcpu *vcpu, struct vcpu_reg_state *vrs)
             PATENTRY(6, PAT_UCMINUS) | PATENTRY(7, PAT_UC);
 
 	/* NPT */
-	vmcb->v_np_enable = 1;
+	vmcb->v_np_enable = SVM_ENABLE_NP;
 	vmcb->v_n_cr3 = vcpu->vc_parent->vm_map->pmap->pm_pdirpa;
 
+	/* SEV */
+	if (vcpu->vc_sev)
+		vmcb->v_np_enable |= SVM_ENABLE_SEV;
+
 	/* Enable SVME in EFER (must always be set) */
 	vmcb->v_efer |= EFER_SVME;
 
@@ -1998,7 +1994,6 @@ vcpu_reset_regs_svm(struct vcpu *vcpu, struct vcpu_reg_state *vrs)
 
 	vcpu->vc_parent->vm_map->pmap->eptp = 0;
 
-exit:
 	return ret;
 }
 
@@ -3086,6 +3081,7 @@ vcpu_reset_regs(struct vcpu *vcpu, struct vcpu_reg_state *vrs)
  *
  * Parameters:
  *  vcpu: the VCPU structure being initialized
+ *  vcp: parameters provided by vmd(8)
  *
  * Return values:
  *  0: the VCPU was initialized successfully
@@ -3093,8 +3089,9 @@ vcpu_reset_regs(struct vcpu *vcpu, struct vcpu_reg_state *vrs)
  *  EINVAL: an error occurred during VCPU initialization
  */
 int
-vcpu_init_svm(struct vcpu *vcpu)
+vcpu_init_svm(struct vcpu *vcpu, struct vm_create_params *vcp)
 {
+	uint16_t asid;
 	int ret = 0;
 
 	/* Allocate VMCB VA */
@@ -3176,6 +3173,21 @@ vcpu_init_svm(struct vcpu *vcpu)
 	    (uint64_t)vcpu->vc_svm_ioio_va,
 	    (uint64_t)vcpu->vc_svm_ioio_pa);
 
+	/* Guest VCPU ASID */
+	if (vmm_alloc_vpid(&asid)) {
+		DPRINTF("%s: could not allocate asid\n", __func__);
+		ret = EINVAL;
+		goto exit;
+	}
+	vcpu->vc_vpid = asid;
+
+	/* Shall we enable SEV? */
+	vcpu->vc_sev = vcp->vcp_sev;
+
+	/* Inform vmd(8) about ASID and C bit position. */
+	vcp->vcp_poscbit = amd64_pos_cbit;
+	vcp->vcp_asid[vcpu->vc_id] = vcpu->vc_vpid;
+
 exit:
 	if (ret)
 		vcpu_deinit_svm(vcpu);
@@ -3189,7 +3201,7 @@ exit:
  * Calls the architecture-specific VCPU init routine
  */
 int
-vcpu_init(struct vcpu *vcpu)
+vcpu_init(struct vcpu *vcpu, struct vm_create_params *vcp)
 {
 	int ret = 0;
 
@@ -3207,7 +3219,7 @@ vcpu_init(struct vcpu *vcpu)
 	if (vmm_softc->mode == VMM_MODE_EPT)
 		ret = vcpu_init_vmx(vcpu);
 	else if (vmm_softc->mode == VMM_MODE_RVI)
-		ret = vcpu_init_svm(vcpu);
+		ret = vcpu_init_svm(vcpu, vcp);
 	else
 		panic("%s: unknown vmm mode: %d", __func__, vmm_softc->mode);
 
@@ -6285,7 +6297,7 @@ vmm_handle_cpuid(struct vcpu *vcpu)
 		*rdx = 0;
 		break;
 	case 0x80000000:	/* Extended function level */
-		*rax = 0x80000008; /* curcpu()->ci_pnfeatset */
+		*rax = 0x8000001f; /* curcpu()->ci_pnfeatset */
 		*rbx = 0;
 		*rcx = 0;
 		*rdx = 0;
@@ -6345,6 +6357,12 @@ vmm_handle_cpuid(struct vcpu *vcpu)
 		*rcx = ecx;
 		*rdx = edx;
 		break;
+	case 0x8000001f:	/* encryption features (AMD) */
+		*rax = eax;
+		*rbx = ebx;
+		*rcx = ecx;
+		*rdx = edx;
+		break;
 	default:
 		DPRINTF("%s: unsupported rax=0x%llx\n", __func__, *rax);
 		*rax = 0;
diff --git a/sys/arch/amd64/include/cpu.h b/sys/arch/amd64/include/cpu.h
index f5aada4d4d3..67cef9c37b8 100644
--- a/sys/arch/amd64/include/cpu.h
+++ b/sys/arch/amd64/include/cpu.h
@@ -415,6 +415,7 @@ void	x86_print_cacheinfo(struct cpu_info *);
 void	identifycpu(struct cpu_info *);
 int	cpu_amd64speed(int *);
 extern int cpuspeed;
+extern int amd64_pos_cbit;
 
 /* machdep.c */
 void	dumpconf(void);
diff --git a/sys/arch/amd64/include/vmmvar.h b/sys/arch/amd64/include/vmmvar.h
index 3f35e520926..855855ad2a7 100644
--- a/sys/arch/amd64/include/vmmvar.h
+++ b/sys/arch/amd64/include/vmmvar.h
@@ -624,6 +624,7 @@ enum {
 
 /* Forward declarations */
 struct vm;
+struct vm_create_params;
 
 /*
  * Implementation-specific cpu state
@@ -636,6 +637,9 @@ struct vmcb_segment {
 	uint64_t			vs_base;		/* 008h */
 };
 
+#define SVM_ENABLE_NP	(1ULL << 0)
+#define SVM_ENABLE_SEV	(1ULL << 1)
+
 struct vmcb {
 	union {
 		struct {
@@ -893,6 +897,7 @@ struct vcpu {
 	paddr_t vc_svm_hsa_pa;
 	vaddr_t vc_svm_ioio_va;
 	paddr_t vc_svm_ioio_pa;
+	int vc_sev;				/* [I] */
 };
 
 SLIST_HEAD(vcpu_head, vcpu);
@@ -921,7 +926,7 @@ int	vmm_start(void);
 int	vmm_stop(void);
 int	vm_impl_init(struct vm *, struct proc *);
 void	vm_impl_deinit(struct vm *);
-int	vcpu_init(struct vcpu *);
+int	vcpu_init(struct vcpu *, struct vm_create_params *);
 void	vcpu_deinit(struct vcpu *);
 int	vm_rwregs(struct vm_rwregs_params *, int);
 int	vcpu_reset_regs(struct vcpu *, struct vcpu_reg_state *);
diff --git a/sys/dev/vmm/vmm.c b/sys/dev/vmm/vmm.c
index 4d4866f70dc..35c00943302 100644
--- a/sys/dev/vmm/vmm.c
+++ b/sys/dev/vmm/vmm.c
@@ -399,13 +399,13 @@ vm_create(struct vm_create_params *vcp, struct proc *p)
 		vcpu = pool_get(&vcpu_pool, PR_WAITOK | PR_ZERO);
 
 		vcpu->vc_parent = vm;
-		if ((ret = vcpu_init(vcpu)) != 0) {
+		vcpu->vc_id = vm->vm_vcpu_ct;
+		vm->vm_vcpu_ct++;
+		if ((ret = vcpu_init(vcpu, vcp)) != 0) {
 			printf("failed to init vcpu %d for vm %p\n", i, vm);
 			vm_teardown(&vm);
 			return (ret);
 		}
-		vcpu->vc_id = vm->vm_vcpu_ct;
-		vm->vm_vcpu_ct++;
 		/* Publish vcpu to list, inheriting the reference. */
 		SLIST_INSERT_HEAD(&vm->vm_vcpu_list, vcpu, vc_vcpu_link);
 	}
diff --git a/sys/dev/vmm/vmm.h b/sys/dev/vmm/vmm.h
index ac682bc2c45..677885c8fc0 100644
--- a/sys/dev/vmm/vmm.h
+++ b/sys/dev/vmm/vmm.h
@@ -49,9 +49,12 @@ struct vm_create_params {
 	size_t			vcp_ncpus;
 	struct vm_mem_range	vcp_memranges[VMM_MAX_MEM_RANGES];
 	char			vcp_name[VMM_MAX_NAME_LEN];
+	int			vcp_sev;
 
         /* Output parameter from VMM_IOC_CREATE */
         uint32_t		vcp_id;
+        uint32_t		vcp_poscbit;
+        uint32_t		vcp_asid[VMM_MAX_VCPUS];
 };
 
 struct vm_info_result {