On Sun, Mar 28, 2021 at 08:38:13AM -0400, Dave Voutila wrote: > abieber@ found the latest 9front release ends up in a boot loop if > hosted on an AMD system. I tracked it down to 9front (oddly) trying to > read the PAT msr prior to writing it. [1] The problem is vmm(4)'s msr > handling for svm injects #GP exceptions into the guest for most msr > reads (since we don't emulate more than a few). > > For those (two? few? dozen?) 9front users of AMD hardware and -current, > can you try the below diff? > > vmm(4)'s vmx msr handlers ignores this instruction and only logs the > rdmsr information if the kernel is built with VMM_DEBUG. vmm(4) will > advance the instruction pointer regardless and it's up to the guest to > deal with any resulting issues. > > The diff syncs the logic between the svm and vmx msr vm-exit handlers by > injecting #GP *ONLY* on attempts to read the SMBASE msr. > > For context, this is the vmx rdmsr handler's (vmx_handle_rdmsr) logic: > > switch (*rcx) { > case MSR_SMBASE: > /* > * 34.15.6.3 - Saving Guest State (SMM) > * > * Unsupported, so inject #GP and return without > * advancing %rip. > */ > ret = vmm_inject_gp(vcpu); > return (ret); > } > > It is *not* a design for emulating PAT access and manipulation by a > guest. > > (As an aside, OpenBSD doesn't bother reading the msr [2] before writing > to it, neither does Linux. Why is 9front special? ¯\_(ツ)_/¯) > > -Dave > > [1] https://code.9front.org/hg/plan9front/rev/10cd3e23a8c1 > [2] > https://github.com/openbsd/src/blob/36fd90dcf1acf2ddb4ef5dbabe5313b3a8d46ee2/sys/arch/amd64/amd64/cpu.c#L1145-L1168 > > > Index: sys/arch/amd64/amd64/vmm.c > =================================================================== > RCS file: /cvs/src/sys/arch/amd64/amd64/vmm.c,v > retrieving revision 1.278 > diff -u -p -r1.278 vmm.c > --- sys/arch/amd64/amd64/vmm.c 11 Mar 2021 11:16:55 -0000 1.278 > +++ sys/arch/amd64/amd64/vmm.c 28 Mar 2021 00:45:08 -0000 > @@ -6545,10 +6545,16 @@ svm_handle_msr(struct vcpu *vcpu) > *rax = 0; > *rdx = 0; > break; > - default: > - DPRINTF("%s: guest read msr 0x%llx, injecting " > - "#GP\n", __func__, *rcx); > + case MSR_SMBASE: > + /* Unsupported, inject #GP w/o advancing %rip */ > ret = vmm_inject_gp(vcpu); > return (ret); > +#ifdef VMM_DEBUG > + default: > + /* Log the access to identify unknown MSRs */ > + DPRINTF("%s: rdmsr exit, msr=0x%llx, data " > + "returned to guest=0x%llx:0x%llx\n", > + __func__, *rcx, *rdx, *rax); > +#endif /* VMM_DEBUG */ > } > }
I'm not sure this is correct, doesn't this mean that registers will contain whatevever garbage that was in them beforehand, without injecting #GP host does the guest kernel to know the MSR read failed? I was initially concerned as this touches the codepath pd@ fixed last Feb where MSR reads were being passed through to the host, but still I think that injecting the #GP for unsupported MSR reads is right. -Bryan.