Re: [Xen-devel] [PATCH V2] x86/vm_event: Added support for VM_EVENT_REASON_INTERRUPT

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 10/11/16 15:53, Razvan Cojocaru wrote:
> On 11/10/2016 05:47 PM, Jan Beulich wrote:
>>>>> On 10.11.16 at 09:35, <rcojocaru@xxxxxxxxxxxxxxx> wrote:
>>> Changes since V1:
>>>  - Modified the if() in hvm_do_resume() for readability.
>>>  - Replaced hard tab with spaces.
>>>  - Removed a local variable used only once.
>>>  - Moved cr2 assignment to the common part of the code.
>>>  - Now listing the new event in the x86 vm_event capability list.
>>>  - Moved struct variables for readability.
>> Hmm, looks like you've moved the field in the structure declaration,
>> but not the two initializers (in SVM and VMX code).
> I'll modify those as well.
>
>>> --- a/xen/arch/x86/hvm/hvm.c
>>> +++ b/xen/arch/x86/hvm/hvm.c
>>> @@ -535,9 +535,24 @@ void hvm_do_resume(struct vcpu *v)
>>>      /* Inject pending hw/sw trap */
>>>      if ( v->arch.hvm_vcpu.inject_trap.vector != -1 )
>>>      {
>>> -        hvm_inject_trap(&v->arch.hvm_vcpu.inject_trap);
>>> +        if ( !hvm_event_pending(v) )
>>> +            hvm_inject_trap(&v->arch.hvm_vcpu.inject_trap);
>>> +
>>>          v->arch.hvm_vcpu.inject_trap.vector = -1;
>>>      }
>>> +
>>> +    if ( unlikely(v->arch.vm_event) &&
>>> +         v->arch.vm_event->monitor_next_interrupt )
>>> +    {
>>> +        struct hvm_trap info;
>>> +
>>> +        if ( hvm_get_pending_event(v, &info) )
>>> +        {
>>> +            hvm_monitor_interrupt(info.vector, info.type, info.error_code,
>>> +                                  info.cr2);
>>> +            v->arch.vm_event->monitor_next_interrupt = false;
>>> +        }
>>> +    }
>>>  }
>>>  
>>>  static int hvm_print_line(
>>> @@ -6047,6 +6062,12 @@ void hvm_domain_soft_reset(struct domain *d)
>>>      hvm_destroy_all_ioreq_servers(d);
>>>  }
>>>  
>>> +bool hvm_get_pending_event(struct vcpu *v, struct hvm_trap *info)
>>> +{
>>> +    info->cr2 = v->arch.hvm_vcpu.guest_cr[2];
>>> +    return hvm_funcs.get_pending_event(v, info);
>>> +}
>> Unless you expect more callers, I'm tempted to suggest to make this
>> static for now (and move it up ahead of its only caller).
> Will do.
>
>>> --- a/xen/arch/x86/vm_event.c
>>> +++ b/xen/arch/x86/vm_event.c
>>> @@ -134,6 +134,12 @@ void vm_event_set_registers(struct vcpu *v, 
>>> vm_event_response_t *rsp)
>>>      v->arch.user_regs.eip = rsp->data.regs.x86.rip;
>>>  }
>>>  
>>> +void vm_event_monitor_next_interrupt(struct vcpu *v)
>>> +{
>>> +    ASSERT(v->arch.vm_event);
>>> +    v->arch.vm_event->monitor_next_interrupt = true;
>>> +}
>> I think at this point we're determined to no longer permit ASSERT()s
>> like this: Either use a simple if() or a BUG_ON(). Andrew, please
>> correct me if I've misunderstood earlier discussions.
> I'll change it to a simple if().
>
>>> @@ -259,6 +266,14 @@ struct vm_event_cpuid {
>>>      uint32_t _pad;
>>>  };
>>>  
>>> +struct vm_event_interrupt_x86 {
>>> +    uint32_t vector;
>>> +    uint32_t type;
>>> +    uint32_t error_code;
>>> +    uint64_t cr2;
>>> +    uint32_t _pad;
>>> +};
>> I'm pretty certain this is not what you want, as this make the layout
>> vary between 32-bit (compat) and 64-bit (native) callers.
> I'll remove the _pad.

You need to invert the pad and cr2, so cr2 starts at 16 bytes into the
structure.

Remember that uint64_t has 8 byte alignment in 64bit, but only 4 byte
alignment in 32bit.

~Andrew

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