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

[Razvan Cojocaru <rcojocaru@bitdefender.com>]

On 06/26/2015 10:02 AM, Jan Beulich wrote:
>>>> On 15.06.15 at 11:03, <rcojocaru@bitdefender.com> 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.

You're right, I've tested it on a 64-bit guest but indeed compiled the
executable as a Win32 binary, hence the confusion. I'll correct the
patch comment.

Will look into the NULL parameter option.

>> --- 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.

Ack.

>> --- 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 ...

Not sure this warrants XSM checks, but sure, if they're required I'll
try to put them in.

>> --- 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.

Ack, will make it plain int.

>> --- 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?

Yes, but we still need Xen tools to subscribe to the events, control how
they are received (in a dom0 or similarly privileged domain) and process
them. That was my train of thought anyway, maybe I'm missing something
(or I'm misinterpreting the macros).


Thanks,
Razvan