On Mon, May 01, 2017 at 10:08:28PM +0200, Mark Kettenis wrote:
> > Date: Mon, 1 May 2017 12:35:32 -0700
> > From: Mike Larkin <mlar...@azathoth.net>
> >
> > Yes, the list is locked down to those listed in the diff (those are the
> > only
> > ones in the permissions bitmap anyway).
> >
> > I don't think there are any bits we should not allow touching via
> > this mechanism. The guest can already do that itself (since we
> > whitelist these MSRs in the bitmap). If we find there is a bit, say,
> > in EFER, that we need to lock down, then we need to do that
> > differently (there will be more changes than just this diff). I
> > think this can go in and we can address that potential issue later
> > (but I don't think there is an issue).
> >
> > One thing that will also need to be addressed (later) is the
> > handling of FPU state, which was recently improved. We can choose to
> > either just flush the state and set CR0_TS on the target after
> > receive, or try to figure out all the state that needs to be sent
> > over, and transfer that. Of course, there's always the possibility
> > that we are sending to a machine with less FPU capability (or more)
> > than the source, and we'll need to improve checking there as well
> > (that probably amounts to transferring %xcr0 and checking that what
> > the "sending side" wants is a subset of what the "receiving side"
> > can accept).
>
> I fear that you'll have to make sure that vmd on the new machine masks
> off any functionality not offered by the old machine and refuse a
> transfer if the capability set of the new machine isn't a strict
> superset of the old machine.
>
Yes, FPU is just the tip of the iceberg here. More stuff for these guys
to implement :)
> > I'll work with Pratik and the rest of the team to clean that up later.
> >
> > Other than the reserved bits question, any other thoughts/ ok?
>
> Nope. Go ahead.
>
Thanks, I'll commit it tonight.
-ml
> > > > > ===================================================================
> > > > > RCS file: /home/pdvyas/cvs/src/sys/arch/amd64/amd64/vmm.c,v
> > > > > retrieving revision 1.137
> > > > > diff -u -p -a -u -r1.137 vmm.c
> > > > > --- sys/arch/amd64/amd64/vmm.c 28 Apr 2017 10:09:37 -0000
> > > > > 1.137
> > > > > +++ sys/arch/amd64/amd64/vmm.c 30 Apr 2017 00:01:21 -0000
> > > > > @@ -1334,7 +1334,9 @@ vcpu_readregs_vmx(struct vcpu *vcpu, uin
> > > > > uint64_t sel, limit, ar;
> > > > > uint64_t *gprs = vrs->vrs_gprs;
> > > > > uint64_t *crs = vrs->vrs_crs;
> > > > > + uint64_t *msrs = vrs->vrs_msrs;
> > > > > struct vcpu_segment_info *sregs = vrs->vrs_sregs;
> > > > > + struct vmx_msr_store *msr_store;
> > > > >
> > > > > if (vcpu_reload_vmcs_vmx(&vcpu->vc_control_pa))
> > > > > return (EINVAL);
> > > > > @@ -1402,6 +1404,14 @@ vcpu_readregs_vmx(struct vcpu *vcpu, uin
> > > > > goto errout;
> > > > > }
> > > > >
> > > > > + msr_store = (struct vmx_msr_store
> > > > > *)vcpu->vc_vmx_msr_exit_save_va;
> > > > > +
> > > > > + if (regmask & VM_RWREGS_MSRS) {
> > > > > + for (i = 0; i < VCPU_REGS_NMSRS; i++) {
> > > > > + msrs[i] = msr_store[i].vms_data;
> > > > > + }
> > > > > + }
> > > > > +
> > > > > goto out;
> > > > >
> > > > > errout:
> > > > > @@ -1448,7 +1458,9 @@ vcpu_writeregs_vmx(struct vcpu *vcpu, ui
> > > > > uint64_t limit, ar;
> > > > > uint64_t *gprs = vrs->vrs_gprs;
> > > > > uint64_t *crs = vrs->vrs_crs;
> > > > > + uint64_t *msrs = vrs->vrs_msrs;
> > > > > struct vcpu_segment_info *sregs = vrs->vrs_sregs;
> > > > > + struct vmx_msr_store *msr_store;
> > > > >
> > > > > if (loadvmcs) {
> > > > > if (vcpu_reload_vmcs_vmx(&vcpu->vc_control_pa))
> > > > > @@ -1518,6 +1530,14 @@ vcpu_writeregs_vmx(struct vcpu *vcpu, ui
> > > > > goto errout;
> > > > > }
> > > > >
> > > > > + msr_store = (struct vmx_msr_store
> > > > > *)vcpu->vc_vmx_msr_exit_save_va;
> > > > > +
> > > > > + if (regmask & VM_RWREGS_MSRS) {
> > > > > + for (i = 0; i < VCPU_REGS_NMSRS; i++) {
> > > > > + msr_store[i].vms_data = msrs[i];
> > > > > + }
> > > > > + }
> > > > > +
> > > > > goto out;
> > > > >
> > > > > errout:
> > > > > @@ -2128,7 +2148,7 @@ vcpu_reset_regs_vmx(struct vcpu *vcpu, s
> > > > > * IA32_VMX_LOAD_DEBUG_CONTROLS
> > > > > * IA32_VMX_LOAD_IA32_PERF_GLOBAL_CTRL_ON_ENTRY
> > > > > */
> > > > > - if (ug == 1)
> > > > > + if (ug == 1 && !(vrs->vrs_msrs[VCPU_REGS_EFER] & EFER_LMA))
> > > > > want1 = 0;
> > > > > else
> > > > > want1 = IA32_VMX_IA32E_MODE_GUEST;
> > > > > @@ -2277,26 +2297,12 @@ vcpu_reset_regs_vmx(struct vcpu *vcpu, s
> > > > > */
> > > > > msr_store = (struct vmx_msr_store
> > > > > *)vcpu->vc_vmx_msr_exit_save_va;
> > > > >
> > > > > - /*
> > > > > - * Make sure LME is enabled in EFER if restricted guest mode is
> > > > > - * needed.
> > > > > - */
> > > > > - msr_store[0].vms_index = MSR_EFER;
> > > > > - if (ug == 1)
> > > > > - msr_store[0].vms_data = 0ULL; /* Initial value */
> > > > > - else
> > > > > - msr_store[0].vms_data = EFER_LME;
> > > > > -
> > > > > - msr_store[1].vms_index = MSR_STAR;
> > > > > - msr_store[1].vms_data = 0ULL; /* Initial value */
> > > > > - msr_store[2].vms_index = MSR_LSTAR;
> > > > > - msr_store[2].vms_data = 0ULL; /* Initial value */
> > > > > - msr_store[3].vms_index = MSR_CSTAR;
> > > > > - msr_store[3].vms_data = 0ULL; /* Initial value */
> > > > > - msr_store[4].vms_index = MSR_SFMASK;
> > > > > - msr_store[4].vms_data = 0ULL; /* Initial value */
> > > > > - msr_store[5].vms_index = MSR_KERNELGSBASE;
> > > > > - msr_store[5].vms_data = 0ULL; /* Initial value */
> > > > > + msr_store[VCPU_REGS_EFER].vms_index = MSR_EFER;
> > > > > + msr_store[VCPU_REGS_STAR].vms_index = MSR_STAR;
> > > > > + msr_store[VCPU_REGS_LSTAR].vms_index = MSR_LSTAR;
> > > > > + msr_store[VCPU_REGS_CSTAR].vms_index = MSR_CSTAR;
> > > > > + msr_store[VCPU_REGS_SFMASK].vms_index = MSR_SFMASK;
> > > > > + msr_store[VCPU_REGS_KGSBASE].vms_index = MSR_KERNELGSBASE;
> > > > >
> > > > > /*
> > > > > * Currently we have the same count of entry/exit MSRs
> > > > > loads/stores
> > > > > @@ -2359,6 +2365,13 @@ vcpu_reset_regs_vmx(struct vcpu *vcpu, s
> > > > > ret = vcpu_writeregs_vmx(vcpu, VM_RWREGS_ALL, 0, vrs);
> > > > >
> > > > > /*
> > > > > + * Make sure LME is enabled in EFER if restricted guest mode is
> > > > > + * needed.
> > > > > + */
> > > > > + if (ug == 0)
> > > > > + msr_store[VCPU_REGS_EFER].vms_data |= EFER_LME;
> > > > > +
> > > > > + /*
> > > > > * Set up the MSR bitmap
> > > > > */
> > > > > memset((uint8_t *)vcpu->vc_msr_bitmap_va, 0xFF, PAGE_SIZE);
> > > > > @@ -4317,7 +4330,7 @@ vmx_handle_cr0_write(struct vcpu *vcpu,
> > > > > return
> > > > > (EINVAL);
> > > > > }
> > > > >
> > > > > - if (msr_store[0].vms_data & EFER_LME)
> > > > > + if (msr_store[VCPU_REGS_EFER].vms_data & EFER_LME)
> > > > > ectls |= IA32_VMX_IA32E_MODE_GUEST;
> > > > > else
> > > > > ectls &= ~IA32_VMX_IA32E_MODE_GUEST;
> > > > > Index: sys/arch/amd64/include/vmmvar.h
> > > > > ===================================================================
> > > > > RCS file: /home/pdvyas/cvs/src/sys/arch/amd64/include/vmmvar.h,v
> > > > > retrieving revision 1.35
> > > > > diff -u -p -a -u -r1.35 vmmvar.h
> > > > > --- sys/arch/amd64/include/vmmvar.h 28 Apr 2017 07:44:36 -0000
> > > > > 1.35
> > > > > +++ sys/arch/amd64/include/vmmvar.h 30 Apr 2017 00:01:21 -0000
> > > > > @@ -340,9 +340,18 @@ struct vcpu_segment_info {
> > > > > #define VCPU_REGS_TR 7
> > > > > #define VCPU_REGS_NSREGS (VCPU_REGS_TR + 1)
> > > > >
> > > > > +#define VCPU_REGS_EFER 0
> > > > > +#define VCPU_REGS_STAR 1
> > > > > +#define VCPU_REGS_LSTAR 2
> > > > > +#define VCPU_REGS_CSTAR 3
> > > > > +#define VCPU_REGS_SFMASK 4
> > > > > +#define VCPU_REGS_KGSBASE 5
> > > > > +#define VCPU_REGS_NMSRS (VCPU_REGS_KGSBASE + 1)
> > > > > +
> > > > > struct vcpu_reg_state {
> > > > > uint64_t vrs_gprs[VCPU_REGS_NGPRS];
> > > > > uint64_t vrs_crs[VCPU_REGS_NCRS];
> > > > > + uint64_t vrs_msrs[VCPU_REGS_NMSRS];
> > > > > struct vcpu_segment_info vrs_sregs[VCPU_REGS_NSREGS];
> > > > > struct vcpu_segment_info vrs_gdtr;
> > > > > struct vcpu_segment_info vrs_idtr;
> > > > > @@ -427,7 +436,9 @@ struct vm_intr_params {
> > > > > #define VM_RWREGS_GPRS 0x1 /* read/write GPRs */
> > > > > #define VM_RWREGS_SREGS 0x2 /* read/write segment registers
> > > > > */
> > > > > #define VM_RWREGS_CRS 0x4 /* read/write CRs */
> > > > > -#define VM_RWREGS_ALL (VM_RWREGS_GPRS | VM_RWREGS_SREGS |
> > > > > VM_RWREGS_CRS)
> > > > > +#define VM_RWREGS_MSRS 0x8 /* read/write MSRs */
> > > > > +#define VM_RWREGS_ALL (VM_RWREGS_GPRS | VM_RWREGS_SREGS |
> > > > > VM_RWREGS_CRS | \
> > > > > + VM_RWREGS_MSRS)
> > > > >
> > > > > struct vm_rwregs_params {
> > > > > uint32_t vrwp_vm_id;
> > > > > Index: usr.sbin/vmd/vm.c
> > > > > ===================================================================
> > > > > RCS file: /home/pdvyas/cvs/src/usr.sbin/vmd/vm.c,v
> > > > > retrieving revision 1.13
> > > > > diff -u -p -a -u -r1.13 vm.c
> > > > > --- usr.sbin/vmd/vm.c 25 Apr 2017 06:44:35 -0000 1.13
> > > > > +++ usr.sbin/vmd/vm.c 30 Apr 2017 00:01:21 -0000
> > > > > @@ -133,6 +133,12 @@ static const struct vcpu_reg_state vcpu_
> > > > > .vrs_idtr = { 0x0, 0xFFFF, 0x0, 0x0},
> > > > > .vrs_sregs[VCPU_REGS_LDTR] = { 0x0, 0xFFFF, 0x0082, 0x0},
> > > > > .vrs_sregs[VCPU_REGS_TR] = { 0x0, 0xFFFF, 0x008B, 0x0},
> > > > > + .vrs_msrs[VCPU_REGS_EFER] = 0ULL,
> > > > > + .vrs_msrs[VCPU_REGS_STAR] = 0ULL,
> > > > > + .vrs_msrs[VCPU_REGS_LSTAR] = 0ULL,
> > > > > + .vrs_msrs[VCPU_REGS_CSTAR] = 0ULL,
> > > > > + .vrs_msrs[VCPU_REGS_SFMASK] = 0ULL,
> > > > > + .vrs_msrs[VCPU_REGS_KGSBASE] = 0ULL
> > > > > };
> > > > >
> > > > > /*
> > > > > @@ -161,6 +167,12 @@ static const struct vcpu_reg_state vcpu_
> > > > > .vrs_idtr = { 0x0, 0xFFFF, 0x0, 0x0},
> > > > > .vrs_sregs[VCPU_REGS_LDTR] = { 0x0, 0xFFFF, 0x0082, 0x0},
> > > > > .vrs_sregs[VCPU_REGS_TR] = { 0x0, 0xFFFF, 0x008B, 0x0},
> > > > > + .vrs_msrs[VCPU_REGS_EFER] = 0ULL,
> > > > > + .vrs_msrs[VCPU_REGS_STAR] = 0ULL,
> > > > > + .vrs_msrs[VCPU_REGS_LSTAR] = 0ULL,
> > > > > + .vrs_msrs[VCPU_REGS_CSTAR] = 0ULL,
> > > > > + .vrs_msrs[VCPU_REGS_SFMASK] = 0ULL,
> > > > > + .vrs_msrs[VCPU_REGS_KGSBASE] = 0ULL
> > > > > };
> > > > >
> > > > > /*
> > > > >
> > > >
> > > >
> >
