Re: [PATCH v2 2/2] ddekit: Use libirqhelp

[Samuel Thibault]

Damien Zammit, le jeu. 22 sept. 2022 04:42:41 +0000, a ecrit:
> This makes ddekit depend on irqhelp and cleans up the irq registration.
> 
> TESTED: using this change and recompiling netdde,
> I was able to remotely access the vm over ssh.

That's not enough of a test. IRQs are tricky, you'd have to transfer
some GB of data to be a bit more confident that it's not too bad.

> ---
>  libddekit/Makefile    |   2 +-
>  libddekit/interrupt.c | 231 ++++--------------------------------------
>  2 files changed, 19 insertions(+), 214 deletions(-)
> 
> diff --git a/libddekit/Makefile b/libddekit/Makefile
> index 88a0c8909..c74ec1128 100644
> --- a/libddekit/Makefile
> +++ b/libddekit/Makefile
> @@ -39,7 +39,7 @@ LCLHDRS = $(installhdrs)    \
> 
>  OBJS = $(sort $(SRCS:.c=.o))
> 
> -HURDLIBS = ports shouldbeinlibc hurd-slab
> +HURDLIBS = ports shouldbeinlibc hurd-slab irqhelp
>  LDLIBS += -lpthread
> 
>  MIGCOMSFLAGS = -prefix dde_
> diff --git a/libddekit/interrupt.c b/libddekit/interrupt.c
> index 803a00c02..9f05d80c7 100644
> --- a/libddekit/interrupt.c
> +++ b/libddekit/interrupt.c
> @@ -4,8 +4,6 @@
>   * \author  Christian Helmuth <ch12@os.inf.tu-dresden.de>
>   * \date    2007-01-22
>   *
> - * FIXME could intloop_param freed after startup?
> - * FIXME use consume flag to indicate IRQ was handled
>   */
> 
>  #include <stdio.h>
> @@ -13,154 +11,21 @@
>  #include <mach.h>
>  #include <hurd.h>
> 
> -#include <device/notify.h>
> -#include <device/device.h>
> +#include "libirqhelp/irqhelp.h"
> 
>  #include "ddekit/interrupt.h"
> -#include "ddekit/semaphore.h"
>  #include "ddekit/printf.h"
> -#include "ddekit/memory.h"
> -#include "ddekit/condvar.h"
> 
> -#define DEBUG_INTERRUPTS  0
> -
> -#define MAX_INTERRUPTS   32
> -
> -#define BLOCK_IRQ         0
> -
> -/*
> - * Internal type for interrupt loop parameters
> +/* Token acquired to unregister interrupt handler
> + * NB: we only need one of these for now
>   */
> -struct intloop_params
> -{
> -     unsigned          irq;       /* irq number */
> -     int               shared;    /* irq sharing supported? */
> -     void(*thread_init)(void *);  /* thread initialization */
> -     void(*handler)(void *);      /* IRQ handler function */
> -     void             *priv;      /* private token */
> -     mach_port_t       irqport;   /* delivery port for notifications */
> -     ddekit_sem_t     *started;
> -
> -     int               start_err;
> -};
> +static int irq_cookie;
> 
> -static struct
> +static void unwrapped_server_loop(void *arg)
>  {
> -     int              handle_irq; /* nested irq disable count */
> -     ddekit_lock_t    irqlock;
> -     struct ddekit_condvar   *cond;
> -     ddekit_thread_t  *irq_thread; /* thread ID for detaching from IRQ later 
> on */
> -     boolean_t        thread_exit;
> -     thread_t         mach_thread;
> -} ddekit_irq_ctrl[MAX_INTERRUPTS];
> -
> -static mach_port_t master_host;
> -static mach_port_t master_device;
> -static device_t irq_dev;
> -
> -/**
> - * Interrupt service loop
> - *
> - */
> -static void intloop(void *arg)
> -{
> -     kern_return_t ret;
> -     struct intloop_params *params = arg;
> -     mach_port_t delivery_port;
> -     mach_port_t pset, psetcntl;
> -     int my_index;
> -
> -     ret = mach_port_allocate (mach_task_self (), MACH_PORT_RIGHT_RECEIVE,
> -                               &delivery_port);
> -     if (ret)
> -       error (2, ret, "mach_port_allocate");
> -
> -     my_index = params->irq;
> -     params->irqport = delivery_port;
> -     ddekit_irq_ctrl[my_index].mach_thread = mach_thread_self ();
> -     ret = thread_get_assignment (mach_thread_self (), &pset);
> -     if (ret)
> -             error (0, ret, "thread_get_assignment");
> -     ret = host_processor_set_priv (master_host, pset, &psetcntl);
> -     if (ret)
> -             error (0, ret, "host_processor_set_priv");
> -     thread_max_priority (mach_thread_self (), psetcntl, 0);
> -     ret = thread_priority (mach_thread_self (), DDEKIT_IRQ_PRIO, 0);
> -     if (ret)
> -             error (0, ret, "thread_priority");
> -
> -     // TODO the flags for shared irq should be indicated by params->shared.
> -     // Flags needs to be 0 for new irq interface for now.
> -     // Otherwise, the interrupt handler cannot be installed in the kernel.
> -     ret = device_intr_register (irq_dev, my_index,
> -                               0, delivery_port,
> -                               MACH_MSG_TYPE_MAKE_SEND);
> -     ddekit_printf ("device_intr_register returns %d\n", ret);
> -     if (ret) {
> -             /* inform thread creator of error */
> -             /* XXX does omega0 error code have any meaning to DDEKit users? 
> */
> -             params->start_err = ret;
> -             ddekit_sem_up(params->started);
> -             ddekit_printf ("cannot install irq %d\n", my_index);
> -             return;
> -     }
> -
> -#if 0
> -     /*
> -      * Setup an exit fn. This will make sure that we clean up everything,
> -      * before shutting down an IRQ thread.
> -      */
> -     if (l4thread_on_exit(&exit_fn, (void *)my_index) < 0)
> -             ddekit_panic("Could not set exit handler for IRQ fn.");
> -#endif
> -
> -     /* after successful initialization call thread_init() before doing 
> anything
> -      * else here */
> -     if (params->thread_init) params->thread_init(params->priv);
> -
> -     /* save handle + inform thread creator of success */
> -     params->start_err = 0;
> -     ddekit_sem_up(params->started);
> -
> -     int irq_server (mach_msg_header_t *inp, mach_msg_header_t *outp) {
> -             device_intr_notification_t *intr_header = 
> (device_intr_notification_t *) inp;
> -
> -             ((mig_reply_header_t *) outp)->RetCode = MIG_NO_REPLY;
> -             if (inp->msgh_id != DEVICE_INTR_NOTIFY)
> -                     return 0;
> -
> -             /* It's an interrupt not for us. It shouldn't happen. */
> -             if (intr_header->id != params->irq) {
> -                     ddekit_printf ("We get interrupt %d, %d is expected",
> -                                    intr_header->id, params->irq);
> -                     return 1;
> -             }
> -
> -             /* only call registered handler function, if IRQ is not 
> disabled */
> -             ddekit_lock_lock (&ddekit_irq_ctrl[my_index].irqlock);
> -             while (ddekit_irq_ctrl[my_index].handle_irq <= 0) {
> -                     ddekit_condvar_wait (ddekit_irq_ctrl[my_index].cond,
> -                                     &ddekit_irq_ctrl[my_index].irqlock);
> -                     // TODO if it's edged-triggered irq, the interrupt will 
> be lost.

Yes, that way of handling disabled irqs looked bizarre.

However, I believe we still do want to keep some kind of disabling, as
the drivers will not expect the handler to be called at all when the
interrupt is disabled. In our case, disabling would mean pausing the
reception of the interrupt notifications, e.g. by using a pthread_cond_t
to make the interrupt thread sleep until the interrupt is enabled again.

> -             }
> -             params->handler(params->priv);
> -
> -             /* Acknowledge the interrupt */
> -             device_intr_ack (irq_dev, params->irqport, 
> MACH_MSG_TYPE_MAKE_SEND);
> -
> -             if (ddekit_irq_ctrl[my_index].thread_exit) {
> -                     ddekit_lock_unlock (&ddekit_irq_ctrl[my_index].irqlock);
> -                     ddekit_thread_exit();
> -                     return 1;
> -             }
> -             ddekit_lock_unlock (&ddekit_irq_ctrl[my_index].irqlock);
> -             return 1;
> -     }
> -
> -     mach_msg_server (irq_server, 0, delivery_port);
> +     irqhelp_server_loop(arg);
>  }
> 
> -
>  /**
>   * Attach to hardware interrupt
>   *
> @@ -178,54 +43,26 @@ ddekit_thread_t *ddekit_interrupt_attach(int irq, int 
> shared,
>                                           void(*thread_init)(void *),
>                                           void(*handler)(void *), void *priv)
>  {
> -     struct intloop_params *params;
> -     ddekit_thread_t *thread;
>       char thread_name[10];
> -
> -     /* We cannot attach the interrupt to the irq which has been used. */
> -     if (ddekit_irq_ctrl[irq].irq_thread)
> -       return NULL;
> -
> -     /* initialize info structure for interrupt loop */
> -     params = ddekit_simple_malloc(sizeof(*params));
> -     if (!params) return NULL;
> -
> -     // TODO make sure irq is 0-15 instead of 1-16.
> -     params->irq         = irq;
> -     params->thread_init = thread_init;
> -     params->handler     = handler;
> -     params->priv        = priv;
> -     params->started     = ddekit_sem_init(0);
> -     params->start_err   = 0;
> -     params->irqport     = MACH_PORT_NULL;
> -     params->shared      = shared;
> +     ddekit_thread_t *thread;
> +     void *irqhelp_priv;
> 
>       /* construct name */
> -     snprintf(thread_name, 10, "irq%02X", irq);
> +     snprintf(thread_name, sizeof(thread_name), "irq%02X", irq);
> +
> +     irqhelp_priv = irqhelp_install_interrupt_handler(irq, -1, -1, -1,
> +                                                      handler, priv, 
> &irq_cookie);
> 
> -     ddekit_irq_ctrl[irq].handle_irq = 1; /* IRQ nesting level is initially 
> 1 */
> -     ddekit_lock_init_unlocked (&ddekit_irq_ctrl[irq].irqlock);
> -     ddekit_irq_ctrl[irq].cond = ddekit_condvar_init ();
> -     ddekit_irq_ctrl[irq].thread_exit = FALSE;
> +     /* after successful initialization call thread_init() before doing 
> anything
> +      * else here */
> +     if (thread_init)
> +             thread_init(priv);

