Bryan Steele writes:
> 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?
Yes, whatever was in rax/rdx should still be there. Writes to the PAT
MSR are just ignored.
>
> 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.
Even with my diff, the rdmsr isn't passed through or emulated. It's
effectively ignored like vmm(4) does with vmx hosts.
Looking at pd's commit [1] and the 9front code causing this, I see a few
alternatives to my diff:
1. Mask the PAT cpuid feature so guests see it as not supported.
2. Allow reads of the host PAT (but not writes) by setting the msr as
readable in the msr permissions bitmap which I believe also removes
the vm-exit event.
3. Do nothing different...9front is the one being odd!
Option 2 feels logical to me as I haven't found any reason why a guest
knowing the host's PAT would be an issue.
The secondary issue this raises is vmm(4)'s msr handling is different
between vmx and svm. That should be reconciled. The vmx code doesn't add
the PAT MSR to the bitmap in any capacity, so guests like 9front that
read PAT on vmx hosts are getting "garbage" like my diff would provide.
-Dave
[1]
https://cvsweb.openbsd.org/cgi-bin/cvsweb/src/sys/arch/amd64/amd64/vmm.c.diff?r1=1.262&r2=1.263
