Re: [PATCH RFC v13 06/20] pvh: vmx-specific changes

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

On 26/09/13 16:29, Jan Beulich wrote:
>>>> On 23.09.13 at 18:49, George Dunlap <george.dunlap@eu.citrix.com> 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>
>> ---
>> v13:
>>   - Fix up default cr0 settings
>>   - Get rid of some unnecessary PVH-related changes
>>   - Return EOPNOTSUPP instead of ENOSYS if hardware features are not present
>>   - Remove an unnecessary variable from pvh_check_requirements
>> CC: Jan Beulich <jbeulich@suse.com>
>> CC: Tim Deegan <tim@xen.org>
>> CC: Keir Fraser <keir@xen.org>
>> ---
>>   xen/arch/x86/hvm/vmx/vmcs.c |  130
>> +++++++++++++++++++++++++++++++++++++++++--
>>   1 file changed, 126 insertions(+), 4 deletions(-)
>>
>> diff --git a/xen/arch/x86/hvm/vmx/vmcs.c b/xen/arch/x86/hvm/vmx/vmcs.c
>> index cf54d18..53fccdf 100644
>> --- a/xen/arch/x86/hvm/vmx/vmcs.c
>> +++ b/xen/arch/x86/hvm/vmx/vmcs.c
>> @@ -828,6 +828,60 @@ 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;
>> +
>> +    /* Check for required hardware features */
>> +    if ( !cpu_has_vmx_ept )
>> +    {
>> +        printk(XENLOG_G_INFO "PVH: CPU does not have EPT support\n");
>> +        return -EOPNOTSUPP;
>> +    }
>> +    if ( !cpu_has_vmx_pat )
>> +    {
>> +        printk(XENLOG_G_INFO "PVH: CPU does not have PAT support\n");
>> +        return -EOPNOTSUPP;
>> +    }
>> +    if ( !cpu_has_vmx_msr_bitmap )
>> +    {
>> +        printk(XENLOG_G_INFO "PVH: CPU does not have msr bitmap\n");
>> +        return -EOPNOTSUPP;
>> +    }
>> +    if ( !cpu_has_vmx_secondary_exec_control )
>> +    {
>> +        printk(XENLOG_G_INFO "CPU Secondary exec is required to run PVH\n");
>> +        return -EOPNOTSUPP;
>> +    }
> Up to here the checks are VMX specific, and hence belong in a VMX
> specific file, ...
>
>> +    required = X86_CR4_PAE | X86_CR4_VMXE | X86_CR4_OSFXSR;
>> +    if ( (real_cr4_to_pv_guest_cr4(mmu_cr4_features) & required) != required )
>> +    {
>> +        printk(XENLOG_G_INFO "PVH: required CR4 features not available:%lx\n",
>> +               required);
>> +        return -EOPNOTSUPP;
>> +    }
>> +
>> +    /* Check for required configuration options */
>> +    if ( !paging_mode_hap(v->domain) )
>> +    {
>> +        printk(XENLOG_G_INFO "HAP is required for PVH guest.\n");
>> +        return -EINVAL;
>> +    }
>> +    /*
>> +     * 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 -EINVAL;
>> +    }
> ... but all of these are pretty generic (apart from the X86_CR4_VMXE
> in the CR4 mask checked above, but I wonder whether that
> shouldn't be checked much earlier - for HVM guests no such check
> exists here afaik).

The hap check can be removed and checked in hvm_domain_initialise(), and 
the vtsc one moved to tsc_set_info().

Is it really necessary to check these bits in cr4?  Or is this more or 
less an ASSERT() to make sure people haven't accidentally disabled these 
in the real->guest cr4 conversion?

  -George