Re: [Xen-devel] [PATCH 1/6] x86/vmx: Simplfy the default cases in vmx_msr_{read, write}_intercept()

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 27/02/18 12:43, Roger Pau Monné wrote:
> On Mon, Feb 26, 2018 at 05:35:14PM +0000, Andrew Cooper wrote:
>> The default case of vmx_msr_write_intercept() in particular is very tangled.
>>
>> First of all, fold long_mode_do_msr_{read,write}() into their callers.  These
>> functions were split out in the past because of the 32bit build of Xen, but 
>> it
>> is unclear why the cases weren't simply #ifdef'd in place.
>>
>> Next, invert the vmx_write_guest_msr()/is_last_branch_msr() logic to break if
>> the condition is satisfied, rather than nesting if it wasn't.  This allows 
>> the
>> wrmsr_hypervisor_regs() call to be un-nested with respect to the other 
>> default
>> logic.
>>
>> No practical difference from a guests point of view.
> I think there's a difference from guest PoV now, guest wrmsr of
> GS/FS/LSTAR/CSTAR with non-canonical addresses will get a #GP.
>
>> @@ -3090,6 +3015,45 @@ static int vmx_msr_write_intercept(unsigned int msr, 
>> uint64_t msr_content)
>>              goto gp_fault;
>>          __vmwrite(GUEST_SYSENTER_EIP, msr_content);
>>          break;
>> +
>> +    case MSR_FS_BASE:
>> +    case MSR_GS_BASE:
>> +    case MSR_SHADOW_GS_BASE:
>> +        if ( !is_canonical_address(msr_content) )
>> +            goto gp_fault;
> This is AFAICT different from previous behaviour, previous code would
> just return X86EMUL_EXCEPTION without injecting a fault. From the SDM
> I see injecting a #GP is the correct behaviour.

The gp_fault label simply returns X86EMUL_EXCEPTION, but is more
descriptive when named like this.

It is up to the top level to convert EXCEPTION into an
hvm_inject_hw_exception() if it wants, because there is at least one
case in the emulator where we need to squash the exception rather than
propagating it.

Previously, non-canonical addresses on the wrmsr side did have their
EXCEPTION propagated up from long_mode_do_msr_write(), so after careful
re-review, I still think the behaviour is the same.

>
>> +
>> +        if ( msr == MSR_FS_BASE )
>> +            __vmwrite(GUEST_FS_BASE, msr_content);
>> +        else if ( msr == MSR_GS_BASE )
>> +            __vmwrite(GUEST_GS_BASE, msr_content);
>> +        else
>> +            wrmsrl(MSR_SHADOW_GS_BASE, msr_content);
>> +
>> +        break;
>> +
>> +    case MSR_STAR:
>> +        v->arch.hvm_vmx.star = msr_content;
>> +        wrmsrl(MSR_STAR, msr_content);
>> +        break;
>> +
>> +    case MSR_LSTAR:
>> +        if ( !is_canonical_address(msr_content) )
>> +            goto gp_fault;
>> +        v->arch.hvm_vmx.lstar = msr_content;
>> +        wrmsrl(MSR_LSTAR, msr_content);
>> +        break;
>> +
>> +    case MSR_CSTAR:
>> +        if ( !is_canonical_address(msr_content) )
>> +            goto gp_fault;
>> +        v->arch.hvm_vmx.cstar = msr_content;
> Likely a stupid question, but why is CSTAR not written here? (like it's
> done for LSTAR)

CSTAR on Intel is a weird corner case.  The MSR exists and can be
interacted with via RDMSR/WRMSR, but SYSCALL fails with #UD outside of
64bit mode, there is nothing in the pipeline which will make use of the
value.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel