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

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

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.

> +
> +        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)

Thanks, Roger.

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