Re: [Xen-devel] [PATCH 4/7] vm-event/x86: use vm_event_vcpu_enter properly

[Corneliu ZUZU <czuzu@xxxxxxxxxxxxxxx>]

On 6/17/2016 10:20 AM, Jan Beulich wrote:
> > > > On 16.06.16 at 22:20, <czuzu@xxxxxxxxxxxxxxx> wrote:
> > On 6/16/2016 6:00 PM, Jan Beulich wrote:
> > > > > > On 16.06.16 at 16:09, <czuzu@xxxxxxxxxxxxxxx> wrote:
> > > > @@ -1432,18 +1430,16 @@ static void vmx_update_guest_cr(struct vcpu *v, 
> > > > unsigned int cr)
> > > >            if ( paging_mode_hap(v->domain) )
> > > >            {
> > > >                /* Manage GUEST_CR3 when CR0.PE=0. */
> > > > +            uint32_t old_ctls = v->arch.hvm_vmx.exec_control;
> > > >                uint32_t cr3_ctls = (CPU_BASED_CR3_LOAD_EXITING |
> > > >                                     CPU_BASED_CR3_STORE_EXITING);
> > > > +
> > > >                v->arch.hvm_vmx.exec_control &= ~cr3_ctls;
> > > >                if ( !hvm_paging_enabled(v) && !vmx_unrestricted_guest(v) )
> > > >                    v->arch.hvm_vmx.exec_control |= cr3_ctls;
> > > >    
> > > > -            /* Trap CR3 updates if CR3 memory events are enabled. */
> > > > -            if ( v->domain->arch.monitor.write_ctrlreg_enabled &
> > > > -                 monitor_ctrlreg_bitmask(VM_EVENT_X86_CR3) )
> > > > -                v->arch.hvm_vmx.exec_control |= CPU_BASED_CR3_LOAD_EXITING;
> > > > -
> > > > -            vmx_update_cpu_exec_control(v);
> > > > +            if ( old_ctls != v->arch.hvm_vmx.exec_control )
> > > > +                vmx_update_cpu_exec_control(v);
> > > >            }
> > > How does this match up with the rest of this patch?
> > And by 'this' you mean slightly optimizing this sequence by adding in
> > old_ctls?
> > It seems pretty straight-forward to me, I figured if I am to move the
> > monitor.write_ctrlreg_enabled part from here
> > it wouldn't be much of a stretch to also do this little
> > optimization...what would have been appropriate?
> > To do this in a separate patch? To mention it in the commit message?
> At least the latter, and perhaps better the former. Without even
> mentioning it the readers (reviewers) have to guess whether this
> is an integral part of the change, or - as you now confirm - just a
> minor optimization done along the road.
>
> Jan
Ack, will split in separate patch in v2.
You're right, I've got to be more attentive to always separate unrelated 
code changes, however minor they are :)
Thanks,
Corneliu.

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