Re: [PATCH v3 1/2] libirqhelp: user interrupt handler helper library

[Samuel Thibault]

Damien Zammit, le dim. 25 sept. 2022 04:12:27 +0000, a ecrit:
> +static void
> +toggle_irq(struct irq *irq, bool on)
> +{
> +  pthread_mutex_lock (&irq->irqlock);
> +  irq->enabled = on;
> +  pthread_cond_signal (&irq->irqcond);

Better only signal when `on` is true, since nobody is waiting for
`irq->enabled` to become false.

> +  pthread_mutex_unlock (&irq->irqlock);
> +}
> +
> +void
> +irqhelp_disable_irq(int gsi)
> +{
> +  struct irq *irq = lookup_irq_structure(gsi);
> +  if (!irq)
> +    return;

I'd say at least somehow warn? That's not expected.

> +
> +  toggle_irq(irq, false);
> +}
> +
> +void
> +irqhelp_enable_irq(int gsi)
> +{
> +  struct irq *irq = lookup_irq_structure(gsi);
> +  if (!irq)
> +    return;
> +
> +  toggle_irq(irq, true);
> +}
> +
> +void *
> +irqhelp_server_loop(void *arg)
> +{
> +  struct irq *irq = (struct irq *)arg;
> +
> +  if (!irq)
> +    return NULL;
> +
> +  /* init hook */
> +  if (irq->init_hook)
> +    irq->init_hook(irq->context);

I don't understand why these hooks. If the library user wants to do
something at the beginning of irqhelp_server_loop, it call just do it
before calling irqhelp_server_loop.

> +  int interrupt_demuxer (mach_msg_header_t *inp,
> +                      mach_msg_header_t *outp)
> +  {
> +    device_intr_notification_t *n = (device_intr_notification_t *) inp;
> +
> +    ((mig_reply_header_t *) outp)->RetCode = MIG_NO_REPLY;
> +    if (n->intr_header.msgh_id != DEVICE_INTR_NOTIFY)
> +      return 0;  /* not an interrupt */
> +
> +    /* FIXME: id <-> gsi now has an indirection, assuming 1:1 */
> +    if (n->id != irq->gsi)
> +      return 0;  /* interrupt not for us */
> +
> +    /* wait if irq disabled */
> +    pthread_mutex_lock (&irq->irqlock);
> +    while (!irq->enabled)
> +      pthread_cond_wait (&irq->irqcond, &irq->irqlock);
> +    pthread_mutex_unlock (&irq->irqlock);
> +
> +    /* pre-handler */
> +    if (irq->pre_hook)
> +      irq->pre_hook(irq->context);
> +
> +    /* call handler */
> +    irq->handler(irq->context);
> +
> +    /* post-handler */
> +    if (irq->post_hook)
> +      irq->post_hook(irq->context);

And similarly if the library user wants to do something before and after
the handler is called, it can just create its own handler that does
pre-stuff, calls the underlying handler, and does post-stuff. AIUI in
the ddekit case we don't even need it.

> +    /* ACK interrupt */
> +    device_intr_ack (irqdev, irq->port, MACH_MSG_TYPE_MAKE_SEND);
> +
> +    return 1;
> +  }
> +
> +  /* Server loop */
> +  mach_msg_server (interrupt_demuxer, 0, irq->port);
> +
> +  return NULL;
> +}
> +
> +static struct irq *
> +interrupt_register(int gsi,
> +                int bus,
> +                int dev,
> +                int fun,
> +                void (*handler)(void *),
> +                void *context,
> +                int *cookie)
> +{
> +  mach_port_t delivery_port;
> +  mach_port_t pset, psetcntl;
> +  error_t err;
> +  struct irq *irq = NULL;
> +
> +  irq = malloc(sizeof(struct irq));
> +  if (!irq)
> +    return NULL;
> +
> +  LIST_INSERT_HEAD(&irqs, irq, entries);

It doesn't seem to be used?  I don't think we'll use it actually, if we
make irqhelp_remove_interrupt_handler just take the irq structure.

> +  irq->handler = handler;
> +  irq->context = context;
> +  irq->gsi = gsi;
> +  irq->init_hook = NULL;     /* can be overriden later by caller */
> +  irq->pre_hook = NULL;              /* can be overriden later by caller */
> +  irq->post_hook = NULL;     /* can be overriden later by caller */
> +  irq->enabled = true;               /* don't require initial explicit 
> enable */
> +  pthread_mutex_init (&irq->irqlock, NULL);
> +  pthread_cond_init (&irq->irqcond, NULL);
> +
> +  err = mach_port_allocate (mach_task_self (), MACH_PORT_RIGHT_RECEIVE,
> +                         &delivery_port);
> +  if (err)
> +    goto fail;
> +
> +  irq->port = delivery_port;
> +
> +  err = thread_get_assignment (mach_thread_self (), &pset);
> +  if (err)
> +    goto fail;
> +
> +  err = host_processor_set_priv (master_host, pset, &psetcntl);
> +  if (err)
> +    goto fail;
> +
> +  thread_max_priority (mach_thread_self (), psetcntl, 0);
> +  err = thread_priority (mach_thread_self (), IRQ_THREAD_PRIORITY, 0);
> +  if (err)
> +    goto fail;
> +
> +  err = device_intr_register(irqdev, irq->gsi,
> +                             0, irq->port,
> +                             MACH_MSG_TYPE_MAKE_SEND);
> +  if (err)
> +    goto fail;
> +
> +  *cookie = irq->cookie = atomic_increment (&refcnt);

Do we still need a cookie? Since we already return an opaque structure
that the library user could just pass to irqhelp_remove_interrupt_handler?

> +  return irq;
> +
> +fail:
> +  pthread_cond_destroy(&irq->irqcond);
> +  pthread_mutex_destroy(&irq->irqlock);
> +  free(irq);
> +  return NULL;
> +}
> +
> +
> +/* Accepts gsi or bus/dev/fun or both, but cant be all -1.
> +   If gsi is -1, will lookup the gsi via ACPI.
> +   If bus/dev/fun are -1, must pass in gsi.
> +   Accepts a pointer to a cookie to be used to
> +   deregister the handler later.  */
> +struct irqhelp *
> +irqhelp_install_interrupt_handler(int gsi,
> +                               int bus,
> +                               int dev,
> +                               int fun,
> +                               void (*handler)(void *),
> +                               void *context,
> +                               int *cookie)
> +{
> +  struct irq *irq;
> +  error_t err;
> +
> +  if (!handler)
> +    return NULL;
> +
> +  if (!cookie)
> +    return NULL;
> +
> +  err = get_irq();
> +  if (err)
> +    return NULL;
> +
> +  if (gsi < 0)
> +    {
> +      if ((bus < 0) || (dev < 0) || (fun < 0))
> +        return NULL;
> +
> +      err = get_acpi();
> +      if (err)
> +        return NULL;
> +
> +      /* We need to call acpi translator to get gsi */
> +      err = acpi_get_pci_irq (acpidev, bus, dev, fun, &gsi);
> +      if (err)
> +        return NULL;
> +    }
> +
> +  err = get_privileged_ports (&master_host, 0);
> +  if (err)
> +    return NULL;
> +
> +  irq = interrupt_register(gsi, bus, dev, fun, handler, context, cookie);
> +
> +  mach_port_deallocate (mach_task_self (), master_host);
> +  return (struct irqhelp *)irq;
> +}
> +
> +error_t
> +irqhelp_remove_interrupt_handler(int gsi,
> +                              int bus,
> +                              int dev,
> +                              int fun,
> +                              int cookie)
> +{
> +  /* Not implemented yet, but don't fail */
> +  return 0;
> +}
> diff --git a/libirqhelp/irqhelp.h b/libirqhelp/irqhelp.h
> new file mode 100644
> index 00000000..292d912d
> --- /dev/null
> +++ b/libirqhelp/irqhelp.h
> @@ -0,0 +1,39 @@
> +/* Library providing helper functions for userspace irq handling.
> +   Copyright (C) 2022 Free Software Foundation, Inc.
> +
> +   This program is free software; you can redistribute it and/or
> +   modify it under the terms of the GNU General Public License as
> +   published by the Free Software Foundation; either version 2, or (at
> +   your option) any later version.
> +
> +   This program is distributed in the hope that it will be useful, but
> +   WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> +   General Public License for more details.
> +
> +   You should have received a copy of the GNU General Public License
> +   along with this program; if not, write to the Free Software
> +   Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA. */
> +
> +#ifndef _HURD_IRQHELP_
> +#define _HURD_IRQHELP_
> +
> +#include <mach.h>
> +#include <hurd/hurd_types.h>
> +#include <pthread.h>
> +#include <stdlib.h>
> +
> +struct irqhelp {
> +  void (*init_hook)(void *);
> +  void (*pre_hook)(void *);
> +  void (*post_hook)(void *);
> +};
> +
> +struct irqhelp * irqhelp_install_interrupt_handler(int gsi, int bus, int 
> dev, int fun,
> +                                                void (*handler)(void *), 
> void *context, int *cookie);
> +error_t irqhelp_remove_interrupt_handler(int gsi, int bus, int dev, int fun, 
> int cookie);
> +void * irqhelp_server_loop(void *arg);
> +void irqhelp_enable_irq(int gsi);
> +void irqhelp_disable_irq(int gsi);
> +
> +#endif
> --
> 2.34.1
> 
> 
>