* [PATCH 00/16] x86/vm-event: numerous adjustments & fixes
@ 2016-07-09 4:11 Corneliu ZUZU
2016-07-09 4:12 ` [PATCH 01/16] x86/vmx_update_guest_cr: minor optimization Corneliu ZUZU
` (16 more replies)
0 siblings, 17 replies; 54+ messages in thread
From: Corneliu ZUZU @ 2016-07-09 4:11 UTC (permalink / raw)
To: xen-devel
Cc: Kevin Tian, Stefano Stabellini, Jan Beulich, Razvan Cojocaru,
George Dunlap, Andrew Cooper, Julien Grall, Paul Durrant,
Tamas K Lengyel, Jun Nakajima
Adjustments & fixes to the vm-events code, mostly monitor-related.
As I hadn't got the time/proper context to test these changes on a real machine,
for now please consider them only for review, I'll let you know when and how
actual testing went.
Thanks,
Corneliu ZUZU (16):
x86/vmx_update_guest_cr: minor optimization
x86: fix: make atomic_read() param const
x86/monitor: mechanical renames
x86/monitor: relocate vm_event_register_write_resume() function to
monitor code
x86/monitor: relocate code more appropriately
x86/monitor: fix: set msr_bitmap to NULL after xfree
x86/vm-event: fix: call cleanup when init fails, to free partial
allocs
x86/vm-event: call monitor init & cleanup funcs from respective
vm_event funcs
arm/monitor: move d->monitor cleanup to monitor_cleanup_domain()
x86/vm-event: centralize vcpu-destroy cleanup in vm-events code
x86/monitor: fix: treat -monitor- properly, as a subsys of the
vm-event subsys
x86/vm-event: fix: move cleanup of mem_access_emulate_each_rep to
monitor stub
x86/monitor: introduce writes_pending field in monitor_write_data
x86/monitor: clarify separation between monitor subsys and vm-event as
a whole
x86/monitor: fix: don't compromise a monitor_write_data with pending
CR writes
x86/monitor: fix: xc_monitor _write_ctrlreg w/o previous _enable must
fail
xen/arch/x86/domain.c | 4 +-
xen/arch/x86/hvm/emulate.c | 11 +-
xen/arch/x86/hvm/hvm.c | 92 ++++++++--------
xen/arch/x86/hvm/vmx/vmx.c | 64 +++++++++--
xen/arch/x86/mm/p2m.c | 7 +-
xen/arch/x86/monitor.c | 218 +++++++++++++++++++++++++++++++++++---
xen/arch/x86/vm_event.c | 82 +++++++-------
xen/common/monitor.c | 4 +-
xen/common/vm_event.c | 37 ++++---
xen/include/asm-arm/monitor.h | 17 +--
xen/include/asm-arm/vm_event.h | 21 ++--
xen/include/asm-x86/atomic.h | 4 +-
xen/include/asm-x86/domain.h | 31 ++++--
xen/include/asm-x86/hvm/vmx/vmx.h | 1 +
xen/include/asm-x86/monitor.h | 32 ++++--
xen/include/asm-x86/vm_event.h | 27 ++---
xen/include/xen/monitor.h | 11 +-
xen/include/xen/sched.h | 1 +
18 files changed, 473 insertions(+), 191 deletions(-)
--
2.5.0
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 54+ messages in thread
* [PATCH 01/16] x86/vmx_update_guest_cr: minor optimization
2016-07-09 4:11 [PATCH 00/16] x86/vm-event: numerous adjustments & fixes Corneliu ZUZU
@ 2016-07-09 4:12 ` Corneliu ZUZU
2016-07-11 6:24 ` Corneliu ZUZU
2016-07-09 4:12 ` [PATCH 02/16] x86: fix: make atomic_read() param const Corneliu ZUZU
` (15 subsequent siblings)
16 siblings, 1 reply; 54+ messages in thread
From: Corneliu ZUZU @ 2016-07-09 4:12 UTC (permalink / raw)
To: xen-devel; +Cc: Andrew Cooper, Kevin Tian, Jan Beulich, Jun Nakajima
Minor optimization @ vmx_update_guest_cr: checks if v->arch.hvm_vmx.exec_control
was modified before actually calling vmx_update_cpu_exec_control(v).
Signed-off-by: Corneliu ZUZU <czuzu@bitdefender.com>
---
xen/arch/x86/hvm/vmx/vmx.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
index 0776d12..0798245 100644
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -1433,8 +1433,10 @@ static void vmx_update_guest_cr(struct vcpu *v, unsigned int cr)
if ( paging_mode_hap(v->domain) )
{
/* Manage GUEST_CR3 when CR0.PE=0. */
+ uint32_t old_ctls = v->arch.hvm_vmx.exec_control;
uint32_t cr3_ctls = (CPU_BASED_CR3_LOAD_EXITING |
CPU_BASED_CR3_STORE_EXITING);
+
v->arch.hvm_vmx.exec_control &= ~cr3_ctls;
if ( !hvm_paging_enabled(v) && !vmx_unrestricted_guest(v) )
v->arch.hvm_vmx.exec_control |= cr3_ctls;
@@ -1444,7 +1446,8 @@ static void vmx_update_guest_cr(struct vcpu *v, unsigned int cr)
monitor_ctrlreg_bitmask(VM_EVENT_X86_CR3) )
v->arch.hvm_vmx.exec_control |= CPU_BASED_CR3_LOAD_EXITING;
- vmx_update_cpu_exec_control(v);
+ if ( old_ctls != v->arch.hvm_vmx.exec_control )
+ vmx_update_cpu_exec_control(v);
}
if ( !nestedhvm_vcpu_in_guestmode(v) )
--
2.5.0
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 54+ messages in thread
* [PATCH 02/16] x86: fix: make atomic_read() param const
2016-07-09 4:11 [PATCH 00/16] x86/vm-event: numerous adjustments & fixes Corneliu ZUZU
2016-07-09 4:12 ` [PATCH 01/16] x86/vmx_update_guest_cr: minor optimization Corneliu ZUZU
@ 2016-07-09 4:12 ` Corneliu ZUZU
2016-07-11 15:18 ` Andrew Cooper
2016-07-09 4:13 ` [PATCH 03/16] x86/monitor: mechanical renames Corneliu ZUZU
` (14 subsequent siblings)
16 siblings, 1 reply; 54+ messages in thread
From: Corneliu ZUZU @ 2016-07-09 4:12 UTC (permalink / raw)
To: xen-devel; +Cc: Andrew Cooper, Jan Beulich
This wouldn't let me make a param of a function that used atomic_read() const.
Signed-off-by: Corneliu ZUZU <czuzu@bitdefender.com>
---
xen/include/asm-x86/atomic.h | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/xen/include/asm-x86/atomic.h b/xen/include/asm-x86/atomic.h
index d246b70..0b250c8 100644
--- a/xen/include/asm-x86/atomic.h
+++ b/xen/include/asm-x86/atomic.h
@@ -94,7 +94,7 @@ typedef struct { int counter; } atomic_t;
*
* Atomically reads the value of @v.
*/
-static inline int atomic_read(atomic_t *v)
+static inline int atomic_read(const atomic_t *v)
{
return read_atomic(&v->counter);
}
@@ -105,7 +105,7 @@ static inline int atomic_read(atomic_t *v)
*
* Non-atomically reads the value of @v
*/
-static inline int _atomic_read(atomic_t v)
+static inline int _atomic_read(const atomic_t v)
{
return v.counter;
}
--
2.5.0
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 54+ messages in thread
* [PATCH 03/16] x86/monitor: mechanical renames
2016-07-09 4:11 [PATCH 00/16] x86/vm-event: numerous adjustments & fixes Corneliu ZUZU
2016-07-09 4:12 ` [PATCH 01/16] x86/vmx_update_guest_cr: minor optimization Corneliu ZUZU
2016-07-09 4:12 ` [PATCH 02/16] x86: fix: make atomic_read() param const Corneliu ZUZU
@ 2016-07-09 4:13 ` Corneliu ZUZU
2016-07-09 18:10 ` Tamas K Lengyel
2016-07-09 4:14 ` [PATCH 04/16] x86/monitor: relocate vm_event_register_write_resume() function to monitor code Corneliu ZUZU
` (13 subsequent siblings)
16 siblings, 1 reply; 54+ messages in thread
From: Corneliu ZUZU @ 2016-07-09 4:13 UTC (permalink / raw)
To: xen-devel
Cc: Tamas K Lengyel, Razvan Cojocaru, Andrew Cooper, Julien Grall,
Stefano Stabellini, Jan Beulich
Arch-specific vm-event functions in x86/vm_event.h -e.g. vm_event_init_domain()-
don't have an 'arch_' prefix. Apply the same rule for monitor functions -
originally the only two monitor functions that had an 'arch_' prefix were
arch_monitor_domctl_event() and arch_monitor_domctl_op(), but I gave them that
prefix because -they had a counterpart function in common code-, that being
monitor_domctl().
Let this also be the rule for future 'arch_' functions additions, and with this
patch remove the 'arch_' prefix from the monitor functions that don't have a
counterpart in common-code (all but those 2 aforementioned).
Signed-off-by: Corneliu ZUZU <czuzu@bitdefender.com>
---
xen/arch/x86/monitor.c | 4 ++--
xen/common/monitor.c | 4 ++--
xen/common/vm_event.c | 4 ++--
xen/include/asm-arm/monitor.h | 8 +++-----
xen/include/asm-x86/monitor.h | 12 ++++++------
5 files changed, 15 insertions(+), 17 deletions(-)
diff --git a/xen/arch/x86/monitor.c b/xen/arch/x86/monitor.c
index 205df41..043815a 100644
--- a/xen/arch/x86/monitor.c
+++ b/xen/arch/x86/monitor.c
@@ -22,7 +22,7 @@
#include <asm/monitor.h>
#include <public/vm_event.h>
-int arch_monitor_init_domain(struct domain *d)
+int monitor_init_domain(struct domain *d)
{
if ( !d->arch.monitor.msr_bitmap )
d->arch.monitor.msr_bitmap = xzalloc(struct monitor_msr_bitmap);
@@ -33,7 +33,7 @@ int arch_monitor_init_domain(struct domain *d)
return 0;
}
-void arch_monitor_cleanup_domain(struct domain *d)
+void monitor_cleanup_domain(struct domain *d)
{
xfree(d->arch.monitor.msr_bitmap);
diff --git a/xen/common/monitor.c b/xen/common/monitor.c
index c73d1d5..5219238 100644
--- a/xen/common/monitor.c
+++ b/xen/common/monitor.c
@@ -50,12 +50,12 @@ int monitor_domctl(struct domain *d, struct xen_domctl_monitor_op *mop)
if ( unlikely(mop->event > 31) )
return -EINVAL;
/* Check if event type is available. */
- if ( unlikely(!(arch_monitor_get_capabilities(d) & (1U << mop->event))) )
+ if ( unlikely(!(monitor_get_capabilities(d) & (1U << mop->event))) )
return -EOPNOTSUPP;
break;
case XEN_DOMCTL_MONITOR_OP_GET_CAPABILITIES:
- mop->event = arch_monitor_get_capabilities(d);
+ mop->event = monitor_get_capabilities(d);
return 0;
default:
diff --git a/xen/common/vm_event.c b/xen/common/vm_event.c
index 941345b..b4f9fd3 100644
--- a/xen/common/vm_event.c
+++ b/xen/common/vm_event.c
@@ -669,7 +669,7 @@ int vm_event_domctl(struct domain *d, xen_domctl_vm_event_op_t *vec,
{
case XEN_VM_EVENT_ENABLE:
/* domain_pause() not required here, see XSA-99 */
- rc = arch_monitor_init_domain(d);
+ rc = monitor_init_domain(d);
if ( rc )
break;
rc = vm_event_enable(d, vec, ved, _VPF_mem_access,
@@ -682,7 +682,7 @@ int vm_event_domctl(struct domain *d, xen_domctl_vm_event_op_t *vec,
{
domain_pause(d);
rc = vm_event_disable(d, ved);
- arch_monitor_cleanup_domain(d);
+ monitor_cleanup_domain(d);
domain_unpause(d);
}
break;
diff --git a/xen/include/asm-arm/monitor.h b/xen/include/asm-arm/monitor.h
index 4af707a..c72ee42 100644
--- a/xen/include/asm-arm/monitor.h
+++ b/xen/include/asm-arm/monitor.h
@@ -46,20 +46,18 @@ int arch_monitor_domctl_event(struct domain *d,
return -EOPNOTSUPP;
}
-static inline
-int arch_monitor_init_domain(struct domain *d)
+static inline int monitor_init_domain(struct domain *d)
{
/* No arch-specific domain initialization on ARM. */
return 0;
}
-static inline
-void arch_monitor_cleanup_domain(struct domain *d)
+static inline void monitor_cleanup_domain(struct domain *d)
{
/* No arch-specific domain cleanup on ARM. */
}
-static inline uint32_t arch_monitor_get_capabilities(struct domain *d)
+static inline uint32_t monitor_get_capabilities(struct domain *d)
{
uint32_t capabilities = 0;
diff --git a/xen/include/asm-x86/monitor.h b/xen/include/asm-x86/monitor.h
index 0501ca2..c5ae7ef 100644
--- a/xen/include/asm-x86/monitor.h
+++ b/xen/include/asm-x86/monitor.h
@@ -60,7 +60,10 @@ int arch_monitor_domctl_op(struct domain *d, struct xen_domctl_monitor_op *mop)
return rc;
}
-static inline uint32_t arch_monitor_get_capabilities(struct domain *d)
+int arch_monitor_domctl_event(struct domain *d,
+ struct xen_domctl_monitor_op *mop);
+
+static inline uint32_t monitor_get_capabilities(struct domain *d)
{
uint32_t capabilities = 0;
@@ -84,12 +87,9 @@ static inline uint32_t arch_monitor_get_capabilities(struct domain *d)
return capabilities;
}
-int arch_monitor_domctl_event(struct domain *d,
- struct xen_domctl_monitor_op *mop);
-
-int arch_monitor_init_domain(struct domain *d);
+int monitor_init_domain(struct domain *d);
-void arch_monitor_cleanup_domain(struct domain *d);
+void monitor_cleanup_domain(struct domain *d);
bool_t monitored_msr(const struct domain *d, u32 msr);
--
2.5.0
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 54+ messages in thread
* [PATCH 04/16] x86/monitor: relocate vm_event_register_write_resume() function to monitor code
2016-07-09 4:11 [PATCH 00/16] x86/vm-event: numerous adjustments & fixes Corneliu ZUZU
` (2 preceding siblings ...)
2016-07-09 4:13 ` [PATCH 03/16] x86/monitor: mechanical renames Corneliu ZUZU
@ 2016-07-09 4:14 ` Corneliu ZUZU
2016-07-09 18:14 ` Tamas K Lengyel
2016-07-09 4:15 ` [PATCH 05/16] x86/monitor: relocate code more appropriately Corneliu ZUZU
` (12 subsequent siblings)
16 siblings, 1 reply; 54+ messages in thread
From: Corneliu ZUZU @ 2016-07-09 4:14 UTC (permalink / raw)
To: xen-devel
Cc: Tamas K Lengyel, Razvan Cojocaru, Andrew Cooper, Julien Grall,
Stefano Stabellini, Jan Beulich
vm_event_register_write_resume is part of the monitor subsystem, relocate and
rename appropriately.
Signed-off-by: Corneliu ZUZU <czuzu@bitdefender.com>
---
xen/arch/x86/monitor.c | 38 ++++++++++++++++++++++++++++++++++++++
xen/arch/x86/vm_event.c | 37 -------------------------------------
xen/common/vm_event.c | 2 +-
xen/include/asm-arm/monitor.h | 6 ++++++
xen/include/asm-arm/vm_event.h | 6 ------
xen/include/asm-x86/monitor.h | 2 ++
xen/include/asm-x86/vm_event.h | 2 --
7 files changed, 47 insertions(+), 46 deletions(-)
diff --git a/xen/arch/x86/monitor.c b/xen/arch/x86/monitor.c
index 043815a..06d21b1 100644
--- a/xen/arch/x86/monitor.c
+++ b/xen/arch/x86/monitor.c
@@ -20,6 +20,7 @@
*/
#include <asm/monitor.h>
+#include <asm/vm_event.h>
#include <public/vm_event.h>
int monitor_init_domain(struct domain *d)
@@ -41,6 +42,43 @@ void monitor_cleanup_domain(struct domain *d)
memset(&d->monitor, 0, sizeof(d->monitor));
}
+void monitor_ctrlreg_write_resume(struct vcpu *v, vm_event_response_t *rsp)
+{
+ if ( rsp->flags & VM_EVENT_FLAG_DENY )
+ {
+ struct monitor_write_data *w;
+
+ ASSERT(v->arch.vm_event);
+
+ /* deny flag requires the vCPU to be paused */
+ if ( !atomic_read(&v->vm_event_pause_count) )
+ return;
+
+ w = &v->arch.vm_event->write_data;
+
+ switch ( rsp->reason )
+ {
+ case VM_EVENT_REASON_MOV_TO_MSR:
+ w->do_write.msr = 0;
+ break;
+ case VM_EVENT_REASON_WRITE_CTRLREG:
+ switch ( rsp->u.write_ctrlreg.index )
+ {
+ case VM_EVENT_X86_CR0:
+ w->do_write.cr0 = 0;
+ break;
+ case VM_EVENT_X86_CR3:
+ w->do_write.cr3 = 0;
+ break;
+ case VM_EVENT_X86_CR4:
+ w->do_write.cr4 = 0;
+ break;
+ }
+ break;
+ }
+ }
+}
+
static unsigned long *monitor_bitmap_for_msr(const struct domain *d, u32 *msr)
{
ASSERT(d->arch.monitor.msr_bitmap && msr);
diff --git a/xen/arch/x86/vm_event.c b/xen/arch/x86/vm_event.c
index e938ca3..6e19df8 100644
--- a/xen/arch/x86/vm_event.c
+++ b/xen/arch/x86/vm_event.c
@@ -66,43 +66,6 @@ void vm_event_toggle_singlestep(struct domain *d, struct vcpu *v)
hvm_toggle_singlestep(v);
}
-void vm_event_register_write_resume(struct vcpu *v, vm_event_response_t *rsp)
-{
- if ( rsp->flags & VM_EVENT_FLAG_DENY )
- {
- struct monitor_write_data *w;
-
- ASSERT(v->arch.vm_event);
-
- /* deny flag requires the vCPU to be paused */
- if ( !atomic_read(&v->vm_event_pause_count) )
- return;
-
- w = &v->arch.vm_event->write_data;
-
- switch ( rsp->reason )
- {
- case VM_EVENT_REASON_MOV_TO_MSR:
- w->do_write.msr = 0;
- break;
- case VM_EVENT_REASON_WRITE_CTRLREG:
- switch ( rsp->u.write_ctrlreg.index )
- {
- case VM_EVENT_X86_CR0:
- w->do_write.cr0 = 0;
- break;
- case VM_EVENT_X86_CR3:
- w->do_write.cr3 = 0;
- break;
- case VM_EVENT_X86_CR4:
- w->do_write.cr4 = 0;
- break;
- }
- break;
- }
- }
-}
-
void vm_event_set_registers(struct vcpu *v, vm_event_response_t *rsp)
{
ASSERT(atomic_read(&v->vm_event_pause_count));
diff --git a/xen/common/vm_event.c b/xen/common/vm_event.c
index b4f9fd3..e60e1f2 100644
--- a/xen/common/vm_event.c
+++ b/xen/common/vm_event.c
@@ -397,7 +397,7 @@ void vm_event_resume(struct domain *d, struct vm_event_domain *ved)
case VM_EVENT_REASON_MOV_TO_MSR:
#endif
case VM_EVENT_REASON_WRITE_CTRLREG:
- vm_event_register_write_resume(v, &rsp);
+ monitor_ctrlreg_write_resume(v, &rsp);
break;
#ifdef CONFIG_HAS_MEM_ACCESS
diff --git a/xen/include/asm-arm/monitor.h b/xen/include/asm-arm/monitor.h
index c72ee42..5a0fc65 100644
--- a/xen/include/asm-arm/monitor.h
+++ b/xen/include/asm-arm/monitor.h
@@ -57,6 +57,12 @@ static inline void monitor_cleanup_domain(struct domain *d)
/* No arch-specific domain cleanup on ARM. */
}
+static inline
+void monitor_ctrlreg_write_resume(struct vcpu *v, vm_event_response_t *rsp)
+{
+ /* Not supported on ARM. */
+}
+
static inline uint32_t monitor_get_capabilities(struct domain *d)
{
uint32_t capabilities = 0;
diff --git a/xen/include/asm-arm/vm_event.h b/xen/include/asm-arm/vm_event.h
index ccc4b60..0129b04 100644
--- a/xen/include/asm-arm/vm_event.h
+++ b/xen/include/asm-arm/vm_event.h
@@ -40,12 +40,6 @@ static inline void vm_event_toggle_singlestep(struct domain *d, struct vcpu *v)
}
static inline
-void vm_event_register_write_resume(struct vcpu *v, vm_event_response_t *rsp)
-{
- /* Not supported on ARM. */
-}
-
-static inline
void vm_event_set_registers(struct vcpu *v, vm_event_response_t *rsp)
{
/* Not supported on ARM. */
diff --git a/xen/include/asm-x86/monitor.h b/xen/include/asm-x86/monitor.h
index c5ae7ef..adca378 100644
--- a/xen/include/asm-x86/monitor.h
+++ b/xen/include/asm-x86/monitor.h
@@ -91,6 +91,8 @@ int monitor_init_domain(struct domain *d);
void monitor_cleanup_domain(struct domain *d);
+void monitor_ctrlreg_write_resume(struct vcpu *v, vm_event_response_t *rsp);
+
bool_t monitored_msr(const struct domain *d, u32 msr);
#endif /* __ASM_X86_MONITOR_H__ */
diff --git a/xen/include/asm-x86/vm_event.h b/xen/include/asm-x86/vm_event.h
index 7e6adff..c53effb 100644
--- a/xen/include/asm-x86/vm_event.h
+++ b/xen/include/asm-x86/vm_event.h
@@ -37,8 +37,6 @@ void vm_event_cleanup_domain(struct domain *d);
void vm_event_toggle_singlestep(struct domain *d, struct vcpu *v);
-void vm_event_register_write_resume(struct vcpu *v, vm_event_response_t *rsp);
-
void vm_event_set_registers(struct vcpu *v, vm_event_response_t *rsp);
void vm_event_fill_regs(vm_event_request_t *req);
--
2.5.0
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 54+ messages in thread
* [PATCH 05/16] x86/monitor: relocate code more appropriately
2016-07-09 4:11 [PATCH 00/16] x86/vm-event: numerous adjustments & fixes Corneliu ZUZU
` (3 preceding siblings ...)
2016-07-09 4:14 ` [PATCH 04/16] x86/monitor: relocate vm_event_register_write_resume() function to monitor code Corneliu ZUZU
@ 2016-07-09 4:15 ` Corneliu ZUZU
2016-07-11 6:19 ` Corneliu ZUZU
2016-07-09 4:15 ` [PATCH 06/16] x86/monitor: fix: set msr_bitmap to NULL after xfree Corneliu ZUZU
` (11 subsequent siblings)
16 siblings, 1 reply; 54+ messages in thread
From: Corneliu ZUZU @ 2016-07-09 4:15 UTC (permalink / raw)
To: xen-devel
Cc: Kevin Tian, Tamas K Lengyel, Jan Beulich, Razvan Cojocaru,
Andrew Cooper, Jun Nakajima
For readability:
* Add function monitor_ctrlreg_write_data() (in x86/monitor.c) and separate
handling of monitor_write_data there (previously done directly in
hvm_do_resume()).
* Separate enabling/disabling of CPU_BASED_CR3_LOAD_EXITING for CR3 monitor
vm-events from CR0 node @ vmx_update_guest_cr(v, 0) into newly added function
vmx_vm_event_update_cr3_traps(d); also, for a better interface, create generic
functions monitor_ctrlreg_adjust_traps() and monitor_ctrlreg_disable_traps() (in
x86/monitor.c) which deal with setting/unsetting any traps for cr-write monitor
vm-events.
Signed-off-by: Corneliu ZUZU <czuzu@bitdefender.com>
---
xen/arch/x86/hvm/hvm.c | 46 ++++++++++----------------
xen/arch/x86/hvm/vmx/vmx.c | 59 ++++++++++++++++++++++++++++++---
xen/arch/x86/monitor.c | 68 +++++++++++++++++++++++++++++++++++----
xen/include/asm-x86/hvm/vmx/vmx.h | 1 +
xen/include/asm-x86/monitor.h | 2 ++
5 files changed, 135 insertions(+), 41 deletions(-)
diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index 7f99087..79ba185 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -473,8 +473,6 @@ void hvm_do_resume(struct vcpu *v)
if ( unlikely(v->arch.vm_event) )
{
- struct monitor_write_data *w = &v->arch.vm_event->write_data;
-
if ( unlikely(v->arch.vm_event->emulate_flags) )
{
enum emul_kind kind = EMUL_KIND_NORMAL;
@@ -492,29 +490,7 @@ void hvm_do_resume(struct vcpu *v)
v->arch.vm_event->emulate_flags = 0;
}
- if ( w->do_write.msr )
- {
- hvm_msr_write_intercept(w->msr, w->value, 0);
- w->do_write.msr = 0;
- }
-
- if ( w->do_write.cr0 )
- {
- hvm_set_cr0(w->cr0, 0);
- w->do_write.cr0 = 0;
- }
-
- if ( w->do_write.cr4 )
- {
- hvm_set_cr4(w->cr4, 0);
- w->do_write.cr4 = 0;
- }
-
- if ( w->do_write.cr3 )
- {
- hvm_set_cr3(w->cr3, 0);
- w->do_write.cr3 = 0;
- }
+ monitor_ctrlreg_write_data(v);
}
/* Inject pending hw/sw trap */
@@ -2204,7 +2180,10 @@ int hvm_set_cr0(unsigned long value, bool_t may_defer)
if ( hvm_monitor_crX(CR0, value, old_value) )
{
- /* The actual write will occur in hvm_do_resume(), if permitted. */
+ /*
+ * The actual write will occur in monitor_ctrlreg_write_data(), if
+ * permitted.
+ */
v->arch.vm_event->write_data.do_write.cr0 = 1;
v->arch.vm_event->write_data.cr0 = value;
@@ -2306,7 +2285,10 @@ int hvm_set_cr3(unsigned long value, bool_t may_defer)
if ( hvm_monitor_crX(CR3, value, old) )
{
- /* The actual write will occur in hvm_do_resume(), if permitted. */
+ /*
+ * The actual write will occur in monitor_ctrlreg_write_data(), if
+ * permitted.
+ */
v->arch.vm_event->write_data.do_write.cr3 = 1;
v->arch.vm_event->write_data.cr3 = value;
@@ -2386,7 +2368,10 @@ int hvm_set_cr4(unsigned long value, bool_t may_defer)
if ( hvm_monitor_crX(CR4, value, old_cr) )
{
- /* The actual write will occur in hvm_do_resume(), if permitted. */
+ /*
+ * The actual write will occur in monitor_ctrlreg_write_data(), if
+ * permitted.
+ */
v->arch.vm_event->write_data.do_write.cr4 = 1;
v->arch.vm_event->write_data.cr4 = value;
@@ -3765,7 +3750,10 @@ int hvm_msr_write_intercept(unsigned int msr, uint64_t msr_content,
{
ASSERT(v->arch.vm_event);
- /* The actual write will occur in hvm_do_resume() (if permitted). */
+ /*
+ * The actual write will occur in monitor_ctrlreg_write_data(), if
+ * permitted.
+ */
v->arch.vm_event->write_data.do_write.msr = 1;
v->arch.vm_event->write_data.msr = msr;
v->arch.vm_event->write_data.value = msr_content;
diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
index 0798245..7a31582 100644
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -1441,11 +1441,6 @@ static void vmx_update_guest_cr(struct vcpu *v, unsigned int cr)
if ( !hvm_paging_enabled(v) && !vmx_unrestricted_guest(v) )
v->arch.hvm_vmx.exec_control |= cr3_ctls;
- /* Trap CR3 updates if CR3 memory events are enabled. */
- if ( v->domain->arch.monitor.write_ctrlreg_enabled &
- monitor_ctrlreg_bitmask(VM_EVENT_X86_CR3) )
- v->arch.hvm_vmx.exec_control |= CPU_BASED_CR3_LOAD_EXITING;
-
if ( old_ctls != v->arch.hvm_vmx.exec_control )
vmx_update_cpu_exec_control(v);
}
@@ -3898,6 +3893,60 @@ void vmx_vmenter_helper(const struct cpu_user_regs *regs)
}
/*
+ * Adjusts domain CR3 load-exiting execution control for CR3-write monitor
+ * vm-event.
+ */
+void vmx_vm_event_update_cr3_traps(const struct domain *d)
+{
+ struct vcpu *v;
+ struct arch_vmx_struct *avmx;
+ unsigned int cr3_bitmask;
+ bool_t cr3_vmevent, cr3_ldexit;
+
+ /* Domain must be paused. */
+ ASSERT(atomic_read(&d->pause_count));
+
+ /* Non-hap domains trap CR3 writes unconditionally. */
+ if ( !paging_mode_hap(d) )
+ {
+#ifndef NDEBUG
+ for_each_vcpu ( d, v )
+ ASSERT(v->arch.hvm_vmx.exec_control & CPU_BASED_CR3_LOAD_EXITING);
+#endif
+ return;
+ }
+
+ cr3_bitmask = monitor_ctrlreg_bitmask(VM_EVENT_X86_CR3);
+ cr3_vmevent = !!(d->arch.monitor.write_ctrlreg_enabled & cr3_bitmask);
+
+ for_each_vcpu ( d, v )
+ {
+ avmx = &v->arch.hvm_vmx;
+ cr3_ldexit = !!(avmx->exec_control & CPU_BASED_CR3_LOAD_EXITING);
+
+ if ( cr3_vmevent == cr3_ldexit )
+ continue;
+
+ /*
+ * If domain paging is disabled (CR0.PG=0) and
+ * the domain is not in real-mode, then CR3 load-exiting
+ * must remain enabled (see vmx_update_guest_cr(v, cr) when cr = 0).
+ */
+ if ( cr3_ldexit && !hvm_paging_enabled(v) && !vmx_unrestricted_guest(v) )
+ continue;
+
+ if ( cr3_vmevent )
+ avmx->exec_control |= CPU_BASED_CR3_LOAD_EXITING;
+ else
+ avmx->exec_control &= ~CPU_BASED_CR3_LOAD_EXITING;
+
+ vmx_vmcs_enter(v);
+ vmx_update_cpu_exec_control(v);
+ vmx_vmcs_exit(v);
+ }
+}
+
+/*
* Local variables:
* mode: C
* c-file-style: "BSD"
diff --git a/xen/arch/x86/monitor.c b/xen/arch/x86/monitor.c
index 06d21b1..29188e5 100644
--- a/xen/arch/x86/monitor.c
+++ b/xen/arch/x86/monitor.c
@@ -19,10 +19,39 @@
* License along with this program; If not, see <http://www.gnu.org/licenses/>.
*/
+#include <asm/hvm/vmx/vmx.h>
#include <asm/monitor.h>
#include <asm/vm_event.h>
#include <public/vm_event.h>
+static inline
+void monitor_ctrlreg_adjust_traps(struct domain *d, unsigned int index)
+{
+ /* For now, VMX only. */
+ ASSERT(cpu_has_vmx);
+
+ /* Other CRs than CR3 are always trapped. */
+ if ( VM_EVENT_X86_CR3 == index )
+ vmx_vm_event_update_cr3_traps(d);
+}
+
+static inline void monitor_ctrlreg_disable_traps(struct domain *d)
+{
+ unsigned int old = d->arch.monitor.write_ctrlreg_enabled;
+
+ d->arch.monitor.write_ctrlreg_enabled = 0;
+
+ if ( old )
+ {
+ /* For now, VMX only. */
+ ASSERT(cpu_has_vmx);
+
+ /* Was CR3 load-exiting enabled due to monitoring? */
+ if ( old & monitor_ctrlreg_bitmask(VM_EVENT_X86_CR3) )
+ vmx_vm_event_update_cr3_traps(d);
+ }
+}
+
int monitor_init_domain(struct domain *d)
{
if ( !d->arch.monitor.msr_bitmap )
@@ -38,6 +67,8 @@ void monitor_cleanup_domain(struct domain *d)
{
xfree(d->arch.monitor.msr_bitmap);
+ monitor_ctrlreg_disable_traps(d);
+
memset(&d->arch.monitor, 0, sizeof(d->arch.monitor));
memset(&d->monitor, 0, sizeof(d->monitor));
}
@@ -79,6 +110,35 @@ void monitor_ctrlreg_write_resume(struct vcpu *v, vm_event_response_t *rsp)
}
}
+void monitor_ctrlreg_write_data(struct vcpu *v)
+{
+ struct monitor_write_data *w = &v->arch.vm_event->write_data;
+
+ if ( w->do_write.msr )
+ {
+ hvm_msr_write_intercept(w->msr, w->value, 0);
+ w->do_write.msr = 0;
+ }
+
+ if ( w->do_write.cr0 )
+ {
+ hvm_set_cr0(w->cr0, 0);
+ w->do_write.cr0 = 0;
+ }
+
+ if ( w->do_write.cr4 )
+ {
+ hvm_set_cr4(w->cr4, 0);
+ w->do_write.cr4 = 0;
+ }
+
+ if ( w->do_write.cr3 )
+ {
+ hvm_set_cr3(w->cr3, 0);
+ w->do_write.cr3 = 0;
+ }
+}
+
static unsigned long *monitor_bitmap_for_msr(const struct domain *d, u32 *msr)
{
ASSERT(d->arch.monitor.msr_bitmap && msr);
@@ -197,13 +257,7 @@ int arch_monitor_domctl_event(struct domain *d,
else
ad->monitor.write_ctrlreg_enabled &= ~ctrlreg_bitmask;
- if ( VM_EVENT_X86_CR3 == mop->u.mov_to_cr.index )
- {
- struct vcpu *v;
- /* Latches new CR3 mask through CR0 code. */
- for_each_vcpu ( d, v )
- hvm_update_guest_cr(v, 0);
- }
+ monitor_ctrlreg_adjust_traps(d, mop->u.mov_to_cr.index);
domain_unpause(d);
diff --git a/xen/include/asm-x86/hvm/vmx/vmx.h b/xen/include/asm-x86/hvm/vmx/vmx.h
index 359b2a9..301df56 100644
--- a/xen/include/asm-x86/hvm/vmx/vmx.h
+++ b/xen/include/asm-x86/hvm/vmx/vmx.h
@@ -101,6 +101,7 @@ void vmx_update_debug_state(struct vcpu *v);
void vmx_update_exception_bitmap(struct vcpu *v);
void vmx_update_cpu_exec_control(struct vcpu *v);
void vmx_update_secondary_exec_control(struct vcpu *v);
+void vmx_vm_event_update_cr3_traps(const struct domain *d);
#define POSTED_INTR_ON 0
#define POSTED_INTR_SN 1
diff --git a/xen/include/asm-x86/monitor.h b/xen/include/asm-x86/monitor.h
index adca378..3ae24dd 100644
--- a/xen/include/asm-x86/monitor.h
+++ b/xen/include/asm-x86/monitor.h
@@ -93,6 +93,8 @@ void monitor_cleanup_domain(struct domain *d);
void monitor_ctrlreg_write_resume(struct vcpu *v, vm_event_response_t *rsp);
+void monitor_ctrlreg_write_data(struct vcpu *v);
+
bool_t monitored_msr(const struct domain *d, u32 msr);
#endif /* __ASM_X86_MONITOR_H__ */
--
2.5.0
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 54+ messages in thread
* [PATCH 06/16] x86/monitor: fix: set msr_bitmap to NULL after xfree
2016-07-09 4:11 [PATCH 00/16] x86/vm-event: numerous adjustments & fixes Corneliu ZUZU
` (4 preceding siblings ...)
2016-07-09 4:15 ` [PATCH 05/16] x86/monitor: relocate code more appropriately Corneliu ZUZU
@ 2016-07-09 4:15 ` Corneliu ZUZU
2016-07-09 4:16 ` [PATCH 07/16] x86/vm-event: fix: call cleanup when init fails, to free partial allocs Corneliu ZUZU
` (10 subsequent siblings)
16 siblings, 0 replies; 54+ messages in thread
From: Corneliu ZUZU @ 2016-07-09 4:15 UTC (permalink / raw)
To: xen-devel; +Cc: Andrew Cooper, Tamas K Lengyel, Jan Beulich, Razvan Cojocaru
Fix: set d->arch.monitor.msr_bitmap to NULL after xfree, as the equivalence of
it being NULL and xfreed is repeatedly presumed in the codebase. Along with this
change, also properly reposition an 'if' targeting the aforementioned msr_bitmap
when it is allocated and add likely/unlikely accordingly.
Signed-off-by: Corneliu ZUZU <czuzu@bitdefender.com>
---
xen/arch/x86/monitor.c | 10 ++++++----
1 file changed, 6 insertions(+), 4 deletions(-)
diff --git a/xen/arch/x86/monitor.c b/xen/arch/x86/monitor.c
index 29188e5..8f41f21 100644
--- a/xen/arch/x86/monitor.c
+++ b/xen/arch/x86/monitor.c
@@ -54,11 +54,12 @@ static inline void monitor_ctrlreg_disable_traps(struct domain *d)
int monitor_init_domain(struct domain *d)
{
- if ( !d->arch.monitor.msr_bitmap )
+ if ( likely(!d->arch.monitor.msr_bitmap) )
+ {
d->arch.monitor.msr_bitmap = xzalloc(struct monitor_msr_bitmap);
-
- if ( !d->arch.monitor.msr_bitmap )
- return -ENOMEM;
+ if ( unlikely(!d->arch.monitor.msr_bitmap) )
+ return -ENOMEM;
+ }
return 0;
}
@@ -66,6 +67,7 @@ int monitor_init_domain(struct domain *d)
void monitor_cleanup_domain(struct domain *d)
{
xfree(d->arch.monitor.msr_bitmap);
+ d->arch.monitor.msr_bitmap = NULL;
monitor_ctrlreg_disable_traps(d);
--
2.5.0
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 54+ messages in thread
* [PATCH 07/16] x86/vm-event: fix: call cleanup when init fails, to free partial allocs
2016-07-09 4:11 [PATCH 00/16] x86/vm-event: numerous adjustments & fixes Corneliu ZUZU
` (5 preceding siblings ...)
2016-07-09 4:15 ` [PATCH 06/16] x86/monitor: fix: set msr_bitmap to NULL after xfree Corneliu ZUZU
@ 2016-07-09 4:16 ` Corneliu ZUZU
2016-07-09 4:17 ` [PATCH 08/16] x86/vm-event: call monitor init & cleanup funcs from respective vm_event funcs Corneliu ZUZU
` (9 subsequent siblings)
16 siblings, 0 replies; 54+ messages in thread
From: Corneliu ZUZU @ 2016-07-09 4:16 UTC (permalink / raw)
To: xen-devel; +Cc: Andrew Cooper, Tamas K Lengyel, Jan Beulich, Razvan Cojocaru
From vm_event_init_domain(d), call vm_event_cleanup_domain(d) if per-vcpu
allocations failed -at some point- to cleanup partial allocations, if any were
done along the way.
Also minor adjustments: add unlikely in if's and add some comments.
Signed-off-by: Corneliu ZUZU <czuzu@bitdefender.com>
---
xen/arch/x86/vm_event.c | 20 +++++++++++++++-----
1 file changed, 15 insertions(+), 5 deletions(-)
diff --git a/xen/arch/x86/vm_event.c b/xen/arch/x86/vm_event.c
index 6e19df8..22c63ea 100644
--- a/xen/arch/x86/vm_event.c
+++ b/xen/arch/x86/vm_event.c
@@ -23,20 +23,29 @@
/* Implicitly serialized by the domctl lock. */
int vm_event_init_domain(struct domain *d)
{
+ int rc = 0;
struct vcpu *v;
+ /* Per-vcpu initializations. */
for_each_vcpu ( d, v )
{
- if ( v->arch.vm_event )
+ if ( unlikely(v->arch.vm_event) )
continue;
v->arch.vm_event = xzalloc(struct arch_vm_event);
-
- if ( !v->arch.vm_event )
- return -ENOMEM;
+ if ( unlikely(!v->arch.vm_event) )
+ {
+ rc = -ENOMEM;
+ goto err;
+ }
}
- return 0;
+err:
+ if ( unlikely(rc) )
+ /* On fail cleanup whatever resources have been partially allocated. */
+ vm_event_cleanup_domain(d);
+
+ return rc;
}
/*
@@ -47,6 +56,7 @@ void vm_event_cleanup_domain(struct domain *d)
{
struct vcpu *v;
+ /* Per-vcpu uninitializations. */
for_each_vcpu ( d, v )
{
xfree(v->arch.vm_event);
--
2.5.0
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 54+ messages in thread
* [PATCH 08/16] x86/vm-event: call monitor init & cleanup funcs from respective vm_event funcs
2016-07-09 4:11 [PATCH 00/16] x86/vm-event: numerous adjustments & fixes Corneliu ZUZU
` (6 preceding siblings ...)
2016-07-09 4:16 ` [PATCH 07/16] x86/vm-event: fix: call cleanup when init fails, to free partial allocs Corneliu ZUZU
@ 2016-07-09 4:17 ` Corneliu ZUZU
2016-07-09 4:18 ` [PATCH 09/16] arm/monitor: move d->monitor cleanup to monitor_cleanup_domain() Corneliu ZUZU
` (8 subsequent siblings)
16 siblings, 0 replies; 54+ messages in thread
From: Corneliu ZUZU @ 2016-07-09 4:17 UTC (permalink / raw)
To: xen-devel
Cc: Tamas K Lengyel, Razvan Cojocaru, Andrew Cooper, Julien Grall,
Stefano Stabellini, Jan Beulich
For clarity: do init/cleanup of the vm-events subsystems (e.g. functions
monitor_init,cleanup_domain()) from the vm-event_init,cleanup_domain()
functions. Done by passing-on the ved param from vm_event_enable,disable()
functions down to the vm_event_init,cleanup_domain() functions.
This makes it easier to follow the init/cleanup sequences for all vm-events
subsystems.
In the process, we also replace the vec param of vm_event_enable() with a port
param, since the op/mode fields of vec are already handled before the function
is called. This was also done because conceptually it was strange relying on the
ved param in vm_event_init,cleanup_domain() to determine the target vm-event
subsystem when we already had vec->mode in vm_event_enable().
Signed-off-by: Corneliu ZUZU <czuzu@bitdefender.com>
---
xen/arch/x86/monitor.c | 5 +++++
xen/arch/x86/vm_event.c | 15 ++++++++++++---
xen/common/vm_event.c | 18 +++++++-----------
xen/include/asm-arm/vm_event.h | 15 ++++++++++++---
xen/include/asm-x86/vm_event.h | 4 ++--
5 files changed, 38 insertions(+), 19 deletions(-)
diff --git a/xen/arch/x86/monitor.c b/xen/arch/x86/monitor.c
index 8f41f21..aeee435 100644
--- a/xen/arch/x86/monitor.c
+++ b/xen/arch/x86/monitor.c
@@ -52,6 +52,7 @@ static inline void monitor_ctrlreg_disable_traps(struct domain *d)
}
}
+/* Implicitly serialized by the domctl lock. */
int monitor_init_domain(struct domain *d)
{
if ( likely(!d->arch.monitor.msr_bitmap) )
@@ -64,6 +65,10 @@ int monitor_init_domain(struct domain *d)
return 0;
}
+/*
+ * Implicitly serialized by the domctl lock,
+ * or on domain cleanup paths only.
+ */
void monitor_cleanup_domain(struct domain *d)
{
xfree(d->arch.monitor.msr_bitmap);
diff --git a/xen/arch/x86/vm_event.c b/xen/arch/x86/vm_event.c
index 22c63ea..e795e5e 100644
--- a/xen/arch/x86/vm_event.c
+++ b/xen/arch/x86/vm_event.c
@@ -18,10 +18,11 @@
* License along with this program; If not, see <http://www.gnu.org/licenses/>.
*/
+#include <asm/monitor.h>
#include <asm/vm_event.h>
/* Implicitly serialized by the domctl lock. */
-int vm_event_init_domain(struct domain *d)
+int vm_event_init_domain(struct domain *d, struct vm_event_domain *ved)
{
int rc = 0;
struct vcpu *v;
@@ -40,10 +41,14 @@ int vm_event_init_domain(struct domain *d)
}
}
+ /* Initialize specified subsystem. */
+ if ( &d->vm_event->monitor == ved )
+ rc = monitor_init_domain(d);
+
err:
if ( unlikely(rc) )
/* On fail cleanup whatever resources have been partially allocated. */
- vm_event_cleanup_domain(d);
+ vm_event_cleanup_domain(d, ved);
return rc;
}
@@ -52,10 +57,14 @@ err:
* Implicitly serialized by the domctl lock,
* or on domain cleanup paths only.
*/
-void vm_event_cleanup_domain(struct domain *d)
+void vm_event_cleanup_domain(struct domain *d, struct vm_event_domain *ved)
{
struct vcpu *v;
+ /* Uninitialize specified subsystem. */
+ if ( &d->vm_event->monitor == ved )
+ monitor_cleanup_domain(d);
+
/* Per-vcpu uninitializations. */
for_each_vcpu ( d, v )
{
diff --git a/xen/common/vm_event.c b/xen/common/vm_event.c
index e60e1f2..dafe7bf 100644
--- a/xen/common/vm_event.c
+++ b/xen/common/vm_event.c
@@ -41,8 +41,8 @@
static int vm_event_enable(
struct domain *d,
- xen_domctl_vm_event_op_t *vec,
struct vm_event_domain *ved,
+ unsigned long xen_port,
int pause_flag,
int param,
xen_event_channel_notification_t notification_fn)
@@ -64,7 +64,7 @@ static int vm_event_enable(
vm_event_ring_lock_init(ved);
vm_event_ring_lock(ved);
- rc = vm_event_init_domain(d);
+ rc = vm_event_init_domain(d, ved);
if ( rc < 0 )
goto err;
@@ -83,7 +83,7 @@ static int vm_event_enable(
if ( rc < 0 )
goto err;
- ved->xen_port = vec->port = rc;
+ ved->xen_port = xen_port = rc;
/* Prepare ring buffer */
FRONT_RING_INIT(&ved->front_ring,
@@ -232,7 +232,7 @@ static int vm_event_disable(struct domain *d, struct vm_event_domain *ved)
destroy_ring_for_helper(&ved->ring_page,
ved->ring_pg_struct);
- vm_event_cleanup_domain(d);
+ vm_event_cleanup_domain(d, ved);
vm_event_ring_unlock(ved);
}
@@ -630,7 +630,7 @@ int vm_event_domctl(struct domain *d, xen_domctl_vm_event_op_t *vec,
break;
/* domain_pause() not required here, see XSA-99 */
- rc = vm_event_enable(d, vec, ved, _VPF_mem_paging,
+ rc = vm_event_enable(d, ved, vec->port, _VPF_mem_paging,
HVM_PARAM_PAGING_RING_PFN,
mem_paging_notification);
}
@@ -669,10 +669,7 @@ int vm_event_domctl(struct domain *d, xen_domctl_vm_event_op_t *vec,
{
case XEN_VM_EVENT_ENABLE:
/* domain_pause() not required here, see XSA-99 */
- rc = monitor_init_domain(d);
- if ( rc )
- break;
- rc = vm_event_enable(d, vec, ved, _VPF_mem_access,
+ rc = vm_event_enable(d, ved, vec->port, _VPF_mem_access,
HVM_PARAM_MONITOR_RING_PFN,
monitor_notification);
break;
@@ -682,7 +679,6 @@ int vm_event_domctl(struct domain *d, xen_domctl_vm_event_op_t *vec,
{
domain_pause(d);
rc = vm_event_disable(d, ved);
- monitor_cleanup_domain(d);
domain_unpause(d);
}
break;
@@ -721,7 +717,7 @@ int vm_event_domctl(struct domain *d, xen_domctl_vm_event_op_t *vec,
break;
/* domain_pause() not required here, see XSA-99 */
- rc = vm_event_enable(d, vec, ved, _VPF_mem_sharing,
+ rc = vm_event_enable(d, ved, vec->port, _VPF_mem_sharing,
HVM_PARAM_SHARING_RING_PFN,
mem_sharing_notification);
break;
diff --git a/xen/include/asm-arm/vm_event.h b/xen/include/asm-arm/vm_event.h
index 0129b04..93fc4db 100644
--- a/xen/include/asm-arm/vm_event.h
+++ b/xen/include/asm-arm/vm_event.h
@@ -23,14 +23,23 @@
#include <xen/vm_event.h>
#include <public/domctl.h>
-static inline int vm_event_init_domain(struct domain *d)
+static inline
+int vm_event_init_domain(struct domain *d, struct vm_event_domain *ved)
{
- /* Nothing to do. */
+ /* Initialize specified subsystem. */
+ if ( &d->vm_event->monitor == ved )
+ return monitor_init_domain(d);
+
return 0;
}
-static inline void vm_event_cleanup_domain(struct domain *d)
+static inline
+void vm_event_cleanup_domain(struct domain *d, struct vm_event_domain *ved)
{
+ /* Uninitialize specified subsystem. */
+ if ( &d->vm_event->monitor == ved )
+ monitor_cleanup_domain(d);
+
memset(&d->monitor, 0, sizeof(d->monitor));
}
diff --git a/xen/include/asm-x86/vm_event.h b/xen/include/asm-x86/vm_event.h
index c53effb..934f385 100644
--- a/xen/include/asm-x86/vm_event.h
+++ b/xen/include/asm-x86/vm_event.h
@@ -31,9 +31,9 @@ struct arch_vm_event {
struct monitor_write_data write_data;
};
-int vm_event_init_domain(struct domain *d);
+int vm_event_init_domain(struct domain *d, struct vm_event_domain *ved);
-void vm_event_cleanup_domain(struct domain *d);
+void vm_event_cleanup_domain(struct domain *d, struct vm_event_domain *ved);
void vm_event_toggle_singlestep(struct domain *d, struct vcpu *v);
--
2.5.0
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 54+ messages in thread
* [PATCH 09/16] arm/monitor: move d->monitor cleanup to monitor_cleanup_domain()
2016-07-09 4:11 [PATCH 00/16] x86/vm-event: numerous adjustments & fixes Corneliu ZUZU
` (7 preceding siblings ...)
2016-07-09 4:17 ` [PATCH 08/16] x86/vm-event: call monitor init & cleanup funcs from respective vm_event funcs Corneliu ZUZU
@ 2016-07-09 4:18 ` Corneliu ZUZU
2016-07-09 4:19 ` [PATCH 10/16] x86/vm-event: centralize vcpu-destroy cleanup in vm-events code Corneliu ZUZU
` (7 subsequent siblings)
16 siblings, 0 replies; 54+ messages in thread
From: Corneliu ZUZU @ 2016-07-09 4:18 UTC (permalink / raw)
To: xen-devel
Cc: Julien Grall, Tamas K Lengyel, Stefano Stabellini, Razvan Cojocaru
d->monitor is a monitor subsystem resource, clean it up in the proper stub.
Signed-off-by: Corneliu ZUZU <czuzu@bitdefender.com>
---
xen/include/asm-arm/monitor.h | 2 +-
xen/include/asm-arm/vm_event.h | 2 --
2 files changed, 1 insertion(+), 3 deletions(-)
diff --git a/xen/include/asm-arm/monitor.h b/xen/include/asm-arm/monitor.h
index 5a0fc65..9a9734a 100644
--- a/xen/include/asm-arm/monitor.h
+++ b/xen/include/asm-arm/monitor.h
@@ -54,7 +54,7 @@ static inline int monitor_init_domain(struct domain *d)
static inline void monitor_cleanup_domain(struct domain *d)
{
- /* No arch-specific domain cleanup on ARM. */
+ memset(&d->monitor, 0, sizeof(d->monitor));
}
static inline
diff --git a/xen/include/asm-arm/vm_event.h b/xen/include/asm-arm/vm_event.h
index 93fc4db..3a8f585 100644
--- a/xen/include/asm-arm/vm_event.h
+++ b/xen/include/asm-arm/vm_event.h
@@ -39,8 +39,6 @@ void vm_event_cleanup_domain(struct domain *d, struct vm_event_domain *ved)
/* Uninitialize specified subsystem. */
if ( &d->vm_event->monitor == ved )
monitor_cleanup_domain(d);
-
- memset(&d->monitor, 0, sizeof(d->monitor));
}
static inline void vm_event_toggle_singlestep(struct domain *d, struct vcpu *v)
--
2.5.0
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 54+ messages in thread
* [PATCH 10/16] x86/vm-event: centralize vcpu-destroy cleanup in vm-events code
2016-07-09 4:11 [PATCH 00/16] x86/vm-event: numerous adjustments & fixes Corneliu ZUZU
` (8 preceding siblings ...)
2016-07-09 4:18 ` [PATCH 09/16] arm/monitor: move d->monitor cleanup to monitor_cleanup_domain() Corneliu ZUZU
@ 2016-07-09 4:19 ` Corneliu ZUZU
2016-07-09 4:20 ` [PATCH 11/16] x86/monitor: fix: treat -monitor- properly, as a subsys of the vm-event subsys Corneliu ZUZU
` (6 subsequent siblings)
16 siblings, 0 replies; 54+ messages in thread
From: Corneliu ZUZU @ 2016-07-09 4:19 UTC (permalink / raw)
To: xen-devel; +Cc: Andrew Cooper, Tamas K Lengyel, Razvan Cojocaru, Jan Beulich
Move vm-event cleanup sequence on vcpu destroyal to vm_event.h along with the
other vm-event functions.
Signed-off-by: Corneliu ZUZU <czuzu@bitdefender.com>
---
xen/arch/x86/domain.c | 4 ++--
xen/arch/x86/vm_event.c | 5 +----
xen/include/asm-x86/vm_event.h | 10 ++++++++++
3 files changed, 13 insertions(+), 6 deletions(-)
diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
index bb59247..2a702be 100644
--- a/xen/arch/x86/domain.c
+++ b/xen/arch/x86/domain.c
@@ -56,6 +56,7 @@
#include <asm/debugreg.h>
#include <asm/msr.h>
#include <asm/traps.h>
+#include <asm/vm_event.h>
#include <asm/nmi.h>
#include <asm/mce.h>
#include <asm/amd.h>
@@ -492,8 +493,7 @@ int vcpu_initialise(struct vcpu *v)
void vcpu_destroy(struct vcpu *v)
{
- xfree(v->arch.vm_event);
- v->arch.vm_event = NULL;
+ vm_event_cleanup_vcpu_destroy(v);
if ( is_pv_32bit_vcpu(v) )
{
diff --git a/xen/arch/x86/vm_event.c b/xen/arch/x86/vm_event.c
index e795e5e..bb9c0a0 100644
--- a/xen/arch/x86/vm_event.c
+++ b/xen/arch/x86/vm_event.c
@@ -67,10 +67,7 @@ void vm_event_cleanup_domain(struct domain *d, struct vm_event_domain *ved)
/* Per-vcpu uninitializations. */
for_each_vcpu ( d, v )
- {
- xfree(v->arch.vm_event);
- v->arch.vm_event = NULL;
- }
+ vm_event_cleanup_vcpu_destroy(v);
d->arch.mem_access_emulate_each_rep = 0;
}
diff --git a/xen/include/asm-x86/vm_event.h b/xen/include/asm-x86/vm_event.h
index 934f385..c1b82ab 100644
--- a/xen/include/asm-x86/vm_event.h
+++ b/xen/include/asm-x86/vm_event.h
@@ -35,6 +35,16 @@ int vm_event_init_domain(struct domain *d, struct vm_event_domain *ved);
void vm_event_cleanup_domain(struct domain *d, struct vm_event_domain *ved);
+/* Called on vCPU destroy. */
+static inline void vm_event_cleanup_vcpu_destroy(struct vcpu *v)
+{
+ if ( likely(!v->arch.vm_event) )
+ return;
+
+ xfree(v->arch.vm_event);
+ v->arch.vm_event = NULL;
+}
+
void vm_event_toggle_singlestep(struct domain *d, struct vcpu *v);
void vm_event_set_registers(struct vcpu *v, vm_event_response_t *rsp);
--
2.5.0
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 54+ messages in thread
* [PATCH 11/16] x86/monitor: fix: treat -monitor- properly, as a subsys of the vm-event subsys
2016-07-09 4:11 [PATCH 00/16] x86/vm-event: numerous adjustments & fixes Corneliu ZUZU
` (9 preceding siblings ...)
2016-07-09 4:19 ` [PATCH 10/16] x86/vm-event: centralize vcpu-destroy cleanup in vm-events code Corneliu ZUZU
@ 2016-07-09 4:20 ` Corneliu ZUZU
2016-07-09 17:34 ` Tamas K Lengyel
2016-07-09 4:20 ` [PATCH 12/16] x86/vm-event: fix: move cleanup of mem_access_emulate_each_rep to monitor stub Corneliu ZUZU
` (5 subsequent siblings)
16 siblings, 1 reply; 54+ messages in thread
From: Corneliu ZUZU @ 2016-07-09 4:20 UTC (permalink / raw)
To: xen-devel
Cc: Tamas K Lengyel, Razvan Cojocaru, Andrew Cooper, Julien Grall,
Paul Durrant, Stefano Stabellini, Jan Beulich
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(-)
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;
};
--
2.5.0
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 54+ messages in thread
* [PATCH 12/16] x86/vm-event: fix: move cleanup of mem_access_emulate_each_rep to monitor stub
2016-07-09 4:11 [PATCH 00/16] x86/vm-event: numerous adjustments & fixes Corneliu ZUZU
` (10 preceding siblings ...)
2016-07-09 4:20 ` [PATCH 11/16] x86/monitor: fix: treat -monitor- properly, as a subsys of the vm-event subsys Corneliu ZUZU
@ 2016-07-09 4:20 ` Corneliu ZUZU
2016-07-09 4:21 ` [PATCH 13/16] x86/monitor: introduce writes_pending field in monitor_write_data Corneliu ZUZU
` (4 subsequent siblings)
16 siblings, 0 replies; 54+ messages in thread
From: Corneliu ZUZU @ 2016-07-09 4:20 UTC (permalink / raw)
To: xen-devel; +Cc: Andrew Cooper, Tamas K Lengyel, Jan Beulich, Razvan Cojocaru
Move cleanup of mem_access_emulate_each_rep to monitor_cleanup_domain() as the
field is part of the monitor subsystem's resources.
Signed-off-by: Corneliu ZUZU <czuzu@bitdefender.com>
---
xen/arch/x86/monitor.c | 3 +++
xen/arch/x86/vm_event.c | 2 --
2 files changed, 3 insertions(+), 2 deletions(-)
diff --git a/xen/arch/x86/monitor.c b/xen/arch/x86/monitor.c
index 4a29cad..c558f46 100644
--- a/xen/arch/x86/monitor.c
+++ b/xen/arch/x86/monitor.c
@@ -80,6 +80,9 @@ void monitor_cleanup_domain(struct domain *d)
memset(&d->arch.monitor, 0, sizeof(d->arch.monitor));
memset(&d->monitor, 0, sizeof(d->monitor));
+
+ d->arch.mem_access_emulate_each_rep = 0;
+
d->monitor.initialised = 0;
}
diff --git a/xen/arch/x86/vm_event.c b/xen/arch/x86/vm_event.c
index bb9c0a0..e2b258b 100644
--- a/xen/arch/x86/vm_event.c
+++ b/xen/arch/x86/vm_event.c
@@ -68,8 +68,6 @@ void vm_event_cleanup_domain(struct domain *d, struct vm_event_domain *ved)
/* Per-vcpu uninitializations. */
for_each_vcpu ( d, v )
vm_event_cleanup_vcpu_destroy(v);
-
- d->arch.mem_access_emulate_each_rep = 0;
}
void vm_event_toggle_singlestep(struct domain *d, struct vcpu *v)
--
2.5.0
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 54+ messages in thread
* [PATCH 13/16] x86/monitor: introduce writes_pending field in monitor_write_data
2016-07-09 4:11 [PATCH 00/16] x86/vm-event: numerous adjustments & fixes Corneliu ZUZU
` (11 preceding siblings ...)
2016-07-09 4:20 ` [PATCH 12/16] x86/vm-event: fix: move cleanup of mem_access_emulate_each_rep to monitor stub Corneliu ZUZU
@ 2016-07-09 4:21 ` Corneliu ZUZU
2016-07-09 4:22 ` [PATCH 14/16] x86/monitor: clarify separation between monitor subsys and vm-event as a whole Corneliu ZUZU
` (3 subsequent siblings)
16 siblings, 0 replies; 54+ messages in thread
From: Corneliu ZUZU @ 2016-07-09 4:21 UTC (permalink / raw)
To: xen-devel; +Cc: Andrew Cooper, Tamas K Lengyel, Jan Beulich, Razvan Cojocaru
Introduce writes_pending field in monitor_write_data structure to slightly
optimize code @ monitor_write_data(): avoid having to check each bit when -all-
bits are zero (which is likely).
Signed-off-by: Corneliu ZUZU <czuzu@bitdefender.com>
---
xen/arch/x86/monitor.c | 3 +++
xen/include/asm-x86/domain.h | 17 +++++++++++------
2 files changed, 14 insertions(+), 6 deletions(-)
diff --git a/xen/arch/x86/monitor.c b/xen/arch/x86/monitor.c
index c558f46..0763ed7 100644
--- a/xen/arch/x86/monitor.c
+++ b/xen/arch/x86/monitor.c
@@ -127,6 +127,9 @@ void monitor_ctrlreg_write_data(struct vcpu *v)
{
struct monitor_write_data *w = &v->arch.vm_event->write_data;
+ if ( likely(!w->writes_pending) )
+ return;
+
if ( w->do_write.msr )
{
hvm_msr_write_intercept(w->msr, w->value, 0);
diff --git a/xen/include/asm-x86/domain.h b/xen/include/asm-x86/domain.h
index 8f64ae9..ae1dcb4 100644
--- a/xen/include/asm-x86/domain.h
+++ b/xen/include/asm-x86/domain.h
@@ -260,12 +260,17 @@ struct pv_domain
};
struct monitor_write_data {
- struct {
- unsigned int msr : 1;
- unsigned int cr0 : 1;
- unsigned int cr3 : 1;
- unsigned int cr4 : 1;
- } do_write;
+ union {
+ struct {
+ unsigned int msr : 1;
+ unsigned int cr0 : 1;
+ unsigned int cr3 : 1;
+ unsigned int cr4 : 1;
+ } do_write;
+
+ /* Non-zero when at least one of do_write fields is 1. */
+ unsigned int writes_pending;
+ };
uint32_t msr;
uint64_t value;
--
2.5.0
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 54+ messages in thread
* [PATCH 14/16] x86/monitor: clarify separation between monitor subsys and vm-event as a whole
2016-07-09 4:11 [PATCH 00/16] x86/vm-event: numerous adjustments & fixes Corneliu ZUZU
` (12 preceding siblings ...)
2016-07-09 4:21 ` [PATCH 13/16] x86/monitor: introduce writes_pending field in monitor_write_data Corneliu ZUZU
@ 2016-07-09 4:22 ` Corneliu ZUZU
2016-07-09 18:26 ` Tamas K Lengyel
2016-07-09 4:23 ` [PATCH 15/16] x86/monitor: fix: don't compromise a monitor_write_data with pending CR writes Corneliu ZUZU
` (2 subsequent siblings)
16 siblings, 1 reply; 54+ messages in thread
From: Corneliu ZUZU @ 2016-07-09 4:22 UTC (permalink / raw)
To: xen-devel
Cc: Tamas K Lengyel, Razvan Cojocaru, George Dunlap, Andrew Cooper,
Paul Durrant, Jan Beulich
Move fields in arch_vm_event in a structure called arch_vm_event_monitor and
refactor arch_vm_event to only hold a pointer to an instance of the
aforementioned.
Done for a number of reasons:
* to make the separation of monitor resources from other vm-event resources
clearly visible
* to be able to selectively allocate/free monitor-only resources in monitor
init & cleanup stubs
* did _not_ do a v->arch.vm_event -to- v->arch.monitor transformation because we
want the guarantee of not occupying more than 1 pointer in arch_vcpu, i.e. if
in the future e.g. paging vm-events would need per-vcpu resources, one would
make an arch_vm_event_paging structure and add a pointer to an instance of it
as a arch_vm_event.paging field, in which case we will still have only 1
pointer (arch_vm_event) consuming memory in arch_vcpu for vm-event resources
Note also that this breaks the old implication 'vm-event initialized -> monitor
resources (which were in arch_vm_event) initialized', so 2 extra checks on
monitor_domain_initialised() had to be added:
i) in vm_event_resume() before calling monitor_ctrlreg_write_resume(),
also an ASSERT in monitor_ctrlreg_write_resume()
ii) in vm_event_resume() before calling mem_access_resume(),
also an ASSERT on that code-path in p2m_mem_access_emulate_check()
Finally, also note that the definition of arch_vm_event had to be moved to
asm-x86/domain.h due to a cyclic header dependency it caused between
asm-x86/vm_event.h and asm-x86/monitor.h.
Signed-off-by: Corneliu ZUZU <czuzu@bitdefender.com>
---
xen/arch/x86/hvm/emulate.c | 8 +++---
xen/arch/x86/hvm/hvm.c | 31 ++++++++++++------------
xen/arch/x86/mm/p2m.c | 7 ++++--
xen/arch/x86/monitor.c | 55 ++++++++++++++++++++++++++++++++++++------
xen/arch/x86/vm_event.c | 17 +++++++++++--
xen/common/vm_event.c | 17 +++++++++++++
xen/include/asm-x86/domain.h | 15 ++++++++++++
xen/include/asm-x86/monitor.h | 7 ++++++
xen/include/asm-x86/vm_event.h | 13 +++-------
9 files changed, 128 insertions(+), 42 deletions(-)
diff --git a/xen/arch/x86/hvm/emulate.c b/xen/arch/x86/hvm/emulate.c
index a0094e9..7a9c6af 100644
--- a/xen/arch/x86/hvm/emulate.c
+++ b/xen/arch/x86/hvm/emulate.c
@@ -76,10 +76,10 @@ static int set_context_data(void *buffer, unsigned int size)
if ( unlikely(monitor_domain_initialised(curr->domain)) )
{
- unsigned int safe_size =
- min(size, curr->arch.vm_event->emul_read_data.size);
+ struct arch_vm_event_monitor *vem = curr->arch.vm_event->monitor;
+ unsigned int safe_size = min(size, vem->emul_read_data.size);
- memcpy(buffer, curr->arch.vm_event->emul_read_data.data, safe_size);
+ memcpy(buffer, vem->emul_read_data.data, safe_size);
memset(buffer + safe_size, 0, size - safe_size);
return X86EMUL_OKAY;
}
@@ -524,7 +524,7 @@ static int hvmemul_virtual_to_linear(
* vm_event being triggered for repeated writes to a whole page.
*/
if ( unlikely(current->domain->arch.mem_access_emulate_each_rep) &&
- current->arch.vm_event->emulate_flags != 0 )
+ current->arch.vm_event->monitor->emulate_flags != 0 )
max_reps = 1;
/*
diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index 46fed33..1bfd9d4 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -65,7 +65,6 @@
#include <asm/altp2m.h>
#include <asm/mtrr.h>
#include <asm/apic.h>
-#include <asm/vm_event.h>
#include <public/sched.h>
#include <public/hvm/ioreq.h>
#include <public/version.h>
@@ -473,21 +472,21 @@ void hvm_do_resume(struct vcpu *v)
if ( unlikely(monitor_domain_initialised(v->domain)) )
{
- if ( unlikely(v->arch.vm_event->emulate_flags) )
+ struct arch_vm_event_monitor *vem = v->arch.vm_event->monitor;
+
+ if ( unlikely(vem->emulate_flags) )
{
enum emul_kind kind = EMUL_KIND_NORMAL;
- if ( v->arch.vm_event->emulate_flags &
- VM_EVENT_FLAG_SET_EMUL_READ_DATA )
+ if ( vem->emulate_flags & VM_EVENT_FLAG_SET_EMUL_READ_DATA )
kind = EMUL_KIND_SET_CONTEXT;
- else if ( v->arch.vm_event->emulate_flags &
- VM_EVENT_FLAG_EMULATE_NOWRITE )
+ else if ( vem->emulate_flags & VM_EVENT_FLAG_EMULATE_NOWRITE )
kind = EMUL_KIND_NOWRITE;
hvm_mem_access_emulate_one(kind, TRAP_invalid_op,
HVM_DELIVER_NO_ERROR_CODE);
- v->arch.vm_event->emulate_flags = 0;
+ vem->emulate_flags = 0;
}
monitor_ctrlreg_write_data(v);
@@ -2184,8 +2183,8 @@ int hvm_set_cr0(unsigned long value, bool_t may_defer)
* The actual write will occur in monitor_ctrlreg_write_data(), if
* permitted.
*/
- v->arch.vm_event->write_data.do_write.cr0 = 1;
- v->arch.vm_event->write_data.cr0 = value;
+ v->arch.vm_event->monitor->write_data.do_write.cr0 = 1;
+ v->arch.vm_event->monitor->write_data.cr0 = value;
return X86EMUL_OKAY;
}
@@ -2289,8 +2288,8 @@ int hvm_set_cr3(unsigned long value, bool_t may_defer)
* The actual write will occur in monitor_ctrlreg_write_data(), if
* permitted.
*/
- v->arch.vm_event->write_data.do_write.cr3 = 1;
- v->arch.vm_event->write_data.cr3 = value;
+ v->arch.vm_event->monitor->write_data.do_write.cr3 = 1;
+ v->arch.vm_event->monitor->write_data.cr3 = value;
return X86EMUL_OKAY;
}
@@ -2372,8 +2371,8 @@ int hvm_set_cr4(unsigned long value, bool_t may_defer)
* The actual write will occur in monitor_ctrlreg_write_data(), if
* permitted.
*/
- v->arch.vm_event->write_data.do_write.cr4 = 1;
- v->arch.vm_event->write_data.cr4 = value;
+ v->arch.vm_event->monitor->write_data.do_write.cr4 = 1;
+ v->arch.vm_event->monitor->write_data.cr4 = value;
return X86EMUL_OKAY;
}
@@ -3754,9 +3753,9 @@ int hvm_msr_write_intercept(unsigned int msr, uint64_t msr_content,
* The actual write will occur in monitor_ctrlreg_write_data(), if
* permitted.
*/
- v->arch.vm_event->write_data.do_write.msr = 1;
- v->arch.vm_event->write_data.msr = msr;
- v->arch.vm_event->write_data.value = msr_content;
+ v->arch.vm_event->monitor->write_data.do_write.msr = 1;
+ v->arch.vm_event->monitor->write_data.msr = msr;
+ v->arch.vm_event->monitor->write_data.value = msr_content;
hvm_monitor_msr(msr, msr_content);
return X86EMUL_OKAY;
diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
index 16733a4..92c0576 100644
--- a/xen/arch/x86/mm/p2m.c
+++ b/xen/arch/x86/mm/p2m.c
@@ -1591,12 +1591,15 @@ void p2m_mem_paging_resume(struct domain *d, vm_event_response_t *rsp)
void p2m_mem_access_emulate_check(struct vcpu *v,
const vm_event_response_t *rsp)
{
+ ASSERT(monitor_domain_initialised(v->domain));
+
/* Mark vcpu for skipping one instruction upon rescheduling. */
if ( rsp->flags & VM_EVENT_FLAG_EMULATE )
{
xenmem_access_t access;
bool_t violation = 1;
const struct vm_event_mem_access *data = &rsp->u.mem_access;
+ struct arch_vm_event_monitor *vem = v->arch.vm_event->monitor;
if ( p2m_get_mem_access(v->domain, _gfn(data->gfn), &access) == 0 )
{
@@ -1639,10 +1642,10 @@ void p2m_mem_access_emulate_check(struct vcpu *v,
}
}
- v->arch.vm_event->emulate_flags = violation ? rsp->flags : 0;
+ vem->emulate_flags = violation ? rsp->flags : 0;
if ( (rsp->flags & VM_EVENT_FLAG_SET_EMUL_READ_DATA) )
- v->arch.vm_event->emul_read_data = rsp->data.emul_read_data;
+ vem->emul_read_data = rsp->data.emul_read_data;
}
}
diff --git a/xen/arch/x86/monitor.c b/xen/arch/x86/monitor.c
index 0763ed7..c3123bc 100644
--- a/xen/arch/x86/monitor.c
+++ b/xen/arch/x86/monitor.c
@@ -21,7 +21,6 @@
#include <asm/hvm/vmx/vmx.h>
#include <asm/monitor.h>
-#include <asm/vm_event.h>
#include <public/vm_event.h>
static inline
@@ -55,16 +54,43 @@ static inline void monitor_ctrlreg_disable_traps(struct domain *d)
/* Implicitly serialized by the domctl lock. */
int monitor_init_domain(struct domain *d)
{
- if ( likely(!d->arch.monitor.msr_bitmap) )
+ int rc = 0;
+ struct vcpu *v;
+
+ /* Already initialised? */
+ if ( unlikely(monitor_domain_initialised(d)) )
+ return 0;
+
+ /* Per-vcpu initializations. */
+ for_each_vcpu(d, v)
+ {
+ ASSERT(v->arch.vm_event);
+ ASSERT(!v->arch.vm_event->monitor);
+
+ v->arch.vm_event->monitor = xzalloc(struct arch_vm_event_monitor);
+ if ( unlikely(!v->arch.vm_event->monitor) )
+ {
+ rc = -ENOMEM;
+ goto err;
+ }
+ }
+
+ ASSERT(!d->arch.monitor.msr_bitmap);
+ d->arch.monitor.msr_bitmap = xzalloc(struct monitor_msr_bitmap);
+ if ( unlikely(!d->arch.monitor.msr_bitmap) )
{
- d->arch.monitor.msr_bitmap = xzalloc(struct monitor_msr_bitmap);
- if ( unlikely(!d->arch.monitor.msr_bitmap) )
- return -ENOMEM;
+ rc = -ENOMEM;
+ goto err;
}
d->monitor.initialised = 1;
- return 0;
+err:
+ if ( unlikely(rc) )
+ /* On fail cleanup whatever resources have been partially allocated. */
+ monitor_cleanup_domain(d);
+
+ return rc;
}
/*
@@ -73,9 +99,20 @@ int monitor_init_domain(struct domain *d)
*/
void monitor_cleanup_domain(struct domain *d)
{
+ struct vcpu *v;
+
xfree(d->arch.monitor.msr_bitmap);
d->arch.monitor.msr_bitmap = NULL;
+ for_each_vcpu(d, v)
+ {
+ if ( unlikely(!v->arch.vm_event || !v->arch.vm_event->monitor) )
+ continue;
+
+ xfree(v->arch.vm_event->monitor);
+ v->arch.vm_event->monitor = NULL;
+ }
+
monitor_ctrlreg_disable_traps(d);
memset(&d->arch.monitor, 0, sizeof(d->arch.monitor));
@@ -88,6 +125,8 @@ void monitor_cleanup_domain(struct domain *d)
void monitor_ctrlreg_write_resume(struct vcpu *v, vm_event_response_t *rsp)
{
+ ASSERT(monitor_domain_initialised(v->domain));
+
if ( rsp->flags & VM_EVENT_FLAG_DENY )
{
struct monitor_write_data *w;
@@ -98,7 +137,7 @@ void monitor_ctrlreg_write_resume(struct vcpu *v, vm_event_response_t *rsp)
if ( !atomic_read(&v->vm_event_pause_count) )
return;
- w = &v->arch.vm_event->write_data;
+ w = &v->arch.vm_event->monitor->write_data;
switch ( rsp->reason )
{
@@ -125,7 +164,7 @@ void monitor_ctrlreg_write_resume(struct vcpu *v, vm_event_response_t *rsp)
void monitor_ctrlreg_write_data(struct vcpu *v)
{
- struct monitor_write_data *w = &v->arch.vm_event->write_data;
+ struct monitor_write_data *w = &v->arch.vm_event->monitor->write_data;
if ( likely(!w->writes_pending) )
return;
diff --git a/xen/arch/x86/vm_event.c b/xen/arch/x86/vm_event.c
index e2b258b..20cb1b0 100644
--- a/xen/arch/x86/vm_event.c
+++ b/xen/arch/x86/vm_event.c
@@ -65,9 +65,22 @@ void vm_event_cleanup_domain(struct domain *d, struct vm_event_domain *ved)
if ( &d->vm_event->monitor == ved )
monitor_cleanup_domain(d);
- /* Per-vcpu uninitializations. */
+ /*
+ * Per-vCPU: if all resources of all vm-event subsystems were freed,
+ * also free v->arch.vm_event.
+ */
for_each_vcpu ( d, v )
- vm_event_cleanup_vcpu_destroy(v);
+ {
+ if ( unlikely(!v->arch.vm_event) )
+ continue;
+
+ /* Are there resources of vm-event subsystems left unfreed? */
+ if ( unlikely(v->arch.vm_event->monitor) )
+ continue;
+
+ xfree(v->arch.vm_event);
+ v->arch.vm_event = NULL;
+ }
}
void vm_event_toggle_singlestep(struct domain *d, struct vcpu *v)
diff --git a/xen/common/vm_event.c b/xen/common/vm_event.c
index dafe7bf..c409b80 100644
--- a/xen/common/vm_event.c
+++ b/xen/common/vm_event.c
@@ -22,6 +22,7 @@
#include <xen/sched.h>
#include <xen/event.h>
+#include <xen/monitor.h>
#include <xen/wait.h>
#include <xen/vm_event.h>
#include <xen/mem_access.h>
@@ -397,11 +398,27 @@ void vm_event_resume(struct domain *d, struct vm_event_domain *ved)
case VM_EVENT_REASON_MOV_TO_MSR:
#endif
case VM_EVENT_REASON_WRITE_CTRLREG:
+ if ( unlikely(!monitor_domain_initialised(d)) )
+ {
+ printk(XENLOG_G_WARNING "Vm-event monitor subsystem not"
+ "initialized, cr-write ring response"
+ "ignored (hint: have you called"
+ "xc_monitor_enable()?).\n");
+ continue;
+ }
monitor_ctrlreg_write_resume(v, &rsp);
break;
#ifdef CONFIG_HAS_MEM_ACCESS
case VM_EVENT_REASON_MEM_ACCESS:
+ if ( unlikely(!monitor_domain_initialised(d)) )
+ {
+ printk(XENLOG_G_WARNING "Vm-event monitor subsystem not"
+ "initialized, mem-access ring response"
+ "ignored (hint: have you called"
+ "xc_monitor_enable()?).\n");
+ continue;
+ }
mem_access_resume(v, &rsp);
break;
#endif
diff --git a/xen/include/asm-x86/domain.h b/xen/include/asm-x86/domain.h
index ae1dcb4..7663da2 100644
--- a/xen/include/asm-x86/domain.h
+++ b/xen/include/asm-x86/domain.h
@@ -9,6 +9,7 @@
#include <asm/e820.h>
#include <asm/mce.h>
#include <public/vcpu.h>
+#include <public/vm_event.h>
#include <public/hvm/hvm_info_table.h>
#define has_32bit_shinfo(d) ((d)->arch.has_32bit_shinfo)
@@ -503,6 +504,20 @@ 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 arch_vm_event_monitor {
+ uint32_t emulate_flags;
+ struct vm_event_emul_read_data emul_read_data;
+ struct monitor_write_data write_data;
+};
+
+struct arch_vm_event {
+ struct arch_vm_event_monitor *monitor;
+};
+
struct arch_vcpu
{
/*
diff --git a/xen/include/asm-x86/monitor.h b/xen/include/asm-x86/monitor.h
index 4014f8d..11497ef 100644
--- a/xen/include/asm-x86/monitor.h
+++ b/xen/include/asm-x86/monitor.h
@@ -90,6 +90,13 @@ int monitor_init_domain(struct domain *d);
void monitor_cleanup_domain(struct domain *d);
+/* Called on vCPU destroy. */
+static inline void monitor_cleanup_vcpu_destroy(struct vcpu *v)
+{
+ ASSERT(v->arch.vm_event);
+ xfree(v->arch.vm_event->monitor);
+}
+
void monitor_ctrlreg_write_resume(struct vcpu *v, vm_event_response_t *rsp);
void monitor_ctrlreg_write_data(struct vcpu *v);
diff --git a/xen/include/asm-x86/vm_event.h b/xen/include/asm-x86/vm_event.h
index c1b82ab..3c14d4f 100644
--- a/xen/include/asm-x86/vm_event.h
+++ b/xen/include/asm-x86/vm_event.h
@@ -20,16 +20,7 @@
#define __ASM_X86_VM_EVENT_H__
#include <xen/sched.h>
-
-/*
- * Should we emulate the next matching instruction on VCPU resume
- * after a vm_event?
- */
-struct arch_vm_event {
- uint32_t emulate_flags;
- struct vm_event_emul_read_data emul_read_data;
- struct monitor_write_data write_data;
-};
+#include <asm/monitor.h>
int vm_event_init_domain(struct domain *d, struct vm_event_domain *ved);
@@ -41,6 +32,8 @@ static inline void vm_event_cleanup_vcpu_destroy(struct vcpu *v)
if ( likely(!v->arch.vm_event) )
return;
+ monitor_cleanup_vcpu_destroy(v);
+
xfree(v->arch.vm_event);
v->arch.vm_event = NULL;
}
--
2.5.0
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 54+ messages in thread
* [PATCH 15/16] x86/monitor: fix: don't compromise a monitor_write_data with pending CR writes
2016-07-09 4:11 [PATCH 00/16] x86/vm-event: numerous adjustments & fixes Corneliu ZUZU
` (13 preceding siblings ...)
2016-07-09 4:22 ` [PATCH 14/16] x86/monitor: clarify separation between monitor subsys and vm-event as a whole Corneliu ZUZU
@ 2016-07-09 4:23 ` Corneliu ZUZU
2016-07-09 4:23 ` [PATCH 16/16] x86/monitor: fix: xc_monitor _write_ctrlreg w/o previous _enable must fail Corneliu ZUZU
2016-07-11 2:54 ` [PATCH 00/16] x86/vm-event: numerous adjustments & fixes Tian, Kevin
16 siblings, 0 replies; 54+ messages in thread
From: Corneliu ZUZU @ 2016-07-09 4:23 UTC (permalink / raw)
To: xen-devel
Cc: Tamas K Lengyel, Razvan Cojocaru, George Dunlap, Andrew Cooper,
Paul Durrant, Jan Beulich
The arch_vm_event.monitor structure is dynamically allocated and freed e.g. when
the toolstack user calls xc_monitor_disable(), which in turn effectively
discards any information that was in arch_vm_event.monitor->write_data.
But this can yield unexpected behavior since if a CR-write was awaiting to be
committed (non-zero write_data.writes_pending) on the scheduling tail
(hvm_do_resume()->monitor_ctrlreg_write_data()) before xc_monitor_disable() is
called, then the domain CR-write is wrongfully ignored, which of course, in
these cases, can easily render a domain crash.
To fix the issue, this patch:
* makes arch_vm_event.monitor->emul_read_data dynamically allocated and only
frees the entire arch_vm_event.monitor in monitor_cleanup_domain() if there
are no pending CR-writes, otherwise it only frees emul_read_data
* if there were pending CR-writes when monitor_cleanup_domain() was called,
arch_vm_event.monitor will eventually be freed either after the CR-writes are
committed (monitor_ctrlreg_write_data()) or when the domain/vcpu is destroyed
* calls monitor_ctrlreg_write_data() even if the monitor subsystem is not
initialized
Signed-off-by: Corneliu ZUZU <czuzu@bitdefender.com>
---
xen/arch/x86/hvm/emulate.c | 4 ++--
xen/arch/x86/hvm/hvm.c | 7 ++++++-
xen/arch/x86/mm/p2m.c | 2 +-
xen/arch/x86/monitor.c | 49 +++++++++++++++++++++++++++++++++++++++-----
xen/include/asm-x86/domain.h | 3 +--
5 files changed, 54 insertions(+), 11 deletions(-)
diff --git a/xen/arch/x86/hvm/emulate.c b/xen/arch/x86/hvm/emulate.c
index 7a9c6af..e08d9c6 100644
--- a/xen/arch/x86/hvm/emulate.c
+++ b/xen/arch/x86/hvm/emulate.c
@@ -77,9 +77,9 @@ static int set_context_data(void *buffer, unsigned int size)
if ( unlikely(monitor_domain_initialised(curr->domain)) )
{
struct arch_vm_event_monitor *vem = curr->arch.vm_event->monitor;
- unsigned int safe_size = min(size, vem->emul_read_data.size);
+ unsigned int safe_size = min(size, vem->emul_read_data->size);
- memcpy(buffer, vem->emul_read_data.data, safe_size);
+ memcpy(buffer, vem->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 1bfd9d4..a2568fd 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -488,9 +488,14 @@ void hvm_do_resume(struct vcpu *v)
vem->emulate_flags = 0;
}
+ }
+ /*
+ * Handle pending monitor CR-writes. Note that the monitor subsystem
+ * might be uninitialized.
+ */
+ if ( unlikely(v->arch.vm_event && v->arch.vm_event->monitor) )
monitor_ctrlreg_write_data(v);
- }
/* Inject pending hw/sw trap */
if ( v->arch.hvm_vcpu.inject_trap.vector != -1 )
diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
index 92c0576..b280dc0 100644
--- a/xen/arch/x86/mm/p2m.c
+++ b/xen/arch/x86/mm/p2m.c
@@ -1645,7 +1645,7 @@ void p2m_mem_access_emulate_check(struct vcpu *v,
vem->emulate_flags = violation ? rsp->flags : 0;
if ( (rsp->flags & VM_EVENT_FLAG_SET_EMUL_READ_DATA) )
- vem->emul_read_data = rsp->data.emul_read_data;
+ *vem->emul_read_data = rsp->data.emul_read_data;
}
}
diff --git a/xen/arch/x86/monitor.c b/xen/arch/x86/monitor.c
index c3123bc..05a2f0d 100644
--- a/xen/arch/x86/monitor.c
+++ b/xen/arch/x86/monitor.c
@@ -56,6 +56,7 @@ int monitor_init_domain(struct domain *d)
{
int rc = 0;
struct vcpu *v;
+ struct arch_vm_event_monitor *vem;
/* Already initialised? */
if ( unlikely(monitor_domain_initialised(d)) )
@@ -65,10 +66,22 @@ int monitor_init_domain(struct domain *d)
for_each_vcpu(d, v)
{
ASSERT(v->arch.vm_event);
- ASSERT(!v->arch.vm_event->monitor);
- v->arch.vm_event->monitor = xzalloc(struct arch_vm_event_monitor);
- if ( unlikely(!v->arch.vm_event->monitor) )
+ if ( likely(!v->arch.vm_event->monitor) )
+ {
+ v->arch.vm_event->monitor = xzalloc(struct arch_vm_event_monitor);
+ if ( unlikely(!v->arch.vm_event->monitor) )
+ {
+ rc = -ENOMEM;
+ goto err;
+ }
+ }
+
+ vem = v->arch.vm_event->monitor;
+
+ ASSERT(!vem->emul_read_data);
+ vem->emul_read_data = xzalloc(struct vm_event_emul_read_data);
+ if ( unlikely(!vem->emul_read_data) )
{
rc = -ENOMEM;
goto err;
@@ -100,6 +113,7 @@ err:
void monitor_cleanup_domain(struct domain *d)
{
struct vcpu *v;
+ struct arch_vm_event_monitor *vem;
xfree(d->arch.monitor.msr_bitmap);
d->arch.monitor.msr_bitmap = NULL;
@@ -109,8 +123,22 @@ void monitor_cleanup_domain(struct domain *d)
if ( unlikely(!v->arch.vm_event || !v->arch.vm_event->monitor) )
continue;
- xfree(v->arch.vm_event->monitor);
- v->arch.vm_event->monitor = NULL;
+ vem = v->arch.vm_event->monitor;
+ xfree(vem->emul_read_data);
+ vem->emul_read_data = NULL;
+
+ /*
+ * Only invalidate write_data if no CR-writes are pending or domain is
+ * dead (domain_kill(d) code-path). If CR-writes are pending, write_data
+ * will eventually be freed when those writes are committed (see
+ * monitor_ctrlreg_write_data() function).
+ */
+ if ( likely(!vem->write_data.writes_pending) ||
+ unlikely(DOMDYING_dead == d->is_dying) )
+ {
+ xfree(vem);
+ v->arch.vm_event->monitor = NULL;
+ }
}
monitor_ctrlreg_disable_traps(d);
@@ -192,6 +220,17 @@ void monitor_ctrlreg_write_data(struct vcpu *v)
hvm_set_cr3(w->cr3, 0);
w->do_write.cr3 = 0;
}
+
+ /*
+ * Was the monitor subsystem actually uninitialized
+ * and only waiting for pending CR-writes?
+ */
+ if ( unlikely(!monitor_domain_initialised(v->domain)) )
+ {
+ xfree(v->arch.vm_event->monitor);
+ xfree(v->arch.vm_event);
+ v->arch.vm_event = NULL;
+ }
}
static unsigned long *monitor_bitmap_for_msr(const struct domain *d, u32 *msr)
diff --git a/xen/include/asm-x86/domain.h b/xen/include/asm-x86/domain.h
index 7663da2..e337cb3 100644
--- a/xen/include/asm-x86/domain.h
+++ b/xen/include/asm-x86/domain.h
@@ -9,7 +9,6 @@
#include <asm/e820.h>
#include <asm/mce.h>
#include <public/vcpu.h>
-#include <public/vm_event.h>
#include <public/hvm/hvm_info_table.h>
#define has_32bit_shinfo(d) ((d)->arch.has_32bit_shinfo)
@@ -510,7 +509,7 @@ typedef enum __packed {
*/
struct arch_vm_event_monitor {
uint32_t emulate_flags;
- struct vm_event_emul_read_data emul_read_data;
+ struct vm_event_emul_read_data *emul_read_data;
struct monitor_write_data write_data;
};
--
2.5.0
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 54+ messages in thread
* [PATCH 16/16] x86/monitor: fix: xc_monitor _write_ctrlreg w/o previous _enable must fail
2016-07-09 4:11 [PATCH 00/16] x86/vm-event: numerous adjustments & fixes Corneliu ZUZU
` (14 preceding siblings ...)
2016-07-09 4:23 ` [PATCH 15/16] x86/monitor: fix: don't compromise a monitor_write_data with pending CR writes Corneliu ZUZU
@ 2016-07-09 4:23 ` Corneliu ZUZU
2016-07-09 4:34 ` Corneliu ZUZU
2016-07-11 2:54 ` [PATCH 00/16] x86/vm-event: numerous adjustments & fixes Tian, Kevin
16 siblings, 1 reply; 54+ messages in thread
From: Corneliu ZUZU @ 2016-07-09 4:23 UTC (permalink / raw)
To: xen-devel; +Cc: Andrew Cooper, Tamas K Lengyel, Jan Beulich, Razvan Cojocaru
Enforce presence of a monitor vm-event subscriber when the toolstack user calls
xc_monitor_write_ctrlreg() (XEN_DOMCTL_MONITOR_EVENT_WRITE_CTRLREG domctl).
Without this change, "ASSERT(monitor_domain_initialised(v->domain));" @
hvm_set_cr0() and such would fail if the toolstack user calls
xc_monitor_write_ctrlreg(...) w/ enable = true, without first calling
xc_monitor_enable().
Also adjust returned error code for similar check from -EINVAL to more
descriptive -ENODEV (XEN_DOMCTL_MONITOR_OP_EMULATE_EACH_REP).
Signed-off-by: Corneliu ZUZU <czuzu@bitdefender.com>
---
xen/arch/x86/monitor.c | 4 ++++
xen/include/asm-x86/monitor.h | 2 +-
2 files changed, 5 insertions(+), 1 deletion(-)
diff --git a/xen/arch/x86/monitor.c b/xen/arch/x86/monitor.c
index 05a2f0d..4cf018a 100644
--- a/xen/arch/x86/monitor.c
+++ b/xen/arch/x86/monitor.c
@@ -324,6 +324,10 @@ int arch_monitor_domctl_event(struct domain *d,
unsigned int ctrlreg_bitmask;
bool_t old_status;
+ /* Meaningless without a monitor vm-events subscriber. */
+ if ( unlikely(!monitor_domain_initialised(d)) )
+ return -ENODEV;
+
/* sanity check: avoid left-shift undefined behavior */
if ( unlikely(mop->u.mov_to_cr.index > 31) )
return -EINVAL;
diff --git a/xen/include/asm-x86/monitor.h b/xen/include/asm-x86/monitor.h
index 11497ef..a6022db 100644
--- a/xen/include/asm-x86/monitor.h
+++ b/xen/include/asm-x86/monitor.h
@@ -47,7 +47,7 @@ int arch_monitor_domctl_op(struct domain *d, struct xen_domctl_monitor_op *mop)
if ( likely(monitor_domain_initialised(d)) )
d->arch.mem_access_emulate_each_rep = !!mop->event;
else
- rc = -EINVAL;
+ rc = -ENODEV;
domain_unpause(d);
break;
--
2.5.0
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 54+ messages in thread
* Re: [PATCH 16/16] x86/monitor: fix: xc_monitor _write_ctrlreg w/o previous _enable must fail
2016-07-09 4:23 ` [PATCH 16/16] x86/monitor: fix: xc_monitor _write_ctrlreg w/o previous _enable must fail Corneliu ZUZU
@ 2016-07-09 4:34 ` Corneliu ZUZU
0 siblings, 0 replies; 54+ messages in thread
From: Corneliu ZUZU @ 2016-07-09 4:34 UTC (permalink / raw)
To: xen-devel; +Cc: Andrew Cooper, Tamas K Lengyel, Razvan Cojocaru, Jan Beulich
On 7/9/2016 7:23 AM, Corneliu ZUZU wrote:
> Enforce presence of a monitor vm-event subscriber when the toolstack user calls
> xc_monitor_write_ctrlreg() (XEN_DOMCTL_MONITOR_EVENT_WRITE_CTRLREG domctl).
> Without this change, "ASSERT(monitor_domain_initialised(v->domain));" @
> hvm_set_cr0() and such would fail if the toolstack user calls
> xc_monitor_write_ctrlreg(...) w/ enable = true, without first calling
> xc_monitor_enable().
>
> Also adjust returned error code for similar check from -EINVAL to more
> descriptive -ENODEV (XEN_DOMCTL_MONITOR_OP_EMULATE_EACH_REP).
>
> Signed-off-by: Corneliu ZUZU <czuzu@bitdefender.com>
> ---
> xen/arch/x86/monitor.c | 4 ++++
> xen/include/asm-x86/monitor.h | 2 +-
> 2 files changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/xen/arch/x86/monitor.c b/xen/arch/x86/monitor.c
> index 05a2f0d..4cf018a 100644
> --- a/xen/arch/x86/monitor.c
> +++ b/xen/arch/x86/monitor.c
> @@ -324,6 +324,10 @@ int arch_monitor_domctl_event(struct domain *d,
> unsigned int ctrlreg_bitmask;
> bool_t old_status;
>
> + /* Meaningless without a monitor vm-events subscriber. */
> + if ( unlikely(!monitor_domain_initialised(d)) )
> + return -ENODEV;
> +
> /* sanity check: avoid left-shift undefined behavior */
> if ( unlikely(mop->u.mov_to_cr.index > 31) )
> return -EINVAL;
> diff --git a/xen/include/asm-x86/monitor.h b/xen/include/asm-x86/monitor.h
> index 11497ef..a6022db 100644
> --- a/xen/include/asm-x86/monitor.h
> +++ b/xen/include/asm-x86/monitor.h
> @@ -47,7 +47,7 @@ int arch_monitor_domctl_op(struct domain *d, struct xen_domctl_monitor_op *mop)
> if ( likely(monitor_domain_initialised(d)) )
> d->arch.mem_access_emulate_each_rep = !!mop->event;
> else
> - rc = -EINVAL;
> + rc = -ENODEV;
>
> domain_unpause(d);
> break;
I might have forgotten to think about domain pausing (for all patches),
where it needs to be done.
I'll leave that for v2 (obviously), I just wanted to let you know in
case you guys have feedback on the matter until then.
Zuzu.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [PATCH 11/16] x86/monitor: fix: treat -monitor- properly, as a subsys of the vm-event subsys
2016-07-09 4:20 ` [PATCH 11/16] x86/monitor: fix: treat -monitor- properly, as a subsys of the vm-event subsys Corneliu ZUZU
@ 2016-07-09 17:34 ` Tamas K Lengyel
2016-07-09 17:46 ` Corneliu ZUZU
0 siblings, 1 reply; 54+ messages in thread
From: Tamas K Lengyel @ 2016-07-09 17:34 UTC (permalink / raw)
To: Corneliu ZUZU
Cc: Stefano Stabellini, Razvan Cojocaru, Andrew Cooper, Xen-devel,
Julien Grall, Paul Durrant, Jan Beulich
> 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
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [PATCH 11/16] x86/monitor: fix: treat -monitor- properly, as a subsys of the vm-event subsys
2016-07-09 17:34 ` Tamas K Lengyel
@ 2016-07-09 17:46 ` Corneliu ZUZU
2016-07-11 16:38 ` Tamas K Lengyel
0 siblings, 1 reply; 54+ messages in thread
From: Corneliu ZUZU @ 2016-07-09 17:46 UTC (permalink / raw)
To: Tamas K Lengyel
Cc: Stefano Stabellini, Razvan Cojocaru, Andrew Cooper, Xen-devel,
Julien Grall, Paul Durrant, Jan Beulich
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.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [PATCH 03/16] x86/monitor: mechanical renames
2016-07-09 4:13 ` [PATCH 03/16] x86/monitor: mechanical renames Corneliu ZUZU
@ 2016-07-09 18:10 ` Tamas K Lengyel
2016-07-09 18:46 ` Corneliu ZUZU
0 siblings, 1 reply; 54+ messages in thread
From: Tamas K Lengyel @ 2016-07-09 18:10 UTC (permalink / raw)
To: Corneliu ZUZU
Cc: Stefano Stabellini, Razvan Cojocaru, Andrew Cooper, Xen-devel,
Julien Grall, Jan Beulich
On Fri, Jul 8, 2016 at 10:13 PM, Corneliu ZUZU <czuzu@bitdefender.com> wrote:
> Arch-specific vm-event functions in x86/vm_event.h -e.g. vm_event_init_domain()-
> don't have an 'arch_' prefix. Apply the same rule for monitor functions -
> originally the only two monitor functions that had an 'arch_' prefix were
> arch_monitor_domctl_event() and arch_monitor_domctl_op(), but I gave them that
> prefix because -they had a counterpart function in common code-, that being
> monitor_domctl().
This should actually be the other way around - ie adding the arch_
prefix to vm_event functions that lack it. Having the arch_ prefix is
helpful to know that the function is dealing with the arch specific
structs and not common. Similarly that's why we have the hvm_ prefix
for functions in hvm/monitor.
>
> Let this also be the rule for future 'arch_' functions additions, and with this
> patch remove the 'arch_' prefix from the monitor functions that don't have a
> counterpart in common-code (all but those 2 aforementioned).
Even if there are no common counter-parts to the function, the arch_
prefix should remain, so I won't be able to ack this patch.
Tamas
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [PATCH 04/16] x86/monitor: relocate vm_event_register_write_resume() function to monitor code
2016-07-09 4:14 ` [PATCH 04/16] x86/monitor: relocate vm_event_register_write_resume() function to monitor code Corneliu ZUZU
@ 2016-07-09 18:14 ` Tamas K Lengyel
2016-07-09 18:47 ` Corneliu ZUZU
0 siblings, 1 reply; 54+ messages in thread
From: Tamas K Lengyel @ 2016-07-09 18:14 UTC (permalink / raw)
To: Corneliu ZUZU
Cc: Stefano Stabellini, Razvan Cojocaru, Andrew Cooper, Xen-devel,
Julien Grall, Jan Beulich
On Fri, Jul 8, 2016 at 10:14 PM, Corneliu ZUZU <czuzu@bitdefender.com> wrote:
> vm_event_register_write_resume is part of the monitor subsystem, relocate and
> rename appropriately.
>
> Signed-off-by: Corneliu ZUZU <czuzu@bitdefender.com>
> ---
> xen/arch/x86/monitor.c | 38 ++++++++++++++++++++++++++++++++++++++
> xen/arch/x86/vm_event.c | 37 -------------------------------------
> xen/common/vm_event.c | 2 +-
> xen/include/asm-arm/monitor.h | 6 ++++++
> xen/include/asm-arm/vm_event.h | 6 ------
> xen/include/asm-x86/monitor.h | 2 ++
> xen/include/asm-x86/vm_event.h | 2 --
> 7 files changed, 47 insertions(+), 46 deletions(-)
>
> diff --git a/xen/arch/x86/monitor.c b/xen/arch/x86/monitor.c
> index 043815a..06d21b1 100644
> --- a/xen/arch/x86/monitor.c
> +++ b/xen/arch/x86/monitor.c
> @@ -20,6 +20,7 @@
> */
>
> #include <asm/monitor.h>
> +#include <asm/vm_event.h>
I'm not sure, is this include necessary?
Tamas
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [PATCH 14/16] x86/monitor: clarify separation between monitor subsys and vm-event as a whole
2016-07-09 4:22 ` [PATCH 14/16] x86/monitor: clarify separation between monitor subsys and vm-event as a whole Corneliu ZUZU
@ 2016-07-09 18:26 ` Tamas K Lengyel
2016-07-09 18:57 ` Corneliu ZUZU
0 siblings, 1 reply; 54+ messages in thread
From: Tamas K Lengyel @ 2016-07-09 18:26 UTC (permalink / raw)
To: Corneliu ZUZU
Cc: Razvan Cojocaru, George Dunlap, Andrew Cooper, Xen-devel,
Paul Durrant, Jan Beulich
> diff --git a/xen/include/asm-x86/domain.h b/xen/include/asm-x86/domain.h
> index ae1dcb4..7663da2 100644
> --- a/xen/include/asm-x86/domain.h
> +++ b/xen/include/asm-x86/domain.h
> @@ -9,6 +9,7 @@
> #include <asm/e820.h>
> #include <asm/mce.h>
> #include <public/vcpu.h>
> +#include <public/vm_event.h>
> #include <public/hvm/hvm_info_table.h>
>
> #define has_32bit_shinfo(d) ((d)->arch.has_32bit_shinfo)
> @@ -503,6 +504,20 @@ 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 arch_vm_event_monitor {
This should be named struct arch_vcpu_monitor.
> + uint32_t emulate_flags;
> + struct vm_event_emul_read_data emul_read_data;
This should probably get renamed as well at some point to struct
monitor_emul_read_data.
> + struct monitor_write_data write_data;
> +};
> +
> +struct arch_vm_event {
> + struct arch_vm_event_monitor *monitor;
> +};
IMHO there is not much point in defining struct arch_vm_event this
way, we could just as well store the pointer to the arch_monitor
directly in arch_vcpu as we do right now.
> +
> struct arch_vcpu
> {
> /*
Tamas
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [PATCH 03/16] x86/monitor: mechanical renames
2016-07-09 18:10 ` Tamas K Lengyel
@ 2016-07-09 18:46 ` Corneliu ZUZU
2016-07-11 16:43 ` Tamas K Lengyel
0 siblings, 1 reply; 54+ messages in thread
From: Corneliu ZUZU @ 2016-07-09 18:46 UTC (permalink / raw)
To: Tamas K Lengyel
Cc: Stefano Stabellini, Razvan Cojocaru, Andrew Cooper, Xen-devel,
Julien Grall, Jan Beulich
On 7/9/2016 9:10 PM, Tamas K Lengyel wrote:
> On Fri, Jul 8, 2016 at 10:13 PM, Corneliu ZUZU <czuzu@bitdefender.com> wrote:
>> Arch-specific vm-event functions in x86/vm_event.h -e.g. vm_event_init_domain()-
>> don't have an 'arch_' prefix. Apply the same rule for monitor functions -
>> originally the only two monitor functions that had an 'arch_' prefix were
>> arch_monitor_domctl_event() and arch_monitor_domctl_op(), but I gave them that
>> prefix because -they had a counterpart function in common code-, that being
>> monitor_domctl().
> This should actually be the other way around - ie adding the arch_
> prefix to vm_event functions that lack it.
Given that the majority of the arch-specific functions called from
common-code don't have an 'arch_' prefix unless they have a common
counterpart, I was guessing that was the rule. It made sense in my head
since I saw in that the intention of avoiding naming conflicts (i.e you
can't have monitor_domctl() both on the common-side and on the
arch-side, so prepend 'arch_' to the latter). I noticed you guys also
'skip' the prefix when sending patches, so that reinforced my assumption.
> Having the arch_ prefix is
> helpful to know that the function is dealing with the arch specific
> structs and not common.
Personally I don't see much use in 'knowing that the function is dealing
with the arch structs' from the call-site and you can tell that from the
implementation-site just by looking at the path of its source file.
Also, the code is pretty much localized in the arch directory anyway so
usually one wouldn't have to go back and forth between common and arch
that often. What really bothers me though is always having to read
'arch_' when spelling a function-name and also that it makes the
function name longer without much benefit. Your suggestion of adding it
to pretty much all functions that make up the interface to common just
adds to that headache. :-D
> Similarly that's why we have the hvm_ prefix
> for functions in hvm/monitor.
'hvm_' doesn't seem to me more special than 'monitor_', for instance,
but maybe that's just me.
>> Let this also be the rule for future 'arch_' functions additions, and with this
>> patch remove the 'arch_' prefix from the monitor functions that don't have a
>> counterpart in common-code (all but those 2 aforementioned).
> Even if there are no common counter-parts to the function, the arch_
> prefix should remain, so I won't be able to ack this patch.
>
> Tamas
Having said the above, are you still of the same opinion?
Zuzu.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [PATCH 04/16] x86/monitor: relocate vm_event_register_write_resume() function to monitor code
2016-07-09 18:14 ` Tamas K Lengyel
@ 2016-07-09 18:47 ` Corneliu ZUZU
0 siblings, 0 replies; 54+ messages in thread
From: Corneliu ZUZU @ 2016-07-09 18:47 UTC (permalink / raw)
To: Tamas K Lengyel
Cc: Stefano Stabellini, Razvan Cojocaru, Andrew Cooper, Xen-devel,
Julien Grall, Jan Beulich
On 7/9/2016 9:14 PM, Tamas K Lengyel wrote:
> On Fri, Jul 8, 2016 at 10:14 PM, Corneliu ZUZU <czuzu@bitdefender.com> wrote:
>> vm_event_register_write_resume is part of the monitor subsystem, relocate and
>> rename appropriately.
>>
>> Signed-off-by: Corneliu ZUZU <czuzu@bitdefender.com>
>> ---
>> xen/arch/x86/monitor.c | 38 ++++++++++++++++++++++++++++++++++++++
>> xen/arch/x86/vm_event.c | 37 -------------------------------------
>> xen/common/vm_event.c | 2 +-
>> xen/include/asm-arm/monitor.h | 6 ++++++
>> xen/include/asm-arm/vm_event.h | 6 ------
>> xen/include/asm-x86/monitor.h | 2 ++
>> xen/include/asm-x86/vm_event.h | 2 --
>> 7 files changed, 47 insertions(+), 46 deletions(-)
>>
>> diff --git a/xen/arch/x86/monitor.c b/xen/arch/x86/monitor.c
>> index 043815a..06d21b1 100644
>> --- a/xen/arch/x86/monitor.c
>> +++ b/xen/arch/x86/monitor.c
>> @@ -20,6 +20,7 @@
>> */
>>
>> #include <asm/monitor.h>
>> +#include <asm/vm_event.h>
> I'm not sure, is this include necessary?
>
> Tamas
>
Yes, otherwise it complains about dereferencing an incomplete-typed
pointer (arch_vm_event).
Zuzu.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [PATCH 14/16] x86/monitor: clarify separation between monitor subsys and vm-event as a whole
2016-07-09 18:26 ` Tamas K Lengyel
@ 2016-07-09 18:57 ` Corneliu ZUZU
2016-07-13 4:26 ` Corneliu ZUZU
0 siblings, 1 reply; 54+ messages in thread
From: Corneliu ZUZU @ 2016-07-09 18:57 UTC (permalink / raw)
To: Tamas K Lengyel
Cc: Razvan Cojocaru, George Dunlap, Andrew Cooper, Xen-devel,
Paul Durrant, Jan Beulich
On 7/9/2016 9:26 PM, Tamas K Lengyel wrote:
>> diff --git a/xen/include/asm-x86/domain.h b/xen/include/asm-x86/domain.h
>> index ae1dcb4..7663da2 100644
>> --- a/xen/include/asm-x86/domain.h
>> +++ b/xen/include/asm-x86/domain.h
>> @@ -9,6 +9,7 @@
>> #include <asm/e820.h>
>> #include <asm/mce.h>
>> #include <public/vcpu.h>
>> +#include <public/vm_event.h>
>> #include <public/hvm/hvm_info_table.h>
>>
>> #define has_32bit_shinfo(d) ((d)->arch.has_32bit_shinfo)
>> @@ -503,6 +504,20 @@ 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 arch_vm_event_monitor {
> This should be named struct arch_vcpu_monitor.
Good idea.
>
>> + uint32_t emulate_flags;
>> + struct vm_event_emul_read_data emul_read_data;
> This should probably get renamed as well at some point to struct
> monitor_emul_read_data.
Ack.
>> + struct monitor_write_data write_data;
>> +};
>> +
>> +struct arch_vm_event {
>> + struct arch_vm_event_monitor *monitor;
>> +};
> IMHO there is not much point in defining struct arch_vm_event this
> way, we could just as well store the pointer to the arch_monitor
> directly in arch_vcpu as we do right now.
>
I stated the reason for that in the commit message (see 3rd '*'), Jan
insists it would be preferable to occupy just one pointer in arch_vcpu
(which would still hold if we do as you suggest but I was wondering how
probable would it be in the future for one of the paging/share vm-event
subsystems to need per-vCPU resources). But personally, yeah, I would
have preferred what you suggest as well..
Zuzu.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [PATCH 00/16] x86/vm-event: numerous adjustments & fixes
2016-07-09 4:11 [PATCH 00/16] x86/vm-event: numerous adjustments & fixes Corneliu ZUZU
` (15 preceding siblings ...)
2016-07-09 4:23 ` [PATCH 16/16] x86/monitor: fix: xc_monitor _write_ctrlreg w/o previous _enable must fail Corneliu ZUZU
@ 2016-07-11 2:54 ` Tian, Kevin
2016-07-11 5:32 ` Corneliu ZUZU
16 siblings, 1 reply; 54+ messages in thread
From: Tian, Kevin @ 2016-07-11 2:54 UTC (permalink / raw)
To: Corneliu ZUZU, xen-devel
Cc: Stefano Stabellini, Jan Beulich, Razvan Cojocaru, George Dunlap,
Andrew Cooper, Julien Grall, Paul Durrant, Tamas K Lengyel,
Nakajima, Jun
what's the difference between this series and earlier one?
[PATCH v3 0/8] x86/vm-event: Adjustments & fixes
looks you have some patches cross-posted (e.g. 1/16)...
> -----Original Message-----
> From: Corneliu ZUZU [mailto:czuzu@bitdefender.com]
> Sent: Saturday, July 09, 2016 12:12 PM
> To: xen-devel@lists.xen.org
> Cc: Andrew Cooper; George Dunlap; Jan Beulich; Julien Grall; Nakajima, Jun; Tian, Kevin;
> Paul Durrant; Razvan Cojocaru; Stefano Stabellini; Tamas K Lengyel
> Subject: [PATCH 00/16] x86/vm-event: numerous adjustments & fixes
>
> Adjustments & fixes to the vm-events code, mostly monitor-related.
> As I hadn't got the time/proper context to test these changes on a real machine,
> for now please consider them only for review, I'll let you know when and how
> actual testing went.
>
> Thanks,
>
> Corneliu ZUZU (16):
> x86/vmx_update_guest_cr: minor optimization
> x86: fix: make atomic_read() param const
> x86/monitor: mechanical renames
> x86/monitor: relocate vm_event_register_write_resume() function to
> monitor code
> x86/monitor: relocate code more appropriately
> x86/monitor: fix: set msr_bitmap to NULL after xfree
> x86/vm-event: fix: call cleanup when init fails, to free partial
> allocs
> x86/vm-event: call monitor init & cleanup funcs from respective
> vm_event funcs
> arm/monitor: move d->monitor cleanup to monitor_cleanup_domain()
> x86/vm-event: centralize vcpu-destroy cleanup in vm-events code
> x86/monitor: fix: treat -monitor- properly, as a subsys of the
> vm-event subsys
> x86/vm-event: fix: move cleanup of mem_access_emulate_each_rep to
> monitor stub
> x86/monitor: introduce writes_pending field in monitor_write_data
> x86/monitor: clarify separation between monitor subsys and vm-event as
> a whole
> x86/monitor: fix: don't compromise a monitor_write_data with pending
> CR writes
> x86/monitor: fix: xc_monitor _write_ctrlreg w/o previous _enable must
> fail
>
> xen/arch/x86/domain.c | 4 +-
> xen/arch/x86/hvm/emulate.c | 11 +-
> xen/arch/x86/hvm/hvm.c | 92 ++++++++--------
> xen/arch/x86/hvm/vmx/vmx.c | 64 +++++++++--
> xen/arch/x86/mm/p2m.c | 7 +-
> xen/arch/x86/monitor.c | 218
> +++++++++++++++++++++++++++++++++++---
> xen/arch/x86/vm_event.c | 82 +++++++-------
> xen/common/monitor.c | 4 +-
> xen/common/vm_event.c | 37 ++++---
> xen/include/asm-arm/monitor.h | 17 +--
> xen/include/asm-arm/vm_event.h | 21 ++--
> xen/include/asm-x86/atomic.h | 4 +-
> xen/include/asm-x86/domain.h | 31 ++++--
> xen/include/asm-x86/hvm/vmx/vmx.h | 1 +
> xen/include/asm-x86/monitor.h | 32 ++++--
> xen/include/asm-x86/vm_event.h | 27 ++---
> xen/include/xen/monitor.h | 11 +-
> xen/include/xen/sched.h | 1 +
> 18 files changed, 473 insertions(+), 191 deletions(-)
>
> --
> 2.5.0
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [PATCH 00/16] x86/vm-event: numerous adjustments & fixes
2016-07-11 2:54 ` [PATCH 00/16] x86/vm-event: numerous adjustments & fixes Tian, Kevin
@ 2016-07-11 5:32 ` Corneliu ZUZU
2016-07-12 7:42 ` Tian, Kevin
0 siblings, 1 reply; 54+ messages in thread
From: Corneliu ZUZU @ 2016-07-11 5:32 UTC (permalink / raw)
To: Tian, Kevin, xen-devel
Cc: Stefano Stabellini, Jan Beulich, Razvan Cojocaru, George Dunlap,
Andrew Cooper, Julien Grall, Paul Durrant, Tamas K Lengyel,
Nakajima, Jun
[-- Attachment #1.1: Type: text/plain, Size: 3284 bytes --]
On 7/11/2016 5:54 AM, Tian, Kevin wrote:
> what's the difference between this series and earlier one?
>
> [PATCH v3 0/8] x86/vm-event: Adjustments & fixes
>
> looks you have some patches cross-posted (e.g. 1/16)...
>
>> Corneliu ZUZU (16):
>> x86/vmx_update_guest_cr: minor optimization
>> x86: fix: make atomic_read() param const
>> x86/monitor: mechanical renames
>> x86/monitor: relocate vm_event_register_write_resume() function to
>> monitor code
>> x86/monitor: relocate code more appropriately
>> x86/monitor: fix: set msr_bitmap to NULL after xfree
>> x86/vm-event: fix: call cleanup when init fails, to free partial
>> allocs
>> x86/vm-event: call monitor init & cleanup funcs from respective
>> vm_event funcs
>> arm/monitor: move d->monitor cleanup to monitor_cleanup_domain()
>> x86/vm-event: centralize vcpu-destroy cleanup in vm-events code
>> x86/monitor: fix: treat -monitor- properly, as a subsys of the
>> vm-event subsys
>> x86/vm-event: fix: move cleanup of mem_access_emulate_each_rep to
>> monitor stub
>> x86/monitor: introduce writes_pending field in monitor_write_data
>> x86/monitor: clarify separation between monitor subsys and vm-event as
>> a whole
>> x86/monitor: fix: don't compromise a monitor_write_data with pending
>> CR writes
>> x86/monitor: fix: xc_monitor _write_ctrlreg w/o previous _enable must
>> fail
>>
>> --
>> 2.5.0
>
Hi Kevin,
I'm sorry about the confusion, the _older v3 series is superseded by
this 16-patches one_. I thought the latter was too complex to consider
as a v4 over the former, but I should have at least stated something to
lead you guys to that conclusion (and to specify changes..). The patches
that are still in this series from the older one, the changes and the
acks are as following:
- P 1/16, was 1/8 in v3
* changed since v3: nothing
* acked-by in old v3: Acked-by: Kevin Tian <kevin.tian@intel.com>
- P 5/16, was 2/8 in v3
* changed since v3:
- title slightly changed
- renames due to 3/16:
arch_monitor_write_data()->monitor_ctrlreg_write_data(),
write_ctrlreg_adjust_traps()->monitor_ctrlreg_adjust_traps(),
write_ctrlreg_disable_traps()->monitor_ctrlreg_disable_traps()
- added 'const' attribute to "struct *domain" param of
vmx_vm_event_update_cr3_traps
- 'index' param of monitor_ctrlreg_adjust_traps now of type
'unsigned int'
* acked-by in old v3: no-one
- P 15/16, was 3/8 in v3:
* changed since v3 (IMO changes are too many and depends on
previous patches in this 16-p-series and it would be better if the old
v3 one is simply forgotten-about):
- separated fields in arch_vm_event in another structure and
arch_vm_event only holds a pointer to that struct
(arch_vm_event->monitor->...) - see 14/16 for details
- arch_vm_event.monitor freed entirely when the pending writes
are committed (monitor_ctrlreg_write_data()) or when the domain/vcpu is
destroyed
* acked-by in old v3: no-one
- P 16/16, was 4/8 in v3:
* changed since v3: use ENODEV error code instead of ENOSYS
* acked-by in old v3: no-one
The rest of patches in v3 (P5-P8) already made it to staging.
Sorry again and thanks,
Zuzu C.
[-- Attachment #1.2: Type: text/html, Size: 4055 bytes --]
[-- Attachment #2: Type: text/plain, Size: 127 bytes --]
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [PATCH 05/16] x86/monitor: relocate code more appropriately
2016-07-09 4:15 ` [PATCH 05/16] x86/monitor: relocate code more appropriately Corneliu ZUZU
@ 2016-07-11 6:19 ` Corneliu ZUZU
2016-07-12 7:45 ` Tian, Kevin
0 siblings, 1 reply; 54+ messages in thread
From: Corneliu ZUZU @ 2016-07-11 6:19 UTC (permalink / raw)
To: xen-devel
Cc: Kevin Tian, Tamas K Lengyel, Jan Beulich, Razvan Cojocaru,
Andrew Cooper, Jun Nakajima
This was 2/8 in the old v3-series "x86/vm-event: Adjustments & fixes".
Transferring code-review from "Tian, Kevin <kevin.tian@intel.com>" (July
11, 2016) and responding...
On 7/9/2016 7:15 AM, Corneliu ZUZU wrote:
> For readability:
>
> * Add function monitor_ctrlreg_write_data() (in x86/monitor.c) and separate
> handling of monitor_write_data there (previously done directly in
> hvm_do_resume()).
>
> * Separate enabling/disabling of CPU_BASED_CR3_LOAD_EXITING for CR3 monitor
> vm-events from CR0 node @ vmx_update_guest_cr(v, 0) into newly added function
> vmx_vm_event_update_cr3_traps(d); also, for a better interface, create generic
> functions monitor_ctrlreg_adjust_traps() and monitor_ctrlreg_disable_traps() (in
> x86/monitor.c) which deal with setting/unsetting any traps for cr-write monitor
> vm-events.
>
> Signed-off-by: Corneliu ZUZU <czuzu@bitdefender.com>
> ---
> xen/arch/x86/hvm/hvm.c | 46 ++++++++++----------------
> xen/arch/x86/hvm/vmx/vmx.c | 59 ++++++++++++++++++++++++++++++---
> xen/arch/x86/monitor.c | 68 +++++++++++++++++++++++++++++++++++----
> xen/include/asm-x86/hvm/vmx/vmx.h | 1 +
> xen/include/asm-x86/monitor.h | 2 ++
> 5 files changed, 135 insertions(+), 41 deletions(-)
>
> diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
> index 7f99087..79ba185 100644
> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -473,8 +473,6 @@ void hvm_do_resume(struct vcpu *v)
>
> if ( unlikely(v->arch.vm_event) )
> {
> - struct monitor_write_data *w = &v->arch.vm_event->write_data;
> -
> if ( unlikely(v->arch.vm_event->emulate_flags) )
> {
> enum emul_kind kind = EMUL_KIND_NORMAL;
> @@ -492,29 +490,7 @@ void hvm_do_resume(struct vcpu *v)
> v->arch.vm_event->emulate_flags = 0;
> }
>
> - if ( w->do_write.msr )
> - {
> - hvm_msr_write_intercept(w->msr, w->value, 0);
> - w->do_write.msr = 0;
> - }
> -
> - if ( w->do_write.cr0 )
> - {
> - hvm_set_cr0(w->cr0, 0);
> - w->do_write.cr0 = 0;
> - }
> -
> - if ( w->do_write.cr4 )
> - {
> - hvm_set_cr4(w->cr4, 0);
> - w->do_write.cr4 = 0;
> - }
> -
> - if ( w->do_write.cr3 )
> - {
> - hvm_set_cr3(w->cr3, 0);
> - w->do_write.cr3 = 0;
> - }
> + monitor_ctrlreg_write_data(v);
> }
>
> /* Inject pending hw/sw trap */
> @@ -2204,7 +2180,10 @@ int hvm_set_cr0(unsigned long value, bool_t may_defer)
>
> if ( hvm_monitor_crX(CR0, value, old_value) )
> {
> - /* The actual write will occur in hvm_do_resume(), if permitted. */
> + /*
> + * The actual write will occur in monitor_ctrlreg_write_data(), if
> + * permitted.
> + */
> v->arch.vm_event->write_data.do_write.cr0 = 1;
> v->arch.vm_event->write_data.cr0 = value;
>
> @@ -2306,7 +2285,10 @@ int hvm_set_cr3(unsigned long value, bool_t may_defer)
>
> if ( hvm_monitor_crX(CR3, value, old) )
> {
> - /* The actual write will occur in hvm_do_resume(), if permitted. */
> + /*
> + * The actual write will occur in monitor_ctrlreg_write_data(), if
> + * permitted.
> + */
> v->arch.vm_event->write_data.do_write.cr3 = 1;
> v->arch.vm_event->write_data.cr3 = value;
>
> @@ -2386,7 +2368,10 @@ int hvm_set_cr4(unsigned long value, bool_t may_defer)
>
> if ( hvm_monitor_crX(CR4, value, old_cr) )
> {
> - /* The actual write will occur in hvm_do_resume(), if permitted. */
> + /*
> + * The actual write will occur in monitor_ctrlreg_write_data(), if
> + * permitted.
> + */
> v->arch.vm_event->write_data.do_write.cr4 = 1;
> v->arch.vm_event->write_data.cr4 = value;
>
> @@ -3765,7 +3750,10 @@ int hvm_msr_write_intercept(unsigned int msr, uint64_t msr_content,
> {
> ASSERT(v->arch.vm_event);
>
> - /* The actual write will occur in hvm_do_resume() (if permitted). */
> + /*
> + * The actual write will occur in monitor_ctrlreg_write_data(), if
> + * permitted.
> + */
> v->arch.vm_event->write_data.do_write.msr = 1;
> v->arch.vm_event->write_data.msr = msr;
> v->arch.vm_event->write_data.value = msr_content;
> diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
> index 0798245..7a31582 100644
> --- a/xen/arch/x86/hvm/vmx/vmx.c
> +++ b/xen/arch/x86/hvm/vmx/vmx.c
> @@ -1441,11 +1441,6 @@ static void vmx_update_guest_cr(struct vcpu *v, unsigned int cr)
> if ( !hvm_paging_enabled(v) && !vmx_unrestricted_guest(v) )
> v->arch.hvm_vmx.exec_control |= cr3_ctls;
>
> - /* Trap CR3 updates if CR3 memory events are enabled. */
> - if ( v->domain->arch.monitor.write_ctrlreg_enabled &
> - monitor_ctrlreg_bitmask(VM_EVENT_X86_CR3) )
> - v->arch.hvm_vmx.exec_control |= CPU_BASED_CR3_LOAD_EXITING;
> -
> if ( old_ctls != v->arch.hvm_vmx.exec_control )
> vmx_update_cpu_exec_control(v);
> }
> @@ -3898,6 +3893,60 @@ void vmx_vmenter_helper(const struct cpu_user_regs *regs)
> }
>
> /*
> + * Adjusts domain CR3 load-exiting execution control for CR3-write monitor
> + * vm-event.
> + */
> +void vmx_vm_event_update_cr3_traps(const struct domain *d)
> +{
> + struct vcpu *v;
> + struct arch_vmx_struct *avmx;
[Kevin wrote]:
better move to inner later loop.
---
Ack.
> + unsigned int cr3_bitmask;
> + bool_t cr3_vmevent, cr3_ldexit;
> +
> + /* Domain must be paused. */
> + ASSERT(atomic_read(&d->pause_count));
[Kevin wrote]:
The comment doesn't add more info than the code itself, unless you want to
explain the reason why domain must be paused here.
---
Ack, will remove the comment (or explain why the domain is paused: probably because we're writing exec_control of all its vcpus).
> +
> + /* Non-hap domains trap CR3 writes unconditionally. */
> + if ( !paging_mode_hap(d) )
> + {
> +#ifndef NDEBUG
> + for_each_vcpu ( d, v )
> + ASSERT(v->arch.hvm_vmx.exec_control & CPU_BASED_CR3_LOAD_EXITING);
> +#endif
> + return;
> + }
> +
> + cr3_bitmask = monitor_ctrlreg_bitmask(VM_EVENT_X86_CR3);
> + cr3_vmevent = !!(d->arch.monitor.write_ctrlreg_enabled & cr3_bitmask);
> +
> + for_each_vcpu ( d, v )
> + {
> + avmx = &v->arch.hvm_vmx;
> + cr3_ldexit = !!(avmx->exec_control & CPU_BASED_CR3_LOAD_EXITING);
> +
> + if ( cr3_vmevent == cr3_ldexit )
> + continue;
> +
> + /*
> + * If domain paging is disabled (CR0.PG=0) and
> + * the domain is not in real-mode, then CR3 load-exiting
> + * must remain enabled (see vmx_update_guest_cr(v, cr) when cr = 0).
> + */
[Kevin wrote]:
Please give the real informative message here instead of asking people to
look at other place
---
I only now realized I might have mistakenly assumed something when writing up this function.
I'll amend that in a v2 and consider making this comment clearer (will ask for feedback from you again here before sending v2).
> + if ( cr3_ldexit && !hvm_paging_enabled(v) && !vmx_unrestricted_guest(v) )
> + continue;
[Kevin wrote]:
if !cr3_ldexit is not expected to occur in such scenario, is it more strict as below?
if ( !hvm_paging_enabled(v) && !vmx_unrestricted_guest(v) )
{
ASSERT(cr3_ldexit);
continue;
}
---
That actually seems to be correct, I thought it was possible to have CR3
load-exiting disabled with the above condition true (by looking @ old
the code). I must attentively re-review this entire function so that it
preserves the old behavior.
> +
> + if ( cr3_vmevent )
> + avmx->exec_control |= CPU_BASED_CR3_LOAD_EXITING;
> + else
> + avmx->exec_control &= ~CPU_BASED_CR3_LOAD_EXITING;
> +
> + vmx_vmcs_enter(v);
> + vmx_update_cpu_exec_control(v);
> + vmx_vmcs_exit(v);
> + }
> +}
> +
> +/*
> * Local variables:
> * mode: C
> * c-file-style: "BSD"
> diff --git a/xen/arch/x86/monitor.c b/xen/arch/x86/monitor.c
> index 06d21b1..29188e5 100644
> --- a/xen/arch/x86/monitor.c
> +++ b/xen/arch/x86/monitor.c
> @@ -19,10 +19,39 @@
> * License along with this program; If not, see <http://www.gnu.org/licenses/>.
> */
>
> +#include <asm/hvm/vmx/vmx.h>
> #include <asm/monitor.h>
> #include <asm/vm_event.h>
> #include <public/vm_event.h>
>
> +static inline
> +void monitor_ctrlreg_adjust_traps(struct domain *d, unsigned int index)
> +{
> + /* For now, VMX only. */
> + ASSERT(cpu_has_vmx);
> +
> + /* Other CRs than CR3 are always trapped. */
> + if ( VM_EVENT_X86_CR3 == index )
> + vmx_vm_event_update_cr3_traps(d);
[Kevin wrote]:
Please add above into a hvm function instead of directly calling
vmx in common file. (other arch can have it unimplemented).
Then you don't need change this common code again later when
other archs are added
---
This is not common code, it's in arch/x86/monitor.c (?) and currently,
as the above ASSERT indicates, only VMX is supported. If in the future
support for SVM for example will be added, then the hvm move you suggest
must be done (Jan also suggested this).
Or, I only now realize, if you guys prefer doing this now I could add a
vm_event_update_cr3_traps field in hvm_function_table, but BUG() in the
SVM one..
> +}
> +
> +static inline void monitor_ctrlreg_disable_traps(struct domain *d)
> +{
> + unsigned int old = d->arch.monitor.write_ctrlreg_enabled;
> +
> + d->arch.monitor.write_ctrlreg_enabled = 0;
> +
> + if ( old )
> + {
> + /* For now, VMX only. */
> + ASSERT(cpu_has_vmx);
> +
> + /* Was CR3 load-exiting enabled due to monitoring? */
> + if ( old & monitor_ctrlreg_bitmask(VM_EVENT_X86_CR3) )
> + vmx_vm_event_update_cr3_traps(d);
> + }
> +}
> +
> int monitor_init_domain(struct domain *d)
> {
> if ( !d->arch.monitor.msr_bitmap )
> @@ -38,6 +67,8 @@ void monitor_cleanup_domain(struct domain *d)
> {
> xfree(d->arch.monitor.msr_bitmap);
>
> + monitor_ctrlreg_disable_traps(d);
> +
> memset(&d->arch.monitor, 0, sizeof(d->arch.monitor));
> memset(&d->monitor, 0, sizeof(d->monitor));
> }
> @@ -79,6 +110,35 @@ void monitor_ctrlreg_write_resume(struct vcpu *v, vm_event_response_t *rsp)
> }
> }
>
> +void monitor_ctrlreg_write_data(struct vcpu *v)
> +{
> + struct monitor_write_data *w = &v->arch.vm_event->write_data;
> +
> + if ( w->do_write.msr )
> + {
> + hvm_msr_write_intercept(w->msr, w->value, 0);
> + w->do_write.msr = 0;
> + }
> +
> + if ( w->do_write.cr0 )
> + {
> + hvm_set_cr0(w->cr0, 0);
> + w->do_write.cr0 = 0;
> + }
> +
> + if ( w->do_write.cr4 )
> + {
> + hvm_set_cr4(w->cr4, 0);
> + w->do_write.cr4 = 0;
> + }
> +
> + if ( w->do_write.cr3 )
> + {
> + hvm_set_cr3(w->cr3, 0);
> + w->do_write.cr3 = 0;
> + }
> +}
> +
> static unsigned long *monitor_bitmap_for_msr(const struct domain *d, u32 *msr)
> {
> ASSERT(d->arch.monitor.msr_bitmap && msr);
> @@ -197,13 +257,7 @@ int arch_monitor_domctl_event(struct domain *d,
> else
> ad->monitor.write_ctrlreg_enabled &= ~ctrlreg_bitmask;
>
> - if ( VM_EVENT_X86_CR3 == mop->u.mov_to_cr.index )
> - {
> - struct vcpu *v;
> - /* Latches new CR3 mask through CR0 code. */
> - for_each_vcpu ( d, v )
> - hvm_update_guest_cr(v, 0);
> - }
> + monitor_ctrlreg_adjust_traps(d, mop->u.mov_to_cr.index);
>
> domain_unpause(d);
>
> diff --git a/xen/include/asm-x86/hvm/vmx/vmx.h b/xen/include/asm-x86/hvm/vmx/vmx.h
> index 359b2a9..301df56 100644
> --- a/xen/include/asm-x86/hvm/vmx/vmx.h
> +++ b/xen/include/asm-x86/hvm/vmx/vmx.h
> @@ -101,6 +101,7 @@ void vmx_update_debug_state(struct vcpu *v);
> void vmx_update_exception_bitmap(struct vcpu *v);
> void vmx_update_cpu_exec_control(struct vcpu *v);
> void vmx_update_secondary_exec_control(struct vcpu *v);
> +void vmx_vm_event_update_cr3_traps(const struct domain *d);
>
> #define POSTED_INTR_ON 0
> #define POSTED_INTR_SN 1
> diff --git a/xen/include/asm-x86/monitor.h b/xen/include/asm-x86/monitor.h
> index adca378..3ae24dd 100644
> --- a/xen/include/asm-x86/monitor.h
> +++ b/xen/include/asm-x86/monitor.h
> @@ -93,6 +93,8 @@ void monitor_cleanup_domain(struct domain *d);
>
> void monitor_ctrlreg_write_resume(struct vcpu *v, vm_event_response_t *rsp);
>
> +void monitor_ctrlreg_write_data(struct vcpu *v);
> +
> bool_t monitored_msr(const struct domain *d, u32 msr);
>
> #endif /* __ASM_X86_MONITOR_H__ */
Thanks,
Zuzu C.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [PATCH 01/16] x86/vmx_update_guest_cr: minor optimization
2016-07-09 4:12 ` [PATCH 01/16] x86/vmx_update_guest_cr: minor optimization Corneliu ZUZU
@ 2016-07-11 6:24 ` Corneliu ZUZU
0 siblings, 0 replies; 54+ messages in thread
From: Corneliu ZUZU @ 2016-07-11 6:24 UTC (permalink / raw)
To: xen-devel; +Cc: Andrew Cooper, Kevin Tian, Jun Nakajima, Jan Beulich
[-- Attachment #1.1: Type: text/plain, Size: 1844 bytes --]
This was 1/8 in the old v3-series "x86/vm-event: Adjustments & fixes".
Transferring code-review from "Tian, Kevin <kevin.tian@intel.com>" (July
11, 2016).
On 7/9/2016 7:12 AM, Corneliu ZUZU wrote:
> Minor optimization @ vmx_update_guest_cr: checks if v->arch.hvm_vmx.exec_control
> was modified before actually calling vmx_update_cpu_exec_control(v).
>
> Signed-off-by: Corneliu ZUZU <czuzu@bitdefender.com>
> ---
> xen/arch/x86/hvm/vmx/vmx.c | 5 ++++-
> 1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
> index 0776d12..0798245 100644
> --- a/xen/arch/x86/hvm/vmx/vmx.c
> +++ b/xen/arch/x86/hvm/vmx/vmx.c
> @@ -1433,8 +1433,10 @@ static void vmx_update_guest_cr(struct vcpu *v, unsigned int cr)
> if ( paging_mode_hap(v->domain) )
> {
> /* Manage GUEST_CR3 when CR0.PE=0. */
> + uint32_t old_ctls = v->arch.hvm_vmx.exec_control;
> uint32_t cr3_ctls = (CPU_BASED_CR3_LOAD_EXITING |
> CPU_BASED_CR3_STORE_EXITING);
> +
> v->arch.hvm_vmx.exec_control &= ~cr3_ctls;
> if ( !hvm_paging_enabled(v) && !vmx_unrestricted_guest(v) )
> v->arch.hvm_vmx.exec_control |= cr3_ctls;
> @@ -1444,7 +1446,8 @@ static void vmx_update_guest_cr(struct vcpu *v, unsigned int cr)
> monitor_ctrlreg_bitmask(VM_EVENT_X86_CR3) )
> v->arch.hvm_vmx.exec_control |= CPU_BASED_CR3_LOAD_EXITING;
>
> - vmx_update_cpu_exec_control(v);
> + if ( old_ctls != v->arch.hvm_vmx.exec_control )
> + vmx_update_cpu_exec_control(v);
> }
>
> if ( !nestedhvm_vcpu_in_guestmode(v) )
[Kevin wrote]:
Acked-by: Kevin Tian <kevin.tian@intel.com>
---
Thanks,
Zuzu C.
[-- Attachment #1.2: Type: text/html, Size: 2539 bytes --]
[-- Attachment #2: Type: text/plain, Size: 127 bytes --]
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [PATCH 02/16] x86: fix: make atomic_read() param const
2016-07-09 4:12 ` [PATCH 02/16] x86: fix: make atomic_read() param const Corneliu ZUZU
@ 2016-07-11 15:18 ` Andrew Cooper
2016-07-12 5:11 ` Corneliu ZUZU
0 siblings, 1 reply; 54+ messages in thread
From: Andrew Cooper @ 2016-07-11 15:18 UTC (permalink / raw)
To: Corneliu ZUZU, xen-devel; +Cc: Jan Beulich
On 09/07/16 05:12, Corneliu ZUZU wrote:
> This wouldn't let me make a param of a function that used atomic_read() const.
>
> Signed-off-by: Corneliu ZUZU <czuzu@bitdefender.com>
This is a good improvement, but you must make an identical adjustment to
the arm code, otherwise you will end up with subtle build failures.
If you are really feeling up to it, having a common xen/atomic.h with
typedef struct { int counter; } atomic_t;
#define ATOMIC_INIT(i) { (i) }
and some prototypes such as:
static inline int atomic_read(const atomic_t *v);
would be great, but this looks like it has the possibility to turn into
a rats nest. If it does, then just doubling up this code for arm is ok.
~Andrew
> ---
> xen/include/asm-x86/atomic.h | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/xen/include/asm-x86/atomic.h b/xen/include/asm-x86/atomic.h
> index d246b70..0b250c8 100644
> --- a/xen/include/asm-x86/atomic.h
> +++ b/xen/include/asm-x86/atomic.h
> @@ -94,7 +94,7 @@ typedef struct { int counter; } atomic_t;
> *
> * Atomically reads the value of @v.
> */
> -static inline int atomic_read(atomic_t *v)
> +static inline int atomic_read(const atomic_t *v)
> {
> return read_atomic(&v->counter);
> }
> @@ -105,7 +105,7 @@ static inline int atomic_read(atomic_t *v)
> *
> * Non-atomically reads the value of @v
> */
> -static inline int _atomic_read(atomic_t v)
> +static inline int _atomic_read(const atomic_t v)
> {
> return v.counter;
> }
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [PATCH 11/16] x86/monitor: fix: treat -monitor- properly, as a subsys of the vm-event subsys
2016-07-09 17:46 ` Corneliu ZUZU
@ 2016-07-11 16:38 ` Tamas K Lengyel
2016-07-11 20:20 ` Corneliu ZUZU
0 siblings, 1 reply; 54+ messages in thread
From: Tamas K Lengyel @ 2016-07-11 16:38 UTC (permalink / raw)
To: Corneliu ZUZU
Cc: Stefano Stabellini, Razvan Cojocaru, Andrew Cooper, Xen-devel,
Julien Grall, Paul Durrant, Jan Beulich
>>> 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
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [PATCH 03/16] x86/monitor: mechanical renames
2016-07-09 18:46 ` Corneliu ZUZU
@ 2016-07-11 16:43 ` Tamas K Lengyel
2016-07-12 6:10 ` Corneliu ZUZU
0 siblings, 1 reply; 54+ messages in thread
From: Tamas K Lengyel @ 2016-07-11 16:43 UTC (permalink / raw)
To: Corneliu ZUZU
Cc: Stefano Stabellini, Razvan Cojocaru, Andrew Cooper, Xen-devel,
Julien Grall, Jan Beulich
On Sat, Jul 9, 2016 at 12:46 PM, Corneliu ZUZU <czuzu@bitdefender.com> wrote:
> On 7/9/2016 9:10 PM, Tamas K Lengyel wrote:
>>
>> On Fri, Jul 8, 2016 at 10:13 PM, Corneliu ZUZU <czuzu@bitdefender.com>
>> wrote:
>>>
>>> Arch-specific vm-event functions in x86/vm_event.h -e.g.
>>> vm_event_init_domain()-
>>> don't have an 'arch_' prefix. Apply the same rule for monitor functions -
>>> originally the only two monitor functions that had an 'arch_' prefix were
>>> arch_monitor_domctl_event() and arch_monitor_domctl_op(), but I gave them
>>> that
>>> prefix because -they had a counterpart function in common code-, that
>>> being
>>> monitor_domctl().
>>
>> This should actually be the other way around - ie adding the arch_
>> prefix to vm_event functions that lack it.
>
>
> Given that the majority of the arch-specific functions called from
> common-code don't have an 'arch_' prefix unless they have a common
> counterpart, I was guessing that was the rule. It made sense in my head
> since I saw in that the intention of avoiding naming conflicts (i.e you
> can't have monitor_domctl() both on the common-side and on the arch-side, so
> prepend 'arch_' to the latter). I noticed you guys also 'skip' the prefix
> when sending patches, so that reinforced my assumption.
>
>> Having the arch_ prefix is
>> helpful to know that the function is dealing with the arch specific
>> structs and not common.
>
>
> Personally I don't see much use in 'knowing that the function is dealing
> with the arch structs' from the call-site and you can tell that from the
> implementation-site just by looking at the path of its source file. Also,
> the code is pretty much localized in the arch directory anyway so usually
> one wouldn't have to go back and forth between common and arch that often.
> What really bothers me though is always having to read 'arch_' when spelling
> a function-name and also that it makes the function name longer without much
> benefit. Your suggestion of adding it to pretty much all functions that make
> up the interface to common just adds to that headache. :-D
>
>> Similarly that's why we have the hvm_ prefix
>> for functions in hvm/monitor.
>
>
> 'hvm_' doesn't seem to me more special than 'monitor_', for instance, but
> maybe that's just me.
>
>>> Let this also be the rule for future 'arch_' functions additions, and
>>> with this
>>> patch remove the 'arch_' prefix from the monitor functions that don't
>>> have a
>>> counterpart in common-code (all but those 2 aforementioned).
>>
>> Even if there are no common counter-parts to the function, the arch_
>> prefix should remain, so I won't be able to ack this patch.
>>
>> Tamas
>
>
> Having said the above, are you still of the same opinion?
Yes, I am. While it's not a hard rule to always apply these prefix, it
does make sense to have them so I don't see benefit in removing the
existing prefixes. Adding arch_ prefix to the ones that don't already
have one is optional, I was just pointing out that if you really feel
like standardizing the naming convention, that's where I would like
things to move towards to.
Tamas
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [PATCH 11/16] x86/monitor: fix: treat -monitor- properly, as a subsys of the vm-event subsys
2016-07-11 16:38 ` Tamas K Lengyel
@ 2016-07-11 20:20 ` Corneliu ZUZU
2016-07-11 21:27 ` Tamas K Lengyel
0 siblings, 1 reply; 54+ messages in thread
From: Corneliu ZUZU @ 2016-07-11 20:20 UTC (permalink / raw)
To: Tamas K Lengyel
Cc: Stefano Stabellini, Razvan Cojocaru, Andrew Cooper, Xen-devel,
Julien Grall, Paul Durrant, Jan Beulich
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.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [PATCH 11/16] x86/monitor: fix: treat -monitor- properly, as a subsys of the vm-event subsys
2016-07-11 20:20 ` Corneliu ZUZU
@ 2016-07-11 21:27 ` Tamas K Lengyel
2016-07-11 21:47 ` Corneliu ZUZU
0 siblings, 1 reply; 54+ messages in thread
From: Tamas K Lengyel @ 2016-07-11 21:27 UTC (permalink / raw)
To: Corneliu ZUZU
Cc: Stefano Stabellini, Razvan Cojocaru, Andrew Cooper, Xen-devel,
Julien Grall, Paul Durrant, Jan Beulich
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
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [PATCH 11/16] x86/monitor: fix: treat -monitor- properly, as a subsys of the vm-event subsys
2016-07-11 21:27 ` Tamas K Lengyel
@ 2016-07-11 21:47 ` Corneliu ZUZU
0 siblings, 0 replies; 54+ messages in thread
From: Corneliu ZUZU @ 2016-07-11 21:47 UTC (permalink / raw)
To: Tamas K Lengyel
Cc: Stefano Stabellini, Razvan Cojocaru, Andrew Cooper, Xen-devel,
Julien Grall, Paul Durrant, Jan Beulich
[-- Attachment #1.1: Type: text/plain, Size: 2246 bytes --]
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.
[-- Attachment #1.2: Type: text/html, Size: 5161 bytes --]
[-- Attachment #2: Type: text/plain, Size: 127 bytes --]
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [PATCH 02/16] x86: fix: make atomic_read() param const
2016-07-11 15:18 ` Andrew Cooper
@ 2016-07-12 5:11 ` Corneliu ZUZU
2016-07-12 9:42 ` Andrew Cooper
0 siblings, 1 reply; 54+ messages in thread
From: Corneliu ZUZU @ 2016-07-12 5:11 UTC (permalink / raw)
To: Andrew Cooper, xen-devel; +Cc: Jan Beulich
Hi Andrew,
On 7/11/2016 6:18 PM, Andrew Cooper wrote:
> On 09/07/16 05:12, Corneliu ZUZU wrote:
>> This wouldn't let me make a param of a function that used atomic_read() const.
>>
>> Signed-off-by: Corneliu ZUZU <czuzu@bitdefender.com>
> This is a good improvement, but you must make an identical adjustment to
> the arm code, otherwise you will end up with subtle build failures.
Right, didn't even realize it was X86-specific.
>
> If you are really feeling up to it, having a common xen/atomic.h with
>
> typedef struct { int counter; } atomic_t;
> #define ATOMIC_INIT(i) { (i) }
>
> and some prototypes such as:
>
> static inline int atomic_read(const atomic_t *v);
>
> would be great, but this looks like it has the possibility to turn into
> a rats nest. If it does, then just doubling up this code for arm is ok.
>
> ~Andrew
Yes, that might be more complicated than we expect and I don't know if
making code such as this common would be a good idea, usually these
functions are always architecture-specific. It might be better to keep
them separate - they don't add much anyway since their implementation is
short - than risk unexpected different behavior on a future arch. But
then again I don't know much details of their implementation, so anyway,
I'd surely prefer to do this kind of change in a separate patch.
>
>> ---
>> xen/include/asm-x86/atomic.h | 4 ++--
>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/xen/include/asm-x86/atomic.h b/xen/include/asm-x86/atomic.h
>> index d246b70..0b250c8 100644
>> --- a/xen/include/asm-x86/atomic.h
>> +++ b/xen/include/asm-x86/atomic.h
>> @@ -94,7 +94,7 @@ typedef struct { int counter; } atomic_t;
>> *
>> * Atomically reads the value of @v.
>> */
>> -static inline int atomic_read(atomic_t *v)
>> +static inline int atomic_read(const atomic_t *v)
>> {
>> return read_atomic(&v->counter);
>> }
>> @@ -105,7 +105,7 @@ static inline int atomic_read(atomic_t *v)
>> *
>> * Non-atomically reads the value of @v
>> */
>> -static inline int _atomic_read(atomic_t v)
>> +static inline int _atomic_read(const atomic_t v)
>> {
>> return v.counter;
>> }
>
Thanks,
Zuzu C.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [PATCH 03/16] x86/monitor: mechanical renames
2016-07-11 16:43 ` Tamas K Lengyel
@ 2016-07-12 6:10 ` Corneliu ZUZU
2016-07-15 7:18 ` Corneliu ZUZU
0 siblings, 1 reply; 54+ messages in thread
From: Corneliu ZUZU @ 2016-07-12 6:10 UTC (permalink / raw)
To: Tamas K Lengyel, Jan Beulich
Cc: Andrew Cooper, Julien Grall, Stefano Stabellini, Razvan Cojocaru,
Xen-devel
On 7/11/2016 7:43 PM, Tamas K Lengyel wrote:
> On Sat, Jul 9, 2016 at 12:46 PM, Corneliu ZUZU <czuzu@bitdefender.com> wrote:
>> On 7/9/2016 9:10 PM, Tamas K Lengyel wrote:
>>> On Fri, Jul 8, 2016 at 10:13 PM, Corneliu ZUZU <czuzu@bitdefender.com>
>>> wrote:
>>>> Arch-specific vm-event functions in x86/vm_event.h -e.g.
>>>> vm_event_init_domain()-
>>>> don't have an 'arch_' prefix. Apply the same rule for monitor functions -
>>>> originally the only two monitor functions that had an 'arch_' prefix were
>>>> arch_monitor_domctl_event() and arch_monitor_domctl_op(), but I gave them
>>>> that
>>>> prefix because -they had a counterpart function in common code-, that
>>>> being
>>>> monitor_domctl().
>>> This should actually be the other way around - ie adding the arch_
>>> prefix to vm_event functions that lack it.
>>
>> Given that the majority of the arch-specific functions called from
>> common-code don't have an 'arch_' prefix unless they have a common
>> counterpart, I was guessing that was the rule. It made sense in my head
>> since I saw in that the intention of avoiding naming conflicts (i.e you
>> can't have monitor_domctl() both on the common-side and on the arch-side, so
>> prepend 'arch_' to the latter). I noticed you guys also 'skip' the prefix
>> when sending patches, so that reinforced my assumption.
>>
>>> Having the arch_ prefix is
>>> helpful to know that the function is dealing with the arch specific
>>> structs and not common.
>>
>> Personally I don't see much use in 'knowing that the function is dealing
>> with the arch structs' from the call-site and you can tell that from the
>> implementation-site just by looking at the path of its source file. Also,
>> the code is pretty much localized in the arch directory anyway so usually
>> one wouldn't have to go back and forth between common and arch that often.
>> What really bothers me though is always having to read 'arch_' when spelling
>> a function-name and also that it makes the function name longer without much
>> benefit. Your suggestion of adding it to pretty much all functions that make
>> up the interface to common just adds to that headache. :-D
>>
>>> Similarly that's why we have the hvm_ prefix
>>> for functions in hvm/monitor.
>>
>> 'hvm_' doesn't seem to me more special than 'monitor_', for instance, but
>> maybe that's just me.
>>
>>>> Let this also be the rule for future 'arch_' functions additions, and
>>>> with this
>>>> patch remove the 'arch_' prefix from the monitor functions that don't
>>>> have a
>>>> counterpart in common-code (all but those 2 aforementioned).
>>> Even if there are no common counter-parts to the function, the arch_
>>> prefix should remain, so I won't be able to ack this patch.
>>>
>>> Tamas
>>
>> Having said the above, are you still of the same opinion?
> Yes, I am. While it's not a hard rule to always apply these prefix, it
> does make sense to have them so I don't see benefit in removing the
> existing prefixes.
Well, for one the benefit would be not confusing developers by creating
inconsistencies: what's the rule here, i.e. why isn't a function such as
alloc_domain_struct prefixed w/ 'arch_' but arch_domain_create is? The
reason seems to be the latter having a common counterpart while the
former not, at least that's what I see being done all over the
code-base. Also, I've done this before and you seemed to agree:
https://www.mail-archive.com/xen-devel%40lists.xen.org/msg57617.html
(Q1). You also suggested creating arch-specific functions without the
prefix:
https://www.mail-archive.com/xen-devel%40lists.xen.org/msg57336.html .
Why the sudden change of heart?
2ndly and obviously, removing the prefixes would make function names
shorter and clearer (e.g. -read- "arch_vm_event_vcpu_unpause" and then
read "vm_event_vcpu_unpause").
3rd reason is that adding the prefix to -all- arch-specific functions
called from common would mean having a lot new functions with the
prefix. I'd read the prefix over and over again and at some point I'd
get annoyed and say "ok, ok, it's arch_, I get it; why use this prefix
so much again?".
4th reason is that the advantage of telling that the function accesses
arch structures is much too little considering that idk, >50% of the
codebase is arch-specific, so it doesn't provide much information, this
categorization is too broad to deserve a special prefix. Whereas using
the prefix only for functions that do have a common counterpart gives
you the extra information that the 'operation' is only -partly-
arch-specific, i.e. to see the whole picture, look @ the common-side
implementation. Keep in mind that we'd also be -losing that information-
if we were to apply the 'go with arch_ for all' rule.. (this could be a
5th reason)
> Adding arch_ prefix to the ones that don't already
> have one is optional, I was just pointing out that if you really feel
> like standardizing the naming convention, that's where I would like
> things to move towards to.
>
> Tamas
I don't think I'd say this patch "standardizes the naming convention"
but rather "fixes code that doesn't conform to the -already existing-
standard naming convention". Above stated reasons explain why I'd rather
-not- have this standard go in the direction of adding 'arch_' before a
lot of functions.
Finally, I feel like asking for feedback as I don't like to insist when
the majority agrees to disagree. Jan & others, what's the rule here and
what's your view on this? :-)
Thanks,
Zuzu C.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [PATCH 00/16] x86/vm-event: numerous adjustments & fixes
2016-07-11 5:32 ` Corneliu ZUZU
@ 2016-07-12 7:42 ` Tian, Kevin
0 siblings, 0 replies; 54+ messages in thread
From: Tian, Kevin @ 2016-07-12 7:42 UTC (permalink / raw)
To: Corneliu ZUZU, xen-devel
Cc: Stefano Stabellini, Jan Beulich, Razvan Cojocaru, George Dunlap,
Andrew Cooper, Julien Grall, Paul Durrant, Tamas K Lengyel,
Nakajima, Jun
[-- Attachment #1.1: Type: text/plain, Size: 3703 bytes --]
Thanks for explanation. It makes sense then.
From: Corneliu ZUZU [mailto:czuzu@bitdefender.com]
Sent: Monday, July 11, 2016 1:32 PM
To: Tian, Kevin; xen-devel@lists.xen.org
Cc: Andrew Cooper; George Dunlap; Jan Beulich; Julien Grall; Nakajima, Jun; Paul Durrant; Razvan Cojocaru; Stefano Stabellini; Tamas K Lengyel
Subject: Re: [PATCH 00/16] x86/vm-event: numerous adjustments & fixes
On 7/11/2016 5:54 AM, Tian, Kevin wrote:
what's the difference between this series and earlier one?
[PATCH v3 0/8] x86/vm-event: Adjustments & fixes
looks you have some patches cross-posted (e.g. 1/16)...
Corneliu ZUZU (16):
x86/vmx_update_guest_cr: minor optimization
x86: fix: make atomic_read() param const
x86/monitor: mechanical renames
x86/monitor: relocate vm_event_register_write_resume() function to
monitor code
x86/monitor: relocate code more appropriately
x86/monitor: fix: set msr_bitmap to NULL after xfree
x86/vm-event: fix: call cleanup when init fails, to free partial
allocs
x86/vm-event: call monitor init & cleanup funcs from respective
vm_event funcs
arm/monitor: move d->monitor cleanup to monitor_cleanup_domain()
x86/vm-event: centralize vcpu-destroy cleanup in vm-events code
x86/monitor: fix: treat -monitor- properly, as a subsys of the
vm-event subsys
x86/vm-event: fix: move cleanup of mem_access_emulate_each_rep to
monitor stub
x86/monitor: introduce writes_pending field in monitor_write_data
x86/monitor: clarify separation between monitor subsys and vm-event as
a whole
x86/monitor: fix: don't compromise a monitor_write_data with pending
CR writes
x86/monitor: fix: xc_monitor _write_ctrlreg w/o previous _enable must
fail
--
2.5.0
Hi Kevin,
I'm sorry about the confusion, the _older v3 series is superseded by this 16-patches one_. I thought the latter was too complex to consider as a v4 over the former, but I should have at least stated something to lead you guys to that conclusion (and to specify changes..). The patches that are still in this series from the older one, the changes and the acks are as following:
- P 1/16, was 1/8 in v3
* changed since v3: nothing
* acked-by in old v3: Acked-by: Kevin Tian <kevin.tian@intel.com><mailto:kevin.tian@intel.com>
- P 5/16, was 2/8 in v3
* changed since v3:
- title slightly changed
- renames due to 3/16: arch_monitor_write_data()->monitor_ctrlreg_write_data(), write_ctrlreg_adjust_traps()->monitor_ctrlreg_adjust_traps(), write_ctrlreg_disable_traps()->monitor_ctrlreg_disable_traps()
- added 'const' attribute to "struct *domain" param of vmx_vm_event_update_cr3_traps
- 'index' param of monitor_ctrlreg_adjust_traps now of type 'unsigned int'
* acked-by in old v3: no-one
- P 15/16, was 3/8 in v3:
* changed since v3 (IMO changes are too many and depends on previous patches in this 16-p-series and it would be better if the old v3 one is simply forgotten-about):
- separated fields in arch_vm_event in another structure and arch_vm_event only holds a pointer to that struct (arch_vm_event->monitor->...) - see 14/16 for details
- arch_vm_event.monitor freed entirely when the pending writes are committed (monitor_ctrlreg_write_data()) or when the domain/vcpu is destroyed
* acked-by in old v3: no-one
- P 16/16, was 4/8 in v3:
* changed since v3: use ENODEV error code instead of ENOSYS
* acked-by in old v3: no-one
The rest of patches in v3 (P5-P8) already made it to staging.
Sorry again and thanks,
Zuzu C.
[-- Attachment #1.2: Type: text/html, Size: 9561 bytes --]
[-- Attachment #2: Type: text/plain, Size: 127 bytes --]
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [PATCH 05/16] x86/monitor: relocate code more appropriately
2016-07-11 6:19 ` Corneliu ZUZU
@ 2016-07-12 7:45 ` Tian, Kevin
2016-07-12 8:07 ` Corneliu ZUZU
0 siblings, 1 reply; 54+ messages in thread
From: Tian, Kevin @ 2016-07-12 7:45 UTC (permalink / raw)
To: Corneliu ZUZU, xen-devel
Cc: Nakajima, Jun, Andrew Cooper, Tamas K Lengyel, Razvan Cojocaru,
Jan Beulich
> From: Corneliu ZUZU [mailto:czuzu@bitdefender.com]
> Sent: Monday, July 11, 2016 2:19 PM
> >
> > +static inline
> > +void monitor_ctrlreg_adjust_traps(struct domain *d, unsigned int index)
> > +{
> > + /* For now, VMX only. */
> > + ASSERT(cpu_has_vmx);
> > +
> > + /* Other CRs than CR3 are always trapped. */
> > + if ( VM_EVENT_X86_CR3 == index )
> > + vmx_vm_event_update_cr3_traps(d);
> [Kevin wrote]:
>
> Please add above into a hvm function instead of directly calling
> vmx in common file. (other arch can have it unimplemented).
> Then you don't need change this common code again later when
> other archs are added
>
> ---
>
>
> This is not common code, it's in arch/x86/monitor.c (?) and currently,
> as the above ASSERT indicates, only VMX is supported. If in the future
> support for SVM for example will be added, then the hvm move you suggest
> must be done (Jan also suggested this).
> Or, I only now realize, if you guys prefer doing this now I could add a
> vm_event_update_cr3_traps field in hvm_function_table, but BUG() in the
> SVM one..
>
The latter is desired. Instead of BUG, it makes more sense to return
error on an arch which doesn't register the callback.
Thanks
Kevin
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [PATCH 05/16] x86/monitor: relocate code more appropriately
2016-07-12 7:45 ` Tian, Kevin
@ 2016-07-12 8:07 ` Corneliu ZUZU
2016-07-15 11:41 ` Corneliu ZUZU
0 siblings, 1 reply; 54+ messages in thread
From: Corneliu ZUZU @ 2016-07-12 8:07 UTC (permalink / raw)
To: Tian, Kevin, xen-devel
Cc: Nakajima, Jun, Andrew Cooper, Tamas K Lengyel, Razvan Cojocaru,
Jan Beulich
On 7/12/2016 10:45 AM, Tian, Kevin wrote:
>> From: Corneliu ZUZU [mailto:czuzu@bitdefender.com]
>> Sent: Monday, July 11, 2016 2:19 PM
>>> +static inline
>>> +void monitor_ctrlreg_adjust_traps(struct domain *d, unsigned int index)
>>> +{
>>> + /* For now, VMX only. */
>>> + ASSERT(cpu_has_vmx);
>>> +
>>> + /* Other CRs than CR3 are always trapped. */
>>> + if ( VM_EVENT_X86_CR3 == index )
>>> + vmx_vm_event_update_cr3_traps(d);
>> [Kevin wrote]:
>>
>> Please add above into a hvm function instead of directly calling
>> vmx in common file. (other arch can have it unimplemented).
>> Then you don't need change this common code again later when
>> other archs are added
>>
>> ---
>>
>>
>> This is not common code, it's in arch/x86/monitor.c (?) and currently,
>> as the above ASSERT indicates, only VMX is supported. If in the future
>> support for SVM for example will be added, then the hvm move you suggest
>> must be done (Jan also suggested this).
>> Or, I only now realize, if you guys prefer doing this now I could add a
>> vm_event_update_cr3_traps field in hvm_function_table, but BUG() in the
>> SVM one..
>>
> The latter is desired. Instead of BUG, it makes more sense to return
> error on an arch which doesn't register the callback.
>
> Thanks
> Kevin
Will do.
Thanks,
Zuzu C.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [PATCH 02/16] x86: fix: make atomic_read() param const
2016-07-12 5:11 ` Corneliu ZUZU
@ 2016-07-12 9:42 ` Andrew Cooper
2016-07-12 10:11 ` Corneliu ZUZU
0 siblings, 1 reply; 54+ messages in thread
From: Andrew Cooper @ 2016-07-12 9:42 UTC (permalink / raw)
To: Corneliu ZUZU, xen-devel; +Cc: Jan Beulich
On 12/07/16 06:11, Corneliu ZUZU wrote:
> Hi Andrew,
>
> On 7/11/2016 6:18 PM, Andrew Cooper wrote:
>> On 09/07/16 05:12, Corneliu ZUZU wrote:
>>> This wouldn't let me make a param of a function that used
>>> atomic_read() const.
>>>
>>> Signed-off-by: Corneliu ZUZU <czuzu@bitdefender.com>
>> This is a good improvement, but you must make an identical adjustment to
>> the arm code, otherwise you will end up with subtle build failures.
>
> Right, didn't even realize it was X86-specific.
It isn't x86 specific. (Which is what I presume you meant to say.)
>
>>
>> If you are really feeling up to it, having a common xen/atomic.h with
>>
>> typedef struct { int counter; } atomic_t;
>> #define ATOMIC_INIT(i) { (i) }
>>
>> and some prototypes such as:
>>
>> static inline int atomic_read(const atomic_t *v);
>>
>> would be great, but this looks like it has the possibility to turn into
>> a rats nest. If it does, then just doubling up this code for arm is ok.
>>
>> ~Andrew
>
> Yes, that might be more complicated than we expect and I don't know if
> making code such as this common would be a good idea, usually these
> functions are always architecture-specific.
I only suggested making the prototype common, not the implementation.
As such, the issue you accidentally introduced would become a hard build
failure on affected architectures, rather than a subtle build failure in
common code at some point in the future.
~Andrew
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [PATCH 02/16] x86: fix: make atomic_read() param const
2016-07-12 9:42 ` Andrew Cooper
@ 2016-07-12 10:11 ` Corneliu ZUZU
2016-07-12 10:22 ` Andrew Cooper
0 siblings, 1 reply; 54+ messages in thread
From: Corneliu ZUZU @ 2016-07-12 10:11 UTC (permalink / raw)
To: Andrew Cooper, xen-devel; +Cc: Jan Beulich
On 7/12/2016 12:42 PM, Andrew Cooper wrote:
> On 12/07/16 06:11, Corneliu ZUZU wrote:
>> Hi Andrew,
>>
>> On 7/11/2016 6:18 PM, Andrew Cooper wrote:
>>> On 09/07/16 05:12, Corneliu ZUZU wrote:
>>>> This wouldn't let me make a param of a function that used
>>>> atomic_read() const.
>>>>
>>>> Signed-off-by: Corneliu ZUZU <czuzu@bitdefender.com>
>>> This is a good improvement, but you must make an identical adjustment to
>>> the arm code, otherwise you will end up with subtle build failures.
>> Right, didn't even realize it was X86-specific.
> It isn't x86 specific. (Which is what I presume you meant to say.)
Heh, yes, depends how you look at it, I was referring precisely to (the
implementation of) the function I've modified (which was X86-specific,
called from common, which meant there's an ARM one as well).
>
>>> If you are really feeling up to it, having a common xen/atomic.h with
>>>
>>> typedef struct { int counter; } atomic_t;
>>> #define ATOMIC_INIT(i) { (i) }
>>>
>>> and some prototypes such as:
>>>
>>> static inline int atomic_read(const atomic_t *v);
>>>
>>> would be great, but this looks like it has the possibility to turn into
>>> a rats nest. If it does, then just doubling up this code for arm is ok.
>>>
>>> ~Andrew
>> Yes, that might be more complicated than we expect and I don't know if
>> making code such as this common would be a good idea, usually these
>> functions are always architecture-specific.
> I only suggested making the prototype common, not the implementation.
> As such, the issue you accidentally introduced would become a hard build
> failure on affected architectures, rather than a subtle build failure in
> common code at some point in the future.
>
> ~Andrew
>
Oh, I see, good idea, I've just tested it and it works, what did you
have in mind when you said it could cause problems?
Zuzu C.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [PATCH 02/16] x86: fix: make atomic_read() param const
2016-07-12 10:11 ` Corneliu ZUZU
@ 2016-07-12 10:22 ` Andrew Cooper
2016-07-12 10:35 ` Corneliu ZUZU
2016-07-12 10:38 ` Corneliu ZUZU
0 siblings, 2 replies; 54+ messages in thread
From: Andrew Cooper @ 2016-07-12 10:22 UTC (permalink / raw)
To: Corneliu ZUZU, xen-devel; +Cc: Jan Beulich
On 12/07/16 11:11, Corneliu ZUZU wrote:
>
>>
>>>> If you are really feeling up to it, having a common xen/atomic.h with
>>>>
>>>> typedef struct { int counter; } atomic_t;
>>>> #define ATOMIC_INIT(i) { (i) }
>>>>
>>>> and some prototypes such as:
>>>>
>>>> static inline int atomic_read(const atomic_t *v);
>>>>
>>>> would be great, but this looks like it has the possibility to turn
>>>> into
>>>> a rats nest. If it does, then just doubling up this code for arm
>>>> is ok.
>>>>
>>>> ~Andrew
>>> Yes, that might be more complicated than we expect and I don't know if
>>> making code such as this common would be a good idea, usually these
>>> functions are always architecture-specific.
>> I only suggested making the prototype common, not the implementation.
>> As such, the issue you accidentally introduced would become a hard build
>> failure on affected architectures, rather than a subtle build failure in
>> common code at some point in the future.
>>
>> ~Andrew
>>
>
> Oh, I see, good idea, I've just tested it and it works, what did you
> have in mind when you said it could cause problems?
The build issues would come at some point later when someone attempts to
atomic_read() a constant atomic_t in common code, when the ARM build
would break.
~Andrew
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [PATCH 02/16] x86: fix: make atomic_read() param const
2016-07-12 10:22 ` Andrew Cooper
@ 2016-07-12 10:35 ` Corneliu ZUZU
2016-07-12 10:38 ` Corneliu ZUZU
1 sibling, 0 replies; 54+ messages in thread
From: Corneliu ZUZU @ 2016-07-12 10:35 UTC (permalink / raw)
To: Andrew Cooper, xen-devel; +Cc: Jan Beulich
On 7/12/2016 1:22 PM, Andrew Cooper wrote:
> On 12/07/16 11:11, Corneliu ZUZU wrote:
>>>>> If you are really feeling up to it, having a common xen/atomic.h with
>>>>>
>>>>> typedef struct { int counter; } atomic_t;
>>>>> #define ATOMIC_INIT(i) { (i) }
>>>>>
>>>>> and some prototypes such as:
>>>>>
>>>>> static inline int atomic_read(const atomic_t *v);
>>>>>
>>>>> would be great, but this looks like it has the possibility to turn
>>>>> into
>>>>> a rats nest. If it does, then just doubling up this code for arm
>>>>> is ok.
>>>>>
>>>>> ~Andrew
>>>> Yes, that might be more complicated than we expect and I don't know if
>>>> making code such as this common would be a good idea, usually these
>>>> functions are always architecture-specific.
>>> I only suggested making the prototype common, not the implementation.
>>> As such, the issue you accidentally introduced would become a hard build
>>> failure on affected architectures, rather than a subtle build failure in
>>> common code at some point in the future.
>>>
>>> ~Andrew
>>>
>> Oh, I see, good idea, I've just tested it and it works, what did you
>> have in mind when you said it could cause problems?
> The build issues would come at some point later when someone attempts to
> atomic_read() a constant atomic_t in common code, when the ARM build
> would break.
>
> ~Andrew
Sorry, could you rephrase this? When the ARM build would break (i.e.
fail, I presume) because of what?
Zuzu C.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [PATCH 02/16] x86: fix: make atomic_read() param const
2016-07-12 10:22 ` Andrew Cooper
2016-07-12 10:35 ` Corneliu ZUZU
@ 2016-07-12 10:38 ` Corneliu ZUZU
2016-07-12 12:49 ` Andrew Cooper
1 sibling, 1 reply; 54+ messages in thread
From: Corneliu ZUZU @ 2016-07-12 10:38 UTC (permalink / raw)
To: Andrew Cooper, xen-devel; +Cc: Jan Beulich
On 7/12/2016 1:22 PM, Andrew Cooper wrote:
> On 12/07/16 11:11, Corneliu ZUZU wrote:
>>>>> If you are really feeling up to it, having a common xen/atomic.h with
>>>>>
>>>>> typedef struct { int counter; } atomic_t;
>>>>> #define ATOMIC_INIT(i) { (i) }
>>>>>
>>>>> and some prototypes such as:
>>>>>
>>>>> static inline int atomic_read(const atomic_t *v);
>>>>>
>>>>> would be great, but this looks like it has the possibility to turn
>>>>> into
>>>>> a rats nest. If it does, then just doubling up this code for arm
>>>>> is ok.
>>>>>
>>>>> ~Andrew
>>>> Yes, that might be more complicated than we expect and I don't know if
>>>> making code such as this common would be a good idea, usually these
>>>> functions are always architecture-specific.
>>> I only suggested making the prototype common, not the implementation.
>>> As such, the issue you accidentally introduced would become a hard build
>>> failure on affected architectures, rather than a subtle build failure in
>>> common code at some point in the future.
>>>
>>> ~Andrew
>>>
>> Oh, I see, good idea, I've just tested it and it works, what did you
>> have in mind when you said it could cause problems?
> The build issues would come at some point later when someone attempts to
> atomic_read() a constant atomic_t in common code, when the ARM build
> would break.
>
> ~Andrew
Ooh, no, I was asking what you meant when you said "this looks like it
has the possibility to turn into a rats nest" in your first message, not
the thing about hard build failure..
Zuzu C.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [PATCH 02/16] x86: fix: make atomic_read() param const
2016-07-12 10:38 ` Corneliu ZUZU
@ 2016-07-12 12:49 ` Andrew Cooper
2016-07-12 13:45 ` Corneliu ZUZU
0 siblings, 1 reply; 54+ messages in thread
From: Andrew Cooper @ 2016-07-12 12:49 UTC (permalink / raw)
To: Corneliu ZUZU, xen-devel; +Cc: Jan Beulich
On 12/07/16 11:38, Corneliu ZUZU wrote:
> On 7/12/2016 1:22 PM, Andrew Cooper wrote:
>> On 12/07/16 11:11, Corneliu ZUZU wrote:
>>>>>> If you are really feeling up to it, having a common xen/atomic.h
>>>>>> with
>>>>>>
>>>>>> typedef struct { int counter; } atomic_t;
>>>>>> #define ATOMIC_INIT(i) { (i) }
>>>>>>
>>>>>> and some prototypes such as:
>>>>>>
>>>>>> static inline int atomic_read(const atomic_t *v);
>>>>>>
>>>>>> would be great, but this looks like it has the possibility to turn
>>>>>> into
>>>>>> a rats nest. If it does, then just doubling up this code for arm
>>>>>> is ok.
>>>>>>
>>>>>> ~Andrew
>>>>> Yes, that might be more complicated than we expect and I don't
>>>>> know if
>>>>> making code such as this common would be a good idea, usually these
>>>>> functions are always architecture-specific.
>>>> I only suggested making the prototype common, not the implementation.
>>>> As such, the issue you accidentally introduced would become a hard
>>>> build
>>>> failure on affected architectures, rather than a subtle build
>>>> failure in
>>>> common code at some point in the future.
>>>>
>>>> ~Andrew
>>>>
>>> Oh, I see, good idea, I've just tested it and it works, what did you
>>> have in mind when you said it could cause problems?
>> The build issues would come at some point later when someone attempts to
>> atomic_read() a constant atomic_t in common code, when the ARM build
>> would break.
>>
>> ~Andrew
>
> Ooh, no, I was asking what you meant when you said "this looks like it
> has the possibility to turn into a rats nest" in your first message,
> not the thing about hard build failure..
Ah. sorry.
You would have to invert all the includes of atomic.h to include
<xen/atomic.h> rather than <asm/atomic.h>, and have xen/atomic.h include
asm/atomic.h towards the end, such that the common prototypes are
first. I just suspect that this might not be completely trivial to
untangle (of course, I could also be wrong).
~Andrew
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [PATCH 02/16] x86: fix: make atomic_read() param const
2016-07-12 12:49 ` Andrew Cooper
@ 2016-07-12 13:45 ` Corneliu ZUZU
0 siblings, 0 replies; 54+ messages in thread
From: Corneliu ZUZU @ 2016-07-12 13:45 UTC (permalink / raw)
To: Andrew Cooper, xen-devel; +Cc: Jan Beulich
On 7/12/2016 3:49 PM, Andrew Cooper wrote:
> On 12/07/16 11:38, Corneliu ZUZU wrote:
>> On 7/12/2016 1:22 PM, Andrew Cooper wrote:
>>> On 12/07/16 11:11, Corneliu ZUZU wrote:
>>>>>>> If you are really feeling up to it, having a common xen/atomic.h
>>>>>>> with
>>>>>>>
>>>>>>> typedef struct { int counter; } atomic_t;
>>>>>>> #define ATOMIC_INIT(i) { (i) }
>>>>>>>
>>>>>>> and some prototypes such as:
>>>>>>>
>>>>>>> static inline int atomic_read(const atomic_t *v);
>>>>>>>
>>>>>>> would be great, but this looks like it has the possibility to turn
>>>>>>> into
>>>>>>> a rats nest. If it does, then just doubling up this code for arm
>>>>>>> is ok.
>>>>>>>
>>>>>>> ~Andrew
>>>>>> Yes, that might be more complicated than we expect and I don't
>>>>>> know if
>>>>>> making code such as this common would be a good idea, usually these
>>>>>> functions are always architecture-specific.
>>>>> I only suggested making the prototype common, not the implementation.
>>>>> As such, the issue you accidentally introduced would become a hard
>>>>> build
>>>>> failure on affected architectures, rather than a subtle build
>>>>> failure in
>>>>> common code at some point in the future.
>>>>>
>>>>> ~Andrew
>>>>>
>>>> Oh, I see, good idea, I've just tested it and it works, what did you
>>>> have in mind when you said it could cause problems?
>>> The build issues would come at some point later when someone attempts to
>>> atomic_read() a constant atomic_t in common code, when the ARM build
>>> would break.
>>>
>>> ~Andrew
>> Ooh, no, I was asking what you meant when you said "this looks like it
>> has the possibility to turn into a rats nest" in your first message,
>> not the thing about hard build failure..
> Ah. sorry.
>
> You would have to invert all the includes of atomic.h to include
> <xen/atomic.h> rather than <asm/atomic.h>, and have xen/atomic.h include
> asm/atomic.h towards the end, such that the common prototypes are
> first. I just suspect that this might not be completely trivial to
> untangle (of course, I could also be wrong).
>
> ~Andrew
I did it the other way - included <xen/monitor.h> from <asm/monitor.h>.
Zuzu C.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [PATCH 14/16] x86/monitor: clarify separation between monitor subsys and vm-event as a whole
2016-07-09 18:57 ` Corneliu ZUZU
@ 2016-07-13 4:26 ` Corneliu ZUZU
2016-07-13 18:56 ` Tamas K Lengyel
0 siblings, 1 reply; 54+ messages in thread
From: Corneliu ZUZU @ 2016-07-13 4:26 UTC (permalink / raw)
To: Tamas K Lengyel
Cc: Razvan Cojocaru, George Dunlap, Andrew Cooper, Xen-devel,
Paul Durrant, Jan Beulich
On 7/9/2016 9:57 PM, Corneliu ZUZU wrote:
> On 7/9/2016 9:26 PM, Tamas K Lengyel wrote:
>>> diff --git a/xen/include/asm-x86/domain.h
>>> b/xen/include/asm-x86/domain.h
>>> index ae1dcb4..7663da2 100644
>>> --- a/xen/include/asm-x86/domain.h
>>> +++ b/xen/include/asm-x86/domain.h
>>> @@ -9,6 +9,7 @@
>>> #include <asm/e820.h>
>>> #include <asm/mce.h>
>>> #include <public/vcpu.h>
>>> +#include <public/vm_event.h>
>>> #include <public/hvm/hvm_info_table.h>
>>>
>>> #define has_32bit_shinfo(d) ((d)->arch.has_32bit_shinfo)
>>> @@ -503,6 +504,20 @@ 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 arch_vm_event_monitor {
>> This should be named struct arch_vcpu_monitor.
>
> Good idea.
>
>>
>>> + uint32_t emulate_flags;
>>> + struct vm_event_emul_read_data emul_read_data;
>> This should probably get renamed as well at some point to struct
>> monitor_emul_read_data.
>
> Ack.
>
>>> + struct monitor_write_data write_data;
>>> +};
>>> +
>>> +struct arch_vm_event {
>>> + struct arch_vm_event_monitor *monitor;
>>> +};
>> IMHO there is not much point in defining struct arch_vm_event this
>> way, we could just as well store the pointer to the arch_monitor
>> directly in arch_vcpu as we do right now.
>>
>
> I stated the reason for that in the commit message (see 3rd '*'), Jan
> insists it would be preferable to occupy just one pointer in arch_vcpu
> (which would still hold if we do as you suggest but I was wondering
> how probable would it be in the future for one of the paging/share
> vm-event subsystems to need per-vCPU resources). But personally, yeah,
> I would have preferred what you suggest as well..
>
> Zuzu.
Tamas, any update on this? Do you think we should think now about
per-vCPU resources being occupied in the future by paging/share or leave
that for later and do what you suggested?
Thanks,
Corneliu.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [PATCH 14/16] x86/monitor: clarify separation between monitor subsys and vm-event as a whole
2016-07-13 4:26 ` Corneliu ZUZU
@ 2016-07-13 18:56 ` Tamas K Lengyel
0 siblings, 0 replies; 54+ messages in thread
From: Tamas K Lengyel @ 2016-07-13 18:56 UTC (permalink / raw)
To: Corneliu ZUZU
Cc: Razvan Cojocaru, George Dunlap, Andrew Cooper, Xen-devel,
Paul Durrant, Jan Beulich
On Tue, Jul 12, 2016 at 10:26 PM, Corneliu ZUZU <czuzu@bitdefender.com> wrote:
> On 7/9/2016 9:57 PM, Corneliu ZUZU wrote:
>>
>> On 7/9/2016 9:26 PM, Tamas K Lengyel wrote:
>>>>
>>>> diff --git a/xen/include/asm-x86/domain.h b/xen/include/asm-x86/domain.h
>>>> index ae1dcb4..7663da2 100644
>>>> --- a/xen/include/asm-x86/domain.h
>>>> +++ b/xen/include/asm-x86/domain.h
>>>> @@ -9,6 +9,7 @@
>>>> #include <asm/e820.h>
>>>> #include <asm/mce.h>
>>>> #include <public/vcpu.h>
>>>> +#include <public/vm_event.h>
>>>> #include <public/hvm/hvm_info_table.h>
>>>>
>>>> #define has_32bit_shinfo(d) ((d)->arch.has_32bit_shinfo)
>>>> @@ -503,6 +504,20 @@ 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 arch_vm_event_monitor {
>>>
>>> This should be named struct arch_vcpu_monitor.
>>
>>
>> Good idea.
>>
>>>
>>>> + uint32_t emulate_flags;
>>>> + struct vm_event_emul_read_data emul_read_data;
>>>
>>> This should probably get renamed as well at some point to struct
>>> monitor_emul_read_data.
>>
>>
>> Ack.
>>
>>>> + struct monitor_write_data write_data;
>>>> +};
>>>> +
>>>> +struct arch_vm_event {
>>>> + struct arch_vm_event_monitor *monitor;
>>>> +};
>>>
>>> IMHO there is not much point in defining struct arch_vm_event this
>>> way, we could just as well store the pointer to the arch_monitor
>>> directly in arch_vcpu as we do right now.
>>>
>>
>> I stated the reason for that in the commit message (see 3rd '*'), Jan
>> insists it would be preferable to occupy just one pointer in arch_vcpu
>> (which would still hold if we do as you suggest but I was wondering how
>> probable would it be in the future for one of the paging/share vm-event
>> subsystems to need per-vCPU resources). But personally, yeah, I would have
>> preferred what you suggest as well..
>>
>> Zuzu.
>
>
> Tamas, any update on this? Do you think we should think now about per-vCPU
> resources being occupied in the future by paging/share or leave that for
> later and do what you suggested?
It's highly unlikely that either paging or sharing will have any use
of this struct in the near future provided but subsystems are pretty
much orphaned at this point. If we ever going to need it it's an easy
enough adjustment but for now it's not needed.
Tamas
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [PATCH 03/16] x86/monitor: mechanical renames
2016-07-12 6:10 ` Corneliu ZUZU
@ 2016-07-15 7:18 ` Corneliu ZUZU
2016-07-18 18:07 ` Andrew Cooper
0 siblings, 1 reply; 54+ messages in thread
From: Corneliu ZUZU @ 2016-07-15 7:18 UTC (permalink / raw)
To: Jan Beulich, Andrew Cooper, Julien Grall, Stefano Stabellini
Cc: Tamas K Lengyel, Razvan Cojocaru, Xen-devel
On 7/12/2016 9:10 AM, Corneliu ZUZU wrote:
> On 7/11/2016 7:43 PM, Tamas K Lengyel wrote:
>> On Sat, Jul 9, 2016 at 12:46 PM, Corneliu ZUZU
>> <czuzu@bitdefender.com> wrote:
>>> On 7/9/2016 9:10 PM, Tamas K Lengyel wrote:
>>>> On Fri, Jul 8, 2016 at 10:13 PM, Corneliu ZUZU <czuzu@bitdefender.com>
>>>> wrote:
>>>>> Arch-specific vm-event functions in x86/vm_event.h -e.g.
>>>>> vm_event_init_domain()-
>>>>> don't have an 'arch_' prefix. Apply the same rule for monitor
>>>>> functions -
>>>>> originally the only two monitor functions that had an 'arch_'
>>>>> prefix were
>>>>> arch_monitor_domctl_event() and arch_monitor_domctl_op(), but I
>>>>> gave them
>>>>> that
>>>>> prefix because -they had a counterpart function in common code-, that
>>>>> being
>>>>> monitor_domctl().
>>>> This should actually be the other way around - ie adding the arch_
>>>> prefix to vm_event functions that lack it.
>>>
>>> Given that the majority of the arch-specific functions called from
>>> common-code don't have an 'arch_' prefix unless they have a common
>>> counterpart, I was guessing that was the rule. It made sense in my head
>>> since I saw in that the intention of avoiding naming conflicts (i.e you
>>> can't have monitor_domctl() both on the common-side and on the
>>> arch-side, so
>>> prepend 'arch_' to the latter). I noticed you guys also 'skip' the
>>> prefix
>>> when sending patches, so that reinforced my assumption.
>>>
>>>> Having the arch_ prefix is
>>>> helpful to know that the function is dealing with the arch specific
>>>> structs and not common.
>>>
>>> Personally I don't see much use in 'knowing that the function is
>>> dealing
>>> with the arch structs' from the call-site and you can tell that from
>>> the
>>> implementation-site just by looking at the path of its source file.
>>> Also,
>>> the code is pretty much localized in the arch directory anyway so
>>> usually
>>> one wouldn't have to go back and forth between common and arch that
>>> often.
>>> What really bothers me though is always having to read 'arch_' when
>>> spelling
>>> a function-name and also that it makes the function name longer
>>> without much
>>> benefit. Your suggestion of adding it to pretty much all functions
>>> that make
>>> up the interface to common just adds to that headache. :-D
>>>
>>>> Similarly that's why we have the hvm_ prefix
>>>> for functions in hvm/monitor.
>>>
>>> 'hvm_' doesn't seem to me more special than 'monitor_', for
>>> instance, but
>>> maybe that's just me.
>>>
>>>>> Let this also be the rule for future 'arch_' functions additions, and
>>>>> with this
>>>>> patch remove the 'arch_' prefix from the monitor functions that don't
>>>>> have a
>>>>> counterpart in common-code (all but those 2 aforementioned).
>>>> Even if there are no common counter-parts to the function, the arch_
>>>> prefix should remain, so I won't be able to ack this patch.
>>>>
>>>> Tamas
>>>
>>> Having said the above, are you still of the same opinion?
>> Yes, I am. While it's not a hard rule to always apply these prefix, it
>> does make sense to have them so I don't see benefit in removing the
>> existing prefixes.
>
> Well, for one the benefit would be not confusing developers by
> creating inconsistencies: what's the rule here, i.e. why isn't a
> function such as alloc_domain_struct prefixed w/ 'arch_' but
> arch_domain_create is? The reason seems to be the latter having a
> common counterpart while the former not, at least that's what I see
> being done all over the code-base. Also, I've done this before and you
> seemed to agree:
> https://www.mail-archive.com/xen-devel%40lists.xen.org/msg57617.html
> (Q1). You also suggested creating arch-specific functions without the
> prefix:
> https://www.mail-archive.com/xen-devel%40lists.xen.org/msg57336.html .
> Why the sudden change of heart?
>
> 2ndly and obviously, removing the prefixes would make function names
> shorter and clearer (e.g. -read- "arch_vm_event_vcpu_unpause" and then
> read "vm_event_vcpu_unpause").
>
> 3rd reason is that adding the prefix to -all- arch-specific functions
> called from common would mean having a lot new functions with the
> prefix. I'd read the prefix over and over again and at some point I'd
> get annoyed and say "ok, ok, it's arch_, I get it; why use this prefix
> so much again?".
>
> 4th reason is that the advantage of telling that the function accesses
> arch structures is much too little considering that idk, >50% of the
> codebase is arch-specific, so it doesn't provide much information,
> this categorization is too broad to deserve a special prefix. Whereas
> using the prefix only for functions that do have a common counterpart
> gives you the extra information that the 'operation' is only -partly-
> arch-specific, i.e. to see the whole picture, look @ the common-side
> implementation. Keep in mind that we'd also be -losing that
> information- if we were to apply the 'go with arch_ for all' rule..
> (this could be a 5th reason)
>
>> Adding arch_ prefix to the ones that don't already
>> have one is optional, I was just pointing out that if you really feel
>> like standardizing the naming convention, that's where I would like
>> things to move towards to.
>>
>> Tamas
>
> I don't think I'd say this patch "standardizes the naming convention"
> but rather "fixes code that doesn't conform to the -already existing-
> standard naming convention". Above stated reasons explain why I'd
> rather -not- have this standard go in the direction of adding 'arch_'
> before a lot of functions.
>
> Finally, I feel like asking for feedback as I don't like to insist
> when the majority agrees to disagree. Jan & others, what's the rule
> here and what's your view on this? :-)
>
> Thanks,
> Zuzu C.
Can I get some extra feedback on this (Andrew, Stefano, Julien)? :-)
(btw, is Jan on vacation?)
Thanks,
Corneliu.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [PATCH 05/16] x86/monitor: relocate code more appropriately
2016-07-12 8:07 ` Corneliu ZUZU
@ 2016-07-15 11:41 ` Corneliu ZUZU
0 siblings, 0 replies; 54+ messages in thread
From: Corneliu ZUZU @ 2016-07-15 11:41 UTC (permalink / raw)
To: Tian, Kevin, xen-devel
Cc: Andrew Cooper, Jan Beulich, Tamas K Lengyel, Razvan Cojocaru,
Nakajima, Jun
On 7/12/2016 11:07 AM, Corneliu ZUZU wrote:
> On 7/12/2016 10:45 AM, Tian, Kevin wrote:
>>> From: Corneliu ZUZU [mailto:czuzu@bitdefender.com]
>>> Sent: Monday, July 11, 2016 2:19 PM
>>>> +static inline
>>>> +void monitor_ctrlreg_adjust_traps(struct domain *d, unsigned int
>>>> index)
>>>> +{
>>>> + /* For now, VMX only. */
>>>> + ASSERT(cpu_has_vmx);
>>>> +
>>>> + /* Other CRs than CR3 are always trapped. */
>>>> + if ( VM_EVENT_X86_CR3 == index )
>>>> + vmx_vm_event_update_cr3_traps(d);
>>> [Kevin wrote]:
>>>
>>> Please add above into a hvm function instead of directly calling
>>> vmx in common file. (other arch can have it unimplemented).
>>> Then you don't need change this common code again later when
>>> other archs are added
>>>
>>> ---
>>>
>>>
>>> This is not common code, it's in arch/x86/monitor.c (?) and currently,
>>> as the above ASSERT indicates, only VMX is supported. If in the future
>>> support for SVM for example will be added, then the hvm move you
>>> suggest
>>> must be done (Jan also suggested this).
>>> Or, I only now realize, if you guys prefer doing this now I could add a
>>> vm_event_update_cr3_traps field in hvm_function_table, but BUG() in the
>>> SVM one..
>>>
>> The latter is desired. Instead of BUG, it makes more sense to return
>> error on an arch which doesn't register the callback.
>>
>> Thanks
>> Kevin
>
> Will do.
>
> Thanks,
> Zuzu C.
I've decided to discard separating CR3 load-exiting handling (i.e.
discard vmx_vm_event_update_cr3_traps) entirely since I find that it's
complicated to have to handle the bit from 2 different places
(vmx_update_guest_cr and arch_monitor_domctl_event).
Normally such a situation is resolved by counting the number of
subscribers to the resource (in this case counting the number of
'entities' that want to CR3 load-exiting enabled - i.e. just as we have
a vCPU pause_count to count entities that want the vCPU to be paused),
but it's just a single bit of a lot more and I don't think the overhead
is worth.
Let me know if you disagree and I'm open to suggestions, if you guys
have any.
Thanks,
Corneliu.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [PATCH 03/16] x86/monitor: mechanical renames
2016-07-15 7:18 ` Corneliu ZUZU
@ 2016-07-18 18:07 ` Andrew Cooper
2016-07-19 9:36 ` Corneliu ZUZU
0 siblings, 1 reply; 54+ messages in thread
From: Andrew Cooper @ 2016-07-18 18:07 UTC (permalink / raw)
To: Corneliu ZUZU, Jan Beulich, Julien Grall, Stefano Stabellini
Cc: Tamas K Lengyel, Razvan Cojocaru, Xen-devel
On 15/07/16 08:18, Corneliu ZUZU wrote:
> On 7/12/2016 9:10 AM, Corneliu ZUZU wrote:
>> On 7/11/2016 7:43 PM, Tamas K Lengyel wrote:
>>> On Sat, Jul 9, 2016 at 12:46 PM, Corneliu ZUZU
>>> <czuzu@bitdefender.com> wrote:
>>>> On 7/9/2016 9:10 PM, Tamas K Lengyel wrote:
>>>>> On Fri, Jul 8, 2016 at 10:13 PM, Corneliu ZUZU
>>>>> <czuzu@bitdefender.com>
>>>>> wrote:
>>>>>> Arch-specific vm-event functions in x86/vm_event.h -e.g.
>>>>>> vm_event_init_domain()-
>>>>>> don't have an 'arch_' prefix. Apply the same rule for monitor
>>>>>> functions -
>>>>>> originally the only two monitor functions that had an 'arch_'
>>>>>> prefix were
>>>>>> arch_monitor_domctl_event() and arch_monitor_domctl_op(), but I
>>>>>> gave them
>>>>>> that
>>>>>> prefix because -they had a counterpart function in common code-,
>>>>>> that
>>>>>> being
>>>>>> monitor_domctl().
>>>>> This should actually be the other way around - ie adding the arch_
>>>>> prefix to vm_event functions that lack it.
>>>>
>>>> Given that the majority of the arch-specific functions called from
>>>> common-code don't have an 'arch_' prefix unless they have a common
>>>> counterpart, I was guessing that was the rule. It made sense in my
>>>> head
>>>> since I saw in that the intention of avoiding naming conflicts (i.e
>>>> you
>>>> can't have monitor_domctl() both on the common-side and on the
>>>> arch-side, so
>>>> prepend 'arch_' to the latter). I noticed you guys also 'skip' the
>>>> prefix
>>>> when sending patches, so that reinforced my assumption.
>>>>
>>>>> Having the arch_ prefix is
>>>>> helpful to know that the function is dealing with the arch specific
>>>>> structs and not common.
>>>>
>>>> Personally I don't see much use in 'knowing that the function is
>>>> dealing
>>>> with the arch structs' from the call-site and you can tell that
>>>> from the
>>>> implementation-site just by looking at the path of its source file.
>>>> Also,
>>>> the code is pretty much localized in the arch directory anyway so
>>>> usually
>>>> one wouldn't have to go back and forth between common and arch that
>>>> often.
>>>> What really bothers me though is always having to read 'arch_' when
>>>> spelling
>>>> a function-name and also that it makes the function name longer
>>>> without much
>>>> benefit. Your suggestion of adding it to pretty much all functions
>>>> that make
>>>> up the interface to common just adds to that headache. :-D
>>>>
>>>>> Similarly that's why we have the hvm_ prefix
>>>>> for functions in hvm/monitor.
>>>>
>>>> 'hvm_' doesn't seem to me more special than 'monitor_', for
>>>> instance, but
>>>> maybe that's just me.
>>>>
>>>>>> Let this also be the rule for future 'arch_' functions additions,
>>>>>> and
>>>>>> with this
>>>>>> patch remove the 'arch_' prefix from the monitor functions that
>>>>>> don't
>>>>>> have a
>>>>>> counterpart in common-code (all but those 2 aforementioned).
>>>>> Even if there are no common counter-parts to the function, the arch_
>>>>> prefix should remain, so I won't be able to ack this patch.
>>>>>
>>>>> Tamas
>>>>
>>>> Having said the above, are you still of the same opinion?
>>> Yes, I am. While it's not a hard rule to always apply these prefix, it
>>> does make sense to have them so I don't see benefit in removing the
>>> existing prefixes.
>>
>> Well, for one the benefit would be not confusing developers by
>> creating inconsistencies: what's the rule here, i.e. why isn't a
>> function such as alloc_domain_struct prefixed w/ 'arch_' but
>> arch_domain_create is? The reason seems to be the latter having a
>> common counterpart while the former not, at least that's what I see
>> being done all over the code-base. Also, I've done this before and
>> you seemed to agree:
>> https://www.mail-archive.com/xen-devel%40lists.xen.org/msg57617.html
>> (Q1). You also suggested creating arch-specific functions without the
>> prefix:
>> https://www.mail-archive.com/xen-devel%40lists.xen.org/msg57336.html
>> . Why the sudden change of heart?
>>
>> 2ndly and obviously, removing the prefixes would make function names
>> shorter and clearer (e.g. -read- "arch_vm_event_vcpu_unpause" and
>> then read "vm_event_vcpu_unpause").
>>
>> 3rd reason is that adding the prefix to -all- arch-specific functions
>> called from common would mean having a lot new functions with the
>> prefix. I'd read the prefix over and over again and at some point I'd
>> get annoyed and say "ok, ok, it's arch_, I get it; why use this
>> prefix so much again?".
>>
>> 4th reason is that the advantage of telling that the function
>> accesses arch structures is much too little considering that idk,
>> >50% of the codebase is arch-specific, so it doesn't provide much
>> information, this categorization is too broad to deserve a special
>> prefix. Whereas using the prefix only for functions that do have a
>> common counterpart gives you the extra information that the
>> 'operation' is only -partly- arch-specific, i.e. to see the whole
>> picture, look @ the common-side implementation. Keep in mind that
>> we'd also be -losing that information- if we were to apply the 'go
>> with arch_ for all' rule.. (this could be a 5th reason)
>>
>>> Adding arch_ prefix to the ones that don't already
>>> have one is optional, I was just pointing out that if you really feel
>>> like standardizing the naming convention, that's where I would like
>>> things to move towards to.
>>>
>>> Tamas
>>
>> I don't think I'd say this patch "standardizes the naming convention"
>> but rather "fixes code that doesn't conform to the -already existing-
>> standard naming convention". Above stated reasons explain why I'd
>> rather -not- have this standard go in the direction of adding 'arch_'
>> before a lot of functions.
>>
>> Finally, I feel like asking for feedback as I don't like to insist
>> when the majority agrees to disagree. Jan & others, what's the rule
>> here and what's your view on this? :-)
>>
>> Thanks,
>> Zuzu C.
>
> Can I get some extra feedback on this (Andrew, Stefano, Julien)?
Apologies for the delay.
There is sadly no hard and fast rule. Generally an arch_$FOO() exists
when there is also a common $FOO() which does some setup, then passes to
arch_$FOO() to do some more architecture specific setup.
One thing that we are currently bad at is having proper distinctions.
Common code should never ever poke inside a .arch struct/union, and we
should have architecture specific functions to make that happen.
However, I don't think it is wise to insist on an arch_ prefix for every
per-arch helper.
~Andrew
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [PATCH 03/16] x86/monitor: mechanical renames
2016-07-18 18:07 ` Andrew Cooper
@ 2016-07-19 9:36 ` Corneliu ZUZU
0 siblings, 0 replies; 54+ messages in thread
From: Corneliu ZUZU @ 2016-07-19 9:36 UTC (permalink / raw)
To: Andrew Cooper, Jan Beulich, Julien Grall, Stefano Stabellini
Cc: Tamas K Lengyel, Razvan Cojocaru, Xen-devel
On 7/18/2016 9:07 PM, Andrew Cooper wrote:
> On 15/07/16 08:18, Corneliu ZUZU wrote:
>> On 7/12/2016 9:10 AM, Corneliu ZUZU wrote:
>>> On 7/11/2016 7:43 PM, Tamas K Lengyel wrote:
>>>> On Sat, Jul 9, 2016 at 12:46 PM, Corneliu ZUZU
>>>> <czuzu@bitdefender.com> wrote:
>>>>> On 7/9/2016 9:10 PM, Tamas K Lengyel wrote:
>>>>>> On Fri, Jul 8, 2016 at 10:13 PM, Corneliu ZUZU
>>>>>> <czuzu@bitdefender.com>
>>>>>> wrote:
>>>>>>> Arch-specific vm-event functions in x86/vm_event.h -e.g.
>>>>>>> vm_event_init_domain()-
>>>>>>> don't have an 'arch_' prefix. Apply the same rule for monitor
>>>>>>> functions -
>>>>>>> originally the only two monitor functions that had an 'arch_'
>>>>>>> prefix were
>>>>>>> arch_monitor_domctl_event() and arch_monitor_domctl_op(), but I
>>>>>>> gave them
>>>>>>> that
>>>>>>> prefix because -they had a counterpart function in common code-,
>>>>>>> that
>>>>>>> being
>>>>>>> monitor_domctl().
>>>>>> This should actually be the other way around - ie adding the arch_
>>>>>> prefix to vm_event functions that lack it.
>>>>> Given that the majority of the arch-specific functions called from
>>>>> common-code don't have an 'arch_' prefix unless they have a common
>>>>> counterpart, I was guessing that was the rule. It made sense in my
>>>>> head
>>>>> since I saw in that the intention of avoiding naming conflicts (i.e
>>>>> you
>>>>> can't have monitor_domctl() both on the common-side and on the
>>>>> arch-side, so
>>>>> prepend 'arch_' to the latter). I noticed you guys also 'skip' the
>>>>> prefix
>>>>> when sending patches, so that reinforced my assumption.
>>>>>
>>>>>> Having the arch_ prefix is
>>>>>> helpful to know that the function is dealing with the arch specific
>>>>>> structs and not common.
>>>>> Personally I don't see much use in 'knowing that the function is
>>>>> dealing
>>>>> with the arch structs' from the call-site and you can tell that
>>>>> from the
>>>>> implementation-site just by looking at the path of its source file.
>>>>> Also,
>>>>> the code is pretty much localized in the arch directory anyway so
>>>>> usually
>>>>> one wouldn't have to go back and forth between common and arch that
>>>>> often.
>>>>> What really bothers me though is always having to read 'arch_' when
>>>>> spelling
>>>>> a function-name and also that it makes the function name longer
>>>>> without much
>>>>> benefit. Your suggestion of adding it to pretty much all functions
>>>>> that make
>>>>> up the interface to common just adds to that headache. :-D
>>>>>
>>>>>> Similarly that's why we have the hvm_ prefix
>>>>>> for functions in hvm/monitor.
>>>>> 'hvm_' doesn't seem to me more special than 'monitor_', for
>>>>> instance, but
>>>>> maybe that's just me.
>>>>>
>>>>>>> Let this also be the rule for future 'arch_' functions additions,
>>>>>>> and
>>>>>>> with this
>>>>>>> patch remove the 'arch_' prefix from the monitor functions that
>>>>>>> don't
>>>>>>> have a
>>>>>>> counterpart in common-code (all but those 2 aforementioned).
>>>>>> Even if there are no common counter-parts to the function, the arch_
>>>>>> prefix should remain, so I won't be able to ack this patch.
>>>>>>
>>>>>> Tamas
>>>>> Having said the above, are you still of the same opinion?
>>>> Yes, I am. While it's not a hard rule to always apply these prefix, it
>>>> does make sense to have them so I don't see benefit in removing the
>>>> existing prefixes.
>>> Well, for one the benefit would be not confusing developers by
>>> creating inconsistencies: what's the rule here, i.e. why isn't a
>>> function such as alloc_domain_struct prefixed w/ 'arch_' but
>>> arch_domain_create is? The reason seems to be the latter having a
>>> common counterpart while the former not, at least that's what I see
>>> being done all over the code-base. Also, I've done this before and
>>> you seemed to agree:
>>> https://www.mail-archive.com/xen-devel%40lists.xen.org/msg57617.html
>>> (Q1). You also suggested creating arch-specific functions without the
>>> prefix:
>>> https://www.mail-archive.com/xen-devel%40lists.xen.org/msg57336.html
>>> . Why the sudden change of heart?
>>>
>>> 2ndly and obviously, removing the prefixes would make function names
>>> shorter and clearer (e.g. -read- "arch_vm_event_vcpu_unpause" and
>>> then read "vm_event_vcpu_unpause").
>>>
>>> 3rd reason is that adding the prefix to -all- arch-specific functions
>>> called from common would mean having a lot new functions with the
>>> prefix. I'd read the prefix over and over again and at some point I'd
>>> get annoyed and say "ok, ok, it's arch_, I get it; why use this
>>> prefix so much again?".
>>>
>>> 4th reason is that the advantage of telling that the function
>>> accesses arch structures is much too little considering that idk,
>>>> 50% of the codebase is arch-specific, so it doesn't provide much
>>> information, this categorization is too broad to deserve a special
>>> prefix. Whereas using the prefix only for functions that do have a
>>> common counterpart gives you the extra information that the
>>> 'operation' is only -partly- arch-specific, i.e. to see the whole
>>> picture, look @ the common-side implementation. Keep in mind that
>>> we'd also be -losing that information- if we were to apply the 'go
>>> with arch_ for all' rule.. (this could be a 5th reason)
>>>
>>>> Adding arch_ prefix to the ones that don't already
>>>> have one is optional, I was just pointing out that if you really feel
>>>> like standardizing the naming convention, that's where I would like
>>>> things to move towards to.
>>>>
>>>> Tamas
>>> I don't think I'd say this patch "standardizes the naming convention"
>>> but rather "fixes code that doesn't conform to the -already existing-
>>> standard naming convention". Above stated reasons explain why I'd
>>> rather -not- have this standard go in the direction of adding 'arch_'
>>> before a lot of functions.
>>>
>>> Finally, I feel like asking for feedback as I don't like to insist
>>> when the majority agrees to disagree. Jan & others, what's the rule
>>> here and what's your view on this? :-)
>>>
>>> Thanks,
>>> Zuzu C.
>> Can I get some extra feedback on this (Andrew, Stefano, Julien)?
> Apologies for the delay.
>
> There is sadly no hard and fast rule. Generally an arch_$FOO() exists
> when there is also a common $FOO() which does some setup, then passes to
> arch_$FOO() to do some more architecture specific setup.
Which is then indeed the "unspoken" rule that seemed to be respected
throughout the code-base.
> One thing that we are currently bad at is having proper distinctions.
Which is why, to also avoid having to go through such back-and-forth
discussions, I propose adding this rule in CODING_STYLE. There are, of
course, other deviations from it, but doing this would make future
deviations less likely and would encourage correcting existing ones.
> Common code should never ever poke inside a .arch struct/union, and we
> should have architecture specific functions to make that happen.
>
> However, I don't think it is wise to insist on an arch_ prefix for every
> per-arch helper.
I agree.
> ~Andrew
Thanks for the feedback!
Corneliu.
---
This email has been checked for viruses by Avast antivirus software.
https://www.avast.com/antivirus
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 54+ messages in thread
end of thread, other threads:[~2016-07-19 9:36 UTC | newest]
Thread overview: 54+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-07-09 4:11 [PATCH 00/16] x86/vm-event: numerous adjustments & fixes Corneliu ZUZU
2016-07-09 4:12 ` [PATCH 01/16] x86/vmx_update_guest_cr: minor optimization Corneliu ZUZU
2016-07-11 6:24 ` Corneliu ZUZU
2016-07-09 4:12 ` [PATCH 02/16] x86: fix: make atomic_read() param const Corneliu ZUZU
2016-07-11 15:18 ` Andrew Cooper
2016-07-12 5:11 ` Corneliu ZUZU
2016-07-12 9:42 ` Andrew Cooper
2016-07-12 10:11 ` Corneliu ZUZU
2016-07-12 10:22 ` Andrew Cooper
2016-07-12 10:35 ` Corneliu ZUZU
2016-07-12 10:38 ` Corneliu ZUZU
2016-07-12 12:49 ` Andrew Cooper
2016-07-12 13:45 ` Corneliu ZUZU
2016-07-09 4:13 ` [PATCH 03/16] x86/monitor: mechanical renames Corneliu ZUZU
2016-07-09 18:10 ` Tamas K Lengyel
2016-07-09 18:46 ` Corneliu ZUZU
2016-07-11 16:43 ` Tamas K Lengyel
2016-07-12 6:10 ` Corneliu ZUZU
2016-07-15 7:18 ` Corneliu ZUZU
2016-07-18 18:07 ` Andrew Cooper
2016-07-19 9:36 ` Corneliu ZUZU
2016-07-09 4:14 ` [PATCH 04/16] x86/monitor: relocate vm_event_register_write_resume() function to monitor code Corneliu ZUZU
2016-07-09 18:14 ` Tamas K Lengyel
2016-07-09 18:47 ` Corneliu ZUZU
2016-07-09 4:15 ` [PATCH 05/16] x86/monitor: relocate code more appropriately Corneliu ZUZU
2016-07-11 6:19 ` Corneliu ZUZU
2016-07-12 7:45 ` Tian, Kevin
2016-07-12 8:07 ` Corneliu ZUZU
2016-07-15 11:41 ` Corneliu ZUZU
2016-07-09 4:15 ` [PATCH 06/16] x86/monitor: fix: set msr_bitmap to NULL after xfree Corneliu ZUZU
2016-07-09 4:16 ` [PATCH 07/16] x86/vm-event: fix: call cleanup when init fails, to free partial allocs Corneliu ZUZU
2016-07-09 4:17 ` [PATCH 08/16] x86/vm-event: call monitor init & cleanup funcs from respective vm_event funcs Corneliu ZUZU
2016-07-09 4:18 ` [PATCH 09/16] arm/monitor: move d->monitor cleanup to monitor_cleanup_domain() Corneliu ZUZU
2016-07-09 4:19 ` [PATCH 10/16] x86/vm-event: centralize vcpu-destroy cleanup in vm-events code Corneliu ZUZU
2016-07-09 4:20 ` [PATCH 11/16] x86/monitor: fix: treat -monitor- properly, as a subsys of the vm-event subsys Corneliu ZUZU
2016-07-09 17:34 ` Tamas K Lengyel
2016-07-09 17:46 ` Corneliu ZUZU
2016-07-11 16:38 ` Tamas K Lengyel
2016-07-11 20:20 ` Corneliu ZUZU
2016-07-11 21:27 ` Tamas K Lengyel
2016-07-11 21:47 ` Corneliu ZUZU
2016-07-09 4:20 ` [PATCH 12/16] x86/vm-event: fix: move cleanup of mem_access_emulate_each_rep to monitor stub Corneliu ZUZU
2016-07-09 4:21 ` [PATCH 13/16] x86/monitor: introduce writes_pending field in monitor_write_data Corneliu ZUZU
2016-07-09 4:22 ` [PATCH 14/16] x86/monitor: clarify separation between monitor subsys and vm-event as a whole Corneliu ZUZU
2016-07-09 18:26 ` Tamas K Lengyel
2016-07-09 18:57 ` Corneliu ZUZU
2016-07-13 4:26 ` Corneliu ZUZU
2016-07-13 18:56 ` Tamas K Lengyel
2016-07-09 4:23 ` [PATCH 15/16] x86/monitor: fix: don't compromise a monitor_write_data with pending CR writes Corneliu ZUZU
2016-07-09 4:23 ` [PATCH 16/16] x86/monitor: fix: xc_monitor _write_ctrlreg w/o previous _enable must fail Corneliu ZUZU
2016-07-09 4:34 ` Corneliu ZUZU
2016-07-11 2:54 ` [PATCH 00/16] x86/vm-event: numerous adjustments & fixes Tian, Kevin
2016-07-11 5:32 ` Corneliu ZUZU
2016-07-12 7:42 ` Tian, Kevin
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).