Re: [Xen-devel] [PATCH 16/16] x86/monitor: fix: xc_monitor _write_ctrlreg w/o previous _enable must fail

[Corneliu ZUZU]

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 
---
  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

[Xen-devel] [PATCH 16/16] x86/monitor: fix: xc_monitor _write_ctrlreg w/o previous _enable must fail

[Corneliu ZUZU]

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 
---
 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

[Xen-devel] [PATCH 15/16] x86/monitor: fix: don't compromise a monitor_write_data with pending CR writes

[Corneliu ZUZU]

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 
---
 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;
 
-

[Xen-devel] [PATCH 14/16] x86/monitor: clarify separation between monitor subsys and vm-event as a whole

[Corneliu ZUZU]

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 
---
 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 
 #include 
 #include 
-#include 
 #include 
 #include 
 #include 
@@ -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;
+

[Xen-devel] [PATCH 13/16] x86/monitor: introduce writes_pending field in monitor_write_data

[Corneliu ZUZU]

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 
---
 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 = >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

[Xen-devel] [PATCH 12/16] x86/vm-event: fix: move cleanup of mem_access_emulate_each_rep to monitor stub

[Corneliu ZUZU]

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 
---
 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(>arch.monitor, 0, sizeof(d->arch.monitor));
 memset(>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

[Xen-devel] [PATCH 11/16] x86/monitor: fix: treat -monitor- properly, as a subsys of the vm-event subsys

[Corneliu ZUZU]

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 
---
 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 
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -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(>arch.monitor, 0, sizeof(d->arch.monitor));
 memset(>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(>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-devel] [PATCH 10/16] x86/vm-event: centralize vcpu-destroy cleanup in vm-events code

[Corneliu ZUZU]

Move vm-event cleanup sequence on vcpu destroyal to vm_event.h along with the
other vm-event functions.

Signed-off-by: Corneliu ZUZU 
---
 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 
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -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

[Xen-devel] [PATCH 09/16] arm/monitor: move d->monitor cleanup to monitor_cleanup_domain()

[Corneliu ZUZU]

d->monitor is a monitor subsystem resource, clean it up in the proper stub.

Signed-off-by: Corneliu ZUZU 
---
 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(>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 ( >vm_event->monitor == ved )
 monitor_cleanup_domain(d);
-
-memset(>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

[Xen-devel] [PATCH 08/16] x86/vm-event: call monitor init & cleanup funcs from respective vm_event funcs

[Corneliu ZUZU]

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 
---
 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 .
  */
 
+#include 
 #include 
 
 /* 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 ( >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 ( >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(>front_ring,
@@ -232,7 +232,7 @@ static int vm_event_disable(struct domain *d, struct 
vm_event_domain *ved)
 destroy_ring_for_helper(>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 = 

[Xen-devel] [PATCH 07/16] x86/vm-event: fix: call cleanup when init fails, to free partial allocs

[Corneliu ZUZU]

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 
---
 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

[Xen-devel] [PATCH 06/16] x86/monitor: fix: set msr_bitmap to NULL after xfree

[Corneliu ZUZU]

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 
---
 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

[Xen-devel] [PATCH 05/16] x86/monitor: relocate code more appropriately

[Corneliu ZUZU]

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 
---
 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 = >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)
 }
 
 

[Xen-devel] [PATCH 04/16] x86/monitor: relocate vm_event_register_write_resume() function to monitor code

[Corneliu ZUZU]

vm_event_register_write_resume is part of the monitor subsystem, relocate and
rename appropriately.

Signed-off-by: Corneliu ZUZU 
---
 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 
+#include 
 #include 
 
 int monitor_init_domain(struct domain *d)
@@ -41,6 +42,43 @@ void monitor_cleanup_domain(struct domain *d)
 memset(>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(>vm_event_pause_count) )
+return;
+
+w = >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(>vm_event_pause_count) )
-return;
-
-w = >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(>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, );
+monitor_ctrlreg_write_resume(v, );
 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 

[Xen-devel] [PATCH 03/16] x86/monitor: mechanical renames

[Corneliu ZUZU]

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 
---
 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 
 #include 
 
-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);
 

[Xen-devel] [PATCH 02/16] x86: fix: make atomic_read() param const

[Corneliu ZUZU]

This wouldn't let me make a param of a function that used atomic_read() const.

Signed-off-by: Corneliu ZUZU 
---
 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(>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

[Xen-devel] [PATCH 01/16] x86/vmx_update_guest_cr: minor optimization

[Corneliu ZUZU]

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 
---
 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

[Xen-devel] [PATCH 00/16] x86/vm-event: numerous adjustments & fixes

[Corneliu ZUZU]

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

[Xen-devel] [ovmf test] 96803: regressions - FAIL

[osstest service owner]

flight 96803 ovmf real [real]
http://logs.test-lab.xenproject.org/osstest/logs/96803/

Regressions :-(

Tests which did not succeed and are blocking,
including tests which could not be run:
 test-amd64-i386-xl-qemuu-ovmf-amd64 17 guest-start/debianhvm.repeat fail REGR. 
vs. 94748
 test-amd64-amd64-xl-qemuu-ovmf-amd64 17 guest-start/debianhvm.repeat fail 
REGR. vs. 94748

version targeted for testing:
 ovmf efadd41590b4d5251dd7e254f68bd09b1bb0a4b0
baseline version:
 ovmf dc99315b8732b6e3032d01319d3f534d440b43d0

Last test of basis94748  2016-05-24 22:43:25 Z   45 days
Failing since 94750  2016-05-25 03:43:08 Z   44 days   99 attempts
Testing same since96803  2016-07-08 20:14:08 Z0 days1 attempts


People who touched revisions under test:
  Anandakrishnan Loganathan 
  Ard Biesheuvel 
  Bi, Dandan 
  Bret Barkelew 
  Bruce Cran 
  Bruce Cran 
  Chao Zhang 
  Cinnamon Shia 
  Cohen, Eugene 
  Dandan Bi 
  Darbin Reyes 
  david wei 
  Eric Dong 
  Eugene Cohen 
  Evan Lloyd 
  Evgeny Yakovlev 
  Feng Tian 
  Fu Siyuan 
  Fu, Siyuan 
  Gary Li 
  Gary Lin 
  Giri P Mudusuru 
  Graeme Gregory 
  Hao Wu 
  Hegde Nagaraj P 
  Hegde, Nagaraj P 
  hegdenag 
  Heyi Guo 
  Jan D?bro? 
  Jan Dabros 
  Jeff Fan 
  Jiaxin Wu 
  Jiewen Yao 
  Joe Zhou 
  Jordan Justen 
  Katie Dellaquila 
  Laszlo Ersek 
  Liming Gao 
  Lu, ShifeiX A 
  lushifex 
  Marcin Wojtas 
  Mark Rutland 
  Marvin H?user 
  Marvin Haeuser 
  Maurice Ma 
  Michael Zimmermann 
  Mudusuru, Giri P 
  Ni, Ruiyu 
  Qiu Shumin 
  Ruiyu Ni 
  Ruiyu Ni　
  Ryan Harkin 
  Sami Mujawar 
  Satya Yarlagadda 
  Shannon Zhao 
  Sriram Subramanian 
  Star Zeng 
  Subramanian, Sriram (EG Servers Platform SW) 
  Sunny Wang 
  Tapan Shah 
  Thomas Palmer 
  Yarlagadda, Satya P 
  Yonghong Zhu 
  Zhang Lubo 
  Zhang, Chao B 

jobs:
 build-amd64-xsm  pass
 build-i386-xsm   pass
 build-amd64  pass
 build-i386   pass
 build-amd64-libvirt  pass
 build-i386-libvirt   pass
 build-amd64-pvopspass
 build-i386-pvops pass
 test-amd64-amd64-xl-qemuu-ovmf-amd64 fail
 test-amd64-i386-xl-qemuu-ovmf-amd64  fail



sg-report-flight on osstest.test-lab.xenproject.org
logs: /home/logs/logs
images: /home/logs/images

Logs, config files, etc. are available at
http://logs.test-lab.xenproject.org/osstest/logs

Explanation of these reports, and of osstest in general, is at
http://xenbits.xen.org/gitweb/?p=osstest.git;a=blob;f=README.email;hb=master
http://xenbits.xen.org/gitweb/?p=osstest.git;a=blob;f=README;hb=master

Test harness code can be found at
http://xenbits.xen.org/gitweb?p=osstest.git;a=summary


Not pushing.

(No revision log; it would be 8687 lines long.)

___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH] xen-kbdfront: correct return value checks on xenbus_scanf()

[Dmitry Torokhov]

On Thu, Jul 07, 2016 at 01:53:40AM -0600, Jan Beulich wrote:
> Only a positive return value indicates success.

If I am reading this correctly xenbus_scanf() guarantees that it will
not return 0. From include/xen/xenbus.h:

/* Single read and scanf: returns -errno or num scanned if > 0. */
__scanf(4, 5)
int xenbus_scanf(struct xenbus_transaction t,
 const char *dir, const char *node, const char *fmt, ...);

and the code matches:

...
/* Distinctive errno. */
if (ret == 0)
return -ERANGE;
return ret;

> 
> Signed-off-by: Jan Beulich 
> ---
>  drivers/input/misc/xen-kbdfront.c |5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> --- 4.7-rc6-xenbus_scanf.orig/drivers/input/misc/xen-kbdfront.c
> +++ 4.7-rc6-xenbus_scanf/drivers/input/misc/xen-kbdfront.c
> @@ -127,7 +127,8 @@ static int xenkbd_probe(struct xenbus_de
>   if (!info->page)
>   goto error_nomem;
>  
> - if (xenbus_scanf(XBT_NIL, dev->otherend, "feature-abs-pointer", "%d", 
> ) < 0)
> + if (xenbus_scanf(XBT_NIL, dev->otherend,
> +  "feature-abs-pointer", "%d", ) <= 0)
>   abs = 0;
>   if (abs) {
>   ret = xenbus_printf(XBT_NIL, dev->nodename,
> @@ -324,7 +325,7 @@ static void xenkbd_backend_changed(struc
>  InitWait:
>   ret = xenbus_scanf(XBT_NIL, info->xbdev->otherend,
>  "feature-abs-pointer", "%d", );
> - if (ret < 0)
> + if (ret <= 0)
>   val = 0;
>   if (val) {
>   ret = xenbus_printf(XBT_NIL, info->xbdev->nodename,
> 
> 

Thanks.

-- 
Dmitry

___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

[Xen-devel] [seabios baseline-only test] 66547: tolerable FAIL

[Platform Team regression test user]

This run is configured for baseline tests only.

flight 66547 seabios real [real]
http://osstest.xs.citrite.net/~osstest/testlogs/logs/66547/

Failures :-/ but no regressions.

Regressions which are regarded as allowable (not blocking):
 test-amd64-amd64-xl-qemuu-win7-amd64 16 guest-stop fail like 66515
 test-amd64-i386-xl-qemuu-win7-amd64 16 guest-stop  fail like 66515
 test-amd64-amd64-qemuu-nested-intel 16 debian-hvm-install/l1/l2 fail like 66515

Tests which did not succeed, but are not blocking:
 test-amd64-amd64-qemuu-nested-amd 16 debian-hvm-install/l1/l2  fail never pass
 test-amd64-amd64-libvirt-qemuu-debianhvm-amd64-xsm 10 migrate-support-check 
fail never pass
 test-amd64-i386-libvirt-qemuu-debianhvm-amd64-xsm 10 migrate-support-check 
fail never pass

version targeted for testing:
 seabios  13213a252286372efa5f72b4119faafd5dff5db1
baseline version:
 seabios  20f83d5c7c0f9ae5f775b6701c205349abe003fb

Last test of basis66515  2016-07-03 16:19:58 Z5 days
Testing same since66547  2016-07-08 11:17:27 Z0 days1 attempts


People who touched revisions under test:
  Kevin O'Connor 

jobs:
 build-amd64-xsm  pass
 build-i386-xsm   pass
 build-amd64  pass
 build-i386   pass
 build-amd64-libvirt  pass
 build-i386-libvirt   pass
 build-amd64-pvopspass
 build-i386-pvops pass
 test-amd64-amd64-libvirt-qemuu-debianhvm-amd64-xsm   pass
 test-amd64-i386-libvirt-qemuu-debianhvm-amd64-xsmpass
 test-amd64-amd64-xl-qemuu-debianhvm-amd64-xsmpass
 test-amd64-i386-xl-qemuu-debianhvm-amd64-xsm pass
 test-amd64-amd64-qemuu-nested-amdfail
 test-amd64-i386-qemuu-rhel6hvm-amd   pass
 test-amd64-amd64-xl-qemuu-debianhvm-amd64pass
 test-amd64-i386-xl-qemuu-debianhvm-amd64 pass
 test-amd64-amd64-xl-qemuu-ovmf-amd64 pass
 test-amd64-i386-xl-qemuu-ovmf-amd64  pass
 test-amd64-amd64-xl-qemuu-win7-amd64 fail
 test-amd64-i386-xl-qemuu-win7-amd64  fail
 test-amd64-amd64-qemuu-nested-intel  fail
 test-amd64-i386-qemuu-rhel6hvm-intel pass
 test-amd64-i386-xl-qemuu-winxpsp3-vcpus1 pass
 test-amd64-amd64-xl-qemuu-winxpsp3   pass
 test-amd64-i386-xl-qemuu-winxpsp3pass



sg-report-flight on osstest.xs.citrite.net
logs: /home/osstest/logs
images: /home/osstest/images

Logs, config files, etc. are available at
http://osstest.xs.citrite.net/~osstest/testlogs/logs

Test harness code can be found at
http://xenbits.xensource.com/gitweb?p=osstest.git;a=summary


Push not applicable.


commit 13213a252286372efa5f72b4119faafd5dff5db1
Author: Kevin O'Connor 
Date:   Mon Jul 4 12:31:50 2016 -0400

vgabios: Simplify set_cursor_pos()

Rework set_cursor_pos() to be slightly simpler.

Signed-off-by: Kevin O'Connor 

commit b303687deb4b56a9c187d74ec1869326683da464
Author: Kevin O'Connor 
Date:   Mon Jul 4 12:27:38 2016 -0400

vgabios: Don't check for special case of page==0xff on external calls

The original "lgpl vgabios" internally used page=0xff as a mechanism
for specifying the current page.  It also would allow int1013 calls to
externally specify bh==0xff for the current page.  However, there is
no documentation supporting this as an externally available feature.
SeaVGABIOS does not need the internal shortcut; this patch removes the
code.

Signed-off-by: Kevin O'Connor 

commit e5839eaffcf6ebba9d81f46a385280f7829f15d5
Author: Kevin O'Connor 
Date:   Mon Jul 4 12:20:48 2016 -0400

vgabios: Remove special case of dh==0xff in handle_1013()

The original "lgpl vgabios" had a special case for dh==0xff in its
int1013 (write string) code.  There does not appear to be any VGABIOS
documentation supporting this as an externally available feature.  It
appears this was for its own internal use when writing its strings to
the screen.  SeaVGABIOS doesn't use this hack; this patch removes it
from the code.
  

[Xen-devel] [xen-unstable test] 96795: regressions - FAIL

[osstest service owner]

flight 96795 xen-unstable real [real]
http://logs.test-lab.xenproject.org/osstest/logs/96795/

Regressions :-(

Tests which did not succeed and are blocking,
including tests which could not be run:
 test-armhf-armhf-xl-cubietruck 16 guest-start.2   fail REGR. vs. 96779
 test-armhf-armhf-libvirt-qcow2  6 xen-bootfail REGR. vs. 96779

Regressions which are regarded as allowable (not blocking):
 test-amd64-amd64-xl-rtds  6 xen-boot  fail REGR. vs. 96779
 build-amd64-rumpuserxen   6 xen-buildfail   like 96779
 build-i386-rumpuserxen6 xen-buildfail   like 96779
 test-amd64-i386-xl-qemuu-win7-amd64 16 guest-stop  fail like 96779
 test-amd64-i386-xl-qemut-win7-amd64 16 guest-stop  fail like 96779
 test-amd64-amd64-xl-qemut-win7-amd64 16 guest-stop fail like 96779
 test-amd64-amd64-xl-qemuu-win7-amd64 16 guest-stop fail like 96779

Tests which did not succeed, but are not blocking:
 test-amd64-amd64-rumpuserxen-amd64  1 build-check(1)   blocked n/a
 test-amd64-i386-rumpuserxen-i386  1 build-check(1)   blocked  n/a
 test-amd64-amd64-xl-pvh-amd  11 guest-start  fail   never pass
 test-amd64-amd64-xl-pvh-intel 11 guest-start  fail  never pass
 test-armhf-armhf-libvirt-xsm 12 migrate-support-checkfail   never pass
 test-armhf-armhf-libvirt-xsm 14 guest-saverestorefail   never pass
 test-amd64-i386-libvirt  12 migrate-support-checkfail   never pass
 test-amd64-amd64-libvirt 12 migrate-support-checkfail   never pass
 test-amd64-amd64-libvirt-xsm 12 migrate-support-checkfail   never pass
 test-amd64-i386-libvirt-xsm  12 migrate-support-checkfail   never pass
 test-amd64-amd64-qemuu-nested-amd 16 debian-hvm-install/l1/l2  fail never pass
 test-amd64-amd64-libvirt-qemuu-debianhvm-amd64-xsm 10 migrate-support-check 
fail never pass
 test-armhf-armhf-xl-arndale  12 migrate-support-checkfail   never pass
 test-armhf-armhf-xl-arndale  13 saverestore-support-checkfail   never pass
 test-armhf-armhf-xl-cubietruck 12 migrate-support-checkfail never pass
 test-armhf-armhf-xl-cubietruck 13 saverestore-support-checkfail never pass
 test-amd64-amd64-libvirt-vhd 11 migrate-support-checkfail   never pass
 test-amd64-i386-libvirt-qemuu-debianhvm-amd64-xsm 10 migrate-support-check 
fail never pass
 test-armhf-armhf-xl-multivcpu 13 saverestore-support-checkfail  never pass
 test-armhf-armhf-xl-multivcpu 12 migrate-support-checkfail  never pass
 test-armhf-armhf-xl  12 migrate-support-checkfail   never pass
 test-armhf-armhf-xl  13 saverestore-support-checkfail   never pass
 test-armhf-armhf-libvirt 14 guest-saverestorefail   never pass
 test-armhf-armhf-libvirt 12 migrate-support-checkfail   never pass
 test-armhf-armhf-libvirt-raw 13 guest-saverestorefail   never pass
 test-armhf-armhf-libvirt-raw 11 migrate-support-checkfail   never pass
 test-armhf-armhf-xl-rtds 13 saverestore-support-checkfail   never pass
 test-armhf-armhf-xl-rtds 12 migrate-support-checkfail   never pass
 test-armhf-armhf-xl-credit2  13 saverestore-support-checkfail   never pass
 test-armhf-armhf-xl-credit2  12 migrate-support-checkfail   never pass
 test-armhf-armhf-xl-xsm  13 saverestore-support-checkfail   never pass
 test-armhf-armhf-xl-xsm  12 migrate-support-checkfail   never pass
 test-armhf-armhf-xl-vhd  11 migrate-support-checkfail   never pass
 test-armhf-armhf-xl-vhd  12 saverestore-support-checkfail   never pass

version targeted for testing:
 xen  1e2d167d8faae843b80487e5026b07a135cf4147
baseline version:
 xen  730bdfa418fc8c809695ff5d96bc6f7a3b8827ba

Last test of basis96779  2016-07-08 06:01:36 Z0 days
Testing same since96795  2016-07-08 16:14:43 Z0 days1 attempts


People who touched revisions under test:
  Corneliu ZUZU 
  Dario Faggioli 
  George Dunlap 
  Jan Beulich 
  Julien Grall 
  Razvan Cojocaru 
  Tamas K Lengyel 

jobs:
 build-amd64-xsm  pass
 build-armhf-xsm  pass
 build-i386-xsm   pass
 build-amd64  pass
 build-armhf  pass
 build-i386   pass
 build-amd64-libvirt  pass
 

[Xen-devel] [qemu-mainline test] 96791: tolerable FAIL - PUSHED

[osstest service owner]

flight 96791 qemu-mainline real [real]
http://logs.test-lab.xenproject.org/osstest/logs/96791/

Failures :-/ but no regressions.

Regressions which are regarded as allowable (not blocking):
 test-amd64-amd64-xl-rtds  9 debian-install   fail blocked in 96732
 test-amd64-amd64-xl-qemuu-win7-amd64 16 guest-stop fail like 96732
 test-amd64-i386-xl-qemuu-win7-amd64 16 guest-stop  fail like 96732

Tests which did not succeed, but are not blocking:
 test-amd64-amd64-xl-pvh-amd  11 guest-start  fail   never pass
 test-amd64-amd64-xl-pvh-intel 11 guest-start  fail  never pass
 test-armhf-armhf-libvirt 14 guest-saverestorefail   never pass
 test-armhf-armhf-libvirt 12 migrate-support-checkfail   never pass
 test-amd64-i386-libvirt  12 migrate-support-checkfail   never pass
 test-armhf-armhf-xl-arndale  12 migrate-support-checkfail   never pass
 test-armhf-armhf-xl-arndale  13 saverestore-support-checkfail   never pass
 test-amd64-amd64-qemuu-nested-amd 16 debian-hvm-install/l1/l2  fail never pass
 test-amd64-amd64-libvirt-qemuu-debianhvm-amd64-xsm 10 migrate-support-check 
fail never pass
 test-amd64-amd64-libvirt-xsm 12 migrate-support-checkfail   never pass
 test-amd64-i386-libvirt-qemuu-debianhvm-amd64-xsm 10 migrate-support-check 
fail never pass
 test-armhf-armhf-xl-credit2  13 saverestore-support-checkfail   never pass
 test-armhf-armhf-xl-credit2  12 migrate-support-checkfail   never pass
 test-armhf-armhf-xl-cubietruck 12 migrate-support-checkfail never pass
 test-armhf-armhf-xl-cubietruck 13 saverestore-support-checkfail never pass
 test-armhf-armhf-libvirt-xsm 12 migrate-support-checkfail   never pass
 test-armhf-armhf-libvirt-xsm 14 guest-saverestorefail   never pass
 test-amd64-amd64-libvirt 12 migrate-support-checkfail   never pass
 test-armhf-armhf-xl  12 migrate-support-checkfail   never pass
 test-armhf-armhf-xl  13 saverestore-support-checkfail   never pass
 test-armhf-armhf-libvirt-qcow2 11 migrate-support-checkfail never pass
 test-armhf-armhf-libvirt-qcow2 13 guest-saverestorefail never pass
 test-armhf-armhf-libvirt-raw 13 guest-saverestorefail   never pass
 test-armhf-armhf-libvirt-raw 11 migrate-support-checkfail   never pass
 test-amd64-i386-libvirt-xsm  12 migrate-support-checkfail   never pass
 test-armhf-armhf-xl-xsm  13 saverestore-support-checkfail   never pass
 test-armhf-armhf-xl-xsm  12 migrate-support-checkfail   never pass
 test-armhf-armhf-xl-vhd  11 migrate-support-checkfail   never pass
 test-armhf-armhf-xl-vhd  12 saverestore-support-checkfail   never pass
 test-armhf-armhf-xl-multivcpu 13 saverestore-support-checkfail  never pass
 test-armhf-armhf-xl-multivcpu 12 migrate-support-checkfail  never pass
 test-amd64-amd64-libvirt-vhd 11 migrate-support-checkfail   never pass
 test-armhf-armhf-xl-rtds 13 saverestore-support-checkfail   never pass
 test-armhf-armhf-xl-rtds 12 migrate-support-checkfail   never pass

version targeted for testing:
 qemuu4f4a9ca4a4386c137301b3662faba076455ff15a
baseline version:
 qemuu91d35509903464c7f4b9ed56be223d7370d3597c

Last test of basis96732  2016-07-06 18:12:54 Z2 days
Failing since 96765  2016-07-07 10:44:12 Z1 days3 attempts
Testing same since96776  2016-07-08 00:42:28 Z0 days2 attempts


People who touched revisions under test:
  Jason Wang 
  Jean-Christophe Dubois 
  KONRAD Frederic 
  Paolo Bonzini 
  Peter Maydell 
  Shannon Zhao 

jobs:
 build-amd64-xsm  pass
 build-armhf-xsm  pass
 build-i386-xsm   pass
 build-amd64  pass
 build-armhf  pass
 build-i386   pass
 build-amd64-libvirt  pass
 build-armhf-libvirt  pass
 build-i386-libvirt   pass
 build-amd64-pvopspass
 build-armhf-pvopspass
 build-i386-pvops pass
 test-amd64-amd64-xl  pass
 test-armhf-armhf-xl  pass
 test-amd64-i386-xl 

Re: [Xen-devel] [PATCH v6 03/14] xen: Use a typesafe to define INVALID_MFN

[Elena Ufimtseva]

On Fri, Jul 08, 2016 at 08:20:03PM +0100, Andrew Cooper wrote:
> On 08/07/2016 23:01, Elena Ufimtseva wrote:
> >
> >>> @@ -838,7 +838,6 @@ mfn_t oos_snapshot_lookup(struct domain *d, mfn_t 
> >>> gmfn)
> >>>
> >>>  SHADOW_ERROR("gmfn %lx was OOS but not in hash table\n", 
> >>> mfn_x(gmfn));
> >>>  BUG();
> >>> -return _mfn(INVALID_MFN);
> > Can compiler be unhappy about this?
> 
> This was my suggestion, from a previous round of review.

Ah! Thanks for explanation.
> 
> A while ago, I annotated BUG() with unreachable(), as as execution will
> not continue from a bugframe, but the shadow code is definitely older
> than my change.
> 
> As such, compilers will have been dropping this return statement as part
> of dead-code-elimination anyway.
> 
> This option is better than just replacing one bit of dead code with a
> different bit of dead code.
> 
> ~Andrew

___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

[Xen-devel] [ovmf test] 96787: regressions - FAIL

[osstest service owner]

flight 96787 ovmf real [real]
http://logs.test-lab.xenproject.org/osstest/logs/96787/

Regressions :-(

Tests which did not succeed and are blocking,
including tests which could not be run:
 test-amd64-i386-xl-qemuu-ovmf-amd64 17 guest-start/debianhvm.repeat fail REGR. 
vs. 94748
 test-amd64-amd64-xl-qemuu-ovmf-amd64 17 guest-start/debianhvm.repeat fail 
REGR. vs. 94748

version targeted for testing:
 ovmf 694673c9108f22dd7da66e6ce2c118c929f110dd
baseline version:
 ovmf dc99315b8732b6e3032d01319d3f534d440b43d0

Last test of basis94748  2016-05-24 22:43:25 Z   44 days
Failing since 94750  2016-05-25 03:43:08 Z   44 days   98 attempts
Testing same since96787  2016-07-08 10:27:08 Z0 days1 attempts


People who touched revisions under test:
  Anandakrishnan Loganathan 
  Ard Biesheuvel 
  Bi, Dandan 
  Bret Barkelew 
  Bruce Cran 
  Bruce Cran 
  Chao Zhang 
  Cinnamon Shia 
  Cohen, Eugene 
  Dandan Bi 
  Darbin Reyes 
  david wei 
  Eric Dong 
  Eugene Cohen 
  Evan Lloyd 
  Evgeny Yakovlev 
  Feng Tian 
  Fu Siyuan 
  Fu, Siyuan 
  Gary Li 
  Gary Lin 
  Giri P Mudusuru 
  Graeme Gregory 
  Hao Wu 
  Hegde Nagaraj P 
  Hegde, Nagaraj P 
  hegdenag 
  Heyi Guo 
  Jan D?bro? 
  Jan Dabros 
  Jeff Fan 
  Jiaxin Wu 
  Jiewen Yao 
  Joe Zhou 
  Jordan Justen 
  Katie Dellaquila 
  Laszlo Ersek 
  Liming Gao 
  Lu, ShifeiX A 
  lushifex 
  Marcin Wojtas 
  Mark Rutland 
  Marvin H?user 
  Marvin Haeuser 
  Maurice Ma 
  Michael Zimmermann 
  Mudusuru, Giri P 
  Ni, Ruiyu 
  Qiu Shumin 
  Ruiyu Ni 
  Ruiyu Ni　
  Ryan Harkin 
  Sami Mujawar 
  Satya Yarlagadda 
  Shannon Zhao 
  Sriram Subramanian 
  Star Zeng 
  Subramanian, Sriram (EG Servers Platform SW) 
  Sunny Wang 
  Tapan Shah 
  Thomas Palmer 
  Yarlagadda, Satya P 
  Yonghong Zhu 
  Zhang Lubo 
  Zhang, Chao B 

jobs:
 build-amd64-xsm  pass
 build-i386-xsm   pass
 build-amd64  pass
 build-i386   pass
 build-amd64-libvirt  pass
 build-i386-libvirt   pass
 build-amd64-pvopspass
 build-i386-pvops pass
 test-amd64-amd64-xl-qemuu-ovmf-amd64 fail
 test-amd64-i386-xl-qemuu-ovmf-amd64  fail



sg-report-flight on osstest.test-lab.xenproject.org
logs: /home/logs/logs
images: /home/logs/images

Logs, config files, etc. are available at
http://logs.test-lab.xenproject.org/osstest/logs

Explanation of these reports, and of osstest in general, is at
http://xenbits.xen.org/gitweb/?p=osstest.git;a=blob;f=README.email;hb=master
http://xenbits.xen.org/gitweb/?p=osstest.git;a=blob;f=README;hb=master

Test harness code can be found at
http://xenbits.xen.org/gitweb?p=osstest.git;a=summary


Not pushing.

(No revision log; it would be 8663 lines long.)

___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v6 03/14] xen: Use a typesafe to define INVALID_MFN

[Julien Grall]

Hi Elena,

On 08/07/2016 23:01, Elena Ufimtseva wrote:

On Wed, Jul 06, 2016 at 02:04:17PM +0100, Julien Grall wrote:

@@ -3968,7 +3967,7 @@ void shadow_audit_tables(struct vcpu *v)
 }
 }

-hash_vcpu_foreach(v, mask, callbacks, _mfn(INVALID_MFN));
+hash_vcpu_foreach(v, mask, callbacks, INVALID_MFN_T);


What is INVALID_MFN_T?


This was the name of the unbox type on a previous series. I forgot to 
rename this one and my build scripts did not fail.


Thank you for spotting it!

Regards,

--
Julien Grall

___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v6 03/14] xen: Use a typesafe to define INVALID_MFN

[Andrew Cooper]

On 08/07/2016 23:01, Elena Ufimtseva wrote:
>
>>> @@ -838,7 +838,6 @@ mfn_t oos_snapshot_lookup(struct domain *d, mfn_t gmfn)
>>>
>>>  SHADOW_ERROR("gmfn %lx was OOS but not in hash table\n", mfn_x(gmfn));
>>>  BUG();
>>> -return _mfn(INVALID_MFN);
> Can compiler be unhappy about this?

This was my suggestion, from a previous round of review.

A while ago, I annotated BUG() with unreachable(), as as execution will
not continue from a bugframe, but the shadow code is definitely older
than my change.

As such, compilers will have been dropping this return statement as part
of dead-code-elimination anyway.

This option is better than just replacing one bit of dead code with a
different bit of dead code.

~Andrew

___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v6 04/14] xen: Use a typesafe to define INVALID_GFN

[Elena Ufimtseva]

On Wed, Jul 06, 2016 at 02:05:24PM +0100, Julien Grall wrote:
> (CC Elena)
> 
Thanks Julien!

> On 06/07/16 14:01, Julien Grall wrote:
> >Also take the opportunity to convert arch/x86/debug.c to the typesafe gfn.
> >
> >Signed-off-by: Julien Grall 
> >Reviewed-by: Andrew Cooper 
> >Acked-by: Stefano Stabellini 
> >
> >---
> >Cc: Mukesh Rathor 
> 
> I forgot to update the CC list since GDSX maintainership was taken over by
> Elena. Sorry for that.
> 
> Regards,
> 
> >Cc: Jan Beulich 
> >Cc: Paul Durrant 
> >Cc: Boris Ostrovsky 
> >Cc: Suravee Suthikulpanit 
> >Cc: Jun Nakajima 
> >Cc: Kevin Tian 
> >Cc: George Dunlap 
> >Cc: Tim Deegan 
> >Cc: Feng Wu 
> >
> > Changes in v6:
> > - Add Stefano's acked-by for ARM bits
> > - Remove set of brackets when it is not necessary
> > - Add Andrew's reviewed-by
> >
> > Changes in v5:
> > - Patch added
> >---
> >  xen/arch/arm/p2m.c  |  4 ++--
> >  xen/arch/x86/debug.c| 18 +-
> >  xen/arch/x86/domain.c   |  2 +-
> >  xen/arch/x86/hvm/emulate.c  |  7 ---
> >  xen/arch/x86/hvm/hvm.c  |  6 +++---
> >  xen/arch/x86/hvm/ioreq.c|  8 
> >  xen/arch/x86/hvm/svm/nestedsvm.c|  2 +-
> >  xen/arch/x86/hvm/vmx/vmx.c  |  6 +++---
> >  xen/arch/x86/mm/altp2m.c|  2 +-
> >  xen/arch/x86/mm/hap/guest_walk.c| 10 +-
> >  xen/arch/x86/mm/hap/nested_ept.c|  2 +-
> >  xen/arch/x86/mm/p2m-pod.c   |  6 +++---
> >  xen/arch/x86/mm/p2m.c   | 18 +-
> >  xen/arch/x86/mm/shadow/common.c |  2 +-
> >  xen/arch/x86/mm/shadow/multi.c  |  2 +-
> >  xen/arch/x86/mm/shadow/private.h|  2 +-
> >  xen/drivers/passthrough/amd/iommu_map.c |  2 +-
> >  xen/drivers/passthrough/vtd/iommu.c |  4 ++--
> >  xen/drivers/passthrough/x86/iommu.c |  2 +-
> >  xen/include/asm-x86/guest_pt.h  |  4 ++--
> >  xen/include/asm-x86/p2m.h   |  2 +-
> >  xen/include/xen/mm.h|  2 +-
> >  22 files changed, 57 insertions(+), 56 deletions(-)
> >
> >diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
> >index d690602..c938dde 100644
> >--- a/xen/arch/arm/p2m.c
> >+++ b/xen/arch/arm/p2m.c
> >@@ -479,7 +479,7 @@ static int __p2m_get_mem_access(struct domain *d, gfn_t 
> >gfn,
> >  }
> >
> >  /* If request to get default access. */
> >-if ( gfn_x(gfn) == INVALID_GFN )
> >+if ( gfn_eq(gfn, INVALID_GFN) )
> >  {
> >  *access = memaccess[p2m->default_access];
> >  return 0;
> >@@ -1879,7 +1879,7 @@ long p2m_set_mem_access(struct domain *d, gfn_t gfn, 
> >uint32_t nr,
> >  p2m->mem_access_enabled = true;
> >
> >  /* If request to set default access. */
> >-if ( gfn_x(gfn) == INVALID_GFN )
> >+if ( gfn_eq(gfn, INVALID_GFN) )
> >  {
> >  p2m->default_access = a;
> >  return 0;
> >diff --git a/xen/arch/x86/debug.c b/xen/arch/x86/debug.c
> >index 9213ea7..3030022 100644
> >--- a/xen/arch/x86/debug.c
> >+++ b/xen/arch/x86/debug.c
> >@@ -44,8 +44,7 @@ typedef unsigned char dbgbyte_t;
> >
> >  /* Returns: mfn for the given (hvm guest) vaddr */
> >  static mfn_t
> >-dbg_hvm_va2mfn(dbgva_t vaddr, struct domain *dp, int toaddr,
> >-unsigned long *gfn)
> >+dbg_hvm_va2mfn(dbgva_t vaddr, struct domain *dp, int toaddr, gfn_t *gfn)
> >  {
> >  mfn_t mfn;
> >  uint32_t pfec = PFEC_page_present;
> >@@ -53,14 +52,14 @@ dbg_hvm_va2mfn(dbgva_t vaddr, struct domain *dp, int 
> >toaddr,
> >
> >  DBGP2("vaddr:%lx domid:%d\n", vaddr, dp->domain_id);
> >
> >-*gfn = paging_gva_to_gfn(dp->vcpu[0], vaddr, );
> >-if ( *gfn == INVALID_GFN )
> >+*gfn = _gfn(paging_gva_to_gfn(dp->vcpu[0], vaddr, ));
> >+if ( gfn_eq(*gfn, INVALID_GFN) )
> >  {
> >  DBGP2("kdb:bad gfn from gva_to_gfn\n");
> >  return INVALID_MFN;
> >  }
> >
> >-mfn = get_gfn(dp, *gfn, );
> >+mfn = get_gfn(dp, gfn_x(*gfn), );
> >  if ( p2m_is_readonly(gfntype) && toaddr )
> >  {
> >  DBGP2("kdb:p2m_is_readonly: gfntype:%x\n", gfntype);
> >@@ -72,7 +71,7 @@ dbg_hvm_va2mfn(dbgva_t vaddr, struct domain *dp, int 
> >toaddr,
> >
> >  if ( mfn_eq(mfn, INVALID_MFN) )
> >  {
> >-put_gfn(dp, *gfn);
> >+put_gfn(dp, gfn_x(*gfn));
> >  *gfn = INVALID_GFN;
> >  }
> >
> >@@ -165,7 +164,8 @@ unsigned int dbg_rw_guest_mem(struct domain *dp, void * 
> >__user gaddr,
> >  char *va;
> >  unsigned long addr = (unsigned long)gaddr;
> >  mfn_t mfn;
> >-

Re: [Xen-devel] [PATCH v6 03/14] xen: Use a typesafe to define INVALID_MFN

[Elena Ufimtseva]

On Wed, Jul 06, 2016 at 02:04:17PM +0100, Julien Grall wrote:
> (CC Elena).
> 
> On 06/07/16 14:01, Julien Grall wrote:
> >Also take the opportunity to convert arch/x86/debug.c to the typesafe
> >mfn and use proper printf format for MFN/GFN when the code around is
> >modified.
> >
> >Signed-off-by: Julien Grall 
> >Reviewed-by: Andrew Cooper 
> >Acked-by: Stefano Stabellini 
> >
> >---
> >Cc: Christoph Egger 
> >Cc: Liu Jinsong 
> >Cc: Jan Beulich 
> >Cc: Mukesh Rathor 
> 
> I forgot to update the CC list since GDSX maintainership was take over by
> Elena. Sorry for that.

No problem!
> 
> >Cc: Paul Durrant 
> >Cc: Jun Nakajima 
> >Cc: Kevin Tian 
> >Cc: George Dunlap 
> >Cc: Tim Deegan 
> >
> > Changes in v6:
> > - Add Stefano's acked-by for ARM bits
> > - Use PRI_mfn and PRI_gfn
> > - Remove set of brackets when it is not necessary
> > - Use mfn_add when possible
> > - Add Andrew's reviewed-by
> >
> > Changes in v5:
> > - Patch added
> >---
> >  xen/arch/arm/p2m.c  |  4 +--
> >  xen/arch/x86/cpu/mcheck/mce.c   |  2 +-
> >  xen/arch/x86/debug.c| 58 
> > +
> >  xen/arch/x86/hvm/hvm.c  |  6 ++---
> >  xen/arch/x86/hvm/viridian.c | 12 -
> >  xen/arch/x86/hvm/vmx/vmx.c  |  2 +-
> >  xen/arch/x86/mm/guest_walk.c|  4 +--
> >  xen/arch/x86/mm/hap/hap.c   |  4 +--
> >  xen/arch/x86/mm/p2m-ept.c   |  6 ++---
> >  xen/arch/x86/mm/p2m-pod.c   | 18 ++---
> >  xen/arch/x86/mm/p2m-pt.c| 18 ++---
> >  xen/arch/x86/mm/p2m.c   | 54 +++---
> >  xen/arch/x86/mm/paging.c| 12 -
> >  xen/arch/x86/mm/shadow/common.c | 43 +++---
> >  xen/arch/x86/mm/shadow/multi.c  | 36 -
> >  xen/common/domain.c |  6 ++---
> >  xen/common/grant_table.c|  6 ++---
> >  xen/include/xen/mm.h|  2 +-
> >  18 files changed, 147 insertions(+), 146 deletions(-)
> >
> >diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
> >index 34563bb..d690602 100644
> >--- a/xen/arch/arm/p2m.c
> >+++ b/xen/arch/arm/p2m.c
> >@@ -1461,7 +1461,7 @@ int relinquish_p2m_mapping(struct domain *d)
> >  return apply_p2m_changes(d, RELINQUISH,
> >pfn_to_paddr(p2m->lowest_mapped_gfn),
> >pfn_to_paddr(p2m->max_mapped_gfn),
> >-  pfn_to_paddr(INVALID_MFN),
> >+  pfn_to_paddr(mfn_x(INVALID_MFN)),
> >MATTR_MEM, 0, p2m_invalid,
> >d->arch.p2m.default_access);
> >  }
> >@@ -1476,7 +1476,7 @@ int p2m_cache_flush(struct domain *d, xen_pfn_t 
> >start_mfn, xen_pfn_t end_mfn)
> >  return apply_p2m_changes(d, CACHEFLUSH,
> >   pfn_to_paddr(start_mfn),
> >   pfn_to_paddr(end_mfn),
> >- pfn_to_paddr(INVALID_MFN),
> >+ pfn_to_paddr(mfn_x(INVALID_MFN)),
> >   MATTR_MEM, 0, p2m_invalid,
> >   d->arch.p2m.default_access);
> >  }
> >diff --git a/xen/arch/x86/cpu/mcheck/mce.c b/xen/arch/x86/cpu/mcheck/mce.c
> >index edcbe48..2695b0c 100644
> >--- a/xen/arch/x86/cpu/mcheck/mce.c
> >+++ b/xen/arch/x86/cpu/mcheck/mce.c
> >@@ -1455,7 +1455,7 @@ long do_mca(XEN_GUEST_HANDLE_PARAM(xen_mc_t) u_xen_mc)
> >  gfn = PFN_DOWN(gaddr);
> >  mfn = mfn_x(get_gfn(d, gfn, ));
> >
> >-if ( mfn == INVALID_MFN )
> >+if ( mfn == mfn_x(INVALID_MFN) )
> >  {
> >  put_gfn(d, gfn);
> >  put_domain(d);
> >diff --git a/xen/arch/x86/debug.c b/xen/arch/x86/debug.c
> >index 58cae22..9213ea7 100644
> >--- a/xen/arch/x86/debug.c
> >+++ b/xen/arch/x86/debug.c
> >@@ -43,11 +43,11 @@ typedef unsigned long dbgva_t;
> >  typedef unsigned char dbgbyte_t;
> >
> >  /* Returns: mfn for the given (hvm guest) vaddr */
> >-static unsigned long
> >+static mfn_t
> >  dbg_hvm_va2mfn(dbgva_t vaddr, struct domain *dp, int toaddr,
> >  unsigned long *gfn)
> >  {
> >-unsigned long mfn;
> >+mfn_t mfn;
> >  uint32_t pfec = PFEC_page_present;
> >  p2m_type_t gfntype;
> >
> >@@ -60,16 +60,17 @@ dbg_hvm_va2mfn(dbgva_t vaddr, struct domain *dp, int 
> >toaddr,
> >  return INVALID_MFN;
> >  }
> >
> >-mfn = mfn_x(get_gfn(dp, *gfn, ));
> >+mfn = get_gfn(dp, *gfn, );
> >  if ( p2m_is_readonly(gfntype) && toaddr )
> >  {
> >  

[Xen-devel] [OSSTEST PATCH 07/33] invoke-daemon: Honour OSSTEST_DAEMON_TCLSH

[Ian Jackson]

It appears that tcl8.5 in wheezy has a serious bug which makes `after
idle' not always work.  tcl8.4 has been working well in wheezy but is
not in jessie, where tcl8.5 works (and tcl8.6 has a serious event loop
bug - Debian #826741).

So we need to use different versions of Tcl on different hosts.
Allow this to be specified in ~/.xen-osstest/settings.

This affects only:
 - invoke-daemon (which is normally run from inittab)
 - mg-schema-test-database

sg-run-job and sg-execute-flight are not affected.  They do not
currently use `after idle' so that is OK for now.

Signed-off-by: Ian Jackson 
---
 invoke-daemon   |  2 +-
 mg-schema-test-database | 10 +-
 2 files changed, 10 insertions(+), 2 deletions(-)

diff --git a/invoke-daemon b/invoke-daemon
index ad1434a..e4a47bb 100755
--- a/invoke-daemon
+++ b/invoke-daemon
@@ -24,4 +24,4 @@ fi
 
 cd "${0%/*}"
 if [ "x$2" != x ]; then sleep $2; fi
-exec ./$1 2>&1 | exec logger -t $1 -p local4.info
+exec $OSSTEST_DAEMON_TCLSH ./$1 2>&1 | exec logger -t $1 -p local4.info
diff --git a/mg-schema-test-database b/mg-schema-test-database
index 892e41a..5ebba39 100755
--- a/mg-schema-test-database
+++ b/mg-schema-test-database
@@ -567,7 +567,15 @@ daemons)
for arg in "$@"; do
case "$arg" in
_*) suffix="$arg" ;;
-   owner|queue)wantdaemons+=("./ms-${arg}daemon") ;;
+   owner|queue)
+   tcl="$(bash -ec '
+   set -o posix
+   if [ -e $HOME/.xen-osstest/settings ]; then
+   source $HOME/.xen-osstest/settings
+   fi
+   echo "$OSSTEST_DAEMON_TCLSH"
+   ')"
+   wantdaemons+=("$tcl ./ms-${arg}daemon") ;;
*/*)wantdaemons+=("$arg") ;;
*)  wantdaemons+=("./$arg") ;;
esac
-- 
2.1.4


___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

[Xen-devel] [OSSTEST PATCH 02/33] mg-allocate: Do not treat already-allocated resources as satisfactory

[Ian Jackson]

This was always rather odd for ./mg-allocate HOSTNAME but makes the
more sophisticated uses like ./mg-allocate '{FLAG,FLAG,...}' very much
less useful.

Signed-off-by: Ian Jackson 
---
 mg-allocate | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/mg-allocate b/mg-allocate
index d295997..9b66114 100755
--- a/mg-allocate
+++ b/mg-allocate
@@ -234,8 +234,7 @@ END
 if ($allocate) {
 if ($candrow->{owntaskid} == $tid) {
 logm("$desc: already allocated to $tid");
-$got_shareix= $candrow->{shareix};
-$ok=1; last;
+   next;
 }
 if ($isshared) {
 logm("$desc: available, unsharing");
-- 
2.1.4


___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

[Xen-devel] [OSSTEST PATCH 13/33] Database locking: Tcl: Use db-execute

[Ian Jackson]

Replace open-coded uses of pg_execute dbh STMT with
jobdb::db-execute STMT.

The only functional change is that if OSSTEST_TCL_JOBDB_DEBUG is set,
there will be debugging output.

But we are going to want to make db-execute do something more
complicated involving pg_exec.

Signed-off-by: Ian Jackson 
Acked-by: Ian Campbell 
---
 ms-ownerdaemon | 6 +++---
 ms-queuedaemon | 6 +++---
 2 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/ms-ownerdaemon b/ms-ownerdaemon
index cc2f361..3623d19 100755
--- a/ms-ownerdaemon
+++ b/ms-ownerdaemon
@@ -32,7 +32,7 @@ proc chan-destroy-stuff {chan} {
 jobdb::transaction resources {
 puts-chan-desc $chan "-- $tasks"
 foreach task $tasks {
-pg_execute dbh "
+jobdb::db-execute "
 UPDATE tasks
SET live = 'f'
  WHERE taskid = $task
@@ -62,7 +62,7 @@ proc cmd/create-task {chan desc} {
 set taskdesc $desc
 }
 jobdb::transaction resources {
-pg_execute dbh "
+jobdb::db-execute "
 INSERT INTO tasks
 ( type,  refkey,   refinfo, live)
  VALUES ('ownd', [pg_quote $taskdesc], [clock seconds], 't')
@@ -93,7 +93,7 @@ main-daemon Owner {
 jobdb::db-open
 
 jobdb::transaction resources {
-set nrows [pg_execute dbh "
+set nrows [jobdb::db-execute "
 UPDATE tasks
SET refkey = 'previous ' || refkey
  WHERE type = 'ownd'
diff --git a/ms-queuedaemon b/ms-queuedaemon
index 6e93288..9db8b05 100755
--- a/ms-queuedaemon
+++ b/ms-queuedaemon
@@ -155,7 +155,7 @@ proc runneeded-perhaps-start {} {
 }
 
 jobdb::transaction resources {
-set nrows [pg_execute dbh {
+set nrows [jobdb::db-execute {
 UPDATE resources
SET owntaskid= (SELECT taskid FROM tasks
WHERE type='magic' AND refkey='allocatable')
@@ -167,7 +167,7 @@ proc runneeded-perhaps-start {} {
 if {!($nrows || $needed>=2)} return
 
 jobdb::transaction resources {
-set cleaned [pg_execute dbh {
+set cleaned [jobdb::db-execute {
 DELETE FROM tasks
  WHERE type='ownd'
AND live='f'
@@ -650,7 +650,7 @@ proc cmd/uptime {chan desc seconds} {
 continue
 }
 if {$refinfo > $before} continue
-pg_execute dbh "
+jobdb::db-execute "
 UPDATE tasks
SET live = false,
refinfo = refinfo || ' stale'
-- 
2.1.4


___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

[Xen-devel] [OSSTEST PATCH 19/33] tcl daemons: Move BEGIN within scope of transaction error trapping

[Ian Jackson]

If the db connection has failed, BEGIN will fail.  We want to to
handle this properly.

Right now the effect is that we will now close the connection and it
will then be reopened by the next command.

Signed-off-by: Ian Jackson 
---
 tcl/JobDB-Executive.tcl | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tcl/JobDB-Executive.tcl b/tcl/JobDB-Executive.tcl
index 29ab59a..f2322c4 100644
--- a/tcl/JobDB-Executive.tcl
+++ b/tcl/JobDB-Executive.tcl
@@ -284,8 +284,8 @@ proc transaction {tables script} {
 db-open
 while 1 {
 set ol {}
-db-execute BEGIN
set rc [catch {
+   db-execute BEGIN
db-execute "SET TRANSACTION ISOLATION LEVEL SERIALIZABLE"
lock-tables $tables
uplevel 1 $script
-- 
2.1.4


___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

[Xen-devel] [OSSTEST PATCH 22/33] tcl daemons: Break out db-ensure-open and db-ensure-closed

[Ian Jackson]

To be able to deliberately reconnect to the database, in case of
error, we need functions which actually work with dbh, rather than
simply the refcount.

No functional change as yet.

Signed-off-by: Ian Jackson 
---
 tcl/JobDB-Executive.tcl | 18 --
 1 file changed, 12 insertions(+), 6 deletions(-)

diff --git a/tcl/JobDB-Executive.tcl b/tcl/JobDB-Executive.tcl
index 3ad3693..da9b7e9 100644
--- a/tcl/JobDB-Executive.tcl
+++ b/tcl/JobDB-Executive.tcl
@@ -85,6 +85,17 @@ proc db-open {} {
 
 if {$dbusers > 0} { incr dbusers; return }
 
+db-ensure-open
+incr dbusers
+}
+proc db-close {} {
+variable dbusers
+incr dbusers -1
+if {$dbusers > 0} return
+if {$dbusers} { error "$dbusers ?!" }
+db-ensure-closed
+}
+proc db-ensure-open {} {
 set pl {
use Osstest;
use Osstest::Executive;
@@ -98,13 +109,8 @@ proc db-open {} {
 # is whitespace-separated.
 regsub -all {;} $db_pg_dsn { } conninfo
 pg_connect -conninfo $conninfo -connhandle dbh
-incr dbusers
 }
-proc db-close {} {
-variable dbusers
-incr dbusers -1
-if {$dbusers > 0} return
-if {$dbusers} { error "$dbusers ?!" }
+proc db-ensure-closed {} {
 pg_disconnect dbh
 }
 
-- 
2.1.4


___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

[Xen-devel] [OSSTEST PATCH 12/33] Database locking: Tcl: Use db-execute-array

[Ian Jackson]

Replace open-coded uses of pg_execute -array ARRAYVAR dbh STMT
with jobdb::db-execute-array ARRAYVAR STMT.

The only functional change is that if OSSTEST_TCL_JOBDB_DEBUG is set,
there will be debugging output.

But we are going to want to make db-execute-array do something more
complicated involving pg_exec.

Signed-off-by: Ian Jackson 
Acked-by: Ian Campbell 
---
 ms-ownerdaemon | 2 +-
 ms-queuedaemon | 4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/ms-ownerdaemon b/ms-ownerdaemon
index 5b52339..cc2f361 100755
--- a/ms-ownerdaemon
+++ b/ms-ownerdaemon
@@ -67,7 +67,7 @@ proc cmd/create-task {chan desc} {
 ( type,  refkey,   refinfo, live)
  VALUES ('ownd', [pg_quote $taskdesc], [clock seconds], 't')
 "
-set nrows [pg_execute -array av dbh "
+set nrows [jobdb::db-execute-array av "
 SELECT taskid
   FROM tasks
  WHERE live AND refkey = [pg_quote $taskdesc]
diff --git a/ms-queuedaemon b/ms-queuedaemon
index 8affacc..6e93288 100755
--- a/ms-queuedaemon
+++ b/ms-queuedaemon
@@ -444,7 +444,7 @@ proc cmd/unwait {chan desc} {
 
 proc for-free-resources {varname body} {
 jobdb::transaction resources {
-   pg_execute -array free_resources_row dbh {
+   jobdb::db-execute-array free_resources_row {
SELECT (restype || '/' || resname || '/' || shareix) AS r
  FROM resources
 WHERE NOT (SELECT live FROM tasks WHERE taskid=owntaskid)
@@ -636,7 +636,7 @@ proc cmd/uptime {chan desc seconds} {
 set descpat "[regsub {\:\d+$} $desc {:%}]"
 transaction resources {
 set keys {}
-pg_execute -array task dbh "
+jobdb::db-execute-array task "
 SELECT * FROM tasks
 WHERE type = 'ownd'
   AND ( refkey LIKE [pg_quote $descpat]
-- 
2.1.4


___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

[Xen-devel] [OSSTEST PATCH 18/33] ms-ownerdaemon: Break out db-reopen, and move it to JobDB-Executive

[Ian Jackson]

Also, change the `puts' to a `logputs'.  No other functional change.

Signed-off-by: Ian Jackson 
---
 ms-ownerdaemon  | 4 +---
 tcl/JobDB-Executive.tcl | 6 ++
 2 files changed, 7 insertions(+), 3 deletions(-)

diff --git a/ms-ownerdaemon b/ms-ownerdaemon
index 62ca645..31e6fdd 100755
--- a/ms-ownerdaemon
+++ b/ms-ownerdaemon
@@ -70,9 +70,7 @@ proc record-dead-tasks {} {
 
 proc record-dead-tasks-retry {} {
 after idle record-dead-tasks
-puts "** reconnecting/retrying **"
-catch { jobdb::db-close }
-jobdb::db-open
+jobdb::db-reopen
 }
 
 proc await-endings-notify {} {
diff --git a/tcl/JobDB-Executive.tcl b/tcl/JobDB-Executive.tcl
index 63db4f0..29ab59a 100644
--- a/tcl/JobDB-Executive.tcl
+++ b/tcl/JobDB-Executive.tcl
@@ -109,6 +109,12 @@ proc db-close {} {
 pg_disconnect dbh
 }
 
+proc db-reopen {} {
+logputs stdout "** reopening database **"
+catch { db-close }
+db-open
+}
+
 proc db-update-1 {stmt} {
 # must be in transaction
 set nrows [db-execute $stmt]
-- 
2.1.4


___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

[Xen-devel] [OSSTEST PATCH 27/33] tcl daemons: transaction: Properly match db-open and db-close

[Ian Jackson]

* Do the db-open inside the catch, so that if it fails we do the
  rest of the error handling.

* Do the db-close before deconstructing the error, so that we
  necessarily get the db-open reference count right.

Signed-off-by: Ian Jackson 
---
 tcl/JobDB-Executive.tcl | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tcl/JobDB-Executive.tcl b/tcl/JobDB-Executive.tcl
index 3c2b4db..fe1c946 100644
--- a/tcl/JobDB-Executive.tcl
+++ b/tcl/JobDB-Executive.tcl
@@ -306,10 +306,10 @@ proc step-set-status {flight job stepno st} {
 proc transaction {tables script} {
 global errorInfo errorCode
 set retries 100
-db-open
 while 1 {
 set ol {}
set rc [catch {
+   db-open
db-execute BEGIN
db-execute "SET TRANSACTION ISOLATION LEVEL SERIALIZABLE"
lock-tables $tables
@@ -318,6 +318,7 @@ proc transaction {tables script} {
} result]
set ei $errorInfo
set ec $errorCode
+   db-close
if {$rc} {
db-execute ROLLBACK
switch -glob $errorCode {
@@ -333,7 +334,6 @@ proc transaction {tables script} {
}
}
}
-db-close
return -code $rc -errorinfo $ei -errorcode $ec $result
 }
 }
-- 
2.1.4


___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

[Xen-devel] [OSSTEST PATCH 24/33] tcl daemons: make db-reopen actually work

[Ian Jackson]

Even if the refcount is >0, we want to actually reconnect.
Also, log something if the close fails.

Signed-off-by: Ian Jackson 
---
 tcl/JobDB-Executive.tcl | 7 +--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/tcl/JobDB-Executive.tcl b/tcl/JobDB-Executive.tcl
index 1763b69..6c7e067 100644
--- a/tcl/JobDB-Executive.tcl
+++ b/tcl/JobDB-Executive.tcl
@@ -125,9 +125,12 @@ proc db-ensure-closed {} {
 }
 
 proc db-reopen {} {
+variable dbusers
 logputs stdout "** reopening database **"
-catch { db-close }
-db-open
+if {[catch { db-ensure-closed } emsg]} {
+   logputs stdout "(db disconnect: $emsg)"
+}
+if {$dbusers > 0} db-ensure-open
 }
 
 proc db-update-1 {stmt} {
-- 
2.1.4


___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

[Xen-devel] [OSSTEST PATCH 05/33] mg-schema-test-database: Make `daemons' be cleverer

[Ian Jackson]

Now you can tell it which daemons to run.  This is helpful if you want
to run them separately.

Signed-off-by: Ian Jackson 
---
 mg-schema-test-database | 35 +--
 1 file changed, 25 insertions(+), 10 deletions(-)

diff --git a/mg-schema-test-database b/mg-schema-test-database
index 85cc050..e95b6c5 100755
--- a/mg-schema-test-database
+++ b/mg-schema-test-database
@@ -27,9 +27,14 @@
 # deletes your test database and removes the local-config file
 #
 #
-#  ./mg-schema-test-database daemons [_SUFFIX]
+#  ./mg-schema-test-database daemons [_SUFFIX] [DAEMON...]
 #
-# synchronously runs owner and queue daemons for your test database
+# Synchronously runs owner and queue daemons for your test database.
+# If any DAEMON is specified, runs only those daemons.  DAEMON
+# may be `queue' or `owner'; otherwise it is a command which will
+# be broken at spaces, and have `./' prepended if it contains no `/'.
+# If any DAEMON contains the string `queue', it causes the data-plan.pl
+# to be cleared.
 #
 # NB that you can't drop a test database with these daemons running,
 # because Postgres will refuse to drop a database that anyone is
@@ -557,26 +562,36 @@ END
 #== DAEMONS ==
 
 daemons)
+   wantdaemons=()
+
for arg in "$@"; do
case "$arg" in
_*) suffix="$arg" ;;
-   *)  fail 'bad usage' ;;
+   owner|queue)wantdaemons+=("./ms-${arg}daemon") ;;
+   */*)wantdaemons+=("$arg") ;;
+   *)  wantdaemons+=("./$arg") ;;
esac
done
 
+   if [ "${#wantdaemons[*]}" = 0 ]; then
+   wantdaemons=(./ms-ownerdaemon ./ms-queuedaemon)
+   fi
+
dbname
 
-   printf "Running daemons for %s\n" "$dbname"
+   printf "Running daemons (${wantdaemons[*]}) for %s\n" "$dbname"
 
-   withtest \
-   exec_resetting_sigint ./ms-ownerdaemon &
+   for d in "${wantdaemons[@]}"; do
 
-   sleep 1
+   case "$d" in
+   *queuedaemon*)  rm -f data-plan.pl  ;;
+   esac
 
-   withtest \
-   exec_resetting_sigint ./ms-queuedaemon &
+   withtest \
+   exec_resetting_sigint $d &
 
-   rm -f data-plan.pl
+   sleep 1
+   done
 
wait
 
-- 
2.1.4


___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

[Xen-devel] [OSSTEST PATCH 03/33] mg-schema-test-database: Direct logs to local directory

[Ian Jackson]

Do not pollute a shared log area with logs of flights whose numbers
are valid only in the context of our test database.

Signed-off-by: Ian Jackson 
CC: Wei Liu 
---
 mg-schema-test-database | 8 
 1 file changed, 8 insertions(+)

diff --git a/mg-schema-test-database b/mg-schema-test-database
index 4e0ee68..88a75cf 100755
--- a/mg-schema-test-database
+++ b/mg-schema-test-database
@@ -338,6 +338,14 @@ QueueDaemonPort ${ctrlports#*,}
 ExecutiveDbOwningRoleRegexp .*
 QueueDaemonHoldoff 3
 QueueDaemonRetry 5
+Logs $PWD/logs
+Stash $PWD/logs
+Results $PWD/results
+Publish=undef
+LogsPublish=undef
+ResultsPublish=undef
+HarnessPublishGitUserHost=undef
+HarnessPublishGitRepoDir=undef
 END
mv -f $tcfg.tmp $tcfg
 
-- 
2.1.4


___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

[Xen-devel] [OSSTEST PATCH 08/33] Tcl: Use tclsh8.5

[Ian Jackson]

I have checked that tclsh8.5 and TclX work on osstest.test-lab (and
also osstest.xs.citrite.net).  TclX seems to be provided by tcl8.4 but
work with tcl8.5 (at least on wheezy and jessie).

Deployment note: hosts running Debian wheezy (including
osstest.xs.citrite.net, the Citrix Cambridge instance), will need
OSSTEST_DAEMON_TCLSH=tclsh8.4 in ~/.xen-osstest/settings.

Signed-off-by: Ian Jackson 
---
 README| 2 +-
 ms-ownerdaemon| 2 +-
 ms-queuedaemon| 2 +-
 ms-reportuptime   | 2 +-
 sg-execute-flight | 2 +-
 sg-run-job| 2 +-
 6 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/README b/README
index 0a346dc..3aa35d2 100644
--- a/README
+++ b/README
@@ -246,7 +246,7 @@ To run osstest in standalone mode:
 
  - You need to install
  sqlite3
- tcl8.4 tclx8.4 libsqlite3-tcl
+ tcl8.5 tclx8.4 libsqlite3-tcl
  libdbi-perl libdbd-sqlite3-perl
  pax rsync
  curl
diff --git a/ms-ownerdaemon b/ms-ownerdaemon
index 33ee238..5b52339 100755
--- a/ms-ownerdaemon
+++ b/ms-ownerdaemon
@@ -1,4 +1,4 @@
-#!/usr/bin/tclsh8.4
+#!/usr/bin/tclsh8.5
 # -*- Tcl -*- 
 # usage: ./ms-ownerdaemon  ... | logger
 
diff --git a/ms-queuedaemon b/ms-queuedaemon
index 2b8d621..8affacc 100755
--- a/ms-queuedaemon
+++ b/ms-queuedaemon
@@ -1,4 +1,4 @@
-#!/usr/bin/tclsh8.4
+#!/usr/bin/tclsh8.5
 # -*- Tcl -*- 
 # usage: ./ms-queuedaemon  ... | logger
 
diff --git a/ms-reportuptime b/ms-reportuptime
index aeac483..fe72b78 100755
--- a/ms-reportuptime
+++ b/ms-reportuptime
@@ -1,4 +1,4 @@
-#!/usr/bin/tclsh8.4
+#!/usr/bin/tclsh8.5
 # -*- Tcl -*- 
 # usage: ./ms-reportuptime
 
diff --git a/sg-execute-flight b/sg-execute-flight
index 4e3fcf2..27fc04c 100755
--- a/sg-execute-flight
+++ b/sg-execute-flight
@@ -1,4 +1,4 @@
-#!/usr/bin/tclsh8.4
+#!/usr/bin/tclsh8.5
 # -*- Tcl -*- 
 # usage: ./sg-execute-flight FLIGHT BLESSING
 
diff --git a/sg-run-job b/sg-run-job
index 8b2d5e1..920834a 100755
--- a/sg-run-job
+++ b/sg-run-job
@@ -1,4 +1,4 @@
-#!/usr/bin/tclsh8.4
+#!/usr/bin/tclsh8.5
 
 # This is part of "osstest", an automated testing framework for Xen.
 # Copyright (C) 2009-2013 Citrix Inc.
-- 
2.1.4


___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

[Xen-devel] [OSSTEST PATCH 17/33] ms-ownerdaemon: Cope with db restart. Retry recording dead tasks.

[Ian Jackson]

In chan-destroy-stuff, instead of accessing the db directly, add the
dead task(s) to a queue, and arrange to look at that queue.

Errors are handled by setting an `after' handler which we cancel if we
are successful.

The after handler requeues a queue run attempt as the first thing
(which will arrange that a further retry will occur if things are
still broken) and then attempts to reconnect to the database.

I have tested this with a test instance by renaming the `tasks' table
under its feet, and it functions as expected.

DEPLOYMENT NOTE: The owner daemon cannot be restarted without shutting
everything down.  So this update should first be deployed in
Cambridge, probably, to see how it goes.  Also, it is less critical in
the main Xen production test lab because there the db and the owner
daemon are co-hosted on the same VM.

Signed-off-by: Ian Jackson 
Acked-by: Ian Campbell 
---
v2: Put back the `unset tasks' which was mistakenly removed.  The
effect of its lack is to fail to clear out the task list for
previous uses of the channel (which is named after the fd); this
is mostly harmless apart from log spam but causes the usual
case to be something like
   OK created-task 456354 ownd [10.80.227.94]:44852-876
rather than
   OK created-task 456354 ownd [10.80.227.94]:44852-876
which some of the clients (rightly) don't expect.
---
 Osstest/Executive.pm |  1 +
 ms-ownerdaemon   | 38 ++
 2 files changed, 35 insertions(+), 4 deletions(-)

diff --git a/Osstest/Executive.pm b/Osstest/Executive.pm
index 468031c..0602925 100644
--- a/Osstest/Executive.pm
+++ b/Osstest/Executive.pm
@@ -113,6 +113,7 @@ augmentconfigdefaults(
 augmentconfigdefaults(
 OwnerDaemonHost => $c{ControlDaemonHost},
 QueueDaemonHost => $c{ControlDaemonHost},
+OwnerDaemonDbRetry => $c{QueueDaemonRetry},
 );
 
 #-- configuration reader etc. --
diff --git a/ms-ownerdaemon b/ms-ownerdaemon
index 3623d19..62ca645 100755
--- a/ms-ownerdaemon
+++ b/ms-ownerdaemon
@@ -22,16 +22,38 @@
 source ./tcl/daemonlib.tcl
 
 
+set dead_tasks {}
+
 proc chan-destroy-stuff {chan} {
+global dead_tasks
+
 upvar #0 chanawait($chan) await
 catch { unset await }
 
 upvar #0 chantasks($chan) tasks
 if {![info exists tasks]} return
 
+puts-chan-desc $chan "-- $tasks"
+
+foreach task $tasks {
+   lappend dead_tasks $task
+}
+unset tasks
+after idle record-dead-tasks
+}
+
+proc record-dead-tasks {} {
+global c dead_tasks
+
+if {![llength $dead_tasks]} return
+
+puts "record-dead-tasks ... $dead_tasks"
+
+set retry [expr {$c(OwnerDaemonDbRetry) * 1000}]
+set eafter [after $retry record-dead-tasks-retry]
+
 jobdb::transaction resources {
-puts-chan-desc $chan "-- $tasks"
-foreach task $tasks {
+foreach task $dead_tasks {
 jobdb::db-execute "
 UPDATE tasks
SET live = 'f'
@@ -39,12 +61,20 @@ proc chan-destroy-stuff {chan} {
 "
 }
 }
-puts-chan-desc $chan "== $tasks"
-unset tasks
 
+after cancel $eafter
+puts "record-dead-tasks OK. $dead_tasks"
+set dead_tasks {}
 after idle await-endings-notify
 }
 
+proc record-dead-tasks-retry {} {
+after idle record-dead-tasks
+puts "** reconnecting/retrying **"
+catch { jobdb::db-close }
+jobdb::db-open
+}
+
 proc await-endings-notify {} {
 global chanawait
 foreach chan [array names chanawait] {
-- 
2.1.4


___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

[Xen-devel] [OSSTEST PATCH 06/33] mg-schema-test-database: Change default minflight to -100

[Ian Jackson]

It is tiresome to try to create a test db for playing with and have to
wait for a big copy.  Better to create a small one by default; if the
user has forgotten to specify a minflight, they can always drop it and
run it again.

Signed-off-by: Ian Jackson 
---
 mg-schema-test-database | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/mg-schema-test-database b/mg-schema-test-database
index e95b6c5..892e41a 100755
--- a/mg-schema-test-database
+++ b/mg-schema-test-database
@@ -230,7 +230,7 @@ create)
#-- argument parsing --
 
tasks=''
-   minflight=-1000
+   minflight=-100
for arg in "$@"; do
case "$arg" in
*@*)
-- 
2.1.4


___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

[Xen-devel] [OSSTEST PATCH 30/33] tcl daemons: transaction: Support db autoreconnect

[Ian Jackson]

Provide an `autoreconnect' argument which will automatically reconnect
to the db if the connection has been lost.  It will make only one
reconnection attempt.

No functional change yet because no call sites have been changed.

Signed-off-by: Ian Jackson 
---
 tcl/JobDB-Executive.tcl | 16 +++-
 1 file changed, 15 insertions(+), 1 deletion(-)

diff --git a/tcl/JobDB-Executive.tcl b/tcl/JobDB-Executive.tcl
index 535fbd7..0fc0a6d 100644
--- a/tcl/JobDB-Executive.tcl
+++ b/tcl/JobDB-Executive.tcl
@@ -303,7 +303,7 @@ proc step-set-status {flight job stepno st} {
 }
 }
 
-proc transaction {tables script} {
+proc transaction {tables script {autoreconnect 0}} {
 global errorInfo errorCode
 set retries 100
 while 1 {
@@ -332,6 +332,20 @@ proc transaction {tables script} {
after 500
continue
}
+   {OSSTEST-PSQL * 08*} -
+   {OSSTEST-PSQL * 57*} -
+   {OSSTEST-PSQL-SSL-SYSCALL *} {
+   # Class 08 — Connection Exception
+   # Class 57 — Operator Intervention
+   #(includes various shutdowns)
+   logputs stderr \
+ "db connection exception (autoreconnect=$autoreconnect): $result\n$ei\n$ec\n"
+   if {$autoreconnect} {
+   set autoreconnect 0; # try this only once
+   db-reopen
+   continue
+   }
+   }
}
catch { db-ensure-closed }
}
-- 
2.1.4


___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

[Xen-devel] [OSSTEST PATCH 21/33] tcl daemons: Remove obsolete `global g'

[Ian Jackson]

The global g was removed in 2012 in beb4240a346e "wip reorg,
testing..." but this instance seems to have escaped.

Signed-off-by: Ian Jackson 
---
 tcl/JobDB-Executive.tcl | 1 -
 1 file changed, 1 deletion(-)

diff --git a/tcl/JobDB-Executive.tcl b/tcl/JobDB-Executive.tcl
index c1e5c63..3ad3693 100644
--- a/tcl/JobDB-Executive.tcl
+++ b/tcl/JobDB-Executive.tcl
@@ -81,7 +81,6 @@ proc set-flight {} {
 variable dbusers 0
 
 proc db-open {} {
-global g
 variable dbusers
 
 if {$dbusers > 0} { incr dbusers; return }
-- 
2.1.4


___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

[Xen-devel] [OSSTEST PATCH 15/33] Database locking: Tcl: for errorCode, use pg_exec, not pg_execute

[Ian Jackson]

We would like to be able to retry db transactions.  To do this we need
to know why they failed (if they did).

But pg_execute does not set errorCode.  (This is clearly a bug.)  And
since it immediately discards a failed statement, any error
information has been lost by the time pg_execute returns.

So, instead, use pg_exec, and manually mess about with fishing
suitable information out of a failed statement handle, and generating
an appropriate errorCode.

There are no current consumers of this errorCode: that will come in a
moment.

A wrinkle is that as a result it is no longer possible to use
db-execute on a SELECT statement nor db-execute-array on a non-SELECT
statement.  This is because 1. the `ok' status that we have to
check for is different for statements which are commands and ones
which return tuples and 2. we need to fish a different return value out
of the statement handle (-cmdTuples vs -numTuples).  But all uses in
the codebase are now fine for this distinction.

Signed-off-by: Ian Jackson 
Acked-by: Ian Campbell 
---
 tcl/JobDB-Executive.tcl | 54 ++---
 1 file changed, 51 insertions(+), 3 deletions(-)

diff --git a/tcl/JobDB-Executive.tcl b/tcl/JobDB-Executive.tcl
index bbce6fc..ed9abbb 100644
--- a/tcl/JobDB-Executive.tcl
+++ b/tcl/JobDB-Executive.tcl
@@ -121,13 +121,61 @@ proc db-execute-debug {stmt} {
puts stderr "EXECUTING >$stmt<"
 }
 }
+
+proc db--exec-check {shvar stmt expected_status body} {
+# pg_execute does not set errorCode and it throws away the
+# statement handle so we can't get the error out.  So
+# use pg_exec, as wrapped up here.
+
+# db--exec-check executes stmt and checks that the status is
+# `expected_status'.  If OK, executes body with $shvar set to the
+# stmt handle.   Otherwise throws with errorCode
+#   {OSSTEST-PSQL  }
+
+global errorInfo errorCode
+upvar 1 $shvar sh
+
+set sh [pg_exec dbh $stmt]
+
+set rc [catch {
+   set status [pg_result $sh -status]
+   if {[string compare $status $expected_status]} {
+   set emsg [pg_result $sh -error]
+   set sqlstate [pg_result $sh -error sqlstate]
+   if {![string length $emsg]} {
+   set emsg "osstest expected status $expected_status got $status"
+   }
+   set context [pg_result $sh -error context]
+   error $emsg \
+   "while executing SQL\n$stmt\nin SQL context\n$context" \
+   [list OSSTEST-PSQL $status $sqlstate]
+   }
+   uplevel 1 $body
+} emsg]
+
+set ei $errorInfo
+set ec $errorCode
+catch { pg_result $sh -clear }
+
+return -code $rc -errorinfo $ei -errorcode $ec $emsg
+}
+
 proc db-execute {stmt} {
 db-execute-debug $stmt
-uplevel 1 [list pg_execute dbh $stmt]
+db--exec-check sh $stmt PGRES_COMMAND_OK {
+   return [pg_result $sh -cmdTuples]
+}
 }
-proc db-execute-array {arrayvar stmt args} {
+proc db-execute-array {arrayvar stmt {body {}}} {
 db-execute-debug $stmt
-uplevel 1 [list pg_execute -array $arrayvar dbh $stmt] $args
+db--exec-check sh $stmt PGRES_TUPLES_OK {
+   set nrows [pg_result $sh -numTuples]
+   for {set row 0} {$row < $nrows} {incr row} {
+   uplevel 1 [list pg_result $sh -tupleArray $row $arrayvar]
+   uplevel 1 $body
+   }
+   return $nrows
+}
 }
 
 proc lock-tables {tables} {
-- 
2.1.4


___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

[Xen-devel] [OSSTEST PATCH 29/33] tcl daemons: transaction: Only try ROLLBACK when necessary

[Ian Jackson]

In the deadlock case, we need to ROLLBACK.  In other error cases we
are going to close the connection.  And in those other cases the
ROLLBACK might fail, causing our error recovery to go wrong.

So do ROLLBACK only on the single path where we might continue to use
the connection.

Signed-off-by: Ian Jackson 
---
 tcl/JobDB-Executive.tcl | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tcl/JobDB-Executive.tcl b/tcl/JobDB-Executive.tcl
index c0cd4e9..535fbd7 100644
--- a/tcl/JobDB-Executive.tcl
+++ b/tcl/JobDB-Executive.tcl
@@ -320,10 +320,10 @@ proc transaction {tables script} {
set ec $errorCode
db-close
if {$rc} {
-   db-execute ROLLBACK
switch -glob $errorCode {
{OSSTEST-PSQL * 40P01} {
# DEADLOCK DETECTED
+   db-execute ROLLBACK
logputs stdout "transaction deadlock ($result) retrying ..."
if {[incr retries -1] <= 0} {
error \
-- 
2.1.4


___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

[Xen-devel] [OSSTEST PATCH 32/33] tcl daemons: Provide with-db

[Ian Jackson]

This makes it easier to get the matching of db-open and db-close right.

Signed-off-by: Ian Jackson 
---
 tcl/JobDB-Executive.tcl | 16 
 1 file changed, 16 insertions(+)

diff --git a/tcl/JobDB-Executive.tcl b/tcl/JobDB-Executive.tcl
index 0fc0a6d..07a0438 100644
--- a/tcl/JobDB-Executive.tcl
+++ b/tcl/JobDB-Executive.tcl
@@ -303,6 +303,22 @@ proc step-set-status {flight job stepno st} {
 }
 }
 
+proc with-db {script} {
+global errorInfo errorCode
+set rc [catch {
+   db-open
+   uplevel 1 $script
+} result]
+set ei $errorInfo
+set ec $errorCode
+if {$rc} {
+   catch { db-close }
+} else {
+   db-close
+}
+return -code $rc -errorinfo $ei -errorcode $ec $result
+}
+
 proc transaction {tables script {autoreconnect 0}} {
 global errorInfo errorCode
 set retries 100
-- 
2.1.4


___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

[Xen-devel] [OSSTEST PATCH 33/33] tcl daemons: Use with-db

[Ian Jackson]

Fixes a number of latent bugs where the jobdb refcount might get out
of step.

Signed-off-by: Ian Jackson 
---
 sg-execute-flight   |  82 ++---
 tcl/JobDB-Executive.tcl | 106 
 2 files changed, 93 insertions(+), 95 deletions(-)

diff --git a/sg-execute-flight b/sg-execute-flight
index 14c8cda..7932c29 100755
--- a/sg-execute-flight
+++ b/sg-execute-flight
@@ -28,48 +28,48 @@ proc check {} {
 
 if {$stopping} return
 
-jobdb::db-open
-
-set nqueued [jobdb::db-execute-array dummy "
-SELECT job FROM jobs j
- WHERE j.flight = $flight
-   AND j.status = 'queued'
- LIMIT 1
-"]
-
-set nrunning [llength $running]
-log "flight $flight nqueued=$nqueued nrunning=$nrunning"
-
-if {!$nqueued && !$nrunning} {
-prequit finished
-exec ./cs-flight-bless $flight $blessing running 2>@ stderr
-exit 0
-}
+jobdb::with-db {
+
+   set nqueued [jobdb::db-execute-array dummy "
+   SELECT job FROM jobs j
+WHERE j.flight = $flight
+  AND j.status = 'queued'
+LIMIT 1
+   "]
+
+   set nrunning [llength $running]
+   log "flight $flight nqueued=$nqueued nrunning=$nrunning"
+
+   if {!$nqueued && !$nrunning} {
+   prequit finished
+   exec ./cs-flight-bless $flight $blessing running 2>@ stderr
+   exit 0
+   }
+
+   jobdb::db-execute-array jobinfo "
+   SELECT * FROM jobs j
+WHERE j.flight = $flight
+  AND j.status = 'queued'
+  AND 0 = (SELECT count(*) FROM jobs d
+WHERE d.flight = $flight
+  AND ( d.status = 'queued'
+ OR d.status = 'preparing'
+ OR d.status = 'running'
+ OR d.status = 'retriable' )
+  AND (d.job IN (SELECT val FROM runvars r
+WHERE r.flight = $flight
+  AND r.job = j.job
+  AND r.name LIKE '%job')
+   OR (d.flight || '.' || d.job) IN
+(SELECT val FROM runvars r
+WHERE r.flight = $flight
+  AND r.job = j.job
+  AND r.name LIKE '%job'))
+  )
+ORDER BY job
+   " maybe-spawn-job
 
-jobdb::db-execute-array jobinfo "
-SELECT * FROM jobs j
- WHERE j.flight = $flight
-   AND j.status = 'queued'
-   AND 0 = (SELECT count(*) FROM jobs d
- WHERE d.flight = $flight
-   AND ( d.status = 'queued'
-  OR d.status = 'preparing'
-  OR d.status = 'running'
-  OR d.status = 'retriable' )
-   AND (d.job IN (SELECT val FROM runvars r
- WHERE r.flight = $flight
-   AND r.job = j.job
-   AND r.name LIKE '%job')
-OR (d.flight || '.' || d.job) IN
- (SELECT val FROM runvars r
- WHERE r.flight = $flight
-   AND r.job = j.job
-   AND r.name LIKE '%job'))
-   )
- ORDER BY job
-" maybe-spawn-job
-
-jobdb::db-close
+}
 }
 
 proc prequit {why} {
diff --git a/tcl/JobDB-Executive.tcl b/tcl/JobDB-Executive.tcl
index 07a0438..0f450c1 100644
--- a/tcl/JobDB-Executive.tcl
+++ b/tcl/JobDB-Executive.tcl
@@ -28,36 +28,36 @@ proc logputs {f m} {
 
 proc prepare {job} {
 global flight jobinfo
-db-open
-set found 0
-db-execute-array jobinfo "
-SELECT job, status, recipe FROM jobs
-   WHERE   flight = [pg_quote $flight]
-   AND job = [pg_quote $job]
-" {
-   switch -exact -- $jobinfo(status) {
-   queued - preparing - retriable - play { incr found }
-   default {
-   error "job $flight.$job status $jobinfo(status)"
+with-db {
+   set found 0
+   db-execute-array jobinfo "
+   SELECT job, status, recipe FROM jobs
+   WHERE   flight = [pg_quote $flight]
+   AND job = [pg_quote $job]
+   " {
+   switch -exact -- $jobinfo(status) {
+   queued - preparing - retriable - play { incr found }
+   default {
+   error "job $flight.$job status $jobinfo(status)"
+   }
}
}
-}
-if {!$found} {
-   error "job 

[Xen-devel] [OSSTEST PATCH 10/33] ms-planner: Support ClientNotes

[Ian Jackson]

No users yet.

Signed-off-by: Ian Jackson 
CC: Wei Liu 
---
 README.planner | 22 ++
 ms-planner | 23 +++
 2 files changed, 45 insertions(+)

diff --git a/README.planner b/README.planner
index daccef5..80c2506 100644
--- a/README.planner
+++ b/README.planner
@@ -286,6 +286,13 @@ Plan is:
Live
Job flight.job which made the allocation
 
+   ClientNotes
+   map from note key
+   to one of
+   scalar
+   array of values supplied by clients
+   hash of keys/values supplied by clients
+
* = internal to plan
** = computed by launder_check_plan
+ = as shown to clients
@@ -302,6 +309,12 @@ Booking list is:
Share   optional struct containing
Type
Shares
+   [ClientNotes]
+   as for Plan; merged by
+   for scalar, updating from Booking if specified
+   for array, appending Booking to Plan
+   for hash, updating individual values from Booking
+   type must not have changed!
 
 Sharing resources:
 
@@ -313,3 +326,12 @@ Bookings which do not create the share do not mention the 
master.
 
 Note that whether a resource is free, or simply nonexistent, is not
 represented.
+
+
+Note keys
+-
+
+Note key name (key in ClientNotes):
+   Note value format:
+
+(none yet defined)
diff --git a/ms-planner b/ms-planner
index abf5fac..bce6e13 100755
--- a/ms-planner
+++ b/ms-planner
@@ -245,6 +245,7 @@ sub cmd_reset () {
 $plan->{Start}= time;
 $plan->{Events}= { };
 $plan->{Unprocessed}= [ ];
+$plan->{ClientNotes}= { };
 
 my %magictask;
 foreach my $taskrefkey (qw(preparing shared)) {
@@ -415,6 +416,9 @@ sub cmd_get_plan () {
}
$jplan->{Events}{$reso}= \@jevts;
 }
+
+$jplan->{ClientNotes} = $plan->{ClientNotes};
+
 print to_json($jplan),"\n" or die $!;
 }
 
@@ -529,6 +533,25 @@ sub cmd_book_resources () {
  );
 }
 
+my $jnotes = $jbookings->{ClientNotes} // { };
+foreach my $k (sort keys %$jnotes) {
+   my $v = $jnotes->{$k};
+   my $newt = ref $v // '(none)';
+   if (exists $plan->{ClientNotes}{$k}) {
+   my $oldt = ref $plan->{ClientNotes}{$k} // '(none)';
+   die "$k $oldt -> $newt" unless $oldt eq $newt;
+   }
+   if (!ref $v) {
+   $plan->{ClientNotes}{$k} = $v;
+   } elsif (ref $v eq 'HASH') {
+   $plan->{ClientNotes}{$k}{$_} = $v->{$_} foreach keys %$v;
+   } elsif (ref $v eq 'ARRAY') {
+   push @{ $plan->{ClientNotes}{$_} }, @{ $jnotes->{$_} };
+   } else {
+   die "$k $newt";
+   }
+}
+
 check_write_new_plan();
 }
 
-- 
2.1.4


___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

[Xen-devel] [OSSTEST PATCH 31/33] tcl-daemons: ms-ownerdaemon: Use autoreconnect

[Ian Jackson]

When creating a job, we ask jobdb::transaction to retry on failure.
This makes the error handling more effective.

Signed-off-by: Ian Jackson 
---
 ms-ownerdaemon | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/ms-ownerdaemon b/ms-ownerdaemon
index 31e6fdd..bf0b595 100755
--- a/ms-ownerdaemon
+++ b/ms-ownerdaemon
@@ -102,7 +102,7 @@ proc cmd/create-task {chan desc} {
 "]
 if {$nrows != 1} { error "multiple $taskdesc!" }
 set task $av(taskid)
-}
+} 1 ;#autoreconnect
 lappend tasks $task
 puts-chan $chan "OK created-task $task ownd $taskdesc"
 }
-- 
2.1.4


___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

[Xen-devel] [OSSTEST PATCH 00/33] Database locking and retry

[Ian Jackson]

I've just pushed this to production pretest, since I'm not really
expecting anyone else to review it.  But it ought to have some
visibility, I guess.

This series contains:
 * Overhaul of database locking in Tcl daemons to improve
   concurrency and correctness
 * Overhaul of database error handling in Tcl daemons to improve
   survivability after database restart
 * Update to use Tcl 8.5 (needed for jessie)
 * New planner feature ClientNotes (to support diverse hostalloc flag)
 * Fixes to command line management utilities
 * etc.

Thanks for your (in)attention :-).

Ian.

___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

[Xen-devel] [OSSTEST PATCH 23/33] tcl daemons: db-ensure-open, -close: Make idempotent

[Ian Jackson]

Track whether we think the connection is open in dbopen.

It is now therefore OK to call db-ensure-open and db-ensure-closed in
other contexts.

Signed-off-by: Ian Jackson 
---
 tcl/JobDB-Executive.tcl | 10 ++
 1 file changed, 10 insertions(+)

diff --git a/tcl/JobDB-Executive.tcl b/tcl/JobDB-Executive.tcl
index da9b7e9..1763b69 100644
--- a/tcl/JobDB-Executive.tcl
+++ b/tcl/JobDB-Executive.tcl
@@ -79,6 +79,7 @@ proc set-flight {} {
 }
 
 variable dbusers 0
+variable dbopen 0 ;# 1 means known to be open and good; 0 might mean broken
 
 proc db-open {} {
 variable dbusers
@@ -96,6 +97,11 @@ proc db-close {} {
 db-ensure-closed
 }
 proc db-ensure-open {} {
+variable dbopen
+
+if {$dbopen} return
+catch { db-ensure-closed } ;# clean up any detritus
+
 set pl {
use Osstest;
use Osstest::Executive;
@@ -109,8 +115,12 @@ proc db-ensure-open {} {
 # is whitespace-separated.
 regsub -all {;} $db_pg_dsn { } conninfo
 pg_connect -conninfo $conninfo -connhandle dbh
+
+set dbopen 1
 }
 proc db-ensure-closed {} {
+variable dbopen
+set dbopen 0
 pg_disconnect dbh
 }
 
-- 
2.1.4


___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

[Xen-devel] [OSSTEST PATCH 04/33] mg-schema-test-database: Prepare for `daemons' to be cleverer

[Ian Jackson]

We are going to want to be able to specify daemons individually.

Replace the call to parse_only_suffix so that we have somewhere to
parse extra arguments.  No functional change yet.

Signed-off-by: Ian Jackson 
---
 mg-schema-test-database | 7 ++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/mg-schema-test-database b/mg-schema-test-database
index 88a75cf..85cc050 100755
--- a/mg-schema-test-database
+++ b/mg-schema-test-database
@@ -557,7 +557,12 @@ END
 #== DAEMONS ==
 
 daemons)
-   parse_only_suffix "$@"
+   for arg in "$@"; do
+   case "$arg" in
+   _*) suffix="$arg" ;;
+   *)  fail 'bad usage' ;;
+   esac
+   done
 
dbname
 
-- 
2.1.4


___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

[Xen-devel] [OSSTEST PATCH 26/33] tcl daemons: Recognise `SSL SYSCALL' errors with their own errorCode

[Ian Jackson]

This has no real effect right now but will be useful in a moment.

Signed-off-by: Ian Jackson 
---
 tcl/JobDB-Executive.tcl | 9 -
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/tcl/JobDB-Executive.tcl b/tcl/JobDB-Executive.tcl
index 8f4ed98..3c2b4db 100644
--- a/tcl/JobDB-Executive.tcl
+++ b/tcl/JobDB-Executive.tcl
@@ -170,9 +170,16 @@ proc db--exec-check {shvar stmt expected_status body} {
set emsg "osstest expected status $expected_status got $status"
}
set context [pg_result $sh -error context]
+   set ecode OSSTEST-PSQL
+   if {![string length $sqlstate] &&
+   [string match {SSL SYSCALL *} $emsg]} {
+   # sadly the pg client library doesn't provide a code
+   # for this so we match the error message
+   set ecode OSSTEST-PSQL-SSL-SYSCALL
+   }
error "db exec failed ($status, $sqlstate) $emsg" \
"while executing SQL\n$stmt\nin SQL context\n$context" \
-   [list OSSTEST-PSQL $status $sqlstate]
+   [list $ecode $status $sqlstate]
}
uplevel 1 $body
 } emsg]
-- 
2.1.4


___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

[Xen-devel] [OSSTEST PATCH 28/33] tcl daemons: if error occurs, ensure db is closed afterwards

[Ian Jackson]

When an error occurs which we are not handling, we want to
unconditionally close the db connection.  The next transaction will
reopen it.

Signed-off-by: Ian Jackson 
---
 tcl/JobDB-Executive.tcl | 1 +
 1 file changed, 1 insertion(+)

diff --git a/tcl/JobDB-Executive.tcl b/tcl/JobDB-Executive.tcl
index fe1c946..c0cd4e9 100644
--- a/tcl/JobDB-Executive.tcl
+++ b/tcl/JobDB-Executive.tcl
@@ -333,6 +333,7 @@ proc transaction {tables script} {
continue
}
}
+   catch { db-ensure-closed }
}
return -code $rc -errorinfo $ei -errorcode $ec $result
 }
-- 
2.1.4


___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

[Xen-devel] [OSSTEST PATCH 20/33] tcl daemons: jobdb::transaction: Improve two message generation sites

[Ian Jackson]

* Use logputs rather than puts to report transaction deadlock retry

* Use $ei and $ec rather than $errorInfo and $errorCode when calling
  error due to too many deadlock retries.  This has no functional change
  but is less fragile in case of future addition of new calls to catch
  between the main catch and this throw.

Signed-off-by: Ian Jackson 
---
 tcl/JobDB-Executive.tcl | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tcl/JobDB-Executive.tcl b/tcl/JobDB-Executive.tcl
index f2322c4..c1e5c63 100644
--- a/tcl/JobDB-Executive.tcl
+++ b/tcl/JobDB-Executive.tcl
@@ -298,10 +298,10 @@ proc transaction {tables script} {
switch -glob $errorCode {
{OSSTEST-PSQL * 40P01} {
# DEADLOCK DETECTED
-   puts "transaction deadlock ($result) retrying ..."
+   logputs stdout "transaction deadlock ($result) retrying ..."
if {[incr retries -1] <= 0} {
error \
- "transaction failed, too many retries: $result\n$errorInfo\n$errorCode\n"
+ "transaction failed, too many retries: $result\n$ei\n$ec\n"
}
after 500
continue
-- 
2.1.4


___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

[Xen-devel] [OSSTEST PATCH 01/33] mg-allocate: Fix "issteallable" call

[Ian Jackson]

81cac5a1656e "mg-allocate: Support --steal" introduced an erroneous
call to the subref $issteallable, using { } instead of ( ), producing
this error:
  Not a HASH reference at ./mg-allocate line 225.

Signed-off-by: Ian Jackson 
---
 mg-allocate | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/mg-allocate b/mg-allocate
index 42e35b3..d295997 100755
--- a/mg-allocate
+++ b/mg-allocate
@@ -222,7 +222,7 @@ END
 next if $isallocatable->($sharerow);
 next if $sharerow->{taskid} == $tid;
 next if $sharerow->{taskid} == $magictask{preparing};
-   next if $isstealable->{$sharerow};
+   next if $isstealable->($sharerow);
 logm("$desc: shared, $sharerow->{shareix} locked by ".
  $findowner->($sharerow));
 $allshareok= 0;
-- 
2.1.4


___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

[Xen-devel] [OSSTEST PATCH 25/33] tcl daemons: More info in db--exec-check error

[Ian Jackson]

Signed-off-by: Ian Jackson 
---
 tcl/JobDB-Executive.tcl | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tcl/JobDB-Executive.tcl b/tcl/JobDB-Executive.tcl
index 6c7e067..8f4ed98 100644
--- a/tcl/JobDB-Executive.tcl
+++ b/tcl/JobDB-Executive.tcl
@@ -170,7 +170,7 @@ proc db--exec-check {shvar stmt expected_status body} {
set emsg "osstest expected status $expected_status got $status"
}
set context [pg_result $sh -error context]
-   error $emsg \
+   error "db exec failed ($status, $sqlstate) $emsg" \
"while executing SQL\n$stmt\nin SQL context\n$context" \
[list OSSTEST-PSQL $status $sqlstate]
}
-- 
2.1.4


___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

[Xen-devel] [OSSTEST PATCH 16/33] Database locking: Tcl: Retry only on DEADLOCK DETECTED

[Ian Jackson]

Use the new errorCode coming out of db-execute* to tell when the error
is that we got a database deadlock, which is the situation in which we
should retry.

This involves combining the two catch blocks, so that there is only
one error handling strategy.  Previously errors on COMMIT would be
retried and others would not.  Now errors anywhere might be retried
but only if the DB indicated deadlock.

We now unconditionally execute ROLLBACK.  This is more correct, since
we always previously executed BEGIN.

And, we pass the errorInfo and errorCode from the $body to the caller.

I have tested this with a test db instance, using contrived means to
generate a database deadlock, and it does actually retry.

Signed-off-by: Ian Jackson 
Acked-by: Ian Campbell 
---
 tcl/JobDB-Executive.tcl | 30 --
 1 file changed, 16 insertions(+), 14 deletions(-)

diff --git a/tcl/JobDB-Executive.tcl b/tcl/JobDB-Executive.tcl
index ed9abbb..63db4f0 100644
--- a/tcl/JobDB-Executive.tcl
+++ b/tcl/JobDB-Executive.tcl
@@ -283,25 +283,27 @@ proc transaction {tables script} {
db-execute "SET TRANSACTION ISOLATION LEVEL SERIALIZABLE"
lock-tables $tables
uplevel 1 $script
+   db-execute COMMIT
} result]
-   if {!$rc} {
-   if {[catch {
-   db-execute COMMIT
-   } emsg]} {
-   puts "commit failed: $emsg; retrying ..."
-   db-execute ROLLBACK
-   if {[incr retries -1] <= 0} {
-   error \
- "commit failed, too many retries: $emsg\n$errorInfo\n$errorCode\n"
+   set ei $errorInfo
+   set ec $errorCode
+   if {$rc} {
+   db-execute ROLLBACK
+   switch -glob $errorCode {
+   {OSSTEST-PSQL * 40P01} {
+   # DEADLOCK DETECTED
+   puts "transaction deadlock ($result) retrying ..."
+   if {[incr retries -1] <= 0} {
+   error \
+ "transaction failed, too many retries: $result\n$errorInfo\n$errorCode\n"
+   }
+   after 500
+   continue
}
-   after 500
-   continue
}
-   } else {
-   db-execute ROLLBACK
}
 db-close
-   return -code $rc $result
+   return -code $rc -errorinfo $ei -errorcode $ec $result
 }
 }
 
-- 
2.1.4


___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

[Xen-devel] [OSSTEST PATCH 11/33] Tcl database debugging: Actually work

[Ian Jackson]

Setting OSSTEST_TCL_JOBDB_DEBUG was ineffective (ever since it was
introduced in 44dad3d8) because `env' wasn't imported from the global
scope.

Signed-off-by: Ian Jackson 
Acked-by: Ian Campbell 
---
 tcl/JobDB-Executive.tcl | 1 +
 1 file changed, 1 insertion(+)

diff --git a/tcl/JobDB-Executive.tcl b/tcl/JobDB-Executive.tcl
index 7dba497..239f22c 100644
--- a/tcl/JobDB-Executive.tcl
+++ b/tcl/JobDB-Executive.tcl
@@ -116,6 +116,7 @@ proc db-update-1 {stmt} {
 }
 
 proc db-execute-debug {stmt} {
+global env
 if {[var-or-default env(OSSTEST_TCL_JOBDB_DEBUG) 0]} {
puts stderr "EXECUTING >$stmt<"
 }
-- 
2.1.4


___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

[Xen-devel] [OSSTEST PATCH 09/33] ms-flights-summary: Remove spurious \ in keys \%{ something }

[Ian Jackson]

With some versions of Perl this generates a warning which causes
ms-flights-summary to fail.

Signed-off-by: Ian Jackson 
CC: Wei Liu 
---
 ms-flights-summary | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/ms-flights-summary b/ms-flights-summary
index ea72088..c703d81 100755
--- a/ms-flights-summary
+++ b/ms-flights-summary
@@ -359,7 +359,7 @@ printf("%d flight(s) consisting of %s job(s)%s%s anonymous/rogue
 
 my %summarycounts;
 foreach my $fi (values %flights) {
-$summarycounts{$_} += $fi->{Stats}{$_} foreach (keys \%{ $fi->{Stats} });
+$summarycounts{$_} += $fi->{Stats}{$_} foreach (keys %{ $fi->{Stats} });
 }
 my @summarycounts = sort_stats \%summarycounts;
 
-- 
2.1.4


___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

[Xen-devel] [OSSTEST PATCH 14/33] Database locking: Tcl: Always use db-execute-array for SELECT

[Ian Jackson]

We are going to make it wrong to use db-execute for SELECT statements.

Convert the two existing violation sites (providing a dummy arrayvar).

Signed-off-by: Ian Jackson 
Acked-by: Ian Campbell 
---
 sg-execute-flight   | 2 +-
 tcl/JobDB-Executive.tcl | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/sg-execute-flight b/sg-execute-flight
index 27fc04c..14c8cda 100755
--- a/sg-execute-flight
+++ b/sg-execute-flight
@@ -30,7 +30,7 @@ proc check {} {
 
 jobdb::db-open
 
-set nqueued [jobdb::db-execute "
+set nqueued [jobdb::db-execute-array dummy "
 SELECT job FROM jobs j
  WHERE j.flight = $flight
AND j.status = 'queued'
diff --git a/tcl/JobDB-Executive.tcl b/tcl/JobDB-Executive.tcl
index 239f22c..bbce6fc 100644
--- a/tcl/JobDB-Executive.tcl
+++ b/tcl/JobDB-Executive.tcl
@@ -275,7 +275,7 @@ proc become-task {comment} {
 set username "[id user]@$hostname"
 
 transaction resources {
-set nrows [db-execute "
+set nrows [db-execute-array dummy "
 UPDATE tasks
SET username = [pg_quote $username],
comment = [pg_quote $comment]
-- 
2.1.4


___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

[Xen-devel] [qemu-mainline baseline-only test] 66546: tolerable FAIL

[Platform Team regression test user]

This run is configured for baseline tests only.

flight 66546 qemu-mainline real [real]
http://osstest.xs.citrite.net/~osstest/testlogs/logs/66546/

Failures :-/ but no regressions.

Regressions which are regarded as allowable (not blocking):
 test-amd64-amd64-qemuu-nested-intel 16 debian-hvm-install/l1/l2 fail like 66485
 test-amd64-amd64-amd64-pvgrub 10 guest-start  fail  like 66485

Tests which did not succeed, but are not blocking:
 test-amd64-amd64-xl-pvh-intel 11 guest-start  fail  never pass
 test-armhf-armhf-libvirt 14 guest-saverestorefail   never pass
 test-armhf-armhf-libvirt 12 migrate-support-checkfail   never pass
 test-armhf-armhf-libvirt-xsm 12 migrate-support-checkfail   never pass
 test-armhf-armhf-libvirt-xsm 14 guest-saverestorefail   never pass
 test-amd64-amd64-xl-pvh-amd  11 guest-start  fail   never pass
 test-armhf-armhf-libvirt-qcow2 11 migrate-support-checkfail never pass
 test-armhf-armhf-libvirt-qcow2 13 guest-saverestorefail never pass
 test-armhf-armhf-libvirt-raw 13 guest-saverestorefail   never pass
 test-armhf-armhf-libvirt-raw 11 migrate-support-checkfail   never pass
 test-armhf-armhf-xl-credit2  13 saverestore-support-checkfail   never pass
 test-armhf-armhf-xl-credit2  12 migrate-support-checkfail   never pass
 test-armhf-armhf-xl-multivcpu 13 saverestore-support-checkfail  never pass
 test-armhf-armhf-xl-multivcpu 12 migrate-support-checkfail  never pass
 test-armhf-armhf-xl-xsm  13 saverestore-support-checkfail   never pass
 test-armhf-armhf-xl-xsm  12 migrate-support-checkfail   never pass
 test-armhf-armhf-xl-midway   13 saverestore-support-checkfail   never pass
 test-armhf-armhf-xl-midway   12 migrate-support-checkfail   never pass
 test-armhf-armhf-xl  12 migrate-support-checkfail   never pass
 test-armhf-armhf-xl  13 saverestore-support-checkfail   never pass
 test-armhf-armhf-xl-rtds 13 saverestore-support-checkfail   never pass
 test-armhf-armhf-xl-rtds 12 migrate-support-checkfail   never pass
 test-amd64-amd64-libvirt 12 migrate-support-checkfail   never pass
 test-amd64-i386-libvirt  12 migrate-support-checkfail   never pass
 test-amd64-amd64-libvirt-xsm 12 migrate-support-checkfail   never pass
 test-amd64-i386-libvirt-xsm  12 migrate-support-checkfail   never pass
 test-armhf-armhf-xl-vhd  11 migrate-support-checkfail   never pass
 test-armhf-armhf-xl-vhd  12 saverestore-support-checkfail   never pass
 test-amd64-amd64-qemuu-nested-amd 16 debian-hvm-install/l1/l2  fail never pass
 test-amd64-i386-libvirt-qemuu-debianhvm-amd64-xsm 10 migrate-support-check 
fail never pass
 test-amd64-amd64-libvirt-qemuu-debianhvm-amd64-xsm 10 migrate-support-check 
fail never pass
 test-amd64-amd64-libvirt-vhd 11 migrate-support-checkfail   never pass
 test-amd64-i386-xl-qemuu-win7-amd64 16 guest-stop  fail never pass
 test-amd64-amd64-xl-qemuu-win7-amd64 16 guest-stop fail never pass

version targeted for testing:
 qemuu91d35509903464c7f4b9ed56be223d7370d3597c
baseline version:
 qemuud6550e9ed2e1a60d889dfb721de00d9a4e3bafbe

Last test of basis66485  2016-07-01 10:47:40 Z7 days
Testing same since66546  2016-07-08 11:14:31 Z0 days1 attempts


People who touched revisions under test:
  Aaron Larson 
  Alberto Garcia 
  Aleksandar Markovic 
  Alex Bennée 
  Alex Bligh 
  Alex Williamson 
  Alexander Graf 
  Alexander Shopov 
  Alexey Kardashevskiy 
  Alistair Francis 
  Amit Shah 
  Andrea Arcangeli 
  Andreas Färber 
  Andrew Jeffery 
  Andrew Jones 
  Andrey Smirnov 
  Aneesh Kumar K.V 
  Anthony PERARD 
  Anton Blanchard 
  Ard Biesheuvel 
  Artyom Tarasenko 
  Ashijeet Acharya 
  Aurelien Jarno 
  Bastian Koppelmann 
  Benjamin Herrenschmidt 
  Bharata B Rao 
  Bret Ketchum 
  Cao jin 
  Changlong Xie 
  Chao Peng 
  Chen Fan 
  Christian Borntraeger 
  Christophe Lyon 

Re: [Xen-devel] [PATCH v3 2/8] x86/vm-event/monitor: relocate code-motion more appropriately

[Corneliu ZUZU]

On 7/8/2016 6:50 PM, Tamas K Lengyel wrote:

diff --git a/xen/arch/x86/monitor.c b/xen/arch/x86/monitor.c
index 205df41..42f31bf 100644
--- a/xen/arch/x86/monitor.c
+++ b/xen/arch/x86/monitor.c

[snip]

So with this code-portion appropriately being relocated here I think
we should also rename..


+void arch_monitor_write_data(struct vcpu *v)
+{
+struct monitor_write_data *w = >arch.vm_event->write_data;

.. arch.vm_event to arch.monitor to match the subsystem it is used by.

Thanks,
Tamas


Which is (almost) exactly what I've done in (another..) patch-series 
I'll be sending today I hope (and a few more things). ;-)


Thanks,
Corneliu.

___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v2 0/4] libxl: add framework for device types

[Ian Jackson]

Juergen Gross writes ("[PATCH v2 0/4] libxl: add framework for device types"):
> Instead of duplicate coding for each device type (vtpms, usbctrls, ...)
> especially on domain creation introduce a framework for that purpose.
> 
> I especially found it annoying that e.g. the vtpm callback issued the
> error message for a failed attach of nic devices.

I was tempted to just commit this, having acked all four, but given
the nature of this series I'm going to wait a bit for further comments
:-).

Regards,
Ian.

___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v2 4/4] libxl: move DEFINE_DEVICE* macros to libxl_internal.h

[Ian Jackson]

Juergen Gross writes ("[PATCH v2 4/4] libxl: move DEFINE_DEVICE* macros to 
libxl_internal.h"):
> In order to be able to have all functions related to a device type in
> a single source file move the macros used to generate device type
> specific functions to libxl_internal.h. Rename the macros as they are
> no longer local to a source file. While at it hide device remove and
> device destroy in one macro as those are always used in pairs. Move
> usage of the macros to the appropriate source files.

Acked-by: Ian Jackson 

___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v2 3/4] libxl: refactor domcreate_attach_dtdev() to use device type framework

[Ian Jackson]

Juergen Gross writes ("[PATCH v2 3/4] libxl: refactor domcreate_attach_dtdev() 
to use device type framework"):
> Signed-off-by: Juergen Gross 

Acked-by: Ian Jackson 

___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v2 1/4] libxl: add framework for device types

[Ian Jackson]

Juergen Gross writes ("[PATCH v2 1/4] libxl: add framework for device types"):
> Instead of duplicate coding for each device type (vtpms, usbctrls, ...)
> especially on domain creation introduce a framework for that purpose.

Acked-by: Ian Jackson 

How very pleasing to see so much code being deleted.

Ian.

___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v2 2/4] libxl: refactor domcreate_attach_pci() to use device type framework

[Ian Jackson]

Juergen Gross writes ("[PATCH v2 2/4] libxl: refactor domcreate_attach_pci() to 
use device type framework"):
> Signed-off-by: Juergen Gross 

Acked-by: Ian Jackson 

___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v3 5/5] libxl: only issue cpu-add call to QEMU for not present CPU

[Ian Jackson]

Wei Liu writes ("[PATCH v3 5/5] libxl: only issue cpu-add call to QEMU for not 
present CPU"):
> Calculate the final bitmap for CPUs to add to avoid having annoying
> error messages complaining those CPUs are already present.

Can this be expanded a bit ?  I think perhaps it would benefit from
"When X and Y and Z, qemu prints an annoying message about Q".

I think I have understood it by reverse engineering the code but I
would still like a better commit message.

Thanks,
Ian.

___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v3 3/5] libxl: introduce libxl__qmp_query_cpus

[Ian Jackson]

Wei Liu writes ("[PATCH v3 3/5] libxl: introduce libxl__qmp_query_cpus"):
> It interrogates QEMU for CPUs and update the bitmap accordingly.

Acked-by: Ian Jackson 

___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v3 4/5] libxl: update vcpus bitmap in retrieved guest config

[Ian Jackson]

Wei Liu writes ("[PATCH v3 4/5] libxl: update vcpus bitmap in retrieved guest 
config"):
> ... because the available vcpu bitmap can change during domain life time
> due to cpu hotplug and unplug.
> 
> For QEMU upstream, we interrogate QEMU for the number of vcpus. For
> others, we look directly into xenstore for information.
...
> +static int libxl__update_avail_vcpus_qmp(libxl__gc *gc, uint32_t domid,
> + unsigned int max_vcpus,
> + libxl_bitmap *map)
> +{
> +int rc;
> +
> +/* For QEMU upstream we always need to return the number
> + * of cpus present to QEMU whether they are online or not;
> + * otherwise QEMU won't accept the saved state.

This comment is confusing to me.  Perhaps `return' should read
`provide' ?  But the connection between the body of this function and
the comment is still obscure.

> +static int libxl__update_avail_vcpus_xenstore(libxl__gc *gc, uint32_t domid,
> +  unsigned int max_vcpus,
> +  libxl_bitmap *map)
> +{
> +int rc;
> +unsigned int i;
> +const char *dompath;
...
> +for (i = 0; i < max_vcpus; i++) {
> +const char *path = GCSPRINTF("%s/cpu/%u/availability", dompath, i);
> +const char *content = libxl__xs_read(gc, XBT_NULL, path);
> +if (!strcmp(content, "online"))
> +libxl_bitmap_set(map, i);

Firstly, this should be libxl__xs_read_checked.  Secondly if "content"
is NULL, this will crash.

> @@ -7294,6 +7341,55 @@ int libxl_retrieve_domain_configuration(libxl_ctx 
> *ctx, uint32_t domid,
>  libxl_dominfo_dispose();
>  }
>  
> +/* VCPUs */
> +{
> +libxl_bitmap *map = _config->b_info.avail_vcpus;
> +unsigned int max_vcpus = d_config->b_info.max_vcpus;
> +libxl_device_model_version version;
> +
> +libxl_bitmap_dispose(map);
> +libxl_bitmap_init(map);
> +libxl_bitmap_alloc(CTX, map, max_vcpus);
> +libxl_bitmap_set_none(map);

I see that this function already trashes the domain config when it
fails (leaving it up to the caller to free the partial config) but
that this is not documented :-/.

> +/* If the user did not explicitly specify a device model
> + * version, we need to use the same logic used during domain
> + * creation to derive the device model version.
> + */
> +version = d_config->b_info.device_model_version;
> +if (version == LIBXL_DEVICE_MODEL_VERSION_UNKNOWN)
> +version = libxl__get_device_model_version(gc, _config->b_info);

I think this is rather unfortunate.  Won't changing the default device
model while the domain is running will cause this function to give
wrong answers ?

Thanks,
Ian.

___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v3 2/5] libxl: factor out libxl__get_device_modlel_version

[Ian Jackson]

Wei Liu writes ("[PATCH v3 2/5] libxl: factor out 
libxl__get_device_modlel_version"):

Typo in subject line.

> Factor out the logic to determine device model version to a function. It
> will be used later. Note that code now also checks if the stbudom field
> is actually set before trying to extract a value from it.

With the typo fixed,

Acked-by: Ian Jackson 

___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH] vmx/monitor: CPUID events

[Andrew Cooper]

On 08/07/16 17:59, Tamas K Lengyel wrote:
> On Fri, Jul 8, 2016 at 10:49 AM, Andrew Cooper
>  wrote:
>> On 08/07/16 16:44, Tamas K Lengyel wrote:
>>> On Fri, Jul 8, 2016 at 3:33 AM, Andrew Cooper  
>>> wrote:
 On 08/07/16 03:31, Tamas K Lengyel wrote:
> This patch implements sending notification to a monitor subscriber when an
> x86/vmx guest executes the CPUID instruction.
>
> Signed-off-by: Tamas K Lengyel 
 Is it wise having an on/off control without any further filtering?  (I
 suppose that it is at least a fine first start).
>>> What type of extra filtering do you have in mind?
>> Not sure.  What are you intending to use this facility for?
> Primarily to detect malware that is fingerprinting it's environment by
> looking for hypervisor leafs and/or doing timing based detection by
> benchmarking cpuid with rdtsc.
>
>> Given that the hypervisor is already in complete control of what a guest
>> gets to see via cpuid, mutating the results via the monitor framework
>> doesn't seem like a useful thing to do.
> Indeed, the hypervisor is in control and to a certain extant the user
> is via overriding some leafs in the domain config. However, there are
> CPUID leafs Xen adds that the user is unable to override with the
> domain config. For example in malware analysis it may be very useful
> to be able to hide all hypervisor leafs from the guest, which
> currently requires us to recompile Xen completely. By being able to
> put the monitor system inline of CPUID it can decide which process it
> wants to allow to see what leafs and when. It's very handy.

Fair enough.

For the record, my planned further work for cpuid will make things far
more configurable.  The current abilities of a toolstack, and the
in-hypervisor auditing are woeful.

~Andrew

___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [DRAFT 1] XenSock protocol design document

[Stefano Stabellini]

On Fri, 8 Jul 2016, David Vrabel wrote:
> On 08/07/16 17:52, Stefano Stabellini wrote:
> > 
> >> c) My belief that most of the advantages of this proposal can be
> >> achieved with smarts in the backend.
> > 
> > By backend do you mean netfront/netback? If so, I have already pointed
> > out why that is not the case in previous emails as well as in this
> > design document.
> > 
> > If you remain unconvinced of the usefulness of this work, that's OK, we
> > can agree to disagree. Many people work on things I don't believe
> > particularly useful myself. I am not asking you to spend any time on
> > this if you don't believe it serves a purpose. But please let the rest
> > of the community work constructively together.
> 
> If you're only going to implement userspace frontends and backends, then
> sure I can step out of this discussion.  However, your initial prototype
> drivers suggest you're aiming at in-kernel drivers.
 
We are discussing the protocol here. There could be only userspace
drivers. Reservations about Linux drivers should be expressed elsewhere.

Even if there were kernel drivers, don't worry about maintenance. I
wouldn't ask you to carry the burden. If/when they'll be ready to be
merged, I would be quite happy to take it on (similarly to what happened
with Juergen and his good work on the PV SCSI frontend and backend). Due
to their nature, they'll be quite self-contained. And fortunately we
have many other Linux maintainers and Linux experts in the Xen
community, who can provide constructive feedback. We are an healthy
community after all and we can rely on each others.

___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v3 1/5] libxl: libxl_domain_need_memory shouldn't modify b_info

[Ian Jackson]

Wei Liu writes ("[PATCH v3 1/5] libxl: libxl_domain_need_memory shouldn't 
modify b_info"):
> This function is used to return the memory needed for a guest. It's not
> in a position to modify the b_info passed in (note the _setdefault
> function).
> 
> Use a copy of b_info to do the calculation. Define a macro to mark the
> change in API.

Urgh, how unpleasant.

> diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
> index 5ec4c80..65af9ee 100644
> --- a/tools/libxl/libxl.c
> +++ b/tools/libxl/libxl.c
> @@ -5123,20 +5123,24 @@ int libxl_domain_need_memory(libxl_ctx *ctx, 
> libxl_domain_build_info *b_info,
>   uint32_t *need_memkb)
>  {
>  GC_INIT(ctx);
> +libxl_domain_build_info tmp;

I would suggest, instead:

int libxl_domain_need_memory(libxl_ctx *ctx,
 -   libxl_domain_build_info *b_info,
 +   const libxl_domain_build_info *b_info_in,
...
 + libxl_domain_build_info b_info[1];

If you do this then you do not need to change the bulk of the body of
the function at all.

> +/* LIBXL_HAVE_DOMAIN_NEED_MEMORY_V2
> + *
> + * If this is defined, libxl_domain_need_memory no longer modifies
> + * passed in b_info.
> + */
> +#define LIBXL_HAVE_DOMAIN_NEED_MEMORY_V2

I suggest constifying it and

 LIBXL_HAVE_DOMAIN_NEED_MEMORY_CONST_B_INFO

Ian.

___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH 7/7] oxenstored: honour XEN_RUN_DIR

[Ian Jackson]

Ian Jackson writes ("Re: [PATCH 7/7] oxenstored: honour XEN_RUN_DIR"):
> Wei Liu writes ("[PATCH 7/7] oxenstored: honour XEN_RUN_DIR"):
> > Move default the pid file under XEN_RUN_DIR. Note that it changes the
> > location from /var/run to /var/run/xen.
> 
> Acked-by: Ian Jackson 

Actually, shouldn't this be combined with some init script patch, as
otherwise the tree is not bisectable ?

Ian.

___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH 6/7] libxenstat: honour XEN_RUN_DIR

[Ian Jackson]

Wei Liu writes ("[PATCH 6/7] libxenstat: honour XEN_RUN_DIR"):
> This is because libxl uses XEN_RUN_DIR to generate the socket path for
> libxenstat while libxenstat itself uses hard-coded path, which is not
> necessary in sync with libxl.

This commit message is confusing (and perhaps ungrammatical).  Is
there currently a bug here ?

Maybe this patch or something like it is a backport candidate ?

Ian.

___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH 7/7] oxenstored: honour XEN_RUN_DIR

[Ian Jackson]

Wei Liu writes ("[PATCH 7/7] oxenstored: honour XEN_RUN_DIR"):
> Move default the pid file under XEN_RUN_DIR. Note that it changes the
> location from /var/run to /var/run/xen.

Acked-by: Ian Jackson 

___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH 5/7] hotplug/Linux: honour XEN_RUN_DIR

[Ian Jackson]

Wei Liu writes ("[PATCH 5/7] hotplug/Linux: honour XEN_RUN_DIR"):
> Store various PID files under XEN_RUN_DIR. Note that this change the
> default location from /var/run to /var/run/xen.

Acked-by: Ian Jackson 

___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH 3/7] hotplug/FreeBSD: honour XEN_RUN_DIR

[Ian Jackson]

Wei Liu writes ("[PATCH 3/7] hotplug/FreeBSD: honour XEN_RUN_DIR"):
> Store xldevd.pid under XEN_RUN_DIR. Note that the default location would
> change from /var/run to /var/run/xen.
> 
> Signed-off-by: Wei Liu 

I see Roger has acked this already.

Acked-by: Ian Jackson 

Ian.

___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH 4/7] hotplug/NetBSD: honour XEN_RUN_DIR

[Ian Jackson]

Wei Liu writes ("[PATCH 4/7] hotplug/NetBSD: honour XEN_RUN_DIR"):
> Store xldevd.pid under XEN_RUN_DIR. Note that this will change the
> default location from /var/run to /var/run/xen.

Acked-by: Ian Jackson 

___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH 2/7] tools/helper: honour XEN_RUN_DIR in init-xenstore-domain.c

[Ian Jackson]

Wei Liu writes ("[PATCH 2/7] tools/helper: honour XEN_RUN_DIR in 
init-xenstore-domain.c"):
> Place the PID file under XEN_RUN_DIR. Note that this change the default
> location from /var/run to /var/run/xen.
> 
> Generate a _paths.h as that is required to make this change work.
...
> -init-xenstore-domain: $(INIT_XENSTORE_DOMAIN_OBJS)
> +init-xenstore-domain: $(INIT_XENSTORE_DOMAIN_OBJS) _paths.h

This dependency is surely wrong.  It is the object(s) (ie, the
compile) that depend on _paths.h, not the executable (ie the link).

But otherwise this is fine.

Thanks,
Ian.

___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH 0/7] Honour more configure variables in various places (iteration 3)

[Ian Jackson]

Wei Liu writes ("Re: [Xen-devel] [PATCH 0/7] Honour more configure variables in 
various places (iteration 3)"):
> Ian, ping?

You didn't actually send that ping mail to me, just to the list :-).

I'll look at the patches ...

Thanks,
Ian.

___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v1 20/20] libxl/acpi: Build ACPI tables for HVMlite guests

[Boris Ostrovsky]

On 07/08/2016 12:07 PM, Wei Liu wrote:
> On Fri, Jul 08, 2016 at 10:48:52AM -0400, Boris Ostrovsky wrote:
>> On 07/08/2016 06:55 AM, Wei Liu wrote:
 +
 +/* Map page that will hold RSDP */
 +extent = RSDP_ADDRESS >> ctxt.page_shift;
 +rc = populate_acpi_pages(dom, , 1, );
 +if (rc) {
 +LOG(ERROR, "%s: populate_acpi_pages for rsdp failed with %d",
 +__FUNCTION__, rc);
 +goto out;
 +}
 +config.rsdp = (unsigned long)xc_map_foreign_range(xch, domid,
 +  ctxt.page_size,
 +  PROT_READ | 
 PROT_WRITE,
 +  RSDP_ADDRESS >> 
 ctxt.page_shift);
>>> I think with Anthony's on-going work you should be more flexible for all
>>> you tables.
>> Not sure I understand what you mean here. You want the address
>> (RSDP_ADDRESS) to be a variable as opposed to a macro?
>>
> I'm still trying to wrap my head around the possible interaction between
> Anthony's work and your work.
>
> Anthony's work allows dynamically loading of firmware blobs. If you use
> a fixed address, theoretically it can clash with firmware blobs among
> other things libxc can load. The high address is a safe bet so that
> probably won't happen in practice.
>
> Anthony's work allows loading arbitrary blobs actually. Can you take
> advantage of that mechanism as well? That is, to specify all your tables
> as modules and let libxc handle actually loading them  into guest
> memory.
>
> Does this make sense?
>
> Also CC Anthony here.


My understanding of Anthony's series is that its goal was to provide an
interface to pass DSDT (and maybe some other tables) from the toolstack
to hvmloader.

Here we don't have hvmloader, we are loading the tables directly into
guest's memory.

-boris



___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

[Xen-devel] [xen-unstable-smoke test] 96794: tolerable all pass - PUSHED

[osstest service owner]

flight 96794 xen-unstable-smoke real [real]
http://logs.test-lab.xenproject.org/osstest/logs/96794/

Failures :-/ but no regressions.

Tests which did not succeed, but are not blocking:
 test-amd64-amd64-libvirt 12 migrate-support-checkfail   never pass
 test-armhf-armhf-xl  12 migrate-support-checkfail   never pass
 test-armhf-armhf-xl  13 saverestore-support-checkfail   never pass

version targeted for testing:
 xen  7da483b0236d8974cc97f81780dcf8e559a63175
baseline version:
 xen  1e2d167d8faae843b80487e5026b07a135cf4147

Last test of basis96790  2016-07-08 11:00:55 Z0 days
Testing same since96794  2016-07-08 15:01:26 Z0 days1 attempts


People who touched revisions under test:
  Andrew Cooper 
  Anshul Makkar 
  Daniel De Graaf 
  Rusty Bird 
  Shannon Zhao 
  Wei Liu 

jobs:
 build-amd64  pass
 build-armhf  pass
 build-amd64-libvirt  pass
 test-armhf-armhf-xl  pass
 test-amd64-amd64-xl-qemuu-debianhvm-i386 pass
 test-amd64-amd64-libvirt pass



sg-report-flight on osstest.test-lab.xenproject.org
logs: /home/logs/logs
images: /home/logs/images

Logs, config files, etc. are available at
http://logs.test-lab.xenproject.org/osstest/logs

Explanation of these reports, and of osstest in general, is at
http://xenbits.xen.org/gitweb/?p=osstest.git;a=blob;f=README.email;hb=master
http://xenbits.xen.org/gitweb/?p=osstest.git;a=blob;f=README;hb=master

Test harness code can be found at
http://xenbits.xen.org/gitweb?p=osstest.git;a=summary


Pushing revision :

+ branch=xen-unstable-smoke
+ revision=7da483b0236d8974cc97f81780dcf8e559a63175
+ . ./cri-lock-repos
++ . ./cri-common
+++ . ./cri-getconfig
+++ umask 002
+++ getrepos
 getconfig Repos
 perl -e '
use Osstest;
readglobalconfig();
print $c{"Repos"} or die $!;
'
+++ local repos=/home/osstest/repos
+++ '[' -z /home/osstest/repos ']'
+++ '[' '!' -d /home/osstest/repos ']'
+++ echo /home/osstest/repos
++ repos=/home/osstest/repos
++ repos_lock=/home/osstest/repos/lock
++ '[' x '!=' x/home/osstest/repos/lock ']'
++ OSSTEST_REPOS_LOCK_LOCKED=/home/osstest/repos/lock
++ exec with-lock-ex -w /home/osstest/repos/lock ./ap-push xen-unstable-smoke 
7da483b0236d8974cc97f81780dcf8e559a63175
+ branch=xen-unstable-smoke
+ revision=7da483b0236d8974cc97f81780dcf8e559a63175
+ . ./cri-lock-repos
++ . ./cri-common
+++ . ./cri-getconfig
+++ umask 002
+++ getrepos
 getconfig Repos
 perl -e '
use Osstest;
readglobalconfig();
print $c{"Repos"} or die $!;
'
+++ local repos=/home/osstest/repos
+++ '[' -z /home/osstest/repos ']'
+++ '[' '!' -d /home/osstest/repos ']'
+++ echo /home/osstest/repos
++ repos=/home/osstest/repos
++ repos_lock=/home/osstest/repos/lock
++ '[' x/home/osstest/repos/lock '!=' x/home/osstest/repos/lock ']'
+ . ./cri-common
++ . ./cri-getconfig
++ umask 002
+ select_xenbranch
+ case "$branch" in
+ tree=xen
+ xenbranch=xen-unstable-smoke
+ qemuubranch=qemu-upstream-unstable
+ '[' xxen = xlinux ']'
+ linuxbranch=
+ '[' xqemu-upstream-unstable = x ']'
+ select_prevxenbranch
++ ./cri-getprevxenbranch xen-unstable-smoke
+ prevxenbranch=xen-4.7-testing
+ '[' x7da483b0236d8974cc97f81780dcf8e559a63175 = x ']'
+ : tested/2.6.39.x
+ . ./ap-common
++ : osst...@xenbits.xen.org
+++ getconfig OsstestUpstream
+++ perl -e '
use Osstest;
readglobalconfig();
print $c{"OsstestUpstream"} or die $!;
'
++ :
++ : git://xenbits.xen.org/xen.git
++ : osst...@xenbits.xen.org:/home/xen/git/xen.git
++ : git://xenbits.xen.org/qemu-xen-traditional.git
++ : git://git.kernel.org
++ : git://git.kernel.org/pub/scm/linux/kernel/git
++ : git
++ : git://xenbits.xen.org/libvirt.git
++ : osst...@xenbits.xen.org:/home/xen/git/libvirt.git
++ : git://xenbits.xen.org/libvirt.git
++ : git://xenbits.xen.org/rumpuser-xen.git
++ : git
++ : git://xenbits.xen.org/rumpuser-xen.git
++ : osst...@xenbits.xen.org:/home/xen/git/rumpuser-xen.git
+++ besteffort_repo https://github.com/rumpkernel/rumpkernel-netbsd-src
+++ local repo=https://github.com/rumpkernel/rumpkernel-netbsd-src
+++ cached_repo https://github.com/rumpkernel/rumpkernel-netbsd-src 
'[fetch=try]'
+++ local repo=https://github.com/rumpkernel/rumpkernel-netbsd-src
+++ local 'options=[fetch=try]'
 getconfig GitCacheProxy
 perl -e '
   

Re: [Xen-devel] [DRAFT 1] XenSock protocol design document

[David Vrabel]

On 08/07/16 17:52, Stefano Stabellini wrote:
> 
>> c) My belief that most of the advantages of this proposal can be
>> achieved with smarts in the backend.
> 
> By backend do you mean netfront/netback? If so, I have already pointed
> out why that is not the case in previous emails as well as in this
> design document.
> 
> If you remain unconvinced of the usefulness of this work, that's OK, we
> can agree to disagree. Many people work on things I don't believe
> particularly useful myself. I am not asking you to spend any time on
> this if you don't believe it serves a purpose. But please let the rest
> of the community work constructively together.

If you're only going to implement userspace frontends and backends, then
sure I can step out of this discussion.  However, your initial prototype
drivers suggest you're aiming at in-kernel drivers.

David

___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v3] xen/arm: enable clocks used by the hypervisor

[Michael Turquette]

Quoting Julien Grall (2016-07-08 02:34:43)
> Hi Dirk,
> 
> On 08/07/16 08:44, Dirk Behme wrote:
> > Xen hypervisor drivers might replace native OS drivers. The result is
> > that some important clocks that are enabled by the OS in the non-Xen
> > case are not properly enabled in the presence of Xen. The clocks
> > property enumerates the clocks that must be enabled by the Xen clock
> > consumer.
> >
> > An example is a serial driver enabled by the hypervisor. Xen must
> 
> I would say "An example is the UART used by the hypervisor."
> 
> > consume and enable these clocks in the OS to ensure behavior continues
> > after firmware configures the UART hardware and corresponding clock
> > harder.
> 
> What do you mean by "harder"?
> 
> Also, relying on DOM0 to enable the clock looks very wrong to me and you 
> give an example which prove that. The UART will be used before hand by 
> Xen, however it will not be possible to use it if you expect DOM0 to 
> enable the clock (or even modify the clock frequency).
> 
> The clock should be enabled either by the firmware or Xen. But not DOM0. 
> DOM0 should not touch this clock at all.
> 
> Furthermore, this property could be used for clock associated to device 
> that will be passthrough-ed to a guest. In this case, the clock would be 
> enabled even if the device is not in use which will result more power 
> consumption.

Is there a need to pass clock references through to guests? If so the
unmerged CLK_ENABLE_HAND_OFF[0] feature might be useful to you? If this
flag is set on a given clk, it will be enabled at the time it is
registered by the clk provider driver, and it's enable_count reference
counter will be "handed off" to the first consumer that calls clk_get()
and clk_prepare_enable() on it. This means the clock CAN be gated by
it's proper driver, but it will be enabled at boot time as well.

This is useful for use cases like splash screens where the bootloader
configures the display and plays some animation, and we want the linux
kernel to take over the display controller hardware without cutting
clocks, blanking or reseting it. Handing off the clock reference count
helps achieve this.

[0] lkml.kernel.org/g/1455225554-13267-1-git-send-email-mturque...@baylibre.com

Regards,
Mike

> 
> >
> > Up to now, the workaround for this has been to use the Linux kernel
> > command line parameter 'clk_ignore_unused'. See Xen bug
> >
> > http://bugs.xenproject.org/xen/bug/45
> >
> > too.
> >
> > To fix this, we will add the "unused" clocks in Xen to the hypervisor
> > node. The OS has to consume and enable the clocks from the hypervisor
> > node, then.
> >
> > Therefore, check if there is a "clocks" entry in the hypervisor node
> > and if so consume and enable the given clocks. This prevents the clocks
> > from being disabled by the OS.
> >
> > Signed-off-by: Dirk Behme 
> > ---
> > Changes in v3: Use the xen.txt description proposed by Michael. Thanks!
> >
> > Changes in v2: Drop the Linux implementation details like clk_disable_unused
> >  in xen.txt.
> >
> >   Documentation/devicetree/bindings/arm/xen.txt | 13 
> >   arch/arm/xen/enlighten.c  | 47 
> > +++
> >   2 files changed, 60 insertions(+)
> >
> > diff --git a/Documentation/devicetree/bindings/arm/xen.txt 
> > b/Documentation/devicetree/bindings/arm/xen.txt
> > index c9b9321..00f2165 100644
> > --- a/Documentation/devicetree/bindings/arm/xen.txt
> > +++ b/Documentation/devicetree/bindings/arm/xen.txt
> > @@ -17,6 +17,19 @@ the following properties:
> > A GIC node is also required.
> > This property is unnecessary when booting Dom0 using ACPI.
> >
> > +Optional properties:
> > +
> > +clocks: one or more clocks to be enabled
> > +  Xen hypervisor drivers might replace native OS drivers. The result is
> 
> "native OS" has no meaning on Xen. This seems to be a cumbersome way to 
> say that the device will be used by the hypervisor and hidden to DOM0 
> (aka hardware domain).
> 
> > +  that some important clocks that are enabled by the OS in the non-Xen
> > +  case are not properly enabled in the presence of Xen. The clocks
> > +  property enumerates the clocks that must be enabled by the Xen clock
> > +  consumer.
> > +  An example is a serial driver enabled by the hypervisor. Xen must
> > +  consume and enable these clocks in the OS to ensure behavior continues
> > +  after firmware configures the UART hardware and corresponding clock
> > +  harder.
> > +
> >   To support UEFI on Xen ARM virtual platforms, Xen populates the FDT 
> > "uefi" node
> >   under /hypervisor with following parameters:
> 
> Regards,
> 
> -- 
> Julien Grall

___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [DRAFT 1] XenSock protocol design document

[David Vrabel]

On 08/07/16 12:23, Stefano Stabellini wrote:
> 
> XenSocks provides the following benefits:
> * guest networking works out of the box with VPNs, wireless networks and
>   any other complex configurations on the host

Only in the trivial case where the host only has one external network.
Otherwise, you are going to have to have some sort of configuration to
keep guest traffic isolated from the management or storage network (for
example).

> * guest services listen on ports bound directly to the backend domain IP
>   addresses

I think this could be done with SDN but I'm no expert on this area.

> * localhost becomes a secure namespace for intra-VMs communications

I presume you mean "inter-VM" communication here?  This is already
achievable with a private bridged network for VMs on a host.

> * full visibility of the guest behavior on the backend domain, allowing
>   for inexpensive filtering and manipulation of any guest calls

There's many existing solutions in this space for networking.

> * excellent performance

netback/netfront is pretty good now and further improvements to them
would have wider benefits.

David

___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH 6/6] xenconsoled: handle --log-backups 0 in logfile_rollover

[Ian Jackson]

Wei Liu writes ("[PATCH 6/6] xenconsoled: handle --log-backups 0 in 
logfile_rollover"):
> We now allow user to configure the number of backups to keep. We need to
> handle when the number is set to 0.
> 
> Check if number of backup is 0. If so, just unlink the file. Move the
> rotation to `else' branch so that we skip it altogether.

I would prefer to review one algorithm correct from the start...

Thanks,
Ian.

___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH 5/6] xenconsoled: options to control log rotation

[Ian Jackson]

Wei Liu writes ("[PATCH 5/6] xenconsoled: options to control log rotation"):
> Provide two options: one to control the maximum number of copies kept
> and the other to control the maximum length of each log file.

Ah, OK, good.  However, as I said, I think this makes the rotation
loop wrong because restarting xenconsoled to change the log level can
leave old logfiles ever unrotated.

Ian.

___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH 4/6] xenconsoled: delete two now unused functions

[Ian Jackson]

Wei Liu writes ("[PATCH 4/6] xenconsoled: delete two now unused functions"):
> Signed-off-by: Wei Liu 

Acked-by: Ian Jackson 

___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH] vmx/monitor: CPUID events

[Tamas K Lengyel]

On Fri, Jul 8, 2016 at 10:49 AM, Andrew Cooper
 wrote:
> On 08/07/16 16:44, Tamas K Lengyel wrote:
>> On Fri, Jul 8, 2016 at 3:33 AM, Andrew Cooper  
>> wrote:
>>> On 08/07/16 03:31, Tamas K Lengyel wrote:
 This patch implements sending notification to a monitor subscriber when an
 x86/vmx guest executes the CPUID instruction.

 Signed-off-by: Tamas K Lengyel 
>>> Is it wise having an on/off control without any further filtering?  (I
>>> suppose that it is at least a fine first start).
>> What type of extra filtering do you have in mind?
>
> Not sure.  What are you intending to use this facility for?

Primarily to detect malware that is fingerprinting it's environment by
looking for hypervisor leafs and/or doing timing based detection by
benchmarking cpuid with rdtsc.

>
> Given that the hypervisor is already in complete control of what a guest
> gets to see via cpuid, mutating the results via the monitor framework
> doesn't seem like a useful thing to do.

Indeed, the hypervisor is in control and to a certain extant the user
is via overriding some leafs in the domain config. However, there are
CPUID leafs Xen adds that the user is unable to override with the
domain config. For example in malware analysis it may be very useful
to be able to hide all hypervisor leafs from the guest, which
currently requires us to recompile Xen completely. By being able to
put the monitor system inline of CPUID it can decide which process it
wants to allow to see what leafs and when. It's very handy.

>
>>
>>> cpuid is usually the serialising instruction used with rdtsc for timing
>>> loops.  This is bad enough in VMs because of the VMExit, but becomes
>>> even worse if there is a monitor delay as well.
>> Yes, going the extra route of sending a monitor event out will add to
>> that delay (how much delay will depend on the subscriber and what it
>> decides to do with the event). Wouldn't we be able to mask some of
>> that with tsc offsetting though?
>
> I am going to go out on a limb and say that that is a very large can of
> worms which you don't want to open.

Yea, I'm well aware. However, we might have to go down that rabbit
hole eventually..

>
> The problem is not that time skews from the point of view of the guest,
> but that the timing loop with a fixed number of iterations takes
> proportionally longer.
>

Yes, there is overhead inevitably. For our use-case what would be the
goal is make the detection of this overhead as hard as possible so as
long as the overhead is reasonable (ie. we don't make network
connections drop and such) we can live with the overhead.

Cheers,
Tamas

___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH 3/6] xenconsoled: switch guest log to use logfile abstraction

[Ian Jackson]

Wei Liu writes ("[PATCH 3/6] xenconsoled: switch guest log to use logfile 
abstraction"):
> Note that this causes write_with_timestamp to have no caller. Mark
> it as unused for now to minimise code churn.

This is plausible, although I think I would have structured this
series differently.  (1. provide new api to old code; 2. switch
everyone to new api; 3. replace implementation.)

Acked-by: Ian Jackson 

___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v3] xen/arm: enable clocks used by the hypervisor

[Michael Turquette]

Quoting Julien Grall (2016-07-08 03:49:33)
> 
> 
> On 08/07/16 11:40, Dirk Behme wrote:
> > Hi Michael and Julien,
> >
> > On 08.07.2016 11:34, Julien Grall wrote:
> >> Hi Dirk,
> >>
> >> On 08/07/16 08:44, Dirk Behme wrote:
> >>> Xen hypervisor drivers might replace native OS drivers. The result is
> >>> that some important clocks that are enabled by the OS in the non-Xen
> >>> case are not properly enabled in the presence of Xen. The clocks
> >>> property enumerates the clocks that must be enabled by the Xen clock
> >>> consumer.
> >>>
> >>> An example is a serial driver enabled by the hypervisor. Xen must
> >>
> >> I would say "An example is the UART used by the hypervisor."
> >>
> >>> consume and enable these clocks in the OS to ensure behavior continues
> >>> after firmware configures the UART hardware and corresponding clock
> >>> harder.
> >>
> >> What do you mean by "harder"?
> >>
> >> Also, relying on DOM0 to enable the clock looks very wrong to me and you
> >> give an example which prove that. The UART will be used before hand by
> >> Xen, however it will not be possible to use it if you expect DOM0 to
> >> enable the clock (or even modify the clock frequency).
> >>
> >> The clock should be enabled either by the firmware or Xen. But not DOM0.
> >> DOM0 should not touch this clock at all.
> >>
> >> Furthermore, this property could be used for clock associated to device
> >> that will be passthrough-ed to a guest. In this case, the clock would be
> >> enabled even if the device is not in use which will result more power
> >> consumption.
> >
> >
> > I took the description directly from Michael's proposal
> >
> > http://www.spinics.net/lists/arm-kernel/msg516576.html
> >
> > Would it be possible that you two experts agree on the exact wording you
> > like to see?
> 
> I think the wording suggested by Mark in [1] represents better what we 
> would like to achieve with this property.

Mark's copy is fine by me. The main thing is to avoid talking about
"unused" clocks being disabled by the OS. If you have need clocks to be
enabled, then you simply claim them and enable them. All of the other
stuff is just noise.

Regards,
Mike

> 
> Regards,
> 
> [1] https://www.spinics.net/lists/arm-kernel/msg516158.html
> 
> -- 
> Julien Grall

___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH 1/6] xenconsoled: introduce log file abstraction

[Ian Jackson]

Wei Liu writes ("[PATCH 1/6] xenconsoled: introduce log file abstraction"):
> It will be used to handle hypervsior log and guest console log.

Thanks.

> diff --git a/tools/console/daemon/logfile.h b/tools/console/daemon/logfile.h
> new file mode 100644
> index 000..87f3c51
...
> +#define LOGFILE_MAX_BACKUP_NUM  5
> +#define LOGFILE_MAX_SIZE(1024*128)

These should surely be configurable by the end-user.

> +/* Append only abstraction for log file */
> +struct logfile_entry {
> + int fd;
> + off_t pos;
> + off_t len;
> +};

AFAICT these types could be defined inside logfile.c, and `struct
logfile' declared (but not defined) here in the header file.

That would make the abstraction clearer.

> diff --git a/tools/console/daemon/logfile.c b/tools/console/daemon/logfile.c
> new file mode 100644
> index 000..ad73f8e
> --- /dev/null
> +++ b/tools/console/daemon/logfile.c
...
> +static void logfile_entry_free(struct logfile_entry *entry)
> +{
> + if (!entry) return;
> +
> + if (entry->fd >= 0) close(entry->fd);
> + free(entry);
> +}

I do wonder whether it's actually worth having a whole separate struct
for an `entry'.  Each `struct logfile' has zero or one `struct
logfile_entry's but most of the time exactly one.

Eliminating the separate struct logfile_entry and folding it into
struct logfile would save some memory allocation (and associated error
handling).

> + entry = calloc(1, sizeof(*entry));
> + if (!entry) goto err;

This pattern calls close(0) on error!  Either declare that fd==0 means
"none here" (which would be sane IMO) or initialise fd=-1.

> + entry->fd = open(path, O_CREAT|O_APPEND|O_WRONLY|O_CLOEXEC, mode);

I think we had concluded that O_CLOEXEC wasn't sufficiently portable ?
It's not needed here because xenconsoled does not exec.

> + entry->pos = lseek(entry->fd, 0, SEEK_END);
> + if (entry->pos == (off_t)-1) {
> + dolog(LOG_ERR, "Unable to determine current file offset in %s",
> +   path);
> + goto err;
> + }
> +
> + if (fstat(entry->fd, ) < 0) {
> + dolog(LOG_ERR, "Unable to fstat current file %s", path);
> + goto err;
> + }

I'm not sure why you need to keep track of `len' and `pos'
separately.  AFAICT len is never used.

> +struct logfile *logfile_new(const char *path, mode_t mode)
> +{

Why does logfile_new take a mode for which all callers pass 644 ?
Do we anticipate logs with different permissions ?

> + if (rename(this, next) < 0 && errno != ENOENT) {
> + dolog(LOG_ERR, "Failed to rename %s to %s",
> +   this, next);
> + goto err;
> + }

If LOGFILE_MAX_BACKUP_NUM becomes configurable, this loop will have to
become more sophisticated.  I think it may have to count up and then
down again.

> +static ssize_t write_all(int fd, const char* buf, size_t len)
> +{
...

This is a copy of an identical function in io.c.

> +ssize_t logfile_append(struct logfile *file, const char *buf,
> +size_t len)
> +{
> + ssize_t ret = 0;
> +
> + while (len) {
> + struct logfile_entry *entry = file->entry;
> + size_t towrite = len;
> + bool rollover = false;
> +
> + if (entry->pos > file->maxlen) {
> + rollover = true;
> + towrite = 0;
> + } else if (entry->pos + towrite > file->maxlen) {
> + towrite = file->maxlen - entry->pos;
> + }
...
[and later]
> + if ((entry->pos == file->maxlen && len) || rollover) {

This logic is rather tangled.  Why not

  maxwrite = file->maxlen - file->pos;
  if (towrite > maxwrite) {
  rollover = 1;
  towrite = maxwrite;
  if (towrite < 0)
  towrite = 0;
  }

and then later simply

if (rollover) {

?

> +
> + if (towrite) {
> + if (write_all(entry->fd, buf, towrite) != towrite) {
> + dolog(LOG_ERR, "Unable to write file %s",
> +   file->basepath);
> + return -1;
> + }
> +
> + len -= towrite;
> + buf += towrite;
> + ret += towrite;

I'm not a huge fan of this approach where several control variables
need to be kept in step.  You could save the beginning and end of the
buffer, and then you could do away with ret and no longer need to
modify len.

Ian.

___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH 2/6] xenconsoled: switch hypervisor log to use logfile abstraction

[Ian Jackson]

Wei Liu writes ("[PATCH 2/6] xenconsoled: switch hypervisor log to use logfile 
abstraction"):
> To minimise code churn, copy and paste a some existing functions and
> adapt them to write to logfile. The functions to deal with fd directly
> will go away eventually.
...
> +static int write_logfile(struct logfile *logfile, const char *buf,
> +  size_t len)
> +{
> + ssize_t r = logfile_append(logfile, buf, len);
> +
> + if (r < 0) return -1;
> + return 0;
> +}

That this is necessary suggests that logfile_append has an
inconvenient calling convention.

> +static int write_logfile_with_timestamp(struct logfile *logfile,
> + const char *data, size_t sz,
> + int *needts)
> +{

This should be in logfile.c.  Furthermore, this is at the wrong level
with respect to log rotation:

> + if ((*needts && logfile_append(logfile, ts, tslen))

This could result in a timestamp being split across two different
logfiles which is undesirable.  So I think this needs to be done at
the layer below.

> + data = nl + 1;
> + if (found_nl) {
> + // If we printed a newline, strip all \r following it
> + while (data <= last_byte && *data == '\r')
> + data++;
> + }

I would prefer that we only apply a lossless transformation to
logfiles, even if that means that our lotfiles contain CRs.  Also,
this is rather odd.  Surely it is more usual for the CR to precede the
LF.

I suggest the transformation `after every LF, preface the next byte
with the timestamp of that next byte'.

> - if (fd != -1 && log_time_hv) {
> - if (write_with_timestamp(fd, "Logfile Opened\n",
> -  strlen("Logfile Opened\n"),
> -  _time_hv_needts) < 0) {
> +
> + if (tmp && log_time_hv) {
> + if (write_logfile_with_timestamp(tmp, "Logfile Opened\n",
> +  strlen("Logfile Opened\n"),
> +  _time_hv_needts) < 0) {

I think that if timestamps are turned off, writing the "Logfile
Opened" (with no preceding timestamp) is probably a good idea.  So
this can be made simpler.

>   if (log_hv) {
> - if (log_hv_fd != -1)
> - close(log_hv_fd);
> - log_hv_fd = create_hv_log();
> + if (log_hv_file)
> + logfile_free(log_hv_file);
> + log_hv_file = create_hv_log();

You could separate out the nonfunctional changes if you did it in
stages:

 * provide logfile_valid as a dummy macro that tests against -1

 * replace `log_hv_fd' with `log_hv' throughout; replace
   comparisons with -1 with logfile_valid(log_hv); likewise
   client logfiles - no functional change

 * replace log opening code with calls to logfile_open; change
   types; change implementation of logfile_valid

Ian.

___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [DRAFT 1] XenSock protocol design document

[Stefano Stabellini]

On Fri, 8 Jul 2016, David Vrabel wrote:
> On 08/07/16 15:16, Stefano Stabellini wrote:
> > 
> > http://pubs.opengroup.org/onlinepubs/7908799/xns/connect.html
> 
> Are you really guaranteeing full POSIX semantics for all these calls?
> And not say, POSIX-like except where Linux decides to differ because
> POSIX is dumb?
> 
> How is the guest (which expects the semantics of its own OS) going to
> know that connect(2) to an external IP is going to behave differently to
> say connect(2) to localhost?
>
> a) The difficulties in reconciling the differences in  behaviour and
> features between remoted system calls and local ones.
> 
> b) The difficulty in fully specifying (and thus fully implementing) the
> PV interface.

I'll refrain from replying to these points because they are about the
implementation, rather than the protocol, which it will be discussed
separately when I manage to publish the design document of the drivers.
I am confident we can solve these issue if we work together
constructively. I noticed you are not making any suggestions on how to
solve these issues, which is good form when doing reviews.

I want to address the following point first:


> c) My belief that most of the advantages of this proposal can be
> achieved with smarts in the backend.

By backend do you mean netfront/netback? If so, I have already pointed
out why that is not the case in previous emails as well as in this
design document.

If you remain unconvinced of the usefulness of this work, that's OK, we
can agree to disagree. Many people work on things I don't believe
particularly useful myself. I am not asking you to spend any time on
this if you don't believe it serves a purpose. But please let the rest
of the community work constructively together.

___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH] vmx/monitor: CPUID events

[Andrew Cooper]

On 08/07/16 16:44, Tamas K Lengyel wrote:
> On Fri, Jul 8, 2016 at 3:33 AM, Andrew Cooper  
> wrote:
>> On 08/07/16 03:31, Tamas K Lengyel wrote:
>>> This patch implements sending notification to a monitor subscriber when an
>>> x86/vmx guest executes the CPUID instruction.
>>>
>>> Signed-off-by: Tamas K Lengyel 
>> Is it wise having an on/off control without any further filtering?  (I
>> suppose that it is at least a fine first start).
> What type of extra filtering do you have in mind?

Not sure.  What are you intending to use this facility for?

Given that the hypervisor is already in complete control of what a guest
gets to see via cpuid, mutating the results via the monitor framework
doesn't seem like a useful thing to do.

>
>> cpuid is usually the serialising instruction used with rdtsc for timing
>> loops.  This is bad enough in VMs because of the VMExit, but becomes
>> even worse if there is a monitor delay as well.
> Yes, going the extra route of sending a monitor event out will add to
> that delay (how much delay will depend on the subscriber and what it
> decides to do with the event). Wouldn't we be able to mask some of
> that with tsc offsetting though?

I am going to go out on a limb and say that that is a very large can of
worms which you don't want to open.

The problem is not that time skews from the point of view of the guest,
but that the timing loop with a fixed number of iterations takes
proportionally longer.

~Andrew

___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel