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

From: Corneliu ZUZU <czuzu@bitdefender.com>
To: Jan Beulich <JBeulich@suse.com>
Cc: Kevin Tian <kevin.tian@intel.com>,
	Tamas K Lengyel <tamas@tklengyel.com>,
	Razvan Cojocaru <rcojocaru@bitdefender.com>,
	Andrew Cooper <andrew.cooper3@citrix.com>,
	xen-devel@lists.xen.org, Jun Nakajima <jun.nakajima@intel.com>
Subject: Re: [PATCH 4/7] vm-event/x86: use vm_event_vcpu_enter properly
Date: Thu, 16 Jun 2016 23:20:13 +0300	[thread overview]
Message-ID: <07ba5116-1d76-a6b9-c7a6-c277c26d66b3@bitdefender.com> (raw)
In-Reply-To: <5762DB3702000078000F5BE0@prv-mh.provo.novell.com>

On 6/16/2016 6:00 PM, Jan Beulich wrote:
>>>> On 16.06.16 at 16:09, <czuzu@bitdefender.com> 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@lists.xen.org
http://lists.xen.org/xen-devel