Re: [PATCH 6/7] vm-event/arm: move hvm_event_cr->common vm_event_monitor_cr

[Corneliu ZUZU <czuzu@bitdefender.com>]

On 6/17/2016 11:38 AM, Jan Beulich wrote:
>>>> On 17.06.16 at 10:25, <czuzu@bitdefender.com> wrote:
>> On 6/16/2016 6:16 PM, Jan Beulich wrote:
>>> And looking at all the uses of this variable I get the impression that
>>> you really want a shorthand for &d->arch.monitor (if any such
>>> helper variable is worthwhile to have here in the first place).
>> Well, this was a simple cut-paste operation, not very old content aware :)
>> Personally I prefer the current shorthand (ad) (seems more intuitive and
>> is consistent with the other XEN_DOMCTL_MONITOR_EVENT_* cases), but if
>> you prefer I'll change that shorthand to am = &d->arch.monitor?
> I'd prefer either no shorthand, or one eliminating the longest common
> prefix across all uses.

am = &d->arch.monitor it is then.

>>>> --- a/xen/include/asm-x86/monitor.h
>>>> +++ b/xen/include/asm-x86/monitor.h
>>>> @@ -24,8 +24,6 @@
>>>>    
>>>>    #include <xen/sched.h>
>>>>    
>>>> -#define monitor_ctrlreg_bitmask(ctrlreg_index) (1U << (ctrlreg_index))
>>>> -
>>>>    static inline
>>>>    int arch_monitor_domctl_op(struct domain *d, struct xen_domctl_monitor_op *mop)
>>>>    {
>>>> --- a/xen/include/xen/monitor.h
>>>> +++ b/xen/include/xen/monitor.h
>>>> @@ -25,6 +25,10 @@
>>>>    struct domain;
>>>>    struct xen_domctl_monitor_op;
>>>>    
>>>> +#if CONFIG_X86
>>>> +#define monitor_ctrlreg_bitmask(ctrlreg_index) (1U << (ctrlreg_index))
>>>> +#endif
>>> What's the point in removing this from the x86 header if then it
>>> needs to be put in such a conditional? If the conditional gets
>>> dropped in the next patch, then I think you have two options:
>>> Leave it where it is here, and move it there. Or move it here,
>>> but omit the conditional right away - I can't see this definition
>>> being present to harm the ARM build in any way.
>> Meld comment above.
> I don't follow - having split the patches for reviewability was
> a good idea. I merely commented on some oddities resulting
> from that split, which - afaict - could be easily corrected without
> folding the patches together.
>
> Jan

Ooh, haha, sorry I've re-read your comment now. You're right, there's no 
point in that change, I'll leave it on the X86 side until the ARM part 
is actually implemented (last patch).

Corneliu.

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