[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Xen-devel] [V11 PATCH 17/21] PVH xen: vmcs related changes



On Sat, Aug 24, 2013 at 1:26 AM, Mukesh Rathor <mukesh.rathor@xxxxxxxxxx> wrote:
> On Fri, 23 Aug 2013 09:41:55 +0100
> "Jan Beulich" <JBeulich@xxxxxxxx> wrote:
>
>> >>> On 23.08.13 at 03:19, Mukesh Rathor <mukesh.rathor@xxxxxxxxxx>
>> >>> wrote:
>> > This patch contains vmcs changes related for PVH, mainly creating a
>> > VMCS for PVH guest.
>> >
>> > Changes in V11:
>> >    - Remove pvh_construct_vmcs and make it part of construct_vmcs
>> >
>> > -    v->arch.hvm_vmx.secondary_exec_control &=
>> > ~SECONDARY_EXEC_ENABLE_VPID;
>> > +        /* Intel SDM: resvd bits are 0 */
>> > +        v->arch.hvm_vmx.secondary_exec_control = bits;
>> > +    }
>> > +    else
>> > +    {
>> > +        v->arch.hvm_vmx.secondary_exec_control =
>> > vmx_secondary_exec_control; +
>> > +        /* Disable VPID for now: we decide when to enable it on
>> > VMENTER. */
>> > +        v->arch.hvm_vmx.secondary_exec_control &=
>> > ~SECONDARY_EXEC_ENABLE_VPID;
>> > +    }
>>
>> So this difference in VPID handling needs some explanation, as I
>> don't immediately see how you adjust the code referred to by the
>> non-PVH comment above.
>
>         /* Phase I PVH: we run with minimal secondary exec features */
>         u32 bits = SECONDARY_EXEC_ENABLE_EPT | SECONDARY_EXEC_ENABLE_VPID;
>
> Somehow I had concluded it was harmless to have it that way, but I
> can't remember now. Since, we have common function now, it could just
> do the HVM way too, ie, disabled here since we've already checked
> cpu_has_vmx_vpid in pvh_check_requirements.
>
>> >      if ( paging_mode_hap(d) )
>> >      {
>> >          v->arch.hvm_vmx.exec_control &= ~(CPU_BASED_INVLPG_EXITING
>> > | CPU_BASED_CR3_LOAD_EXITING |
>> >                                            CPU_BASED_CR3_STORE_EXITING);
>> > +        if ( is_pvh_vcpu(v) )
>> > +        {
>> > +            u32 bits = CPU_BASED_ACTIVATE_SECONDARY_CONTROLS |
>> > +                       CPU_BASED_ACTIVATE_MSR_BITMAP;
>> > +            v->arch.hvm_vmx.exec_control |= bits;
>> > +
>> > +            bits = CPU_BASED_USE_TSC_OFFSETING |
>> > CPU_BASED_TPR_SHADOW |
>> > +                   CPU_BASED_VIRTUAL_NMI_PENDING;
>> > +            v->arch.hvm_vmx.exec_control &= ~bits;
>> > +        }
>>
>> Did you notice that the original adjustments were truly HAP-related?
>> Putting your PVH code here just because it takes HAP as a prereq is
>> not the way to go. Wherever the flags you want set get set for the
>> HVM case, you should add your adjustments (if any are necessary
>> at all).
>
> Ok. They, the adjustments, are necessary in Phase I.
>
>> This is misplaced too - we're past the point of determining the set of
>> flags, and already in the process of committing them. The code
>> again should go alongside where the corresponding HVM code sits.
>>
>> > -    if ( cpu_has_vmx_ple )
>> > +    if ( cpu_has_vmx_ple && !is_pvh_vcpu(v) )
>> >      {
>> >          __vmwrite(PLE_GAP, ple_gap);
>> >          __vmwrite(PLE_WINDOW, ple_window);
>>
>> Why would this be conditional upon !PVH?
>
> We don't have intercept for PLE right now for PVH in phase I.
> Easy to add tho.

Or, you could just use the pre-existing VMX exits.  That's what I've
done in my port.

>> > @@ -1032,16 +1150,36 @@ static int construct_vmcs(struct vcpu *v)
>> >
>> >      v->arch.hvm_vmx.exception_bitmap = HVM_TRAP_MASK
>> >                | (paging_mode_hap(d) ? 0 : (1U << TRAP_page_fault))
>> > +              | (is_pvh_vcpu(v) ? (1U << TRAP_int3) | (1U <<
>> > TRAP_debug) : 0) | (1U << TRAP_no_device);
>>
>> What's so special about PVH that it requires these extra intercepts?
>
> HVM does the same in vmx_update_debug_state() called from
> vmx_update_guest_cr() which we don't call for cr0. We need the
> two for gdbsx and kdb, and they are harmless too as we inject back into
> the guest if we don't own the exception, jfyi.

What you mean is, when an HVM guest sets the cr0 to come out of real
mode, these will be set from vmx_update_debug_state().  Since PVH
never goes through that transition, we need to set them at
start-of-day.  Is that right?

It seems like it would be better to just call vmx_update_debug_state()
directly, with a comment about the lack of real-mode transition.

 -George

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel


 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.