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

[Corneliu ZUZU <czuzu@xxxxxxxxxxxxxxx>]

On 6/17/2016 11:38 AM, Jan Beulich wrote:
> > > > 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.
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@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel