Re: [Xen-devel] [PATCH v3] x86/hvm: Allow guest_request vm_events coming from userspace

["Jan Beulich" <jbeulich@xxxxxxxx>]

>>> Alexandru Isaila <aisaila@xxxxxxxxxxxxxxx> 08/03/17 5:29 PM >>>
>--- a/xen/arch/x86/hvm/hypercall.c
>+++ b/xen/arch/x86/hvm/hypercall.c
>@@ -152,9 +152,17 @@ int hvm_hypercall(struct cpu_user_regs *regs)
>{
>case 8:
>eax = regs->rax;
>+        if ( currd->monitor.guest_request_userspace_vmcall &&
>+             eax == __HYPERVISOR_hvm_op &&
>+             regs->rdi == HVMOP_guest_request_vm_event )
>+            break;
>/* Fallthrough to permission check. */
>case 4:
>case 2:
>+        if ( mode != 8 && currd->monitor.guest_request_userspace_vmcall &&
>+             eax == __HYPERVISOR_hvm_op &&
>+             regs->ebx == HVMOP_guest_request_vm_event )
>+            break;

Let's limit ugliness and redundancy as much as possible:

if ( currd->monitor.guest_request_userspace_vmcall &&
    eax == __HYPERVISOR_hvm_op &&
    (mode == 8 ? regs->rdi : regs->ebx) == HVMOP_guest_request_vm_event )

with the first half above dropped altogether.

>--- a/xen/include/public/domctl.h
>+++ b/xen/include/public/domctl.h
>@@ -1073,16 +1073,17 @@ DEFINE_XEN_GUEST_HANDLE(xen_domctl_psr_cmt_op_t);
>#define XEN_DOMCTL_MONITOR_OP_GET_CAPABILITIES  2
>#define XEN_DOMCTL_MONITOR_OP_EMULATE_EACH_REP  3
 >
>-#define XEN_DOMCTL_MONITOR_EVENT_WRITE_CTRLREG         0
>-#define XEN_DOMCTL_MONITOR_EVENT_MOV_TO_MSR            1
>-#define XEN_DOMCTL_MONITOR_EVENT_SINGLESTEP            2
>-#define XEN_DOMCTL_MONITOR_EVENT_SOFTWARE_BREAKPOINT   3
>-#define XEN_DOMCTL_MONITOR_EVENT_GUEST_REQUEST         4
>-#define XEN_DOMCTL_MONITOR_EVENT_DEBUG_EXCEPTION       5
>-#define XEN_DOMCTL_MONITOR_EVENT_CPUID                 6
>-#define XEN_DOMCTL_MONITOR_EVENT_PRIVILEGED_CALL       7
>-#define XEN_DOMCTL_MONITOR_EVENT_INTERRUPT             8
>-#define XEN_DOMCTL_MONITOR_EVENT_DESC_ACCESS           9
>+#define XEN_DOMCTL_MONITOR_EVENT_WRITE_CTRLREG          0
>+#define XEN_DOMCTL_MONITOR_EVENT_MOV_TO_MSR             1
>+#define XEN_DOMCTL_MONITOR_EVENT_SINGLESTEP             2
>+#define XEN_DOMCTL_MONITOR_EVENT_SOFTWARE_BREAKPOINT    3
>+#define XEN_DOMCTL_MONITOR_EVENT_GUEST_REQUEST          4
>+#define XEN_DOMCTL_MONITOR_EVENT_DEBUG_EXCEPTION        5
>+#define XEN_DOMCTL_MONITOR_EVENT_CPUID                  6
>+#define XEN_DOMCTL_MONITOR_EVENT_PRIVILEGED_CALL        7
>+#define XEN_DOMCTL_MONITOR_EVENT_INTERRUPT              8
>+#define XEN_DOMCTL_MONITOR_EVENT_DESC_ACCESS            9

This isn't the first time I see this whole block being re-indented. I'd suggest 
to
either increase indentation to a maximum (i.e. for the right side to be just 
below
80 cols) or to accept extremely long entries to stand out.

+#define XEN_DOMCTL_MONITOR_EVENT_GUEST_USERSPACE_VMCALL 10
 
I dislike the mention of VMCALL (which is an insn mnemonic after all) here,
and I also think the name suggests broader access than is actually being
granted. Realizing the redundancy I'd still think
XEN_DOMCTL_MONITOR_EVENT_GUEST_USERSPACE_EVENT would be better

>--- a/xen/include/xen/sched.h
>+++ b/xen/include/xen/sched.h
>@@ -480,8 +480,9 @@ struct domain
 >
>/* Common monitor options */
>struct {
>-        unsigned int guest_request_enabled       : 1;
>-        unsigned int guest_request_sync          : 1;
>+        unsigned int guest_request_enabled          : 1;
>+        unsigned int guest_request_sync             : 1;
>+        unsigned int guest_request_userspace_vmcall : 1;

Same here then.

Jan


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