Re: [Xen-devel] [PATCH 9/9] x86/vmx: Don't leak EFER.NXE into guest context

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

On Tue, May 22, 2018 at 12:20:46PM +0100, Andrew Cooper wrote:
> Intel hardware only uses 4 bits in MSR_EFER.  Changes to LME and LMA are
> handled automatically via the VMENTRY_CTLS.IA32E_MODE bit.
> 
> SCE is handled by ad-hoc logic in context_switch(), vmx_restore_guest_msrs()
> and vmx_update_guest_efer(), and works by altering the host SCE value to match
> the setting the guest wants.  This works because, in HVM vcpu context, Xen
> never needs to execute a SYSCALL or SYSRET instruction.
> 
> However, NXE has never been context switched.  Unlike SCE, NXE cannot be
> context switched at vcpu boundaries because disabling NXE makes PTE.NX bits
> reserved and cause a pagefault when encountered.  This means that the guest
> always has Xen's setting in effect, irrespective of the bit it can see and
> modify in its virtualised view of MSR_EFER.
> 
> This isn't a major problem for production operating systems because they, like
> Xen, always turn the NXE on when it is available.  However, it does have an
> observable effect on which guest PTE bits are valid, and whether
> PFEC_insn_fetch is visible in a #PF error code.
> 
> Second generation VT-x hardware has host and guest EFER fields in the VMCS,
> and support for loading and saving them automatically.  First generation VT-x
> hardware needs to use MSR load/save lists to cause an atomic switch of
> MSR_EFER on vmentry/exit.
> 
> Therefore we update vmx_init_vmcs_config() to find and use guest/host EFER
> support when available (and MSR load/save lists on older hardware) and drop
> all ad-hoc alteration of SCE.
> 
> There are two complications for shadow guests.  NXE, being a paging setting
> needs to remain under host control, but that is fine as it is also Xen which
> handles the pagefaults.  Also, it turns out that without EPT enabled, hardware
> won't tolerate LME and LMA being different via either the GUEST_EFER VMCS
> setting, or via the guest load list.  This doesn't matter in practice as we
> intercept all writes to CR0 and reads from MSR_EFER, so can provide
> architecturally consistent behaviour from the guests point of view.
> 
> As a result of fixing EFER context switching, we can remove the Intel-special
> case from hvm_nx_enabled() and let guest_walk_tables() work with the real
> guest paging settings.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>

LGTM:

Reviewed-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>

One question below though.

> diff --git a/xen/include/asm-x86/hvm/vmx/vmcs.h 
> b/xen/include/asm-x86/hvm/vmx/vmcs.h
> index cfd174c..6c6897c 100644
> --- a/xen/include/asm-x86/hvm/vmx/vmcs.h
> +++ b/xen/include/asm-x86/hvm/vmx/vmcs.h
> @@ -306,6 +306,8 @@ extern u64 vmx_ept_vpid_cap;
>      (vmx_cpu_based_exec_control & CPU_BASED_MONITOR_TRAP_FLAG)
>  #define cpu_has_vmx_pat \
>      (vmx_vmentry_control & VM_ENTRY_LOAD_GUEST_PAT)
> +#define cpu_has_vmx_efer \
> +    (vmx_vmentry_control & VM_ENTRY_LOAD_GUEST_EFER)

Don't you also need a vmx_vmexit_control & VM_EXIT_SAVE_GUEST_EFER and
vmx_vmexit_control & VM_EXIT_LOAD_HOST_EFER?

Or can the presence of those two be inferred from
VM_ENTRY_LOAD_GUEST_EFER?

Thanks, Roger.

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