No, thread_init needs to be called from the thread that will call the
handler. See libdde's irq_thread_init, it uses
l4dde26_process_add_worker to register the thread to the rest of dde.

> -     /* allocate irq */
>       /* create interrupt loop thread */
> -     thread = ddekit_thread_create(intloop, params, thread_name);
> +     thread = ddekit_thread_create(unwrapped_server_loop, irqhelp_priv, 
> thread_name);
>       if (!thread) {
> -             ddekit_simple_free(params);
> -             return NULL;
> -     }
> -     ddekit_irq_ctrl[irq].irq_thread = thread;
> -
> -
> -     /* wait for intloop initialization result */
> -     ddekit_sem_down(params->started);
> -     ddekit_sem_deinit(params->started);
> -     if (params->start_err) {
> -             ddekit_simple_free(params);
>               return NULL;
>       }
> -
>       return thread;
>  }
> 
> @@ -236,51 +73,19 @@ ddekit_thread_t *ddekit_interrupt_attach(int irq, int 
> shared,
>  void ddekit_interrupt_detach(int irq)
>  {
>       ddekit_interrupt_disable(irq);
> -     // TODO  the code should be removed.
> -     ddekit_lock_lock (&ddekit_irq_ctrl[irq].irqlock);
> -     if (ddekit_irq_ctrl[irq].handle_irq == 0) {
> -             ddekit_irq_ctrl[irq].thread_exit = TRUE;
> -             ddekit_irq_ctrl[irq].irq_thread = NULL;
> -
> -             /* If the irq thread is waiting for interrupt notification
> -              * messages, thread_abort() can force it to return.
> -              * I hope this ugly trick can work. */
> -             thread_abort (ddekit_irq_ctrl[irq].mach_thread);
> -     }
> -     ddekit_lock_unlock (&ddekit_irq_ctrl[irq].irqlock);
> +     irqhelp_remove_interrupt_handler(irq, -1, -1, -1, irq_cookie);
>  }
> 
> 
>  void ddekit_interrupt_disable(int irq)
>  {
> -     if (ddekit_irq_ctrl[irq].irqlock) {
> -             ddekit_lock_lock (&ddekit_irq_ctrl[irq].irqlock);
> -             --ddekit_irq_ctrl[irq].handle_irq;
> -             ddekit_lock_unlock (&ddekit_irq_ctrl[irq].irqlock);
> -     }
>  }
> 
> 
>  void ddekit_interrupt_enable(int irq)
>  {
> -     if (ddekit_irq_ctrl[irq].irqlock) {
> -             ddekit_lock_lock (&ddekit_irq_ctrl[irq].irqlock);
> -             ++ddekit_irq_ctrl[irq].handle_irq;
> -             if (ddekit_irq_ctrl[irq].handle_irq > 0)
> -                     ddekit_condvar_signal (ddekit_irq_ctrl[irq].cond);
> -             ddekit_lock_unlock (&ddekit_irq_ctrl[irq].irqlock);
> -     }
>  }
> 
>  void interrupt_init ()
>  {
> -     error_t err;
> -
> -     err = get_privileged_ports (&master_host, &master_device);
> -     if (err)
> -             error (1, err, "get_privileged_ports");
> -
> -     err = device_open (master_device, D_READ, "irq", &irq_dev);
> -     if (err)
> -             error (2, err, "device_open irq");
>  }
> --
> 2.34.1
> 
> 
>