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

[Corneliu ZUZU <czuzu@xxxxxxxxxxxxxxx>]

On 6/16/2016 5:51 PM, Jan Beulich wrote:
> > > > On 16.06.16 at 16:08, <czuzu@xxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel