Re: [Xen-devel] [PATCH 1/2] x86/monitor: add masking support for write_ctrlreg events

[Petre PIRCALABU <ppircalabu@xxxxxxxxxxxxxxx>]

On Lu, 2017-05-29 at 09:01 -0600, Jan Beulich wrote:
> >
> > >
> > > >
> > > > On 26.05.17 at 15:41, <ppircalabu@xxxxxxxxxxxxxxx> wrote:
> > --- a/xen/include/public/domctl.h
> > +++ b/xen/include/public/domctl.h
> > @@ -37,7 +37,7 @@
> >  #include "hvm/save.h"
> >  #include "memory.h"
> >
> > -#define XEN_DOMCTL_INTERFACE_VERSION 0x0000000d
> > +#define XEN_DOMCTL_INTERFACE_VERSION 0x0000000e
> >
> >  /*
> >   * NB. xen_domctl.domain is an IN/OUT parameter for this
> > operation.
> > @@ -1107,6 +1107,10 @@ struct xen_domctl_monitor_op {
> >              uint8_t sync;
> >              /* Send event only on a change of value */
> >              uint8_t onchangeonly;
> > +            /* Send event only if the changed bit in the control
> > register
> > +             * is not masked
> > +             */
> > +            unsigned long bitmask;
> You can't use "unsigned long" in public headers. Also beware of
> alignment issues between 32-bit and 64-bit callers (I didn't check
> whether there actually is any, I'm merely hinting towards the
> use of uint64_aligned_t).
>
I will change it to uint64_aligned_t and resubmit the patch.
> >
> > --- a/xen/include/public/vm_event.h
> > +++ b/xen/include/public/vm_event.h
> > @@ -155,6 +155,15 @@
> >  #define VM_EVENT_X86_CR4    2
> >  #define VM_EVENT_X86_XCR0   3
> >
> > +/* vm_event_write_ctrlreg default bit mask
> > + * If the changed bit in the control register is masked the event
> > is
> > + * not triggered
> > + */
> Comment style.
>
> >
> > +#define VM_EVENT_X86_CR0_DEFAULT_MASK        0x00000000
> > +#define VM_EVENT_X86_CR3_DEFAULT_MASK        0x00000000
> > +#define VM_EVENT_X86_CR4_DEFAULT_MASK        0x00000080
> > +#define VM_EVENT_X86_XCR0_DEFAULT_MASK        0x00000000
> These are unused - what are they good for? And why would
> you want to special case CR4.PGE (and even without any
> comment)?
>
> Jan
>
The defaults were added because when the introspection engine requests
CR4 events it will get too many as CR4.PGE is used for global TLB
flushes. However, every client can set-up it's own event masking policy
so these macros are not needed globally.
>
> ________________________
> This email was scanned by Bitdefender

Many thanks for your comments,
Petre

________________________
This email was scanned by Bitdefender
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel