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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 17.06.16 at 10:25, <czuzu@xxxxxxxxxxxxxxx> 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.

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


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