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

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 07/11/13 14:50, George Dunlap wrote:
> 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.

As far as this exiting goes, Paul and I looked at it and considered it
bogus in context.  We have turned it off in XenServer trunk and are
waiting for XenRT to test it thoroughly before formally upstreaming the
change.

A partner has indicated that it leads to an order of magnitude
performance degradation for 64bit windows which appears to rewrite
GS_BASE on every context switch.

~Andrew

>   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
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel