Re: [PATCH v5 1/2 hurd] libirqhelp: Add library

[Samuel Thibault]

Hello,

Damien Zammit, le sam. 08 juil. 2023 15:05:11 +0000, a ecrit:
> --- /dev/null
> +++ b/libirqhelp/irqhelp.c
> +#define IRQ_THREAD_PRIORITY  2
> +
> +struct irq {
> +  void (*handler)(void *);
> +  void *context;
> +  int gsi;
> +  mach_port_t port;
> +  bool enabled;
> +  bool shutdown;
> +  pthread_mutex_t irqlock;
> +  pthread_cond_t irqcond;
> +  sem_t sema;
> +};
> +
> +static mach_port_t master_host;

This doesn't seem to need to be a global variable?

> +static mach_port_t irqdev;
> +static mach_port_t acpidev;

This looks odd: get_acpi and get_irq are getting called on each
interrupt installation, thus overwriting the previous value. I guess
we'd rather introduce an irqhelp_init() that gets acpi and irq ports
once for all.

> +static error_t
> +get_acpi(void)
> +{
> +  error_t err = 0;
> +  mach_port_t tryacpi, device_master;
> +
> +  acpidev = MACH_PORT_NULL;
> +  err = get_privileged_ports (0, &device_master);
> +  if (!err)
> +    {
> +      err = device_open (device_master, D_READ, "acpi", &tryacpi);

Move deallocating device_master here.

> +      if (!err)
> +        {
> +       mach_port_deallocate (mach_task_self (), device_master);
> +       acpidev = tryacpi;
> +          return 0;
> +        }
> +
> +      mach_port_deallocate (mach_task_self (), device_master);
> +    }
> +
> +  tryacpi = file_name_lookup (_SERVERS_ACPI, O_RDONLY, 0);
> +  if (tryacpi == MACH_PORT_NULL)
> +    return ENODEV;
> +
> +  acpidev = tryacpi;
> +  return 0;
> +}
> +
> +static error_t
> +get_irq(void)
> +{
> +  error_t err = 0;
> +  mach_port_t tryirq, device_master;
> +
> +  irqdev = MACH_PORT_NULL;
> +
> +  err = get_privileged_ports (0, &device_master);
> +  if (err)
> +    return err;
> +
> +  err = device_open (device_master, D_READ|D_WRITE, "irq", &tryirq);

Move deallocating device master here.

> +  if (err)
> +    {
> +      mach_port_deallocate (mach_task_self (), device_master);
> +      return err;
> +    }
> +
> +  mach_port_deallocate (mach_task_self (), device_master);
> +
> +  irqdev = tryirq;
> +  return err;
> +}
> +
> +static void
> +toggle_irq(struct irq *irq, bool on)
> +{
> +  irq->enabled = on;
> +  if (on)
> +    pthread_cond_signal (&irq->irqcond);

You *need* a mutex_lock around these three lines, otherwise it's unsafe:
the while (!irq->enabled) test in irqhelp_server_loop can see enabled
as false, then toggle_irq set it to true and call cond_signal, then
irqhelp_server_loop call cond_wait, and stay stuck there.

> +}
> +
> +void
> +irqhelp_disable_irq(void *opaque)
> +{
> +  struct irq *irq = (struct irq *)opaque;

Rather expose the "struct irq" name in irqhelp.h (even if not the
content), that'll be simpler both for the caller (to know what it's
supposed to pass) and for the callee (that doesn't have to cast).

> +  if (!irq)
> +    { 
> +      printf("irqhelp cannot disable this irq\n");

Avoid printing on stdout, print on stderr.

> +      return;
> +    }
> +
> +  toggle_irq(irq, false);
> +}
> +
> +void
> +irqhelp_enable_irq(void *opaque)
> +{
> +  struct irq *irq = (struct irq *)opaque;
> +  if (!irq)
> +    {
> +      printf("irqhelp cannot enable this irq\n");
> +      return;
> +    }
> +
> +  toggle_irq(irq, true);
> +}
> +
> +void
> +irqhelp_wait_init(void *opaque)
> +{
> +  struct irq *irq = (struct irq *)opaque;
> +  if (!irq)
> +    {
> +      printf("irqhelp cannot wait on this irq to be ready\n");
> +      return;
> +    }
> +
> +  sem_wait(&irq->sema);
> +}

> I realised that the guts of interrupt_register() needs to be done
> in irqhelp_server_loop() to configure the interrupt thread.

Why? Creating the delivery port and calling device_intr_register can be
done in interrupt_register, can't it? The interrupt thread should only
need to set its priority and process server messages.

