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

[Corneliu ZUZU <czuzu@bitdefender.com>]

On 6/17/2016 10:17 AM, Jan Beulich wrote:
>>>> 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.

"Sorry, that was done out of reflex, should have stated the reasoning."

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

Is that still the case when that reasoning is obvious? (at least it 
seemed to me)

but anyway..

> Or even better, split it off to a follow-on, purely cleanup patch.

I agree with this. Will keep in mind with v2.

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

Personally I prefer the former since I think it also cuts down 
compilation time.
Having header H include every header Ni needed by source S makes H 
unnecessarily bulky at compilation time for other sources <> S that 
don't need headers Ni but which depend on H nonetheless.

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

Corneliu.

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