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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 10.08.13 at 02:23, 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.

But I'm sure you also appreciate the point George makes: The less
code duplication, the easier maintenance will end up to be. So it
really much depends on the number of if()-s needed in the earlier
function. And if we're two go with two instances, then if you re-
ordered things from the original for a good reason, including a patch
to also re-order the original function (so they become more similar
again) would be appreciated. Plus (if you didn't already) include a
comment in both functions referring to the other (so people
updating one have a fair chance of noticing that the other may
need updating too).

Jan


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