| Message ID | 1468038011-6935-1-git-send-email-czuzu@bitdefender.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
> diff --git a/xen/arch/x86/monitor.c b/xen/arch/x86/monitor.c > index aeee435..4a29cad 100644 > --- a/xen/arch/x86/monitor.c > +++ b/xen/arch/x86/monitor.c > @@ -62,6 +62,8 @@ int monitor_init_domain(struct domain *d) > return -ENOMEM; > } > > + d->monitor.initialised = 1; > + > return 0; > } > > @@ -78,6 +80,7 @@ void monitor_cleanup_domain(struct domain *d) > > memset(&d->arch.monitor, 0, sizeof(d->arch.monitor)); > memset(&d->monitor, 0, sizeof(d->monitor)); > + d->monitor.initialised = 0; So d->monitor is already memset to zero above, thus this is not needed here. > } > > void monitor_ctrlreg_write_resume(struct vcpu *v, vm_event_response_t *rsp) > diff --git a/xen/include/asm-arm/monitor.h b/xen/include/asm-arm/monitor.h > index 9a9734a..7ef30f1 100644 [snip] > diff --git a/xen/include/xen/monitor.h b/xen/include/xen/monitor.h > index 2171d04..605caf0 100644 > --- a/xen/include/xen/monitor.h > +++ b/xen/include/xen/monitor.h > @@ -22,12 +22,15 @@ > #ifndef __XEN_MONITOR_H__ > #define __XEN_MONITOR_H__ > > -#include <public/vm_event.h> > - > -struct domain; > -struct xen_domctl_monitor_op; > +#include <xen/sched.h> > > int monitor_domctl(struct domain *d, struct xen_domctl_monitor_op *op); > + > +static inline bool_t monitor_domain_initialised(const struct domain *d) > +{ > + return d->monitor.initialised; This should be !!d->monitor.initialised. > +} > + > void monitor_guest_request(void); > > int monitor_traps(struct vcpu *v, bool_t sync, vm_event_request_t *req); Cheers, Tamas
On 7/9/2016 8:34 PM, Tamas K Lengyel wrote: >> diff --git a/xen/arch/x86/monitor.c b/xen/arch/x86/monitor.c >> index aeee435..4a29cad 100644 >> --- a/xen/arch/x86/monitor.c >> +++ b/xen/arch/x86/monitor.c >> @@ -62,6 +62,8 @@ int monitor_init_domain(struct domain *d) >> return -ENOMEM; >> } >> >> + d->monitor.initialised = 1; >> + >> return 0; >> } >> >> @@ -78,6 +80,7 @@ void monitor_cleanup_domain(struct domain *d) >> >> memset(&d->arch.monitor, 0, sizeof(d->arch.monitor)); >> memset(&d->monitor, 0, sizeof(d->monitor)); >> + d->monitor.initialised = 0; > So d->monitor is already memset to zero above, thus this is not needed here. Yep. > >> } >> >> void monitor_ctrlreg_write_resume(struct vcpu *v, vm_event_response_t *rsp) >> diff --git a/xen/include/asm-arm/monitor.h b/xen/include/asm-arm/monitor.h >> index 9a9734a..7ef30f1 100644 > [snip] I keep seeing '[snip]' lately but I don't know what it means. > >> diff --git a/xen/include/xen/monitor.h b/xen/include/xen/monitor.h >> index 2171d04..605caf0 100644 >> --- a/xen/include/xen/monitor.h >> +++ b/xen/include/xen/monitor.h >> @@ -22,12 +22,15 @@ >> #ifndef __XEN_MONITOR_H__ >> #define __XEN_MONITOR_H__ >> >> -#include <public/vm_event.h> >> - >> -struct domain; >> -struct xen_domctl_monitor_op; >> +#include <xen/sched.h> >> >> int monitor_domctl(struct domain *d, struct xen_domctl_monitor_op *op); >> + >> +static inline bool_t monitor_domain_initialised(const struct domain *d) >> +{ >> + return d->monitor.initialised; > This should be !!d->monitor.initialised. It's the value of a bit, thus should be 0 or 1. Am I missing something? > >> +} >> + >> void monitor_guest_request(void); >> >> int monitor_traps(struct vcpu *v, bool_t sync, vm_event_request_t *req); > Cheers, > Tamas > Thanks, Zuzu.
>>> diff --git a/xen/include/asm-arm/monitor.h >>> b/xen/include/asm-arm/monitor.h >>> index 9a9734a..7ef30f1 100644 >> >> [snip] > > > I keep seeing '[snip]' lately but I don't know what it means. Placeholder for "I've cut some of your patch content here to shorten the message I'm sending". > >> >>> diff --git a/xen/include/xen/monitor.h b/xen/include/xen/monitor.h >>> index 2171d04..605caf0 100644 >>> --- a/xen/include/xen/monitor.h >>> +++ b/xen/include/xen/monitor.h >>> @@ -22,12 +22,15 @@ >>> #ifndef __XEN_MONITOR_H__ >>> #define __XEN_MONITOR_H__ >>> >>> -#include <public/vm_event.h> >>> - >>> -struct domain; >>> -struct xen_domctl_monitor_op; >>> +#include <xen/sched.h> >>> >>> int monitor_domctl(struct domain *d, struct xen_domctl_monitor_op *op); >>> + >>> +static inline bool_t monitor_domain_initialised(const struct domain *d) >>> +{ >>> + return d->monitor.initialised; >> >> This should be !!d->monitor.initialised. > > > It's the value of a bit, thus should be 0 or 1. Am I missing something? Using a bitfield is effectively doing a bitmask for the bit(s). However, returning the value after applying a bitmask is not necessarily 0/1 (ie. bool_t). For example I ran into problems with this in the past (see https://lists.xen.org/archives/html/xen-devel/2015-08/msg01948.html). Tamas
On 7/11/2016 7:38 PM, Tamas K Lengyel wrote: >>>> diff --git a/xen/include/asm-arm/monitor.h >>>> b/xen/include/asm-arm/monitor.h >>>> index 9a9734a..7ef30f1 100644 >>> [snip] >> >> I keep seeing '[snip]' lately but I don't know what it means. > Placeholder for "I've cut some of your patch content here to shorten > the message I'm sending". > >>>> diff --git a/xen/include/xen/monitor.h b/xen/include/xen/monitor.h >>>> index 2171d04..605caf0 100644 >>>> --- a/xen/include/xen/monitor.h >>>> +++ b/xen/include/xen/monitor.h >>>> @@ -22,12 +22,15 @@ >>>> #ifndef __XEN_MONITOR_H__ >>>> #define __XEN_MONITOR_H__ >>>> >>>> -#include <public/vm_event.h> >>>> - >>>> -struct domain; >>>> -struct xen_domctl_monitor_op; >>>> +#include <xen/sched.h> >>>> >>>> int monitor_domctl(struct domain *d, struct xen_domctl_monitor_op *op); >>>> + >>>> +static inline bool_t monitor_domain_initialised(const struct domain *d) >>>> +{ >>>> + return d->monitor.initialised; >>> This should be !!d->monitor.initialised. >> >> It's the value of a bit, thus should be 0 or 1. Am I missing something? > Using a bitfield is effectively doing a bitmask for the bit(s). > However, returning the value after applying a bitmask is not > necessarily 0/1 (ie. bool_t). For example I ran into problems with > this in the past (see > https://lists.xen.org/archives/html/xen-devel/2015-08/msg01948.html). > > Tamas The example you provided actually returns a value &-ed with a flag (bitmask), of course it returns either 0 or the flag itself :-). AFAIK a single-bit item in a bitfield (note a -bitfield-, not e.g. an unsigned int &-ed with (1<<x)) will always be either 0 or 1. Corneliu.
On Mon, Jul 11, 2016 at 2:20 PM, Corneliu ZUZU <czuzu@bitdefender.com> wrote: > On 7/11/2016 7:38 PM, Tamas K Lengyel wrote: >>>>> >>>>> diff --git a/xen/include/asm-arm/monitor.h >>>>> b/xen/include/asm-arm/monitor.h >>>>> index 9a9734a..7ef30f1 100644 >>>> >>>> [snip] >>> >>> >>> I keep seeing '[snip]' lately but I don't know what it means. >> >> Placeholder for "I've cut some of your patch content here to shorten >> the message I'm sending". >> >>>>> diff --git a/xen/include/xen/monitor.h b/xen/include/xen/monitor.h >>>>> index 2171d04..605caf0 100644 >>>>> --- a/xen/include/xen/monitor.h >>>>> +++ b/xen/include/xen/monitor.h >>>>> @@ -22,12 +22,15 @@ >>>>> #ifndef __XEN_MONITOR_H__ >>>>> #define __XEN_MONITOR_H__ >>>>> >>>>> -#include <public/vm_event.h> >>>>> - >>>>> -struct domain; >>>>> -struct xen_domctl_monitor_op; >>>>> +#include <xen/sched.h> >>>>> >>>>> int monitor_domctl(struct domain *d, struct xen_domctl_monitor_op >>>>> *op); >>>>> + >>>>> +static inline bool_t monitor_domain_initialised(const struct domain >>>>> *d) >>>>> +{ >>>>> + return d->monitor.initialised; >>>> >>>> This should be !!d->monitor.initialised. >>> >>> >>> It's the value of a bit, thus should be 0 or 1. Am I missing something? >> >> Using a bitfield is effectively doing a bitmask for the bit(s). >> However, returning the value after applying a bitmask is not >> necessarily 0/1 (ie. bool_t). For example I ran into problems with >> this in the past (see >> https://lists.xen.org/archives/html/xen-devel/2015-08/msg01948.html). >> >> Tamas > > > The example you provided actually returns a value &-ed with a flag > (bitmask), of course it returns either 0 or the flag itself :-). AFAIK a > single-bit item in a bitfield (note a -bitfield-, not e.g. an unsigned int > &-ed with (1<<x)) will always be either 0 or 1. > I would assume as well but I'm not actually sure if that's guaranteed or if it's only compiler defined behavior. Tamas
On 7/12/2016 12:27 AM, Tamas K Lengyel wrote: > On Mon, Jul 11, 2016 at 2:20 PM, Corneliu ZUZU <czuzu@bitdefender.com> wrote: >> On 7/11/2016 7:38 PM, Tamas K Lengyel wrote: >>>>>> diff --git a/xen/include/asm-arm/monitor.h >>>>>> b/xen/include/asm-arm/monitor.h >>>>>> index 9a9734a..7ef30f1 100644 >>>>> [snip] >>>> >>>> I keep seeing '[snip]' lately but I don't know what it means. >>> Placeholder for "I've cut some of your patch content here to shorten >>> the message I'm sending". >>> >>>>>> diff --git a/xen/include/xen/monitor.h b/xen/include/xen/monitor.h >>>>>> index 2171d04..605caf0 100644 >>>>>> --- a/xen/include/xen/monitor.h >>>>>> +++ b/xen/include/xen/monitor.h >>>>>> @@ -22,12 +22,15 @@ >>>>>> #ifndef __XEN_MONITOR_H__ >>>>>> #define __XEN_MONITOR_H__ >>>>>> >>>>>> -#include <public/vm_event.h> >>>>>> - >>>>>> -struct domain; >>>>>> -struct xen_domctl_monitor_op; >>>>>> +#include <xen/sched.h> >>>>>> >>>>>> int monitor_domctl(struct domain *d, struct xen_domctl_monitor_op >>>>>> *op); >>>>>> + >>>>>> +static inline bool_t monitor_domain_initialised(const struct domain >>>>>> *d) >>>>>> +{ >>>>>> + return d->monitor.initialised; >>>>> This should be !!d->monitor.initialised. >>>> >>>> It's the value of a bit, thus should be 0 or 1. Am I missing something? >>> Using a bitfield is effectively doing a bitmask for the bit(s). >>> However, returning the value after applying a bitmask is not >>> necessarily 0/1 (ie. bool_t). For example I ran into problems with >>> this in the past (see >>> https://lists.xen.org/archives/html/xen-devel/2015-08/msg01948.html). >>> >>> Tamas >> >> The example you provided actually returns a value &-ed with a flag >> (bitmask), of course it returns either 0 or the flag itself :-). AFAIK a >> single-bit item in a bitfield (note a -bitfield-, not e.g. an unsigned int >> &-ed with (1<<x)) will always be either 0 or 1. >> > I would assume as well but I'm not actually sure if that's guaranteed > or if it's only compiler defined behavior. > > Tamas > As per http://en.cppreference.com/w/c/language/bit_field . "The number of bits in a bit field (width) sets the limit to the range of values it can hold" So if it's width is 1, it can either be 0 or 1. Corneliu.
diff --git a/xen/arch/x86/hvm/emulate.c b/xen/arch/x86/hvm/emulate.c index 855af4d..a0094e9 100644 --- a/xen/arch/x86/hvm/emulate.c +++ b/xen/arch/x86/hvm/emulate.c @@ -12,6 +12,7 @@ #include <xen/config.h> #include <xen/init.h> #include <xen/lib.h> +#include <xen/monitor.h> #include <xen/sched.h> #include <xen/paging.h> #include <xen/trace.h> @@ -73,7 +74,7 @@ static int set_context_data(void *buffer, unsigned int size) { struct vcpu *curr = current; - if ( curr->arch.vm_event ) + if ( unlikely(monitor_domain_initialised(curr->domain)) ) { unsigned int safe_size = min(size, curr->arch.vm_event->emul_read_data.size); diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c index 79ba185..46fed33 100644 --- a/xen/arch/x86/hvm/hvm.c +++ b/xen/arch/x86/hvm/hvm.c @@ -471,7 +471,7 @@ void hvm_do_resume(struct vcpu *v) if ( !handle_hvm_io_completion(v) ) return; - if ( unlikely(v->arch.vm_event) ) + if ( unlikely(monitor_domain_initialised(v->domain)) ) { if ( unlikely(v->arch.vm_event->emulate_flags) ) { @@ -2176,7 +2176,7 @@ int hvm_set_cr0(unsigned long value, bool_t may_defer) if ( may_defer && unlikely(v->domain->arch.monitor.write_ctrlreg_enabled & monitor_ctrlreg_bitmask(VM_EVENT_X86_CR0)) ) { - ASSERT(v->arch.vm_event); + ASSERT(monitor_domain_initialised(v->domain)); if ( hvm_monitor_crX(CR0, value, old_value) ) { @@ -2281,7 +2281,7 @@ int hvm_set_cr3(unsigned long value, bool_t may_defer) if ( may_defer && unlikely(v->domain->arch.monitor.write_ctrlreg_enabled & monitor_ctrlreg_bitmask(VM_EVENT_X86_CR3)) ) { - ASSERT(v->arch.vm_event); + ASSERT(monitor_domain_initialised(v->domain)); if ( hvm_monitor_crX(CR3, value, old) ) { @@ -2364,7 +2364,7 @@ int hvm_set_cr4(unsigned long value, bool_t may_defer) if ( may_defer && unlikely(v->domain->arch.monitor.write_ctrlreg_enabled & monitor_ctrlreg_bitmask(VM_EVENT_X86_CR4)) ) { - ASSERT(v->arch.vm_event); + ASSERT(monitor_domain_initialised(v->domain)); if ( hvm_monitor_crX(CR4, value, old_cr) ) { @@ -3748,7 +3748,7 @@ int hvm_msr_write_intercept(unsigned int msr, uint64_t msr_content, if ( may_defer && unlikely(monitored_msr(v->domain, msr)) ) { - ASSERT(v->arch.vm_event); + ASSERT(monitor_domain_initialised(v->domain)); /* * The actual write will occur in monitor_ctrlreg_write_data(), if diff --git a/xen/arch/x86/monitor.c b/xen/arch/x86/monitor.c index aeee435..4a29cad 100644 --- a/xen/arch/x86/monitor.c +++ b/xen/arch/x86/monitor.c @@ -62,6 +62,8 @@ int monitor_init_domain(struct domain *d) return -ENOMEM; } + d->monitor.initialised = 1; + return 0; } @@ -78,6 +80,7 @@ void monitor_cleanup_domain(struct domain *d) memset(&d->arch.monitor, 0, sizeof(d->arch.monitor)); memset(&d->monitor, 0, sizeof(d->monitor)); + d->monitor.initialised = 0; } void monitor_ctrlreg_write_resume(struct vcpu *v, vm_event_response_t *rsp) diff --git a/xen/include/asm-arm/monitor.h b/xen/include/asm-arm/monitor.h index 9a9734a..7ef30f1 100644 --- a/xen/include/asm-arm/monitor.h +++ b/xen/include/asm-arm/monitor.h @@ -48,13 +48,14 @@ int arch_monitor_domctl_event(struct domain *d, static inline int monitor_init_domain(struct domain *d) { - /* No arch-specific domain initialization on ARM. */ + d->monitor.initialised = 1; return 0; } static inline void monitor_cleanup_domain(struct domain *d) { memset(&d->monitor, 0, sizeof(d->monitor)); + d->monitor.initialised = 0; } static inline diff --git a/xen/include/asm-x86/monitor.h b/xen/include/asm-x86/monitor.h index 3ae24dd..4014f8d 100644 --- a/xen/include/asm-x86/monitor.h +++ b/xen/include/asm-x86/monitor.h @@ -22,6 +22,7 @@ #ifndef __ASM_X86_MONITOR_H__ #define __ASM_X86_MONITOR_H__ +#include <xen/monitor.h> #include <xen/sched.h> #define monitor_ctrlreg_bitmask(ctrlreg_index) (1U << (ctrlreg_index)) @@ -41,11 +42,9 @@ int arch_monitor_domctl_op(struct domain *d, struct xen_domctl_monitor_op *mop) { case XEN_DOMCTL_MONITOR_OP_EMULATE_EACH_REP: domain_pause(d); - /* - * 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 ) + + /* Meaningless without a monitor vm-events subscriber. */ + if ( likely(monitor_domain_initialised(d)) ) d->arch.mem_access_emulate_each_rep = !!mop->event; else rc = -EINVAL; diff --git a/xen/include/xen/monitor.h b/xen/include/xen/monitor.h index 2171d04..605caf0 100644 --- a/xen/include/xen/monitor.h +++ b/xen/include/xen/monitor.h @@ -22,12 +22,15 @@ #ifndef __XEN_MONITOR_H__ #define __XEN_MONITOR_H__ -#include <public/vm_event.h> - -struct domain; -struct xen_domctl_monitor_op; +#include <xen/sched.h> int monitor_domctl(struct domain *d, struct xen_domctl_monitor_op *op); + +static inline bool_t monitor_domain_initialised(const struct domain *d) +{ + return d->monitor.initialised; +} + void monitor_guest_request(void); int monitor_traps(struct vcpu *v, bool_t sync, vm_event_request_t *req); diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h index 46c82e7..e6bd13f 100644 --- a/xen/include/xen/sched.h +++ b/xen/include/xen/sched.h @@ -472,6 +472,7 @@ struct domain struct { unsigned int guest_request_enabled : 1; unsigned int guest_request_sync : 1; + unsigned int initialised : 1; } monitor; };
As it can be seen looking @ the vm_event_per_domain structure, there are 3 defined vm-event subsystems: share, paging and monitor. In a number of places in the codebase, the monitor vm-events subsystem is mistakenly confounded with the vm-event subsystem as a whole: i.e. it is wrongfully checked if v->arch.vm_event is allocated to determine if the monitor subsystem is initialized. To fix that, add an 'initialised' bit in d->monitor which will determine if the monitor subsystem is initialised, create an inline stub monitor_domain_initialised() and use that instead where it applies. Signed-off-by: Corneliu ZUZU <czuzu@bitdefender.com> --- xen/arch/x86/hvm/emulate.c | 3 ++- xen/arch/x86/hvm/hvm.c | 10 +++++----- xen/arch/x86/monitor.c | 3 +++ xen/include/asm-arm/monitor.h | 3 ++- xen/include/asm-x86/monitor.h | 9 ++++----- xen/include/xen/monitor.h | 11 +++++++---- xen/include/xen/sched.h | 1 + 7 files changed, 24 insertions(+), 16 deletions(-)