Re: [Xen-devel] [V10 PATCH 19/23] PVH xen: vmcs related changes

[George Dunlap <George.Dunlap@xxxxxxxxxxxxx>]

On Mon, Aug 12, 2013 at 11:15 AM, George Dunlap
<George.Dunlap@xxxxxxxxxxxxx> wrote:
> On Sat, Aug 10, 2013 at 1:23 AM, Mukesh Rathor <mukesh.rathor@xxxxxxxxxx> 
> wrote:
>> On Fri, 9 Aug 2013 11:25:36 +0100
>> George Dunlap <dunlapg@xxxxxxxxx> wrote:
>>
>>> On Wed, Jul 24, 2013 at 2:59 AM, Mukesh Rathor
>>> <mukesh.rathor@xxxxxxxxxx> wrote:
>>> > This patch contains vmcs changes related for PVH, mainly creating a
>>> > VMCS for PVH guest.
>> .....
>>> > +     v->arch.hvm_vmx.vmx_realmode = 0;
>>> > +
>>> > +    ept->asr  = pagetable_get_pfn(p2m_get_pagetable(p2m));
>>> > +    __vmwrite(EPT_POINTER, ept_get_eptp(ept));
>>> > +
>>> > +    rdmsrl(MSR_IA32_CR_PAT, host_pat);
>>> > +    __vmwrite(HOST_PAT, host_pat);
>>> > +    __vmwrite(GUEST_PAT, MSR_IA32_CR_PAT_RESET);
>>> > +
>>> > +    /* The paging mode is updated for PVH by
>>> > arch_set_info_guest(). */ +
>>> > +    return 0;
>>> > +}
>>>
>>> The majority of this function seems to be duplicating code in
>>> construct_vmcs(), but in a different order so that it's very difficult
>>> to tell which is which.  Wouldn't it be better to just sprinkle
>>> if(is_pvh_domain()) around consrtuct_vmcs?
>>
>>
>> Nah, just makes the function extremely messy! Other maintainers I
>> consulted with were OK with making it a separate function. The function
>> is mostly orderded by vmx sections in the intel SDM.
>
> Does it?  Messier than the domain building functions where we also do
> a lot of if(is_pvh_domain())'s?
>
> From my analysis, most of the differences are because the HVM code
> allows two ways of doing things (e.g., either HAP or shadow) while you
> only allow one.  These include:
>  - disabling TSC exiting (if in the correct mode, the HVM code would
> also do this)
>  - Disabling invlpg and cr3 load/store exiting (same for HVM in HAP mode)
>  - Unconditionally enabling secondary controls (HVM will enable if present)
>  - Enabling the MSR bitmap (HVM will enable if present)
>  - Updating cpu_based_exec_control directly (HVM has a function that
> switches between this and something required for nested virt)
>  - Unconditionally enabling VPID (HVM will enable it somewhere else if
> appropriate)
>  - &c &c

I should have also said, since you would be calling
pvh_check_requirements() at the beginning of the shared function
anyway, all of these are guaranteed to set things up as required by
PVH.

 -George

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