| Message ID | 1496137567-6574-2-git-send-email-ppircalabu@bitdefender.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, May 30, 2017 at 3:46 AM, Petre Pircalabu <ppircalabu@bitdefender.com> wrote: > Add support for filtering out the write_ctrlreg monitor events if they > are generated only by changing certains bits. > A new parameter (bitmask) was added to the xc_monitor_write_ctrlreg > function in order to mask the event generation if the changed bits are > set. > > Signed-off-by: Petre Pircalabu <ppircalabu@bitdefender.com> Acked-by: Tamas K Lengyel <tamas@tklengyel.com> > --- > tools/libxc/include/xenctrl.h | 2 +- > tools/libxc/xc_monitor.c | 3 ++- > xen/arch/x86/hvm/monitor.c | 3 ++- > xen/arch/x86/monitor.c | 6 ++++++ > xen/include/asm-x86/domain.h | 1 + > xen/include/public/domctl.h | 7 ++++++- > 6 files changed, 18 insertions(+), 4 deletions(-) > > diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h > index 1629f41..8c26cb4 100644 > --- a/tools/libxc/include/xenctrl.h > +++ b/tools/libxc/include/xenctrl.h > @@ -1999,7 +1999,7 @@ int xc_monitor_get_capabilities(xc_interface *xch, domid_t domain_id, > uint32_t *capabilities); > int xc_monitor_write_ctrlreg(xc_interface *xch, domid_t domain_id, > uint16_t index, bool enable, bool sync, > - bool onchangeonly); > + uint64_t bitmask, bool onchangeonly); > /* > * A list of MSR indices can usually be found in /usr/include/asm/msr-index.h. > * Please consult the Intel/AMD manuals for more information on > diff --git a/tools/libxc/xc_monitor.c b/tools/libxc/xc_monitor.c > index f99b6e3..70648d7 100644 > --- a/tools/libxc/xc_monitor.c > +++ b/tools/libxc/xc_monitor.c > @@ -70,7 +70,7 @@ int xc_monitor_get_capabilities(xc_interface *xch, domid_t domain_id, > > int xc_monitor_write_ctrlreg(xc_interface *xch, domid_t domain_id, > uint16_t index, bool enable, bool sync, > - bool onchangeonly) > + uint64_t bitmask, bool onchangeonly) > { > DECLARE_DOMCTL; > > @@ -82,6 +82,7 @@ int xc_monitor_write_ctrlreg(xc_interface *xch, domid_t domain_id, > domctl.u.monitor_op.u.mov_to_cr.index = index; > domctl.u.monitor_op.u.mov_to_cr.sync = sync; > domctl.u.monitor_op.u.mov_to_cr.onchangeonly = onchangeonly; > + domctl.u.monitor_op.u.mov_to_cr.bitmask = bitmask; > > return do_domctl(xch, &domctl); > } > diff --git a/xen/arch/x86/hvm/monitor.c b/xen/arch/x86/hvm/monitor.c > index bde5fd0..a7ccfc4 100644 > --- a/xen/arch/x86/hvm/monitor.c > +++ b/xen/arch/x86/hvm/monitor.c > @@ -38,7 +38,8 @@ bool_t hvm_monitor_cr(unsigned int index, unsigned long value, unsigned long old > > if ( (ad->monitor.write_ctrlreg_enabled & ctrlreg_bitmask) && > (!(ad->monitor.write_ctrlreg_onchangeonly & ctrlreg_bitmask) || > - value != old) ) > + value != old) && > + (!((value ^ old) & ad->monitor.write_ctrlreg_mask[index])) ) > { > bool_t sync = !!(ad->monitor.write_ctrlreg_sync & ctrlreg_bitmask); > > diff --git a/xen/arch/x86/monitor.c b/xen/arch/x86/monitor.c > index 449c64c..d02d2ea 100644 > --- a/xen/arch/x86/monitor.c > +++ b/xen/arch/x86/monitor.c > @@ -155,9 +155,15 @@ int arch_monitor_domctl_event(struct domain *d, > ad->monitor.write_ctrlreg_onchangeonly &= ~ctrlreg_bitmask; > > if ( requested_status ) > + { > + ad->monitor.write_ctrlreg_mask[mop->u.mov_to_cr.index] = mop->u.mov_to_cr.bitmask; > ad->monitor.write_ctrlreg_enabled |= ctrlreg_bitmask; > + } > else > + { > + ad->monitor.write_ctrlreg_mask[mop->u.mov_to_cr.index] = 0; > ad->monitor.write_ctrlreg_enabled &= ~ctrlreg_bitmask; > + } > > if ( VM_EVENT_X86_CR3 == mop->u.mov_to_cr.index ) > { > diff --git a/xen/include/asm-x86/domain.h b/xen/include/asm-x86/domain.h > index 924caac..27d80ee 100644 > --- a/xen/include/asm-x86/domain.h > +++ b/xen/include/asm-x86/domain.h > @@ -406,6 +406,7 @@ struct arch_domain > unsigned int cpuid_enabled : 1; > unsigned int descriptor_access_enabled : 1; > struct monitor_msr_bitmap *msr_bitmap; > + uint64_t write_ctrlreg_mask[4]; > } monitor; > > /* Mem_access emulation control */ > diff --git a/xen/include/public/domctl.h b/xen/include/public/domctl.h > index e6cf211..652fb17 100644 > --- 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,11 @@ 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. > + */ > + uint64_aligned_t bitmask; > } mov_to_cr; > > struct { > -- > 2.7.4
>>> On 16.06.17 at 16:26, <tamas@tklengyel.com> wrote: > On Tue, May 30, 2017 at 3:46 AM, Petre Pircalabu > <ppircalabu@bitdefender.com> wrote: >> Add support for filtering out the write_ctrlreg monitor events if they >> are generated only by changing certains bits. >> A new parameter (bitmask) was added to the xc_monitor_write_ctrlreg >> function in order to mask the event generation if the changed bits are >> set. >> >> Signed-off-by: Petre Pircalabu <ppircalabu@bitdefender.com> > > Acked-by: Tamas K Lengyel <tamas@tklengyel.com> Are you btw in agreement with ... >> @@ -1107,6 +1107,11 @@ 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. >> + */ >> + uint64_aligned_t bitmask; ... the 5-byte gap being introduced here, using of which won't be possible without bumping XEN_DOMCTL_INTERFACE_VERSION again? Generally we aim at making padding explicit and checking it to be zero on input / providing zero on output. Jan
On Fri, Jun 16, 2017 at 9:15 AM, Jan Beulich <JBeulich@suse.com> wrote: >>>> On 16.06.17 at 16:26, <tamas@tklengyel.com> wrote: >> On Tue, May 30, 2017 at 3:46 AM, Petre Pircalabu >> <ppircalabu@bitdefender.com> wrote: >>> Add support for filtering out the write_ctrlreg monitor events if they >>> are generated only by changing certains bits. >>> A new parameter (bitmask) was added to the xc_monitor_write_ctrlreg >>> function in order to mask the event generation if the changed bits are >>> set. >>> >>> Signed-off-by: Petre Pircalabu <ppircalabu@bitdefender.com> >> >> Acked-by: Tamas K Lengyel <tamas@tklengyel.com> > > Are you btw in agreement with ... > >>> @@ -1107,6 +1107,11 @@ 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. >>> + */ >>> + uint64_aligned_t bitmask; > > ... the 5-byte gap being introduced here, using of which won't > be possible without bumping XEN_DOMCTL_INTERFACE_VERSION > again? Generally we aim at making padding explicit and checking > it to be zero on input / providing zero on output. Yes, making the padding explicit would be better (but I'm not the maintainer of the domctl interface so my ack doesn't cover that). Tamas
>>> On 16.06.17 at 17:28, <tamas@tklengyel.com> wrote: > On Fri, Jun 16, 2017 at 9:15 AM, Jan Beulich <JBeulich@suse.com> wrote: >>>>> On 16.06.17 at 16:26, <tamas@tklengyel.com> wrote: >>> On Tue, May 30, 2017 at 3:46 AM, Petre Pircalabu >>> <ppircalabu@bitdefender.com> wrote: >>>> Add support for filtering out the write_ctrlreg monitor events if they >>>> are generated only by changing certains bits. >>>> A new parameter (bitmask) was added to the xc_monitor_write_ctrlreg >>>> function in order to mask the event generation if the changed bits are >>>> set. >>>> >>>> Signed-off-by: Petre Pircalabu <ppircalabu@bitdefender.com> >>> >>> Acked-by: Tamas K Lengyel <tamas@tklengyel.com> >> >> Are you btw in agreement with ... >> >>>> @@ -1107,6 +1107,11 @@ 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. >>>> + */ >>>> + uint64_aligned_t bitmask; >> >> ... the 5-byte gap being introduced here, using of which won't >> be possible without bumping XEN_DOMCTL_INTERFACE_VERSION >> again? Generally we aim at making padding explicit and checking >> it to be zero on input / providing zero on output. > > Yes, making the padding explicit would be better (but I'm not the > maintainer of the domctl interface so my ack doesn't cover that). Well, I have been waiting for a monitor maintainer ack to perhaps give my own for the little bit of it where a REST maintainer one would be needed. Irrespective of file level maintainership I think the public interface parts still belong to their subsystems, and the REST maintainer ack should normally be a purely mechanical last step. Jan
On Fri, Jun 16, 2017 at 9:33 AM, Jan Beulich <JBeulich@suse.com> wrote: >>>> On 16.06.17 at 17:28, <tamas@tklengyel.com> wrote: >> On Fri, Jun 16, 2017 at 9:15 AM, Jan Beulich <JBeulich@suse.com> wrote: >>>>>> On 16.06.17 at 16:26, <tamas@tklengyel.com> wrote: >>>> On Tue, May 30, 2017 at 3:46 AM, Petre Pircalabu >>>> <ppircalabu@bitdefender.com> wrote: >>>>> Add support for filtering out the write_ctrlreg monitor events if they >>>>> are generated only by changing certains bits. >>>>> A new parameter (bitmask) was added to the xc_monitor_write_ctrlreg >>>>> function in order to mask the event generation if the changed bits are >>>>> set. >>>>> >>>>> Signed-off-by: Petre Pircalabu <ppircalabu@bitdefender.com> >>>> >>>> Acked-by: Tamas K Lengyel <tamas@tklengyel.com> >>> >>> Are you btw in agreement with ... >>> >>>>> @@ -1107,6 +1107,11 @@ 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. >>>>> + */ >>>>> + uint64_aligned_t bitmask; >>> >>> ... the 5-byte gap being introduced here, using of which won't >>> be possible without bumping XEN_DOMCTL_INTERFACE_VERSION >>> again? Generally we aim at making padding explicit and checking >>> it to be zero on input / providing zero on output. >> >> Yes, making the padding explicit would be better (but I'm not the >> maintainer of the domctl interface so my ack doesn't cover that). > > Well, I have been waiting for a monitor maintainer ack to perhaps > give my own for the little bit of it where a REST maintainer one > would be needed. Irrespective of file level maintainership I think > the public interface parts still belong to their subsystems, and the > REST maintainer ack should normally be a purely mechanical last > step. I can see the logic behind that. However, I would feel better if it wasn't just a mechanical last step as for example I'm not as well versed in the ABI side of things. Like in here, the padding bits have completely went by me without me noticing. Tamas
diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h index 1629f41..8c26cb4 100644 --- a/tools/libxc/include/xenctrl.h +++ b/tools/libxc/include/xenctrl.h @@ -1999,7 +1999,7 @@ int xc_monitor_get_capabilities(xc_interface *xch, domid_t domain_id, uint32_t *capabilities); int xc_monitor_write_ctrlreg(xc_interface *xch, domid_t domain_id, uint16_t index, bool enable, bool sync, - bool onchangeonly); + uint64_t bitmask, bool onchangeonly); /* * A list of MSR indices can usually be found in /usr/include/asm/msr-index.h. * Please consult the Intel/AMD manuals for more information on diff --git a/tools/libxc/xc_monitor.c b/tools/libxc/xc_monitor.c index f99b6e3..70648d7 100644 --- a/tools/libxc/xc_monitor.c +++ b/tools/libxc/xc_monitor.c @@ -70,7 +70,7 @@ int xc_monitor_get_capabilities(xc_interface *xch, domid_t domain_id, int xc_monitor_write_ctrlreg(xc_interface *xch, domid_t domain_id, uint16_t index, bool enable, bool sync, - bool onchangeonly) + uint64_t bitmask, bool onchangeonly) { DECLARE_DOMCTL; @@ -82,6 +82,7 @@ int xc_monitor_write_ctrlreg(xc_interface *xch, domid_t domain_id, domctl.u.monitor_op.u.mov_to_cr.index = index; domctl.u.monitor_op.u.mov_to_cr.sync = sync; domctl.u.monitor_op.u.mov_to_cr.onchangeonly = onchangeonly; + domctl.u.monitor_op.u.mov_to_cr.bitmask = bitmask; return do_domctl(xch, &domctl); } diff --git a/xen/arch/x86/hvm/monitor.c b/xen/arch/x86/hvm/monitor.c index bde5fd0..a7ccfc4 100644 --- a/xen/arch/x86/hvm/monitor.c +++ b/xen/arch/x86/hvm/monitor.c @@ -38,7 +38,8 @@ bool_t hvm_monitor_cr(unsigned int index, unsigned long value, unsigned long old if ( (ad->monitor.write_ctrlreg_enabled & ctrlreg_bitmask) && (!(ad->monitor.write_ctrlreg_onchangeonly & ctrlreg_bitmask) || - value != old) ) + value != old) && + (!((value ^ old) & ad->monitor.write_ctrlreg_mask[index])) ) { bool_t sync = !!(ad->monitor.write_ctrlreg_sync & ctrlreg_bitmask); diff --git a/xen/arch/x86/monitor.c b/xen/arch/x86/monitor.c index 449c64c..d02d2ea 100644 --- a/xen/arch/x86/monitor.c +++ b/xen/arch/x86/monitor.c @@ -155,9 +155,15 @@ int arch_monitor_domctl_event(struct domain *d, ad->monitor.write_ctrlreg_onchangeonly &= ~ctrlreg_bitmask; if ( requested_status ) + { + ad->monitor.write_ctrlreg_mask[mop->u.mov_to_cr.index] = mop->u.mov_to_cr.bitmask; ad->monitor.write_ctrlreg_enabled |= ctrlreg_bitmask; + } else + { + ad->monitor.write_ctrlreg_mask[mop->u.mov_to_cr.index] = 0; ad->monitor.write_ctrlreg_enabled &= ~ctrlreg_bitmask; + } if ( VM_EVENT_X86_CR3 == mop->u.mov_to_cr.index ) { diff --git a/xen/include/asm-x86/domain.h b/xen/include/asm-x86/domain.h index 924caac..27d80ee 100644 --- a/xen/include/asm-x86/domain.h +++ b/xen/include/asm-x86/domain.h @@ -406,6 +406,7 @@ struct arch_domain unsigned int cpuid_enabled : 1; unsigned int descriptor_access_enabled : 1; struct monitor_msr_bitmap *msr_bitmap; + uint64_t write_ctrlreg_mask[4]; } monitor; /* Mem_access emulation control */ diff --git a/xen/include/public/domctl.h b/xen/include/public/domctl.h index e6cf211..652fb17 100644 --- 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,11 @@ 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. + */ + uint64_aligned_t bitmask; } mov_to_cr; struct {
Add support for filtering out the write_ctrlreg monitor events if they are generated only by changing certains bits. A new parameter (bitmask) was added to the xc_monitor_write_ctrlreg function in order to mask the event generation if the changed bits are set. Signed-off-by: Petre Pircalabu <ppircalabu@bitdefender.com> --- tools/libxc/include/xenctrl.h | 2 +- tools/libxc/xc_monitor.c | 3 ++- xen/arch/x86/hvm/monitor.c | 3 ++- xen/arch/x86/monitor.c | 6 ++++++ xen/include/asm-x86/domain.h | 1 + xen/include/public/domctl.h | 7 ++++++- 6 files changed, 18 insertions(+), 4 deletions(-)