From mboxrd@z Thu Jan 1 00:00:00 1970 From: Paolo Bonzini Subject: Re: [RFC PATCH v2 1/1] kvm: Add documentation and ABI/API header for VM introspection Date: Mon, 10 Jul 2017 19:03:06 +0200 Message-ID: References: <20170707143416.11195-1-alazar@bitdefender.com> <20170707143416.11195-2-alazar@bitdefender.com> <7104167e-0747-92fe-05df-1b7e1848d65f@redhat.com> <1499700768.37E8E87C5.13190@host> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit Cc: =?UTF-8?B?UmFkaW0gS3LEjW3DocWZ?= , Jan Kiszka , Stefan Hajnoczi , Mihai Dontu To: alazar@bitdefender.com, kvm@vger.kernel.org Return-path: Received: from mx1.redhat.com ([209.132.183.28]:38532 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754488AbdGJRDL (ORCPT ); Mon, 10 Jul 2017 13:03:11 -0400 In-Reply-To: <1499700768.37E8E87C5.13190@host> Content-Language: en-US Sender: kvm-owner@vger.kernel.org List-ID: On 10/07/2017 17:32, alazar@bitdefender.com wrote: > Thanks for your review, Paolo. > Below are some answers. > Will have to chew on the others. I'm not sure what you think of removing KVMI_EVENT_ACTION_SET_REGS and more or less standardizing on actions SKIP/RETRY/ALLOW/CRASH. The main remaining issue seems to be map/unmap. Paolo > On Fri, 7 Jul 2017 18:52:45 +0200, Paolo Bonzini wrote: >> On 07/07/2017 16:34, Adalbert Lazar wrote: >>> +:Returns: ↴ >> >> What is this character? :) > > I thought using a nice right-down arrow instead of a boring/inexpressive > ASCII char in order to avoid trailing spaces will be cool. At least it > was fun :) > >>> + struct kvmi_get_version_reply { >>> + __s32 err; >>> + __u32 version; >>> + }; >>> + >>> +Returns the introspection API version (the KVMI_VERSION constant) and the >>> +error code (zero). In case of an unlikely error, the version will have an >>> +undefined value. >> >> Would it make sense to return a few more information fields, for example >> the set of supported KVMI_CONTROL_EVENTS events? > > Sure. > >>> + struct kvmi_get_guest_info_reply { >>> + __s32 err; >>> + __u16 vcpu_count; >>> + __u16 padding; >>> + __u64 tsc_speed; >>> + }; >>> + >>> +Returns the number of online vcpus, and the TSC frequency in HZ, if supported >>> +by the architecture (otherwise is 0). >> >> Can the TSC frequency be specified only if the guest is using TSC >> scaling? Defining the TSC frequency on older hosts is a bit tricky. 0 >> would be the host. >> >> Maybe define the second padding to be >> >> __u16 arch; >> >> (0 = x86) followed by an arch-specific payload. > > Sure. > >>> +5. KVMI_SHUTDOWN_GUEST >>> +---------------------- >>> + >>> +Ungracefully shutdown the guest. >> >> Note that all KVM can do here is pass down a request asynchronously to >> userspace, failing further KVM_RUN ioctls. I'm suggesting below an >> alternative action. > > We were thinking about sending an "ungraceful" TERM signal :D > > Adding another reponse (exit-to-userspace-with-shutdown) on events, > as you suggested below, makes sense. > >>> +12. KVMI_SET_PAGE_ACCESS >>> +------------------------ >>> + >>> +Sets the spte flags (rwx - present, write & user) - access - for the specified >>> +vCPU and guest physical address. >> >> rwx or pwu? I suppose RWX. Maybe #define the constants in the >> documentation. > > RWX. > Will do. > >>> +17. KVMI_CONTROL_EVENTS >>> +----------------------- >>> + >>> +:Architectures: all >>> +:Versions: >= 1 >>> +:Parameters: ↴ >>> + >>> +:: >>> + >>> + struct kvmi_control_events { >>> + __u16 vcpu; >>> + __u16 padding; >>> + __u32 events; >>> + }; >>> + >>> +:Returns: ↴ >>> + >>> +:: >>> + >>> + struct kvmi_error_code { >>> + __s32 err; >>> + __u32 padding; >>> + }; >>> + >>> +Enables/disables vCPU introspection events, by setting/clearing one or more >>> +of the following bits (see 'Events' below) : >>> + >>> + KVMI_EVENT_CR >>> + KVMI_EVENT_MSR >>> + KVMI_EVENT_XSETBV >>> + KVMI_EVENT_BREAKPOINT >>> + KVMI_EVENT_USER_CALL >>> + KVMI_EVENT_PAGE_FAULT >>> + KVMI_EVENT_TRAP >>> + >>> +Trying to enable unsupported events (~KVMI_KNOWN_EVENTS) by the current >>> +architecture would fail and -EINVAL will be returned. >> >> I would prefer the interface to allow enable (set bits), disable (clear >> bits) in addition to set (what you have here) the events. > > Calling KVMI_CONTROL_EVENTS with event=KVMI_EVENT_CR|KVMI_EVENT_MSR will > enable CR and MSR events and disable all the other events. It functions like > a set and clear, in the same time. > > It will be better to have KVMI_ENABLE_CONTROL_EVENTS and > KVMI_DISABLE_CONTROL_EVENTS instead? > >> The return value could indicate the set of enabled events. > > Indeed. > >>> +Events >>> +------ >>> + >>> +All vcpu events are sent using the KVMI_EVENT_VCPU message id. No event will >>> +be sent unless enabled with a KVMI_CONTROL_EVENTS command. >>> + >>> +For x86, the message data starts with a common structure:: >>> + >>> + struct kvmi_event_x86 { >>> + __u16 vcpu; >>> + __u8 mode; >>> + __u8 padding1; >>> + __u32 event; >>> + struct kvm_regs regs; >>> + struct kvm_sregs sregs; >>> + struct { >>> + __u64 sysenter_cs; >>> + __u64 sysenter_esp; >>> + __u64 sysenter_eip; >>> + __u64 efer; >>> + __u64 star; >>> + __u64 lstar; >> >> cstar too, for AMD CPUs. > > Thanks. Will do. > >>> + } msrs; >>> + }; >>> + >>> +In order to help the introspection tool with the event analysis while >>> +avoiding unnecessary introspection commands, the message data holds some >>> +registers (kvm_regs, kvm_sregs and a couple of MSR-s) beside >>> +the vCPU id, its mode (in bytes) and the event (one of the flags set >>> +with the KVMI_CONTROL_EVENTS command). >>> + >>> +The replies to events also start with a common structure, having the >>> +KVMI_EVENT_VCPU_REPLY message id:: >>> + >>> + struct kvmi_event_x86_reply { >>> + struct kvm_regs regs; >> >> Put regs last? >> >>> + __u32 actions; >>> + __u32 padding; >>> + }; > > Right. > >>> +5. KVMI_EVENT_USER_CALL >>> +----------------------- >> >> Please rename this to KVMI_EVENT_HCALL or HYPERCALL or VMCALL. > > Sure. > >>> + >>> +:Architectures: x86 >>> +:Versions: >= 1 >>> +:Parameters: ↴ >>> + >>> +:: >>> + >>> + struct kvmi_event_x86; >>> + >>> +:Returns: ↴ >>> + >>> +:: >>> + >>> + struct kvmi_event_x86_reply; >>> + >>> +This event is sent on a user hypercall and the introspection has already >>> +already been enabled for this kind of event (KVMI_CONTROL_EVENTS). >>> + >>> +kvmi_event_x86 is sent to the introspector, which can respond with the >>> +KVMI_EVENT_ACTION_SET_REGS bit set in 'actions', instructing the host >>> +kernel to override the general purpose registers using the values from >>> +introspector (regs). >> >> Does KVMI_EVENT_ACTION_SET_REGS bypass the hypercall? Why is >> KVMI_EVENT_ACTION_ALLOW not allowed? As before, I'd prefer >> SKIP/RETRY/ALLOW/CRASH as the actions. > > We've used this as way for guest code to communicate with the introspector. > KVMI_EVENT_ACTION_ALLOW is implied here. > >>> --- /dev/null >>> +++ b/include/uapi/linux/kvmi.h > ... >> >> Errnos values are not portable, I'd prefer to have them defined >> explicitly in the header. > > We were thinking that the introspection tool will take care of this, > as with the endianness. > > Surely, it will be much more clear to have the errnos defined here. > > Thanks again, > > Adalbert >