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

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

On Sat, Aug 10, 2013 at 1:23 AM, Mukesh Rathor <mukesh.rathor@oracle.com> wrote:
> On Fri, 9 Aug 2013 11:25:36 +0100
> George Dunlap <dunlapg@umich.edu> wrote:
>
>> On Wed, Jul 24, 2013 at 2:59 AM, Mukesh Rathor
>> <mukesh.rathor@oracle.com> 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

As far as I can tell, *substantial* differences mostly have to do with
starting in 64-bit mode, and not supporting debug registers at the
moment; and those should easily be able to be put in if() statements.

 -George