Re: [Xen-devel] [PATCH V2 2/3] xen/vm_event: Support for guest-requested events

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 15.06.15 at 11:03, <rcojocaru@xxxxxxxxxxxxxxx> wrote:
> Added support for a new class of vm_events: VM_EVENT_REASON_REQUEST,
> sent via HVMOP_request_vm_event. The guest can request that a
> generic vm_event (containing only the vm_event-filled guest registers
> as information) be sent to userspace by setting up the correct
> registers and doing a VMCALL. For example, for a 64-bit guest, this
> means: EAX = 34 (hvmop), EBX = 24 (HVMOP_request_vm_event).

I suppose you mean a 32-bit guest here? Also I'm not sure it's a good
idea to explicitly define a guest exposed hypercall to omit one of the
arguments normally required for it (the interface structure pointer):
Should there ever be a reason to allow the guest to control further
aspects of the operation by passing a structure, you'd then have to
define a new sub-op instead of being able to re-use the current one.
I.e. I'd strongly recommend requiring NULL to be passed here, and
checking this in the implementation of the handler.

> --- a/xen/arch/x86/hvm/event.c
> +++ b/xen/arch/x86/hvm/event.c
> @@ -126,6 +126,20 @@ void hvm_event_msr(unsigned int msr, uint64_t value)
>          hvm_event_traps(1, &req);
>  }
>  
> +void hvm_event_requested(void)
> +{
> +    struct vcpu *curr = current;
> +    struct arch_domain *currad = &curr->domain->arch;
> +
> +    vm_event_request_t req = {

Please avoid blank lines between declarations - a blank line following
declaration is supposed to delimit them from statements.

> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -6373,6 +6373,10 @@ long do_hvm_op(unsigned long op, 
> XEN_GUEST_HANDLE_PARAM(void) arg)
>          break;
>      }
>  
> +    case HVMOP_request_vm_event:
> +        hvm_event_requested();
> +        break;

No XSM check here or in the handler? Shouldn't the admin controlling
guest properties from the host perspective be permitted control here?
Cc-ing Daniel for his input ...

> --- a/xen/include/asm-x86/domain.h
> +++ b/xen/include/asm-x86/domain.h
> @@ -342,13 +342,15 @@ struct arch_domain
>  
>      /* Monitor options */
>      struct {
> -        uint16_t write_ctrlreg_enabled       : 4;
> -        uint16_t write_ctrlreg_sync          : 4;
> -        uint16_t write_ctrlreg_onchangeonly  : 4;
> -        uint16_t mov_to_msr_enabled          : 1;
> -        uint16_t mov_to_msr_extended         : 1;
> -        uint16_t singlestep_enabled          : 1;
> -        uint16_t software_breakpoint_enabled : 1;
> +        uint32_t write_ctrlreg_enabled       : 4;
> +        uint32_t write_ctrlreg_sync          : 4;
> +        uint32_t write_ctrlreg_onchangeonly  : 4;
> +        uint32_t mov_to_msr_enabled          : 1;
> +        uint32_t mov_to_msr_extended         : 1;
> +        uint32_t singlestep_enabled          : 1;
> +        uint32_t software_breakpoint_enabled : 1;
> +        uint32_t request_enabled             : 1;
> +        uint32_t request_sync                : 1;

Can you please switch to plain unsigned int if you already have to
touch this? There's no reason I can see to use a fixed width integer
type here.

> --- a/xen/include/public/hvm/hvm_op.h
> +++ b/xen/include/public/hvm/hvm_op.h
> @@ -389,6 +389,10 @@ DEFINE_XEN_GUEST_HANDLE(xen_hvm_evtchn_upcall_vector_t);
>  
>  #endif /* defined(__i386__) || defined(__x86_64__) */
>  
> +#if defined(__XEN__) || defined(__XEN_TOOLS__)
> +#define HVMOP_request_vm_event 24
> +#endif /* defined(__XEN__) || defined(__XEN_TOOLS__) */

Isn't this _specifically_ meant to be usable by a guest?

Jan


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