Re: [PATCH RFC V2 4/6] xen: Support for VMCALL mem_events

[Razvan Cojocaru <rcojocaru@bitdefender.com>]

On 07/11/2014 08:23 PM, Andrew Cooper wrote:
> On 11/07/14 16:43, Razvan Cojocaru wrote:
>> Added support for VMCALL events (the memory introspection library
>> will have the guest trigger VMCALLs, which will then be sent along
>> via the mem_event mechanism).
>>
>> Changes since V1:
>>  - Added a #define and an comment explaining a previous magic
>>    constant.
>>  - Had MEM_EVENT_REASON_VMCALL explicitly not honour
>>    HVMPME_onchangeonly.
>>
>> Signed-off-by: Razvan Cojocaru <rcojocaru@bitdefender.com>
>> ---
>>  xen/arch/x86/hvm/hvm.c          |    9 +++++++++
>>  xen/arch/x86/hvm/vmx/vmx.c      |   18 +++++++++++++++++-
>>  xen/include/asm-x86/hvm/hvm.h   |    1 +
>>  xen/include/public/hvm/params.h |    4 +++-
>>  xen/include/public/mem_event.h  |    5 +++++
>>  5 files changed, 35 insertions(+), 2 deletions(-)
>>
>> diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
>> index 89a0382..6e86d7c 100644
>> --- a/xen/arch/x86/hvm/hvm.c
>> +++ b/xen/arch/x86/hvm/hvm.c
>> @@ -5564,6 +5564,7 @@ long do_hvm_op(unsigned long op, XEN_GUEST_HANDLE_PARAM(void) arg)
>>              case HVM_PARAM_MEMORY_EVENT_INT3:
>>              case HVM_PARAM_MEMORY_EVENT_SINGLE_STEP:
>>              case HVM_PARAM_MEMORY_EVENT_MSR:
>> +            case HVM_PARAM_MEMORY_EVENT_VMCALL:
>>                  if ( d == current->domain )
>>                  {
>>                      rc = -EPERM;
>> @@ -6199,6 +6200,14 @@ void hvm_memory_event_msr(unsigned long msr, unsigned long value)
>>                             value, ~value, 1, msr);
>>  }
>>  
>> +void hvm_memory_event_vmcall(unsigned long rip, unsigned long eax)
>> +{
>> +    hvm_memory_event_traps(current->domain->arch.hvm_domain
>> +                             .params[HVM_PARAM_MEMORY_EVENT_VMCALL],
>> +                           MEM_EVENT_REASON_VMCALL,
>> +                           rip, ~rip, 1, eax);
>> +}
>> +
>>  int hvm_memory_event_int3(unsigned long gla) 
>>  {
>>      uint32_t pfec = PFEC_page_present;
>> diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
>> index 2caa04a..6c63225 100644
>> --- a/xen/arch/x86/hvm/vmx/vmx.c
>> +++ b/xen/arch/x86/hvm/vmx/vmx.c
>> @@ -2879,8 +2879,24 @@ void vmx_vmexit_handler(struct cpu_user_regs *regs)
>>      case EXIT_REASON_VMCALL:
>>      {
>>          int rc;
>> +        unsigned long eax = regs->eax;
>> +
>>          HVMTRACE_1D(VMMCALL, regs->eax);
>> -        rc = hvm_do_hypercall(regs);
>> +
>> +        /* Don't send a VMCALL mem_event unless something
>> +         * caused the guests's eax register to contain the
>> +         * VMCALL_EVENT_REQUEST constant. */
>> +        if ( regs->eax != VMCALL_EVENT_REQUEST )
>> +        {
>> +            rc = hvm_do_hypercall(regs);
>> +        }
>> +        else
>> +        {
>> +            hvm_memory_event_vmcall(guest_cpu_user_regs()->eip, eax);
>> +            update_guest_eip();
>> +            break;
>> +        }
> 
> Thinking more about this, it is really a hypercall pretending not to
> be.  It would be better to introduce a real HVMOP_send_mem_event.
> 
> From the point of view of your in-guest agent, it would be a vmcall with
> rax = 34 (hvmop) rdi = $N (send_mem_event subop) rsi = data or pointer
> to struct containing data, depending on how exactly you implement the
> hypercall.
> 
> You would have the bonus of being able to detect errors, e.g. -ENOENT
> for "mem_event not active", get SVM support for free, and not need magic
> numbers, or vendor specific terms like "vmcall" finding their way into
> the Xen public API.

Actually, this only seems to be the case where mode == 8 in
hvm_do_hypercall() (xen/arch/x86/hvm/hvm.c):

4987                      : hvm_hypercall64_table)[eax](rdi, rsi, rdx,
r10, r8, r9);

Otherwise (and this seems to be the case with my Xen build), ebx seems
to be used for the subop:

5033         regs->_eax = hvm_hypercall32_table[eax](ebx, ecx, edx, esi,
edi, ebp);

So, ebx needs to be $N (send_mem_event subop), not rdi. Is this intended
(rdi in one case and ebx in the other)?


Thanks,
Razvan