* [PATCH] x86/monitor: Disallow setting mem_access_emulate_each_rep when vm_event is NULL
@ 2016-04-08 7:10 Razvan Cojocaru
2016-04-08 7:16 ` Razvan Cojocaru
0 siblings, 1 reply; 4+ messages in thread
From: Razvan Cojocaru @ 2016-04-08 7:10 UTC (permalink / raw)
To: xen-devel; +Cc: andrew.cooper3, keir, Razvan Cojocaru, jbeulich
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 )
+ {
+ domain_unpause(d);
+ return -EINVAL;
+ }
+ d->arch.mem_access_emulate_each_rep = value;
domain_unpause(d);
break;
-
+ }
default:
return -EOPNOTSUPP;
}
--
1.9.1
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] x86/monitor: Disallow setting mem_access_emulate_each_rep when vm_event is NULL
2016-04-08 7:10 [PATCH] x86/monitor: Disallow setting mem_access_emulate_each_rep when vm_event is NULL Razvan Cojocaru
@ 2016-04-08 7:16 ` Razvan Cojocaru
2016-04-08 8:17 ` Andrew Cooper
0 siblings, 1 reply; 4+ messages in thread
From: Razvan Cojocaru @ 2016-04-08 7:16 UTC (permalink / raw)
To: xen-devel; +Cc: andrew.cooper3, keir, jbeulich
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
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] x86/monitor: Disallow setting mem_access_emulate_each_rep when vm_event is NULL
2016-04-08 7:16 ` Razvan Cojocaru
@ 2016-04-08 8:17 ` Andrew Cooper
2016-04-08 8:29 ` Razvan Cojocaru
0 siblings, 1 reply; 4+ messages in thread
From: Andrew Cooper @ 2016-04-08 8:17 UTC (permalink / raw)
To: Razvan Cojocaru, xen-devel; +Cc: keir, jbeulich
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
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] x86/monitor: Disallow setting mem_access_emulate_each_rep when vm_event is NULL
2016-04-08 8:17 ` Andrew Cooper
@ 2016-04-08 8:29 ` Razvan Cojocaru
0 siblings, 0 replies; 4+ messages in thread
From: Razvan Cojocaru @ 2016-04-08 8:29 UTC (permalink / raw)
To: Andrew Cooper, xen-devel; +Cc: keir, jbeulich
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
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2016-04-08 8:29 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-04-08 7:10 [PATCH] x86/monitor: Disallow setting mem_access_emulate_each_rep when vm_event is NULL Razvan Cojocaru
2016-04-08 7:16 ` Razvan Cojocaru
2016-04-08 8:17 ` Andrew Cooper
2016-04-08 8:29 ` Razvan Cojocaru
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).