Re: [PATCH RFC v12 08/21] pvh: vmx-specific changes

[George Dunlap <george.dunlap@eu.citrix.com>]

On 13/09/13 17:25, George Dunlap wrote:
> Changes:
> * Enforce HAP mode for now
> * Disable exits related to virtual interrupts or emulated APICs
> * Disable changing paging mode
>   - "unrestricted guest" (i.e., real mode for EPT) disabled
>   - write guest EFER disabled
> * Start in 64-bit mode
> * Force TSC mode to be "none"
> * Paging mode update to happen in arch_set_info_guest
>
> Signed-off-by: George Dunlap <george.dunlap@eu.citrix.com>
> Signed-off-by: Mukesh Rathor <mukesh.rathor@oracle.com>
> CC: Jan Beulich <jan.beulich@suse.com>
> CC: Tim Deegan <tim@xen.org>
> CC: Keir Fraser <keir@xen.org>
> ---
>   xen/arch/x86/hvm/vmx/vmcs.c |  140 +++++++++++++++++++++++++++++++++++++++++--
>   1 file changed, 135 insertions(+), 5 deletions(-)
>
> diff --git a/xen/arch/x86/hvm/vmx/vmcs.c b/xen/arch/x86/hvm/vmx/vmcs.c
> index fa90493..f016343 100644
> --- a/xen/arch/x86/hvm/vmx/vmcs.c
> +++ b/xen/arch/x86/hvm/vmx/vmcs.c
> @@ -828,6 +828,58 @@ void virtual_vmcs_vmwrite(void *vvmcs, u32 vmcs_encoding, u64 val)
>       virtual_vmcs_exit(vvmcs);
>   }
>   
> +static int pvh_check_requirements(struct vcpu *v)
> +{
> +    u64 required, tmpval = real_cr4_to_pv_guest_cr4(mmu_cr4_features);
> +
> +    if ( !paging_mode_hap(v->domain) )
> +    {
> +        printk(XENLOG_G_INFO "HAP is required for PVH guest.\n");
> +        return -EINVAL;
> +    }
> +    if ( !cpu_has_vmx_ept )
> +    {
> +        printk(XENLOG_G_INFO "PVH: CPU does not have EPT support\n");
> +        return -ENOSYS;
> +    }
> +    if ( !cpu_has_vmx_pat )
> +    {
> +        printk(XENLOG_G_INFO "PVH: CPU does not have PAT support\n");
> +        return -ENOSYS;
> +    }
> +    if ( !cpu_has_vmx_msr_bitmap )
> +    {
> +        printk(XENLOG_G_INFO "PVH: CPU does not have msr bitmap\n");
> +        return -ENOSYS;
> +    }
> +    if ( !cpu_has_vmx_secondary_exec_control )
> +    {
> +        printk(XENLOG_G_INFO "CPU Secondary exec is required to run PVH\n");
> +        return -ENOSYS;
> +    }
> +
> +    /*
> +     * If rdtsc exiting is turned on and it goes thru emulate_privileged_op,
> +     * then pv_vcpu.ctrlreg must be added to the pvh struct.
> +     */
> +    if ( v->domain->arch.vtsc )
> +    {
> +        printk(XENLOG_G_INFO
> +               "At present PVH only supports the default timer mode\n");
> +        return -ENOSYS;
> +    }
> +
> +    required = X86_CR4_PAE | X86_CR4_VMXE | X86_CR4_OSFXSR;
> +    if ( (tmpval & required) != required )
> +    {
> +        printk(XENLOG_G_INFO "PVH: required CR4 features not available:%lx\n",
> +               required);
> +        return -ENOSYS;
> +    }
> +
> +    return 0;
> +}
> +
>   static int construct_vmcs(struct vcpu *v)
>   {
>       struct domain *d = v->domain;
> @@ -836,6 +888,13 @@ static int construct_vmcs(struct vcpu *v)
>       u32 vmexit_ctl = vmx_vmexit_control;
>       u32 vmentry_ctl = vmx_vmentry_control;
>   
> +    if ( is_pvh_domain(d) )
> +    {
> +        int rc = pvh_check_requirements(v);
> +        if ( rc )
> +            return rc;
> +    }
> +
>       vmx_vmcs_enter(v);
>   
>       /* VMCS controls. */
> @@ -874,7 +933,44 @@ static int construct_vmcs(struct vcpu *v)
>       /* Do not enable Monitor Trap Flag unless start single step debug */
>       v->arch.hvm_vmx.exec_control &= ~CPU_BASED_MONITOR_TRAP_FLAG;
>   
> +    if ( is_pvh_domain(d) )
> +    {
> +        /* Disable virtual apics, TPR */
> +        v->arch.hvm_vmx.secondary_exec_control &=
> +            ~(SECONDARY_EXEC_VIRTUALIZE_APIC_ACCESSES
> +              | SECONDARY_EXEC_APIC_REGISTER_VIRT
> +              | SECONDARY_EXEC_VIRTUAL_INTR_DELIVERY);
> +        v->arch.hvm_vmx.exec_control &= ~CPU_BASED_TPR_SHADOW;
> +
> +        /* Disable wbinvd (only necessary for MMIO),
> +         * unrestricted guest (real mode for EPT) */
> +        v->arch.hvm_vmx.secondary_exec_control &=
> +            ~(SECONDARY_EXEC_UNRESTRICTED_GUEST
> +              | SECONDARY_EXEC_WBINVD_EXITING);
> +
> +        ASSERT(v->arch.hvm_vmx.exec_control & CPU_BASED_ACTIVATE_SECONDARY_CONTROLS);
> +        ASSERT(v->arch.hvm_vmx.exec_control & CPU_BASED_ACTIVATE_MSR_BITMAP);
> +        ASSERT(!(v->arch.hvm_vmx.exec_control & CPU_BASED_RDTSC_EXITING));
> +
> +        /*
> +         * Note: we run with default VM_ENTRY_LOAD_DEBUG_CTLS of 1, which means
> +         * upon vmentry, the cpu reads/loads VMCS.DR7 and VMCS.DEBUGCTLS, and not
> +         * use the host values. 0 would cause it to not use the VMCS values.
> +         */
> +
> +        /* PVH: I don't think these are necessary */
> +        v->arch.hvm_vmx.exec_control &= ~CPU_BASED_VIRTUAL_NMI_PENDING;
> +        vmentry_ctl &= ~VM_ENTRY_LOAD_GUEST_EFER;
> +        vmentry_ctl &= ~VM_ENTRY_SMM;
> +        vmentry_ctl &= ~VM_ENTRY_DEACT_DUAL_MONITOR;

Mukesh, I couldn't figure out why you had set these.  What are these 
about, and are they really necessary?

> +    /* PVH: Why is ET not set? */
> +    v->arch.hvm_vcpu.guest_cr[0] = is_pvh_domain(d) ?
> +        ( X86_CR0_PG | X86_CR0_NE | X86_CR0_PE | X86_CR0_WP )
> +        : ( X86_CR0_PE | X86_CR0_ET );

I also couldn't figure out why CR0_ET wasn't set for PVH vms; is it not 
set for PV vms?

  -George