Thanks for your review, Paolo.
Below are some answers.
Will have to chew on the others.
On Fri, 7 Jul 2017 18:52:45 +0200, Paolo Bonzini <pbonzini@xxxxxxxxxx> 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
