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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 09.11.16 at 10:42, <rcojocaru@xxxxxxxxxxxxxxx> wrote:
> Added support for a new event type, VM_EVENT_REASON_INTERRUPT,
> which is now fired in a one-shot manner when enabled via the new
> VM_EVENT_FLAG_GET_NEXT_INTERRUPT vm_event response flag.
> The patch also fixes the behaviour of the xc_hvm_inject_trap()
> hypercall, which would lead to non-architectural interrupts
> overwriting pending (specifically reinjected) architectural ones.

Looks quite okay, just some more or less mechanical comments.

> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -532,11 +532,23 @@ void hvm_do_resume(struct vcpu *v)
>          }
>      }
>  
> -    /* Inject pending hw/sw trap */
> -    if ( v->arch.hvm_vcpu.inject_trap.vector != -1 )
> -    {
> +    /* Inject pending hw/sw trap if there are no other pending interrupts. */
> +    if ( v->arch.hvm_vcpu.inject_trap.vector != -1 && !hvm_event_pending(v) )
>          hvm_inject_trap(&v->arch.hvm_vcpu.inject_trap);
> -        v->arch.hvm_vcpu.inject_trap.vector = -1;
> +
> +    v->arch.hvm_vcpu.inject_trap.vector = -1;

I don't see why you pull this out of the if() body.

> +    if ( unlikely(v->arch.vm_event) &&
> +      v->arch.vm_event->monitor_next_interrupt )

Hard tab.

> --- a/xen/arch/x86/hvm/monitor.c
> +++ b/xen/arch/x86/hvm/monitor.c
> @@ -150,6 +150,21 @@ int hvm_monitor_cpuid(unsigned long insn_length, 
> unsigned int leaf,
>      return monitor_traps(curr, 1, &req);
>  }
>  
> +void hvm_monitor_interrupt(unsigned int vector, unsigned int type,
> +                           unsigned int err, uint64_t cr2)
> +{
> +    struct vcpu *curr = current;

Pointless local variable (used just once).

> --- a/xen/arch/x86/hvm/svm/svm.c
> +++ b/xen/arch/x86/hvm/svm/svm.c
> @@ -2220,6 +2220,21 @@ static void svm_invlpg(struct vcpu *v, unsigned long 
> vaddr)
>      svm_asid_g_invlpg(v, vaddr);
>  }
>  
> +static bool svm_get_pending_event(struct vcpu *v, struct hvm_trap *info)
> +{
> +    struct vmcb_struct *vmcb = v->arch.hvm_svm.vmcb;
> +
> +    if ( vmcb->eventinj.fields.v )
> +        return false;
> +
> +    info->vector = vmcb->eventinj.fields.vector;
> +    info->type = vmcb->eventinj.fields.type;
> +    info->error_code = vmcb->eventinj.fields.errorcode;
> +    info->cr2 = v->arch.hvm_vcpu.guest_cr[2];

I'd prefer for this last part to be put into generic code (i.e. the
wrapper).

> --- a/xen/include/asm-arm/vm_event.h
> +++ b/xen/include/asm-arm/vm_event.h
> @@ -52,4 +52,10 @@ void vm_event_emulate_check(struct vcpu *v, 
> vm_event_response_t *rsp)
>      /* Not supported on ARM. */
>  }
>  
> +static inline
> +void vm_event_monitor_next_interrupt(struct vcpu *v)
> +{
> +    /* Not supported on ARM. */
> +}

That's unfortunate. If it can't be implemented, shouldn't the caller at
least be advised of this being unavailable? Wasn't there even some
mechanism to report capabilities?

> --- a/xen/include/asm-x86/hvm/hvm.h
> +++ b/xen/include/asm-x86/hvm/hvm.h
> @@ -237,6 +237,8 @@ struct hvm_function_table {
>          /* Architecture function to setup TSC scaling ratio */
>          void (*setup)(struct vcpu *v);
>      } tsc_scaling;
> +
> +    bool (*get_pending_event)(struct vcpu *v, struct hvm_trap *info);
>  };

Stylistically I think this would better go a little earlier.

> --- a/xen/include/asm-x86/vm_event.h
> +++ b/xen/include/asm-x86/vm_event.h
> @@ -32,6 +32,7 @@ struct arch_vm_event {
>          struct vm_event_emul_insn_data insn;
>      } emul;
>      struct monitor_write_data write_data;
> +    bool monitor_next_interrupt;
>  };

I think there's a 32-bit padding hole before write_data, so the new
field would better go earlier (perhaps even right after flags).

> @@ -139,6 +144,8 @@
>   *       These kinds of events will be filtered out in future versions.
>   */
>  #define VM_EVENT_REASON_PRIVILEGED_CALL         11
> +/* Result of toolstack-requested (non-architectural) trap injection. */
> +#define VM_EVENT_REASON_INTERRUPT               12

Considering the event reports all kinds of interruptions, I don't think
the comment is appropriate.

> @@ -259,6 +266,13 @@ struct vm_event_cpuid {
>      uint32_t _pad;
>  };
>  
> +struct vm_event_interrupt {
> +    uint32_t vector;
> +    uint32_t type;
> +    uint32_t error_code;
> +    uint64_t cr2;
> +};

This being x86-specific, I think it should be named or union-ized
accordingly.

Jan

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