From: Dave Voutila Subject: Re: vmm(4) AMD SEV To: Alexander Bluhm Cc: tech@openbsd.org Date: Fri, 23 Aug 2024 15:06:42 -0400 Alexander Bluhm writes: > Hi, > > This is the vmm(4) part of AMD SEV implementation. It allows to > run virtual guests with memory encryption. vmd(8) diff will be > next. Guest implementation has been commited and should already > work. > > I have tested that it does not break non-SEV environments. Full > tests with SEV can be done when all pieces have been finished. > hshoexer@ did all the work and in his environment he is running > OpenBSD guest with AMD SEV on OpenBSD vmm/vmd host. > > This diff 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 > > Note that vmd(8) has to be recompiled with new header files. > > ok? Some feedback below. > > bluhm > > Index: arch/amd64/amd64/identcpu.c > =================================================================== > RCS file: /data/mirror/openbsd/cvs/src/sys/arch/amd64/amd64/identcpu.c,v > diff -u -p -r1.146 identcpu.c > --- arch/amd64/amd64/identcpu.c 8 Jul 2024 14:46:47 -0000 1.146 > +++ arch/amd64/amd64/identcpu.c 23 Aug 2024 18:14:28 -0000 > @@ -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; > > Index: arch/amd64/amd64/vmm_machdep.c > =================================================================== > RCS file: /data/mirror/openbsd/cvs/src/sys/arch/amd64/amd64/vmm_machdep.c,v > diff -u -p -r1.32 vmm_machdep.c > --- arch/amd64/amd64/vmm_machdep.c 22 Aug 2024 04:53:07 -0000 1.32 > +++ arch/amd64/amd64/vmm_machdep.c 23 Aug 2024 18:14:28 -0000 > @@ -82,9 +82,9 @@ int vcpu_reset_regs(struct vcpu *, struc > 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_vmx(struct vcpu *); > -int vcpu_init_svm(struct vcpu *); > +int vcpu_init(struct vcpu *, struct vm_create_params *); > +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. > 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, s > { > 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, s > 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, s > 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, s > > vcpu->vc_parent->vm_map->pmap->eptp = 0; > > -exit: > return ret; > } > > @@ -2823,6 +2818,7 @@ exit: > * > * Parameters: > * vcpu: the VCPU structure being initialized > + * vcp: parameters provided by vmd(8) > * > * Return values: > * 0: the VCPU was initialized successfully > @@ -2830,7 +2826,7 @@ exit: > * EINVAL: an error occurred during VCPU initialization > */ > int > -vcpu_init_vmx(struct vcpu *vcpu) > +vcpu_init_vmx(struct vcpu *vcpu, struct vm_create_params *vcp) > { > struct vmcs *vmcs; > uint64_t msr, eptp; > @@ -3086,6 +3082,7 @@ vcpu_reset_regs(struct vcpu *vcpu, struc > * > * Parameters: > * vcpu: the VCPU structure being initialized > + * vcp: parameters provided by vmd(8) > * > * Return values: > * 0: the VCPU was initialized successfully > @@ -3093,8 +3090,9 @@ vcpu_reset_regs(struct vcpu *vcpu, struc > * 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 +3174,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_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). > exit: > if (ret) > vcpu_deinit_svm(vcpu); > @@ -3189,7 +3202,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; > > @@ -3205,9 +3218,9 @@ vcpu_init(struct vcpu *vcpu) > vcpu->vc_shadow_pat = rdmsr(MSR_CR_PAT); > > if (vmm_softc->mode == VMM_MODE_EPT) > - ret = vcpu_init_vmx(vcpu); > + ret = vcpu_init_vmx(vcpu, vcp); > 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 +6298,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; > @@ -6340,6 +6353,12 @@ vmm_handle_cpuid(struct vcpu *vcpu) > *rdx = edx; > break; > case 0x8000001d: /* cache topology (AMD) */ > + *rax = eax; > + *rbx = ebx; > + *rcx = ecx; > + *rdx = edx; > + break; > + case 0x8000001f: /* encryption features (AMD) */ > *rax = eax; > *rbx = ebx; > *rcx = ecx; > Index: arch/amd64/include/cpu.h > =================================================================== > RCS file: /data/mirror/openbsd/cvs/src/sys/arch/amd64/include/cpu.h,v > diff -u -p -r1.175 cpu.h > --- arch/amd64/include/cpu.h 21 Jul 2024 19:41:31 -0000 1.175 > +++ arch/amd64/include/cpu.h 23 Aug 2024 18:14:28 -0000 > @@ -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); > Index: arch/amd64/include/vmmvar.h > =================================================================== > RCS file: /data/mirror/openbsd/cvs/src/sys/arch/amd64/include/vmmvar.h,v > diff -u -p -r1.104 vmmvar.h > --- arch/amd64/include/vmmvar.h 14 Jul 2024 07:57:42 -0000 1.104 > +++ arch/amd64/include/vmmvar.h 23 Aug 2024 18:14:28 -0000 > @@ -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 *); > Index: dev/vmm/vmm.c > =================================================================== > RCS file: /data/mirror/openbsd/cvs/src/sys/dev/vmm/vmm.c,v > diff -u -p -r1.2 vmm.c > --- dev/vmm/vmm.c 13 May 2023 23:15:28 -0000 1.2 > +++ dev/vmm/vmm.c 23 Aug 2024 18:14:28 -0000 > @@ -399,7 +399,7 @@ vm_create(struct vm_create_params *vcp, > vcpu = pool_get(&vcpu_pool, PR_WAITOK | PR_ZERO); > > vcpu->vc_parent = vm; > - if ((ret = vcpu_init(vcpu)) != 0) { > + if ((ret = vcpu_init(vcpu, vcp)) != 0) { > printf("failed to init vcpu %d for vm %p\n", i, vm); > vm_teardown(&vm); > return (ret); > Index: dev/vmm/vmm.h > =================================================================== > RCS file: /data/mirror/openbsd/cvs/src/sys/dev/vmm/vmm.h,v > diff -u -p -r1.6 vmm.h > --- dev/vmm/vmm.h 10 Jul 2024 10:41:19 -0000 1.6 > +++ dev/vmm/vmm.h 23 Aug 2024 18:14:28 -0000 > @@ -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 {