Re: [Xen-devel] [PATCH v4] x86/hvm: Add per-vcpu evtchn upcalls

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 07.01.15 at 19:06, <paul.durrant@xxxxxxxxxx> wrote:
> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -5462,6 +5462,40 @@ static int hvmop_destroy_ioreq_server(
>      return rc;
>  }
>  
> +static int hvmop_set_evtchn_upcall_vector(
> +    XEN_GUEST_HANDLE_PARAM(xen_hvm_set_evtchn_upcall_vector_t) uop)
> +{
> +    xen_hvm_set_evtchn_upcall_vector_t op;
> +    struct domain *d;
> +    struct vcpu *v;
> +    int rc;
> +
> +    if ( copy_from_guest(&op, uop, 1) )
> +        return -EFAULT;
> +
> +    d = rcu_lock_current_domain();

I don't think you need this, current->domain should be just fine here.

> +
> +    rc = -EINVAL;
> +    if ( !is_hvm_domain(d) )
> +        goto out;
> +
> +    if ( op.vector < 0x10 )
> +        goto out;
> +
> +    rc = -ENOENT;
> +    if ( op.vcpu >= d->max_vcpus || (v = d->vcpu[op.vcpu]) == NULL )
> +        goto out;
> +
> +    printk(XENLOG_G_INFO "%pv: %s %u\n", v, __func__, op.vector);

Rather than printing the function name here, can't you make the
message text more meaningful? Or otherwise this ought to be a
gdprintk(). 

> --- a/xen/arch/x86/hvm/irq.c
> +++ b/xen/arch/x86/hvm/irq.c
> @@ -152,6 +152,13 @@ void hvm_isa_irq_deassert(
>      spin_unlock(&d->arch.hvm_domain.irq_lock);
>  }
>  
> +static void hvm_set_upcall_irq(struct vcpu *v)
> +{
> +    uint8_t vector = v->arch.hvm_vcpu.evtchn_upcall_vector;
> +
> +    vlapic_set_irq(vcpu_vlapic(v), vector, 0);
> +}

Is this small a single-use helper function really warranted?

> @@ -220,6 +227,8 @@ void hvm_assert_evtchn_irq(struct vcpu *v)
>  
>      if ( is_hvm_pv_evtchn_vcpu(v) )
>          vcpu_kick(v);
> +    else if ( v->arch.hvm_vcpu.evtchn_upcall_vector != 0 )
> +        hvm_set_upcall_irq(v);

In the public header comment you say that this new mechanism takes
precedence over callback-via, yet here you place it in an "else" to the
is_hvm_pv_evtchn_vcpu() check. Which one is correct/intended?

> --- a/xen/include/public/hvm/hvm_op.h
> +++ b/xen/include/public/hvm/hvm_op.h
> @@ -369,6 +369,26 @@ 
> DEFINE_XEN_GUEST_HANDLE(xen_hvm_set_ioreq_server_state_t);
>  
>  #endif /* defined(__XEN__) || defined(__XEN_TOOLS__) */
>  
> +#if defined(__i386__) || defined(__x86_64__)
> +
> +/*
> + * HVMOP_set_evtchn_upcall_vector: Set a <vector> that should be used for 
> event
> + *                                 channel upcalls on the specified <vcpu>. 
> If set,
> + *                                 this vector will be used in preference to 
> the
> + *                                 domain callback via (see 
> HVM_PARAM_CALLBACK_IRQ)
> + *                                 and hence allows HVM guests to bind event
> + *                                 event channels to a vcpu other than 0.
> + */

The wording suggests that the callback-via mechanism is tied to vCPU0,
which iiuc it isn't (and I think I said so in response to an earlier version).

Jan


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