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

["Jan Beulich" <JBeulich@xxxxxxxx>]

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

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

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

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

Jan


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