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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 24.08.13 at 02:26, 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:
>> > -    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.

As pointed out before - any such temporary things need to be
properly annotated, so they can be understood by readers and
found when looking for remaining ones.

>> > +        vmx_disable_intercept_for_msr(v, MSR_IA32_SYSENTER_CS,
>> > msr_type);
>> > +        vmx_disable_intercept_for_msr(v, MSR_IA32_SYSENTER_ESP,
>> > msr_type);
>> > +        vmx_disable_intercept_for_msr(v, MSR_IA32_SYSENTER_EIP,
>> > msr_type); +
>> >          if ( cpu_has_vmx_pat && paging_mode_hap(d) )
>> > -            vmx_disable_intercept_for_msr(v, MSR_IA32_CR_PAT,
>> > MSR_TYPE_R | MSR_TYPE_W);
>> > +            vmx_disable_intercept_for_msr(v, MSR_IA32_CR_PAT,
>> > msr_type);
>> 
>> These changes all look pointless; if you really think they're
>> worthwhile cleanup, they don't belong here.
> 
> They violate coding style (lines longer than 80). but anyways..

I wouldn't mind such clean up if you needed to touch the lines
anyway. But in a series that's large and has been causing quite
a bit of discussion pure reformatting should be avoided unless
done in a separate patch.

>> So you set hw_cr[0] while I saw you saying hw_cr[4] won't be
>> touched by PVH in a reply to the v10 series. Such
>> inconsistencies are just calling for subsequent bugs. Either PVH
>> set all valid hw_cr[] fields, or it clearly states somewhere that
>> none of them are used (and then the assignment above ought to
>> go away).
> 
> With commonising PVH with HVM, hw_cr gets used. Otherwise the intention
> was not to use it. I don't see a huge problem at this point with this.

The problem is the inconsistency: Maintenance of this code will be
more difficult if things aren't done in a half way predictable way.
And inconsistency is never predictable.

Jan


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