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

[Corneliu ZUZU <czuzu@bitdefender.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