| Message ID | 1460099459-25694-1-git-send-email-rcojocaru@bitdefender.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 04/08/16 10:10, Razvan Cojocaru wrote: > It is meaningless (and potentially dangerous - see hvmemul_virtual_to_linear()) > to set mem_access_emulate_each_rep before xc_monitor_enable() (which allocates > vcpu->arch.vm_event) has been called, so return an error from the > XEN_DOMCTL_MONITOR_OP_EMULATE_EACH_REP hypercall when that is the case. > > Signed-off-by: Razvan Cojocaru <rcojocaru@bitdefender.com> > --- > xen/include/asm-x86/monitor.h | 15 +++++++++++++-- > 1 file changed, 13 insertions(+), 2 deletions(-) > > diff --git a/xen/include/asm-x86/monitor.h b/xen/include/asm-x86/monitor.h > index 0954b59..0544836 100644 > --- a/xen/include/asm-x86/monitor.h > +++ b/xen/include/asm-x86/monitor.h > @@ -35,11 +35,22 @@ int arch_monitor_domctl_op(struct domain *d, struct xen_domctl_monitor_op *mop) > switch ( mop->op ) > { > case XEN_DOMCTL_MONITOR_OP_EMULATE_EACH_REP: > + { > + bool_t value = !!mop->event; > domain_pause(d); > - d->arch.mem_access_emulate_each_rep = !!mop->event; > + /* > + * Enabling emulate_each_rep without a vm_event subscriber > + * is meaningless. > + */ > + if ( !d->vcpu || !d->vcpu[0]->arch.vm_event ) Sorry, this is a bit convoluted, and wrong: if d->vcpu != NULL it will return -EINVAL even if d->vcpu[0]->arch.vm_event is NULL. I'll rework it. Apologies for the noise. Thanks, Razvan
On 08/04/2016 08:16, Razvan Cojocaru wrote: > On 04/08/16 10:10, Razvan Cojocaru wrote: >> It is meaningless (and potentially dangerous - see hvmemul_virtual_to_linear()) >> to set mem_access_emulate_each_rep before xc_monitor_enable() (which allocates >> vcpu->arch.vm_event) has been called, so return an error from the >> XEN_DOMCTL_MONITOR_OP_EMULATE_EACH_REP hypercall when that is the case. >> >> Signed-off-by: Razvan Cojocaru <rcojocaru@bitdefender.com> >> --- >> xen/include/asm-x86/monitor.h | 15 +++++++++++++-- >> 1 file changed, 13 insertions(+), 2 deletions(-) >> >> diff --git a/xen/include/asm-x86/monitor.h b/xen/include/asm-x86/monitor.h >> index 0954b59..0544836 100644 >> --- a/xen/include/asm-x86/monitor.h >> +++ b/xen/include/asm-x86/monitor.h >> @@ -35,11 +35,22 @@ int arch_monitor_domctl_op(struct domain *d, struct xen_domctl_monitor_op *mop) >> switch ( mop->op ) >> { >> case XEN_DOMCTL_MONITOR_OP_EMULATE_EACH_REP: >> + { >> + bool_t value = !!mop->event; While you are at at, one extra newline here please. >> domain_pause(d); >> - d->arch.mem_access_emulate_each_rep = !!mop->event; >> + /* >> + * Enabling emulate_each_rep without a vm_event subscriber >> + * is meaningless. >> + */ >> + if ( !d->vcpu || !d->vcpu[0]->arch.vm_event ) > Sorry, this is a bit convoluted, and wrong: if d->vcpu != NULL it will > return -EINVAL even if d->vcpu[0]->arch.vm_event is NULL. I'll rework > it. Apologies for the noise. The issue comes about because d->arch.mem_access_emulate_each_rep is a per-domain variable, whereas vm_event listeners are per-vcpu. In principle, it would be legitimate for an introspection engine to only want to introspect a single vcpu. Either we need to enforce that every vcpu has vm_event infrastructure set up (and allow for delivery of events is disabled on a per-vcpu basis), or settings such as emulate_each_rep needs to be made per-vcpu. ~Andrew
On 04/08/16 11:17, Andrew Cooper wrote: > On 08/04/2016 08:16, Razvan Cojocaru wrote: >> On 04/08/16 10:10, Razvan Cojocaru wrote: >>> It is meaningless (and potentially dangerous - see hvmemul_virtual_to_linear()) >>> to set mem_access_emulate_each_rep before xc_monitor_enable() (which allocates >>> vcpu->arch.vm_event) has been called, so return an error from the >>> XEN_DOMCTL_MONITOR_OP_EMULATE_EACH_REP hypercall when that is the case. >>> >>> Signed-off-by: Razvan Cojocaru <rcojocaru@bitdefender.com> >>> --- >>> xen/include/asm-x86/monitor.h | 15 +++++++++++++-- >>> 1 file changed, 13 insertions(+), 2 deletions(-) >>> >>> diff --git a/xen/include/asm-x86/monitor.h b/xen/include/asm-x86/monitor.h >>> index 0954b59..0544836 100644 >>> --- a/xen/include/asm-x86/monitor.h >>> +++ b/xen/include/asm-x86/monitor.h >>> @@ -35,11 +35,22 @@ int arch_monitor_domctl_op(struct domain *d, struct xen_domctl_monitor_op *mop) >>> switch ( mop->op ) >>> { >>> case XEN_DOMCTL_MONITOR_OP_EMULATE_EACH_REP: >>> + { >>> + bool_t value = !!mop->event; > > While you are at at, one extra newline here please. > >>> domain_pause(d); >>> - d->arch.mem_access_emulate_each_rep = !!mop->event; >>> + /* >>> + * Enabling emulate_each_rep without a vm_event subscriber >>> + * is meaningless. >>> + */ >>> + if ( !d->vcpu || !d->vcpu[0]->arch.vm_event ) >> Sorry, this is a bit convoluted, and wrong: if d->vcpu != NULL it will >> return -EINVAL even if d->vcpu[0]->arch.vm_event is NULL. I'll rework >> it. Apologies for the noise. > > The issue comes about because d->arch.mem_access_emulate_each_rep is a > per-domain variable, whereas vm_event listeners are per-vcpu. > > In principle, it would be legitimate for an introspection engine to only > want to introspect a single vcpu. > > Either we need to enforce that every vcpu has vm_event infrastructure > set up (and allow for delivery of events is disabled on a per-vcpu > basis), or settings such as emulate_each_rep needs to be made per-vcpu. Xc_monitor_enable() does set vm_event up for all of the domain's VCPUs, so at least for now it is a fair assumption that if it is set for the first one, it's set for all of them. In other words, your first requirement is currently being implicitly enforced. Introspecting a single VCPU is an interesting idea, but not terribly useful at this point. Thanks, Razvan
diff --git a/xen/include/asm-x86/monitor.h b/xen/include/asm-x86/monitor.h index 0954b59..0544836 100644 --- a/xen/include/asm-x86/monitor.h +++ b/xen/include/asm-x86/monitor.h @@ -35,11 +35,22 @@ int arch_monitor_domctl_op(struct domain *d, struct xen_domctl_monitor_op *mop) switch ( mop->op ) { case XEN_DOMCTL_MONITOR_OP_EMULATE_EACH_REP: + { + bool_t value = !!mop->event; domain_pause(d); - d->arch.mem_access_emulate_each_rep = !!mop->event; + /* + * Enabling emulate_each_rep without a vm_event subscriber + * is meaningless. + */ + if ( !d->vcpu || !d->vcpu[0]->arch.vm_event ) + { + domain_unpause(d); + return -EINVAL; + } + d->arch.mem_access_emulate_each_rep = value; domain_unpause(d); break; - + } default: return -EOPNOTSUPP; }
It is meaningless (and potentially dangerous - see hvmemul_virtual_to_linear()) to set mem_access_emulate_each_rep before xc_monitor_enable() (which allocates vcpu->arch.vm_event) has been called, so return an error from the XEN_DOMCTL_MONITOR_OP_EMULATE_EACH_REP hypercall when that is the case. Signed-off-by: Razvan Cojocaru <rcojocaru@bitdefender.com> --- xen/include/asm-x86/monitor.h | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-)