Re: [PATCH 3/7] vm-event: introduce vm_event_vcpu_enter

From: Corneliu ZUZU <czuzu@bitdefender.com>
To: Jan Beulich <JBeulich@suse.com>
Cc: Kevin Tian <kevin.tian@intel.com>,
	Stefano Stabellini <sstabellini@kernel.org>,
	Razvan Cojocaru <rcojocaru@bitdefender.com>,
	George Dunlap <george.dunlap@eu.citrix.com>,
	Andrew Cooper <andrew.cooper3@citrix.com>,
	xen-devel@lists.xen.org, Julien Grall <julien.grall@arm.com>,
	Paul Durrant <paul.durrant@citrix.com>,
	Tamas K Lengyel <tamas@tklengyel.com>,
	Jun Nakajima <jun.nakajima@intel.com>
Subject: Re: [PATCH 3/7] vm-event: introduce vm_event_vcpu_enter
Date: Thu, 16 Jun 2016 23:10:04 +0300	[thread overview]
Message-ID: <d5b8e876-738e-994f-4e77-435e1daa9b1d@bitdefender.com> (raw)
In-Reply-To: <5762D91402000078000F5BC7@prv-mh.provo.novell.com>

On 6/16/2016 5:51 PM, Jan Beulich wrote:
>>>> On 16.06.16 at 16:08, <czuzu@bitdefender.com> wrote:
>> @@ -509,6 +508,8 @@ void hvm_do_resume(struct vcpu *v)
>>           }
>>       }
>>   
>> +    vm_event_vcpu_enter(v);
> Why here?
Why indeed. It made sense because monitor_write_data handling was 
originally there and then the plan was to move it to vm_event_vcpu_enter 
(which happens in the following commit).
The question is though, why was monitor_write_data handled there in the 
first place? Why was it not put e.g. in vmx_do_resume immediately after 
the call to hvm_do_resume and just before
the reset_stack_and_jump...? And what happens with handling 
monitor_write_data if this:

if ( !handle_hvm_io_completion(v) )
         return;

causes a return?

>> @@ -3874,6 +3875,9 @@ void vmx_vmenter_helper(const struct cpu_user_regs *regs)
>>       }
>>   
>>    out:
>> +    if ( guest_mode(regs) )
>> +        vm_event_vcpu_enter(curr);
>> +
>>       HVMTRACE_ND(VMENTRY, 0, 1/*cycles*/, 0, 0, 0, 0, 0, 0, 0);
>>   
>>       __vmwrite(GUEST_RIP,    regs->rip);
> Why with a conditional? The registers can't possibly hold to non-
> guest state when you're here.

Right, I must've mistakenly taken that from ARM-side @ some point. There 
it made sense, here it doesn't.
I'll remove it in V2.

>
>> --- a/xen/arch/x86/mm/p2m.c
>> +++ b/xen/arch/x86/mm/p2m.c
>> @@ -36,7 +36,6 @@
>>   #include <asm/hvm/nestedhvm.h>
>>   #include <asm/altp2m.h>
>>   #include <asm/hvm/svm/amd-iommu-proto.h>
>> -#include <asm/vm_event.h>
>>   #include <xsm/xsm.h>
> There are way too many of these #include adjustments here. If
> you really mean to clean these up, please don't randomly throw
> this into various unrelated patches.

I haven't thrown anything "randomly into unrelated patches", please 
first ask for my reasoning and then draw such conclusions.
That was removed because xen/vm_event.h includes asm/vm_event.h with 
this patch (because it calls arch_vm_event_vcpu_enter) and this file 
(p2m.c) already included xen/vm_event.h.

>> --- a/xen/common/monitor.c
>> +++ b/xen/common/monitor.c
>> @@ -22,8 +22,8 @@
>>   #include <xen/monitor.h>
>>   #include <xen/sched.h>
>>   #include <xsm/xsm.h>
>> +#include <xen/vm_event.h>
>>   #include <asm/monitor.h>
>> -#include <asm/vm_event.h>
> Please retain at least basic grouping (i.e. all xen/ ones together,
> meaning the insertion should move up by one line).
>
> Jan
>
>

I usually do that, in this case I just didn't notice. Acked.

Corneliu.

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