Re: [PATCH v14 07/17] pvh: vmx-specific changes

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

On 07/11/13 00:27, Tim Deegan wrote:
> At 12:14 +0000 on 04 Nov (1383563696), George Dunlap wrote:
>> +    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);
> WBINVD exiting is used for supporting _real_ MMIO, which PVH guetst
> will still have, right?
>
>> +        if ( is_pvh_domain(d) )
>> +            vmx_disable_intercept_for_msr(v, MSR_SHADOW_GS_BASE, MSR_TYPE_R | MSR_TYPE_W);
>> +
>> +        /*
>> +         * PVH: We don't disable intercepts for MSRs: MSR_STAR, MSR_LSTAR,
>> +         *      MSR_CSTAR, and MSR_SYSCALL_MASK because we need to specify
>> +         *      save/restore area to save/restore at every VM exit and entry.
>> +         *      Instead, let the intercept functions save them into
>> +         *      vmx_msr_state fields. See comment in vmx_restore_host_msrs().
>> +         *      See also vmx_restore_guest_msrs().
>> +         */
> Why are these MSRs special for PVH guests?  Are PVH guests restricted
> in how they can use SHADOW_GS?

Your real question is, why is GS_BASE *less* restricted for PVH mode: in 
HVM mode (as far as I can tell), we exit on accesses to 
MSR_SHADOW_GS_BASE.  It looks like the others are trapped because 
updating them is rare and saving / restoring them on every context 
switch would be expensive.  But according to a comment in vmx.c:

     /*
      * We cannot cache SHADOW_GS_BASE while the VCPU runs, as it can
      * be updated at any time via SWAPGS, which we cannot trap.
      */

So SHADOW_GS_BASE is read and written on every context switch.

Is it OK for PVH not to exit here?  If so, do we actually need to do it 
in HVM mode, or is that an artifact of doing things differently once 
upon a time?

FWIW, at the moment, it looks like the trap for SHADOW_GS_BASE is 
pointless for HVM as well -- all the handler does is pass through the 
read or write without doing anything else -- not even updating 
v->arch.hvm_vmx.shadow_gs.  SHADOW_GS_BASE is saved & restored 
unconditionally on a context switch, so I think we probably could just 
stop intercepting it.

Or, for this series, I think I'll take out the special case, and 
separately send a patch to disable the intercept for SHADOW_GS_BASE for 
all HVM domains.

  -George

  -George