Re: [Xen-devel] [PATCH 02/18] xen: allow global VIRQ handlers to be delegated to other domains

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 11.01.12 at 18:21, Daniel De Graaf <dgdegra@xxxxxxxxxxxxx> wrote:
> @@ -659,6 +659,8 @@ static void complete_domain_destroy(struct rcu_head *head)
>  
>      watchdog_domain_destroy(d);
>  
> +    clear_global_virq_handlers(d);

This is too late, I'm afraid. The domain can't possibly service the vIRQ(s)
anymore as soon as its destruction begins.

> +
>      rangeset_domain_destroy(d);
>  
>      cpupool_rm_domain(d);
> @@ -739,6 +739,67 @@ int send_guest_pirq(struct domain *d, const struct pirq 
> *pirq)
>      return evtchn_set_pending(d->vcpu[chn->notify_vcpu_id], port);
>  }
>  
> +static struct domain* global_virq_handlers[NR_VIRQS];

__read_mostly?

Also, the formatting is wrong (also elsewhere in the patch) - the
space should be before the star, not after.

> +
> +spinlock_t global_virq_handlers_lock = SPIN_LOCK_UNLOCKED;

static? DEFINE_SPINLOCK()?

> +
> +static struct domain* _get_global_virq_handler(int virq)
> +{
> +    struct domain *d;
> +
> +    d = global_virq_handlers[virq];
> +    return d != NULL ? d : dom0;

This can be done in a single line:

    return global_virq_handlers[virq] ?: dom0;

> +}
> +
> +void send_global_virq(int virq)
> +{
> +    ASSERT(virq >= 0 && virq < NR_VIRQS);
> +    ASSERT(virq_is_global(virq));
> +
> +    send_guest_global_virq(_get_global_virq_handler(virq), virq);
> +}
> +
> +int set_global_virq_handler(struct domain *d, int virq)

In the domctl interface structure the virq (correctly) is uint32_t,
so why is it (signed) int here (and elsewhere)?

> +{
> +    struct domain *old;
> +
> +    if (virq < 0 || virq >= NR_VIRQS)

The < 0 part would then become unnecessary.

> +        return -EINVAL;
> +    if (!virq_is_global(virq))
> +        return -EINVAL;
> +
> +    if (global_virq_handlers[virq] == d)
> +        return 0;
> +
> +    if (unlikely(!get_domain(d)))
> +        return -EINVAL;
> +
> +    spin_lock(&global_virq_handlers_lock);
> +
> +    old = global_virq_handlers[virq];
> +    global_virq_handlers[virq] = d;
> +    if (old != NULL)
> +        put_domain(old);

This should happen outside the lock.

> +    spin_unlock(&global_virq_handlers_lock);
> +
> +    return 0;
> +}
> +
> +void clear_global_virq_handlers(struct domain *d)
> +{
> +    int virq;
> +
> +    spin_lock(&global_virq_handlers_lock);
> +
> +    for (virq = 0; virq < NR_VIRQS; virq++) {
> +        if (global_virq_handlers[virq] == d) {
> +            global_virq_handlers[virq] = NULL;
> +            put_domain(d);

Same here (albeit resulting in some code growth).

> +        }
> +    }
> +
> +    spin_unlock(&global_virq_handlers_lock);
> +}
>  
>  static long evtchn_status(evtchn_status_t *status)
>  {
> @@ -912,6 +918,7 @@ struct xen_domctl {
>  #define XEN_DOMCTL_getvcpuextstate               63
>  #define XEN_DOMCTL_set_access_required           64
>  #define XEN_DOMCTL_audit_p2m                     65
> +#define XEN_DOMCTL_set_virq_handler              71

Any reason for picking a non-contiguous value here?

>  #define XEN_DOMCTL_gdbsx_guestmemio            1000
>  #define XEN_DOMCTL_gdbsx_pausevcpu             1001
>  #define XEN_DOMCTL_gdbsx_unpausevcpu           1002

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel