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

[Razvan Cojocaru <rcojocaru@xxxxxxxxxxxxxxx>]

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.


Thanks,
Razvan

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