Re: [Xen-devel] [PATCH] xen/x86: Clean up vm_event-related code in asm-x86/domain.h
On 14.08.15 at 12:30, rcojoc...@bitdefender.com wrote:
On 08/14/2015 01:21 PM, Jan Beulich wrote:
On 14.08.15 at 11:54, rcojoc...@bitdefender.com wrote:
@@ -460,6 +459,18 @@ typedef enum __packed {
SMAP_CHECK_DISABLED,/* disable the check */
} smap_check_policy_t;
+/*
+ * Should we emulate the next matching instruction on VCPU resume
+ * after a vm_event?
+ */
+struct vm_event {
+uint32_t emulate_flags;
+unsigned long gpa;
+unsigned long eip;
+struct vm_event_emul_read_data emul_read_data;
+struct monitor_write_data write_data;
+};
... this would better go into asm-x86/vm_event.h (despite it meaning
that the file will then be included by basically everything).
Ack.
And actually (having looked another time) I don't think this implies
including asm/vm_event.h from asm-x86/domain.h.
Jan
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
[Xen-devel] [PATCH] xen/x86: Clean up vm_event-related code in asm-x86/domain.h
As suggested by Jan Beulich, moved struct monitor_write_data from
struct arch_domain to struct arch_vcpu, as well as moving all
vm_event-related data from asm-x86/domain.h to struct vm_event,
and allocating it dynamically only when needed.
Signed-off-by: Razvan Cojocaru rcojoc...@bitdefender.com
---
xen/arch/x86/domain.c| 10 ++
xen/arch/x86/hvm/emulate.c |6 +++---
xen/arch/x86/hvm/hvm.c | 32
xen/arch/x86/mm/p2m.c| 24
xen/arch/x86/vm_event.c | 26 +++---
xen/include/asm-x86/domain.h | 26 ++
6 files changed, 54 insertions(+), 70 deletions(-)
diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
index 045f6ff..58e173e 100644
--- a/xen/arch/x86/domain.c
+++ b/xen/arch/x86/domain.c
@@ -424,9 +424,6 @@ int vcpu_initialise(struct vcpu *v)
v-arch.flags = TF_kernel_mode;
-/* By default, do not emulate */
-v-arch.vm_event.emulate_flags = 0;
-
rc = mapcache_vcpu_init(v);
if ( rc )
return rc;
@@ -511,8 +508,8 @@ int vcpu_initialise(struct vcpu *v)
void vcpu_destroy(struct vcpu *v)
{
-xfree(v-arch.vm_event.emul_read_data);
-v-arch.vm_event.emul_read_data = NULL;
+xfree(v-arch.vm_event);
+v-arch.vm_event = NULL;
if ( is_pv_32bit_vcpu(v) )
{
@@ -668,9 +665,6 @@ int arch_domain_create(struct domain *d, unsigned int
domcr_flags,
void arch_domain_destroy(struct domain *d)
{
-vfree(d-arch.event_write_data);
-d-arch.event_write_data = NULL;
-
if ( has_hvm_container_domain(d) )
hvm_domain_destroy(d);
diff --git a/xen/arch/x86/hvm/emulate.c b/xen/arch/x86/hvm/emulate.c
index 30acb78..d0f98c8 100644
--- a/xen/arch/x86/hvm/emulate.c
+++ b/xen/arch/x86/hvm/emulate.c
@@ -71,12 +71,12 @@ static int set_context_data(void *buffer, unsigned int size)
{
struct vcpu *curr = current;
-if ( curr-arch.vm_event.emul_read_data )
+if ( curr-arch.vm_event )
{
unsigned int safe_size =
-min(size, curr-arch.vm_event.emul_read_data-size);
+min(size, curr-arch.vm_event-emul_read_data.size);
-memcpy(buffer, curr-arch.vm_event.emul_read_data-data, safe_size);
+memcpy(buffer, curr-arch.vm_event-emul_read_data.data, safe_size);
memset(buffer + safe_size, 0, size - safe_size);
return X86EMUL_OKAY;
}
diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index c957610..9bc698c 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -541,9 +541,9 @@ void hvm_do_resume(struct vcpu *v)
break;
}
-if ( unlikely(d-arch.event_write_data) )
+if ( unlikely(v-arch.vm_event) )
{
-struct monitor_write_data *w = d-arch.event_write_data[v-vcpu_id];
+struct monitor_write_data *w = v-arch.vm_event-write_data;
if ( w-do_write.msr )
{
@@ -571,7 +571,7 @@ void hvm_do_resume(struct vcpu *v)
}
/* Inject pending hw/sw trap */
-if ( v-arch.hvm_vcpu.inject_trap.vector != -1 )
+if ( v-arch.hvm_vcpu.inject_trap.vector != -1 )
{
hvm_inject_trap(v-arch.hvm_vcpu.inject_trap);
v-arch.hvm_vcpu.inject_trap.vector = -1;
@@ -3371,13 +3371,13 @@ int hvm_set_cr0(unsigned long value, bool_t may_defer)
monitor_ctrlreg_bitmask(VM_EVENT_X86_CR0))
value != old_value )
{
-ASSERT(currad-event_write_data != NULL);
+ASSERT(v-arch.vm_event != NULL);
if ( hvm_event_crX(CR0, value, old_value) )
{
/* The actual write will occur in hvm_do_resume(), if permitted. */
-currad-event_write_data[v-vcpu_id].do_write.cr0 = 1;
-currad-event_write_data[v-vcpu_id].cr0 = value;
+v-arch.vm_event-write_data.do_write.cr0 = 1;
+v-arch.vm_event-write_data.cr0 = value;
return X86EMUL_OKAY;
}
@@ -3475,13 +3475,13 @@ int hvm_set_cr3(unsigned long value, bool_t may_defer)
monitor_ctrlreg_bitmask(VM_EVENT_X86_CR3))
value != old )
{
-ASSERT(currad-event_write_data != NULL);
+ASSERT(v-arch.vm_event != NULL);
if ( hvm_event_crX(CR3, value, old) )
{
/* The actual write will occur in hvm_do_resume(), if permitted. */
-currad-event_write_data[v-vcpu_id].do_write.cr3 = 1;
-currad-event_write_data[v-vcpu_id].cr3 = value;
+v-arch.vm_event-write_data.do_write.cr3 = 1;
+v-arch.vm_event-write_data.cr3 = value;
return X86EMUL_OKAY;
}
@@ -3549,13 +3549,13 @@ int hvm_set_cr4(unsigned long value, bool_t may_defer)
monitor_ctrlreg_bitmask(VM_EVENT_X86_CR4))
value != old_cr )
{
-ASSERT(currad-event_write_data != NULL);
+ASSERT(v-arch.vm_event != NULL);
if (
Re: [Xen-devel] [PATCH] xen/x86: Clean up vm_event-related code in asm-x86/domain.h
On 14.08.15 at 11:54, rcojoc...@bitdefender.com wrote:
@@ -571,7 +571,7 @@ void hvm_do_resume(struct vcpu *v)
}
/* Inject pending hw/sw trap */
-if ( v-arch.hvm_vcpu.inject_trap.vector != -1 )
+if ( v-arch.hvm_vcpu.inject_trap.vector != -1 )
Stray whitespace change to unrelated code.
@@ -3371,13 +3371,13 @@ int hvm_set_cr0(unsigned long value, bool_t
may_defer)
monitor_ctrlreg_bitmask(VM_EVENT_X86_CR0))
value != old_value )
{
-ASSERT(currad-event_write_data != NULL);
+ASSERT(v-arch.vm_event != NULL);
May I recommend dropping the redundant != NULL namely in ASSERT()
expressions (the only effect they have is produce larger string literals
in debug builds).
if ( hvm_event_crX(CR0, value, old_value) )
{
/* The actual write will occur in hvm_do_resume(), if permitted.
*/
-currad-event_write_data[v-vcpu_id].do_write.cr0 = 1;
-currad-event_write_data[v-vcpu_id].cr0 = value;
+v-arch.vm_event-write_data.do_write.cr0 = 1;
+v-arch.vm_event-write_data.cr0 = value;
Looks like this leaves just a single use of currad in the function, in
which case I'd like to see the variable go away.
--- a/xen/include/asm-x86/domain.h
+++ b/xen/include/asm-x86/domain.h
@@ -8,6 +8,7 @@
#include asm/hvm/domain.h
#include asm/e820.h
#include asm/mce.h
+#include public/vm_event.h
It looks like both this and ...
@@ -460,6 +459,18 @@ typedef enum __packed {
SMAP_CHECK_DISABLED,/* disable the check */
} smap_check_policy_t;
+/*
+ * Should we emulate the next matching instruction on VCPU resume
+ * after a vm_event?
+ */
+struct vm_event {
+uint32_t emulate_flags;
+unsigned long gpa;
+unsigned long eip;
+struct vm_event_emul_read_data emul_read_data;
+struct monitor_write_data write_data;
+};
... this would better go into asm-x86/vm_event.h (despite it meaning
that the file will then be included by basically everything).
Jan
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH] xen/x86: Clean up vm_event-related code in asm-x86/domain.h
On 08/14/2015 01:21 PM, Jan Beulich wrote:
On 14.08.15 at 11:54, rcojoc...@bitdefender.com wrote:
@@ -571,7 +571,7 @@ void hvm_do_resume(struct vcpu *v)
}
/* Inject pending hw/sw trap */
-if ( v-arch.hvm_vcpu.inject_trap.vector != -1 )
+if ( v-arch.hvm_vcpu.inject_trap.vector != -1 )
Stray whitespace change to unrelated code.
Understood, thought I'd remove the trailing whitespace there, but I will
leave the unrelated code alone.
@@ -3371,13 +3371,13 @@ int hvm_set_cr0(unsigned long value, bool_t
may_defer)
monitor_ctrlreg_bitmask(VM_EVENT_X86_CR0))
value != old_value )
{
-ASSERT(currad-event_write_data != NULL);
+ASSERT(v-arch.vm_event != NULL);
May I recommend dropping the redundant != NULL namely in ASSERT()
expressions (the only effect they have is produce larger string literals
in debug builds).
Of course, I'll change it.
if ( hvm_event_crX(CR0, value, old_value) )
{
/* The actual write will occur in hvm_do_resume(), if
permitted. */
-currad-event_write_data[v-vcpu_id].do_write.cr0 = 1;
-currad-event_write_data[v-vcpu_id].cr0 = value;
+v-arch.vm_event-write_data.do_write.cr0 = 1;
+v-arch.vm_event-write_data.cr0 = value;
Looks like this leaves just a single use of currad in the function, in
which case I'd like to see the variable go away.
I'll remove it.
--- a/xen/include/asm-x86/domain.h
+++ b/xen/include/asm-x86/domain.h
@@ -8,6 +8,7 @@
#include asm/hvm/domain.h
#include asm/e820.h
#include asm/mce.h
+#include public/vm_event.h
It looks like both this and ...
@@ -460,6 +459,18 @@ typedef enum __packed {
SMAP_CHECK_DISABLED,/* disable the check */
} smap_check_policy_t;
+/*
+ * Should we emulate the next matching instruction on VCPU resume
+ * after a vm_event?
+ */
+struct vm_event {
+uint32_t emulate_flags;
+unsigned long gpa;
+unsigned long eip;
+struct vm_event_emul_read_data emul_read_data;
+struct monitor_write_data write_data;
+};
... this would better go into asm-x86/vm_event.h (despite it meaning
that the file will then be included by basically everything).
Ack. Thank you for the prompt review!
Thanks,
Razvan
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
