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

From: "Jan Beulich" <JBeulich@suse.com>
To: Corneliu ZUZU <czuzu@bitdefender.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: Fri, 17 Jun 2016 01:17:58 -0600	[thread overview]
Message-ID: <5763C04602000078000F5E6D@prv-mh.provo.novell.com> (raw)
In-Reply-To: <d5b8e876-738e-994f-4e77-435e1daa9b1d@bitdefender.com>

>>> On 16.06.16 at 22:10, <czuzu@bitdefender.com> wrote:
> 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?

I see Razvan responded to this. I don't have a strong opinion
either way, my only request if for the call to be in exactly one
place.

>>> --- 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.

See patch 1. Plus I don't think I (or in fact any reviewer) should ask
for such reasoning: Instead you should state extra cleanup you do
to unrelated (to the purpose of your patch) files in the description.
Or even better, split it off to a follow-on, purely cleanup patch.
(And to be clear, I much appreciate any form of reduction of the
sometimes extremely long lists of #include-s, just not [apparently
or really] randomly mixed with other, substantial changes. That's
namely because it's not clear whether source files should explicitly
include everything they need, or instead be allowed to rely on
headers they include to include further headers they also
_explicitly_ rely on. IOW there's likely a discussion to be had for
this kind of cleanup, and such a discussion should be a separate
thread from the one on the functional adjustments here.)

Jan

> 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.

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