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

[Corneliu ZUZU <czuzu@xxxxxxxxxxxxxxx>]

On 6/16/2016 6:00 PM, Jan Beulich wrote:
> > > > On 16.06.16 at 16:09, <czuzu@xxxxxxxxxxxxxxx> wrote:
> > @@ -2199,7 +2153,9 @@ int hvm_set_cr0(unsigned long value, bool_t may_defer)
> >   
> >           if ( hvm_event_crX(CR0, value, old_value) )
> >           {
> > -            /* The actual write will occur in hvm_do_resume(), if permitted. */
> > +            /* The actual write will occur in vcpu_enter_write_data(), if
> > +             * permitted.
> > +             */
> Coding style.
Ack.

> > @@ -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?

> > @@ -179,6 +182,105 @@ void vm_event_fill_regs(vm_event_request_t *req)
> >       req->data.regs.x86.cs_arbytes = seg.attr.bytes;
> >   }
> >   
> > +static inline void vcpu_enter_write_data(struct vcpu *v)
> Please allow the compiler to decide whether to inline such larger
> functions.
Ack.

> > +void arch_vm_event_vcpu_enter(struct vcpu *v)
> > +{
> > +    /* vmx only */
> > +    ASSERT( cpu_has_vmx );
> Stray blanks.
>
> Jan
Ack.

Corneliu.

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