Download raw body.
vmm(4) AMD SEV
Alexander Bluhm <bluhm@openbsd.org> 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 {
vmm(4) AMD SEV