Re: [Xen-devel] [PATCH V3] x86/monitor: Disallow setting mem_access_emulate_each_rep when vm_event is NULL
On Tue, May 3, 2016 at 2:18 AM, Razvan Cojocaruwrote: > On 05/03/2016 11:14 AM, Jan Beulich wrote: > On 29.04.16 at 18:12, wrote: > >> On 04/09/16 08:54, 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 > >>> Reviewed-by: Andrew Cooper > >>> > >>> --- > >>> Changes since V2: > >>> - Updated the if() condition as recommended by Andrew Cooper. > >>> - Added Andrew Cooper's Reviewed-by. > >>> --- > >>> xen/include/asm-x86/monitor.h | 16 +--- > >>> 1 file changed, 13 insertions(+), 3 deletions(-) > >>> > >>> diff --git a/xen/include/asm-x86/monitor.h > b/xen/include/asm-x86/monitor.h > >>> index 0954b59..d367099 100644 > >>> --- a/xen/include/asm-x86/monitor.h > >>> +++ b/xen/include/asm-x86/monitor.h > >>> @@ -32,19 +32,29 @@ > >>> static inline > >>> int arch_monitor_domctl_op(struct domain *d, struct > xen_domctl_monitor_op *mop) > >>> { > >>> +int rc = 0; > >>> + > >>> switch ( mop->op ) > >>> { > >>> case XEN_DOMCTL_MONITOR_OP_EMULATE_EACH_REP: > >>> domain_pause(d); > >>> -d->arch.mem_access_emulate_each_rep = !!mop->event; > >>> +/* > >>> + * Enabling mem_access_emulate_each_rep without a vm_event > subscriber > >>> + * is meaningless. > >>> + */ > >>> +if ( d->max_vcpus && d->vcpu[0] && d->vcpu[0]->arch.vm_event ) > >>> +d->arch.mem_access_emulate_each_rep = !!mop->event; > >>> +else > >>> +rc = -EINVAL; > >>> + > >>> domain_unpause(d); > >>> break; > >>> > >>> default: > >>> -return -EOPNOTSUPP; > >>> +rc = -EOPNOTSUPP; > >>> } > >>> > >>> -return 0; > >>> +return rc; > >>> } > >>> > >>> int arch_monitor_domctl_event(struct domain *d, > >> > >> According to the previous list discussion with Andrew Cooper, this fix > >> might be considered for the 4.7 release, so CC-ing Wei for a release > >> ack, as suggested. > > > > Even if - without the pending ./MAINTAINERS adjustment - not > > formally required, I don't understand why you didn't Cc Tamas on > > this patch. I don't think this should go in without his ack. > > Of course, I was under the impression that he was in the recipients list > (I let scripts/maintaners.pl do the work and didn't pay much attention > to its output). > > By all means. > The maintainers file wasn't covering this header properly. Fixed in my other patch-set. Acked-by: Tamas K Lengyel ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH V3] x86/monitor: Disallow setting mem_access_emulate_each_rep when vm_event is NULL
On 05/03/2016 11:14 AM, Jan Beulich wrote: On 29.04.16 at 18:12,wrote: >> On 04/09/16 08:54, 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 >>> Reviewed-by: Andrew Cooper >>> >>> --- >>> Changes since V2: >>> - Updated the if() condition as recommended by Andrew Cooper. >>> - Added Andrew Cooper's Reviewed-by. >>> --- >>> xen/include/asm-x86/monitor.h | 16 +--- >>> 1 file changed, 13 insertions(+), 3 deletions(-) >>> >>> diff --git a/xen/include/asm-x86/monitor.h b/xen/include/asm-x86/monitor.h >>> index 0954b59..d367099 100644 >>> --- a/xen/include/asm-x86/monitor.h >>> +++ b/xen/include/asm-x86/monitor.h >>> @@ -32,19 +32,29 @@ >>> static inline >>> int arch_monitor_domctl_op(struct domain *d, struct xen_domctl_monitor_op >>> *mop) >>> { >>> +int rc = 0; >>> + >>> switch ( mop->op ) >>> { >>> case XEN_DOMCTL_MONITOR_OP_EMULATE_EACH_REP: >>> domain_pause(d); >>> -d->arch.mem_access_emulate_each_rep = !!mop->event; >>> +/* >>> + * Enabling mem_access_emulate_each_rep without a vm_event >>> subscriber >>> + * is meaningless. >>> + */ >>> +if ( d->max_vcpus && d->vcpu[0] && d->vcpu[0]->arch.vm_event ) >>> +d->arch.mem_access_emulate_each_rep = !!mop->event; >>> +else >>> +rc = -EINVAL; >>> + >>> domain_unpause(d); >>> break; >>> >>> default: >>> -return -EOPNOTSUPP; >>> +rc = -EOPNOTSUPP; >>> } >>> >>> -return 0; >>> +return rc; >>> } >>> >>> int arch_monitor_domctl_event(struct domain *d, >> >> According to the previous list discussion with Andrew Cooper, this fix >> might be considered for the 4.7 release, so CC-ing Wei for a release >> ack, as suggested. > > Even if - without the pending ./MAINTAINERS adjustment - not > formally required, I don't understand why you didn't Cc Tamas on > this patch. I don't think this should go in without his ack. Of course, I was under the impression that he was in the recipients list (I let scripts/maintaners.pl do the work and didn't pay much attention to its output). By all means. Thanks, Razvan ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH V3] x86/monitor: Disallow setting mem_access_emulate_each_rep when vm_event is NULL
>>> On 29.04.16 at 18:12,wrote: > On 04/09/16 08:54, 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 >> Reviewed-by: Andrew Cooper >> >> --- >> Changes since V2: >> - Updated the if() condition as recommended by Andrew Cooper. >> - Added Andrew Cooper's Reviewed-by. >> --- >> xen/include/asm-x86/monitor.h | 16 +--- >> 1 file changed, 13 insertions(+), 3 deletions(-) >> >> diff --git a/xen/include/asm-x86/monitor.h b/xen/include/asm-x86/monitor.h >> index 0954b59..d367099 100644 >> --- a/xen/include/asm-x86/monitor.h >> +++ b/xen/include/asm-x86/monitor.h >> @@ -32,19 +32,29 @@ >> static inline >> int arch_monitor_domctl_op(struct domain *d, struct xen_domctl_monitor_op >> *mop) >> { >> +int rc = 0; >> + >> switch ( mop->op ) >> { >> case XEN_DOMCTL_MONITOR_OP_EMULATE_EACH_REP: >> domain_pause(d); >> -d->arch.mem_access_emulate_each_rep = !!mop->event; >> +/* >> + * Enabling mem_access_emulate_each_rep without a vm_event >> subscriber >> + * is meaningless. >> + */ >> +if ( d->max_vcpus && d->vcpu[0] && d->vcpu[0]->arch.vm_event ) >> +d->arch.mem_access_emulate_each_rep = !!mop->event; >> +else >> +rc = -EINVAL; >> + >> domain_unpause(d); >> break; >> >> default: >> -return -EOPNOTSUPP; >> +rc = -EOPNOTSUPP; >> } >> >> -return 0; >> +return rc; >> } >> >> int arch_monitor_domctl_event(struct domain *d, > > According to the previous list discussion with Andrew Cooper, this fix > might be considered for the 4.7 release, so CC-ing Wei for a release > ack, as suggested. Even if - without the pending ./MAINTAINERS adjustment - not formally required, I don't understand why you didn't Cc Tamas on this patch. I don't think this should go in without his ack. Jan ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH V3] x86/monitor: Disallow setting mem_access_emulate_each_rep when vm_event is NULL
On Fri, Apr 29, 2016 at 07:12:47PM +0300, Razvan Cojocaru wrote: > On 04/09/16 08:54, 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> > Reviewed-by: Andrew Cooper Release-acked-by: Wei Liu ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH V3] x86/monitor: Disallow setting mem_access_emulate_each_rep when vm_event is NULL
On 04/09/16 08:54, 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> Reviewed-by: Andrew Cooper > > --- > Changes since V2: > - Updated the if() condition as recommended by Andrew Cooper. > - Added Andrew Cooper's Reviewed-by. > --- > xen/include/asm-x86/monitor.h | 16 +--- > 1 file changed, 13 insertions(+), 3 deletions(-) > > diff --git a/xen/include/asm-x86/monitor.h b/xen/include/asm-x86/monitor.h > index 0954b59..d367099 100644 > --- a/xen/include/asm-x86/monitor.h > +++ b/xen/include/asm-x86/monitor.h > @@ -32,19 +32,29 @@ > static inline > int arch_monitor_domctl_op(struct domain *d, struct xen_domctl_monitor_op > *mop) > { > +int rc = 0; > + > switch ( mop->op ) > { > case XEN_DOMCTL_MONITOR_OP_EMULATE_EACH_REP: > domain_pause(d); > -d->arch.mem_access_emulate_each_rep = !!mop->event; > +/* > + * Enabling mem_access_emulate_each_rep without a vm_event subscriber > + * is meaningless. > + */ > +if ( d->max_vcpus && d->vcpu[0] && d->vcpu[0]->arch.vm_event ) > +d->arch.mem_access_emulate_each_rep = !!mop->event; > +else > +rc = -EINVAL; > + > domain_unpause(d); > break; > > default: > -return -EOPNOTSUPP; > +rc = -EOPNOTSUPP; > } > > -return 0; > +return rc; > } > > int arch_monitor_domctl_event(struct domain *d, Hello, According to the previous list discussion with Andrew Cooper, this fix might be considered for the 4.7 release, so CC-ing Wei for a release ack, as suggested. Thanks, Razvan ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
[Xen-devel] [PATCH V3] x86/monitor: Disallow setting mem_access_emulate_each_rep when vm_event is NULL
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 CojocaruReviewed-by: Andrew Cooper --- Changes since V2: - Updated the if() condition as recommended by Andrew Cooper. - Added Andrew Cooper's Reviewed-by. --- xen/include/asm-x86/monitor.h | 16 +--- 1 file changed, 13 insertions(+), 3 deletions(-) diff --git a/xen/include/asm-x86/monitor.h b/xen/include/asm-x86/monitor.h index 0954b59..d367099 100644 --- a/xen/include/asm-x86/monitor.h +++ b/xen/include/asm-x86/monitor.h @@ -32,19 +32,29 @@ static inline int arch_monitor_domctl_op(struct domain *d, struct xen_domctl_monitor_op *mop) { +int rc = 0; + switch ( mop->op ) { case XEN_DOMCTL_MONITOR_OP_EMULATE_EACH_REP: domain_pause(d); -d->arch.mem_access_emulate_each_rep = !!mop->event; +/* + * Enabling mem_access_emulate_each_rep without a vm_event subscriber + * is meaningless. + */ +if ( d->max_vcpus && d->vcpu[0] && d->vcpu[0]->arch.vm_event ) +d->arch.mem_access_emulate_each_rep = !!mop->event; +else +rc = -EINVAL; + domain_unpause(d); break; default: -return -EOPNOTSUPP; +rc = -EOPNOTSUPP; } -return 0; +return rc; } int arch_monitor_domctl_event(struct domain *d, -- 1.9.1 ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel