| Message ID | 1460181249-13651-1-git-send-email-rcojocaru@bitdefender.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 <rcojocaru@bitdefender.com> > Reviewed-by: Andrew Cooper <andrew.cooper3@citirx.com> > > --- > 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
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 <rcojocaru@bitdefender.com> > > Reviewed-by: Andrew Cooper <andrew.cooper3@citirx.com> Release-acked-by: Wei Liu <wei.liu2@citrix.com>
>>> On 29.04.16 at 18:12, <rcojocaru@bitdefender.com> 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 <rcojocaru@bitdefender.com> >> Reviewed-by: Andrew Cooper <andrew.cooper3@citirx.com> >> >> --- >> 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
On 05/03/2016 11:14 AM, Jan Beulich wrote: >>>> On 29.04.16 at 18:12, <rcojocaru@bitdefender.com> 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 <rcojocaru@bitdefender.com> >>> Reviewed-by: Andrew Cooper <andrew.cooper3@citirx.com> >>> >>> --- >>> 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
On Tue, May 3, 2016 at 2:18 AM, Razvan Cojocaru <rcojocaru@bitdefender.com> wrote: > On 05/03/2016 11:14 AM, Jan Beulich wrote: > >>>> On 29.04.16 at 18:12, <rcojocaru@bitdefender.com> 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 <rcojocaru@bitdefender.com> > >>> Reviewed-by: Andrew Cooper <andrew.cooper3@citirx.com> > >>> > >>> --- > >>> 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 <tamas@tklengyel.com>
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,