> +
> +void *
> +irqhelp_server_loop(void *arg)
> +{
> +  struct irq *irq = (struct irq *)arg;
> +  mach_port_t delivery_port;
> +  mach_port_t pset, psetcntl;
> +  error_t err;
> +
> +  if (!irq)
> +    {
> +      printf("irqhelp cannot start this irq thread\n");
> +      return NULL;
> +    }
> +
> +  err = get_privileged_ports (&master_host, 0);
> +  if (err)
> +    {
> +      printf("irqhelp cannot get master_host port\n");
> +      return 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;
> +
> +  mach_port_deallocate (mach_task_self (), master_host);
> +
> +  err = device_intr_register(irqdev, irq->gsi,
> +                             0, irq->port,
> +                             MACH_MSG_TYPE_MAKE_SEND);
> +  if (err)
> +    goto fail;
> +
> +  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 */

I'd say put at least a one-time warning, so we get to know if that
happens.

> +    /* FIXME: id <-> gsi now has an indirection, assuming 1:1 */
> +    if (n->id != irq->gsi)
> +      return 0;  /* interrupt not for us */

Same here.

> +
> +    /* wait if irq disabled */
> +    pthread_mutex_lock (&irq->irqlock);
> +    while (!irq->enabled)
> +      pthread_cond_wait (&irq->irqcond, &irq->irqlock);
> +    pthread_mutex_unlock (&irq->irqlock);
> +
> +    /* call handler */
> +    irq->handler(irq->context);
> +
> +    /* ACK interrupt */
> +    device_intr_ack (irqdev, irq->port, MACH_MSG_TYPE_MAKE_SEND);
> +
> +    if (irq->shutdown)
> +      mach_port_deallocate (mach_task_self (), irq->port);
> +
> +    return 1;
> +  }
> +
> +  sem_post(&irq->sema);
> +
> +  /* Server loop */
> +  mach_msg_server (interrupt_demuxer, 0, irq->port);
> +  goto done;
> +
> +fail:
> +  printf("irqhelp failed to register irq %d\n", irq->gsi);
> +
> +done:
> +  pthread_cond_destroy(&irq->irqcond);
> +  pthread_mutex_destroy(&irq->irqlock);
> +  mach_port_deallocate (mach_task_self (), master_host);

It was already deallocated above.

> +  free(irq);
> +  return NULL;
> +}
> +
> +static struct irq *
> +interrupt_register(int gsi,
> +                void (*handler)(void *),
> +                void *context)
> +{
> +  struct irq *irq = NULL;
> +
> +  irq = malloc(sizeof(struct irq));
> +  if (!irq)
> +    {
> +      printf("irqhelp cannot malloc irq\n");
> +      return NULL;
> +    }
> +
> +  irq->handler = handler;
> +  irq->context = context;
> +  irq->gsi = gsi;
> +  irq->enabled = false;  /* require initial explicit enable */
> +  irq->shutdown = false;
> +  pthread_mutex_init (&irq->irqlock, NULL);
> +  pthread_cond_init (&irq->irqcond, NULL);
> +  sem_init(&irq->sema, 0, 0);
> +  
> +  return irq;
> +}
> +
> +
> +/* 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.
> +   Returns an opaque pointer to be used to
> +   deregister the handler later.  */

This comment should go to the .h file for people to see it.

> +void *
> +irqhelp_install_interrupt_handler(int gsi,
> +                               int bus,
> +                               int dev,
> +                               int fun,
> +                               void (*handler)(void *),
> +                               void *context)
> +{
> +  struct irq *irq;
> +  error_t err;
> +
> +  if (!handler)
> +    {
> +      printf("irqhelp no handler\n");
> +      return NULL;
> +    }
> +
> +  err = get_irq();
> +  if (err)
> +    {
> +      printf("irqhelp cannot grab irq device\n");
> +      return NULL;
> +    }
> +  if (gsi < 0)
> +    {
> +      if ((bus < 0) || (dev < 0) || (fun < 0))
> +     {
> +       printf("irqhelp invalid b/d/f\n");
> +       return NULL;
> +     }
> +
> +      err = get_acpi();
> +      if (err)
> +     {
> +       printf("irqhelp cannot grab acpi device\n");
> +       return NULL;
> +     }
> +
> +      /* We need to call acpi translator to get gsi */
> +      err = acpi_get_pci_irq (acpidev, bus, dev, fun, &gsi);
> +      if (err)
> +     {
> +       printf("irqhelp tried acpi to get pci gsi and failed for 
> %d:%02x.%d\n", bus, dev, fun);

Bus should be printed as hex as well.

> +       return NULL;
> +     }
> +    }
> +
> +  irq = interrupt_register(gsi, handler, context);
> +
> +  return (void *)irq;
> +}
> +
> +error_t
> +irqhelp_remove_interrupt_handler(void *opaque)
> +{
> +  struct irq *irq = (struct irq *)opaque;
> +  if (!irq)
> +    {
> +      printf("irqhelp cannot deregister this irq\n");
> +      return ENODEV;
> +    }
> +
> +  irq->shutdown = true;
> +  
> +  /* enable irq to fall through to shutdown check */
> +  toggle_irq(irq, true);
> +
> +  return 0;
> +}
> diff --git a/libirqhelp/irqhelp.h b/libirqhelp/irqhelp.h
> new file mode 100644
> index 000000000..4e5654311
> --- /dev/null
> +++ b/libirqhelp/irqhelp.h
> @@ -0,0 +1,34 @@
> +/* 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>
> +
> +void * irqhelp_install_interrupt_handler(int gsi, int bus, int dev, int fun,
> +                                      void (*handler)(void *), void 
> *context);
> +error_t irqhelp_remove_interrupt_handler(void *opaque);
> +void irqhelp_wait_init(void *opaque);
> +void * irqhelp_server_loop(void *arg);
> +void irqhelp_enable_irq(void *opaque);
> +void irqhelp_disable_irq(void *opaque);
> +
> +#endif
> -- 
> 2.40.1
> 
> 
>