Re: [RFC PATCH v6 16/92] kvm: introspection: handle events and event replies

2019-08-13 Thread Adalbert Lazăr
On Tue, 13 Aug 2019 10:55:21 +0200, Paolo Bonzini  wrote:
> On 09/08/19 17:59, Adalbert Lazăr wrote:
> > 
> > +reply->padding2);
> > +
> > +   ivcpu->reply_waiting = false;
> > +   return expected->error;
> > +}
> > +
> >  /*
> 
> Is this missing a wakeup?
> 
> >  
> > +static bool need_to_wait(struct kvm_vcpu *vcpu)
> > +{
> > +   struct kvmi_vcpu *ivcpu = IVCPU(vcpu);
> > +
> > +   return ivcpu->reply_waiting;
> > +}
> > +
> 
> Do you actually need this function?  It seems to me that everywhere you
> call it you already have an ivcpu, so you can just access the field.
> 
> Also, "reply_waiting" means "there is a reply that is waiting".  What
> you mean is "waiting_for_reply".

In an older version, handle_event_reply() was executed from the receiving
thread (having another name) and it contained a wakeup function. Now,
indeed, 'waiting_for_reply' is the right name.
 
> The overall structure of the jobs code is confusing.  The same function
> kvm_run_jobs_and_wait is an infinite loop before and gets a "break"
> later.  It is also not clear why kvmi_job_wait is called through a job.
>  Can you have instead just kvm_run_jobs in KVM_REQ_INTROSPECTION, and
> something like this instead when sending an event:
> 
> int kvmi_wait_for_reply(struct kvm_vcpu *vcpu)
> {
>   struct kvmi_vcpu *ivcpu = IVCPU(vcpu);
> 
>   while (ivcpu->waiting_for_reply) {
>   kvmi_run_jobs(vcpu);
> 
>   err = swait_event_killable(*wq,
>   !ivcpu->waiting_for_reply ||
>   !list_empty(>job_list));
> 
>   if (err)
>   return -EINTR;
>   }
> 
>   return 0;
> }
> 
> ?
> 
> Paolo

Much better :) Thank you.

___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [RFC PATCH v6 16/92] kvm: introspection: handle events and event replies

2019-08-13 Thread Paolo Bonzini
On 09/08/19 17:59, Adalbert Lazăr wrote:
> 
> +  reply->padding2);
> +
> + ivcpu->reply_waiting = false;
> + return expected->error;
> +}
> +
>  /*

Is this missing a wakeup?

>  
> +static bool need_to_wait(struct kvm_vcpu *vcpu)
> +{
> + struct kvmi_vcpu *ivcpu = IVCPU(vcpu);
> +
> + return ivcpu->reply_waiting;
> +}
> +

Do you actually need this function?  It seems to me that everywhere you
call it you already have an ivcpu, so you can just access the field.

Also, "reply_waiting" means "there is a reply that is waiting".  What
you mean is "waiting_for_reply".

The overall structure of the jobs code is confusing.  The same function
kvm_run_jobs_and_wait is an infinite loop before and gets a "break"
later.  It is also not clear why kvmi_job_wait is called through a job.
 Can you have instead just kvm_run_jobs in KVM_REQ_INTROSPECTION, and
something like this instead when sending an event:

int kvmi_wait_for_reply(struct kvm_vcpu *vcpu)
{
struct kvmi_vcpu *ivcpu = IVCPU(vcpu);

while (ivcpu->waiting_for_reply) {
kvmi_run_jobs(vcpu);

err = swait_event_killable(*wq,
!ivcpu->waiting_for_reply ||
!list_empty(>job_list));

if (err)
return -EINTR;
}

return 0;
}

?

Paolo
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

[RFC PATCH v6 16/92] kvm: introspection: handle events and event replies

2019-08-09 Thread Adalbert Lazăr
From: Mihai Donțu 

All events are sent by the vCPU thread, which will handle any
introspection command while waiting for the reply.

The event reply messages contain a common strucure (kvmi_vcpu_hdr), as
any vCPU related command, which allows the receiving worker to dispatch
the reply as it does with any other introspection command sent for a
specific vCPU.

The kernel side will gracefully handle commands coming from an
introspection tool compiled with older or newer versions of KVMI API.
However, it will only accept smaller replies (coming from older versions),
but not the bigger/newer ones (this should make the kernel code simpler).

TODO: Not quite true. An event reply has a common part (kvmi_event_reply)
and an event specific part (eg. the new value for MSR x). If the common
part is smaller, the event will be rejected.

The code from handle_event_reply():

common = sizeof(struct kvmi_vcpu_hdr) + sizeof(*reply);
if (unlikely(msg->size < common))
goto out;

should be changed to

min_common = sizeof(struct kvmi_vcpu_hdr) + offsetof(reply...)
if (unlikely(msg->size < min_common))
goto out;

Signed-off-by: Mihai Donțu 
Co-developed-by: Adalbert Lazăr 
Signed-off-by: Adalbert Lazăr 
---
 Documentation/virtual/kvm/kvmi.rst |  56 +
 arch/x86/include/uapi/asm/kvmi.h   |  29 +++
 arch/x86/kvm/Makefile  |   2 +-
 arch/x86/kvm/kvmi.c|  92 
 arch/x86/kvm/x86.c |  10 +++
 include/linux/kvm_host.h   |   3 +
 include/uapi/linux/kvmi.h  |  16 
 virt/kvm/kvmi.c|  15 
 virt/kvm/kvmi_int.h|  16 
 virt/kvm/kvmi_msg.c| 129 +
 10 files changed, 367 insertions(+), 1 deletion(-)
 create mode 100644 arch/x86/include/uapi/asm/kvmi.h
 create mode 100644 arch/x86/kvm/kvmi.c

diff --git a/Documentation/virtual/kvm/kvmi.rst 
b/Documentation/virtual/kvm/kvmi.rst
index 7f3c4f8fce63..e7d9a3816e00 100644
--- a/Documentation/virtual/kvm/kvmi.rst
+++ b/Documentation/virtual/kvm/kvmi.rst
@@ -427,3 +427,59 @@ in almost all cases, it must reply with: continue, retry, 
crash, etc.
 * -KVM_EINVAL - padding is not zero
 * -KVM_EPERM - the access is restricted by the host
 
+Events
+==
+
+All vCPU events are sent using the *KVMI_EVENT* message id. No event
+will be sent unless explicitly enabled with a *KVMI_CONTROL_EVENTS*
+or a *KVMI_CONTROL_VM_EVENTS* command or requested, as it is the case
+with the *KVMI_EVENT_PAUSE_VCPU* event (see **KVMI_PAUSE_VCPU**).
+
+There is one VM event, *KVMI_EVENT_UNHOOK*, which doesn't have a reply,
+but shares the kvmi_event structure, for consistency with the vCPU events.
+
+The message data begins with a common structure, having the size of the
+structure, the vCPU index and the event id::
+
+   struct kvmi_event {
+   __u16 size;
+   __u16 vcpu;
+   __u8 event;
+   __u8 padding[3];
+   struct kvmi_event_arch arch;
+   }
+
+On x86 the structure looks like this::
+
+   struct kvmi_event_arch {
+   __u8 mode;
+   __u8 padding[7];
+   struct kvm_regs regs;
+   struct kvm_sregs sregs;
+   struct {
+   __u64 sysenter_cs;
+   __u64 sysenter_esp;
+   __u64 sysenter_eip;
+   __u64 efer;
+   __u64 star;
+   __u64 lstar;
+   __u64 cstar;
+   __u64 pat;
+   __u64 shadow_gs;
+   } msrs;
+   };
+
+It contains information about the vCPU state at the time of the event.
+
+The reply to events have the *KVMI_EVENT_REPLY* message id and begins
+with two common structures::
+
+   struct kvmi_vcpu_hdr;
+   struct kvmi_event_reply {
+   __u8 action;
+   __u8 event;
+   __u16 padding1;
+   __u32 padding2;
+   };
+
+Specific data can follow these common structures.
diff --git a/arch/x86/include/uapi/asm/kvmi.h b/arch/x86/include/uapi/asm/kvmi.h
new file mode 100644
index ..551f9ed1ed9c
--- /dev/null
+++ b/arch/x86/include/uapi/asm/kvmi.h
@@ -0,0 +1,29 @@
+/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
+#ifndef _UAPI_ASM_X86_KVMI_H
+#define _UAPI_ASM_X86_KVMI_H
+
+/*
+ * KVM introspection - x86 specific structures and definitions
+ */
+
+#include 
+
+struct kvmi_event_arch {
+   __u8 mode;  /* 2, 4 or 8 */
+   __u8 padding[7];
+   struct kvm_regs regs;
+   struct kvm_sregs sregs;
+   struct {
+   __u64 sysenter_cs;
+   __u64 sysenter_esp;
+   __u64 sysenter_eip;
+   __u64 efer;
+   __u64 star;
+   __u64 lstar;
+   __u64 cstar;
+   __u64 pat;
+