From mboxrd@z Thu Jan  1 00:00:00 1970
From: Razvan Cojocaru <rcojocaru@bitdefender.com>
Subject: Re: [PATCH V5] xen/vm_event: Clean up
 control-register-write vm_events and add XCR0 event
Date: Tue, 26 May 2015 19:21:03 +0300
Message-ID: <55649D6F.6030104@bitdefender.com>
References: <1432542788-9674-1-git-send-email-rcojocaru@bitdefender.com>
	<5564B1B6020000780007DFC9@mail.emea.novell.com>
Mime-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
Return-path: <xen-devel-bounces@lists.xen.org>
In-Reply-To: <5564B1B6020000780007DFC9@mail.emea.novell.com>
List-Unsubscribe: <http://lists.xen.org/cgi-bin/mailman/options/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=unsubscribe>
List-Post: <mailto:xen-devel@lists.xen.org>
List-Help: <mailto:xen-devel-request@lists.xen.org?subject=help>
List-Subscribe: <http://lists.xen.org/cgi-bin/mailman/listinfo/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=subscribe>
Sender: xen-devel-bounces@lists.xen.org
Errors-To: xen-devel-bounces@lists.xen.org
To: Jan Beulich <JBeulich@suse.com>
Cc: tim@xen.org, kevin.tian@intel.com, wei.liu2@citrix.com, eddie.dong@intel.com, stefano.stabellini@eu.citrix.com, andrew.cooper3@citrix.com, ian.jackson@eu.citrix.com, xen-devel@lists.xen.org, jun.nakajima@intel.com, keir@xen.org, ian.campbell@citrix.com
List-Id: xen-devel@lists.xenproject.org

On 05/26/2015 06:47 PM, Jan Beulich wrote:
>>>> On 25.05.15 at 10:33, <rcojocaru@bitdefender.com> wrote:
>> --- a/xen/arch/x86/hvm/event.c
>> +++ b/xen/arch/x86/hvm/event.c
>> @@ -19,6 +19,7 @@
>>  * Place - Suite 330, Boston, MA 02111-1307 USA.
>>  */
>>  
>> +#include <asm/monitor.h>
>>  #include <xen/vm_event.h>
>>  #include <xen/paging.h>
> 
> Just like almost everywhere else, please have asm/ includes follow
> xen/ ones.

Ack. I went the sorted-alphabetically route, obviously that's not the
Xen convention.

>> @@ -3349,7 +3354,7 @@ int hvm_set_cr4(unsigned long value)
>>      }
>>  
>>      hvm_update_cr(v, 4, value);
>> -    hvm_event_cr4(value, old_cr);
>> +    hvm_event_cr(CR4, value, old_cr);
> 
> Considering e.g. this one, ...
> 
>> @@ -2010,7 +2012,7 @@ static int vmx_cr_access(unsigned long exit_qualification)
>>          unsigned long old = curr->arch.hvm_vcpu.guest_cr[0];
>>          curr->arch.hvm_vcpu.guest_cr[0] &= ~X86_CR0_TS;
>>          vmx_update_guest_cr(curr, 0);
>> -        hvm_event_cr0(curr->arch.hvm_vcpu.guest_cr[0], old);
>> +        hvm_event_cr(VM_EVENT_X86_CR0, curr->arch.hvm_vcpu.guest_cr[0], old);
> 
> ... does this build at all? Ah - you put the wrapping #define into
> xen/arch/x86/hvm/hvm.c instead of right alongside the function
> declaration. That's not how I would think this should be done -
> either use the wrapper everywhere, or nowhere.

Ack.

>> @@ -156,14 +158,15 @@ struct vm_event_mem_access {
>>      uint32_t _pad;
>>  };
>>  
>> -struct vm_event_mov_to_cr {
>> +struct vm_event_write_ctrlreg {
>> +    uint32_t index;
>>      uint64_t new_value;
>>      uint64_t old_value;
>> +    uint32_t _pad;
>>  };
> 
> Why would you want to add padding anywhere other than between
> index and new_value?

Ack, will move the _pad.


Thanks,
Razvan