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

[Samuel Thibault]

Hello,

In the end (see also the ddekit review), I believe that again, we want
to expose the server demuxer function, and let the caller manage calling
it, so as to be able to manage the interrupt thread the way it wants.

Samuel

Damien Zammit, le jeu. 22 sept. 2022 04:42:18 +0000, a ecrit:
> --- /dev/null
> +++ b/libirqhelp/irqhelp.c
> @@ -0,0 +1,376 @@

> +#define _GNU_SOURCE 1

You don't need it, it's already in Makeconf.

> +#define MAX_PCI_DEVS         128

Really rather use a dynamic array than hardcoding a limit.

> +#define PCI_COMMAND          0x04
> +#define PCI_COMMAND_INT_DISABLE      0x400
> +#define IRQ_THREAD_PRIORITY  2
> +
> +
> +struct irq {
> +  int bus;
> +  int dev;
> +  int fun;
> +  int gsi;
> +  int cookie;
> +  void (*handler)(void *);
> +  void *context;
> +  mach_port_t port;
> +
> +  LIST_ENTRY(irq) entries;
> +};
> +
> +static LIST_HEAD(, irq) irqs = LIST_HEAD_INITIALIZER(&irqs);
> +
> +static mach_port_t master_host;
> +static mach_port_t irqdev;
> +static mach_port_t acpidev;
> +static int refcnt;
> +
> +static struct pci_device *pci_devices[MAX_PCI_DEVS];
> +static int numdevs;
> +
> +static error_t
> +pci_init(void)

It seems this is not safe against several calls? You can use
pthread_once to easily make the single call to pci_init.

> +{
> +  int i = 0;
> +  error_t err;
> +  struct pci_device_iterator *dev_iter;
> +  struct pci_device *pci_dev;
> +
> +  err = pci_system_init ();
> +  if (err)
> +    return err;
> +
> +  dev_iter = pci_slot_match_iterator_create (NULL);
> +  while (((pci_dev = pci_device_next (dev_iter)) != NULL)
> +      && (i < MAX_PCI_DEVS))
> +    {
> +      pci_device_probe(pci_dev);
> +      pci_devices[i++] = pci_dev;
> +    }
> +  numdevs = i;
> +  return 0;
> +}
> +
> +static error_t
> +pci_confread(int bus, int dev, int fun,
> +             int reg, unsigned int *rv)
> +{
> +  int i;
> +  *rv = 0xffffffff;
> +
> +  for (i = 0; i < numdevs; i++)
> +    {
> +      if ((pci_devices[i]->bus == bus)
> +       && (pci_devices[i]->dev == dev)
> +       && (pci_devices[i]->func == fun))
> +     goto found;
> +    }
> +  return ENODEV;
> +
> +found:
> +  pci_device_cfg_read_u32(pci_devices[i], rv, reg);

I'd say rather create a single function that returns the struct
pci_device, that the caller will call just once, and then be able to
directly call pci_device_cfg_read/write_u32 as many times as it wants.

> +static error_t
> +get_acpi(void)
> +{
> +  error_t err = 0;
> +  mach_port_t tryacpi, device_master;
> +
> +  acpidev = MACH_PORT_NULL;
> +
> +  tryacpi = file_name_lookup (_SERVERS_ACPI, O_RDONLY, 0);

This would try to access the / filesystem, and thus probably pose
problem at bootstrap. I'd say rather try first through the device_master
port and only revert to file_name_lookup if that fails. That's similar
to what libpciaccess does.

> +  if (tryacpi == MACH_PORT_NULL)
> +    {
> +      err = get_privileged_ports (0, &device_master);
> +      if (err)
> +        return err;
> +
> +      err = device_open (device_master, D_READ, "acpi", &tryacpi);
> +      if (err)
> +        {
> +       mach_port_deallocate (mach_task_self (), device_master);
> +          return err;
> +     }
> +
> +      mach_port_deallocate (mach_task_self (), device_master);
> +    }
> +
> +  acpidev = tryacpi;
> +  return err;
> +}
> +
> +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);
> +  if (err)
> +    {
> +      mach_port_deallocate (mach_task_self (), device_master);
> +      return err;
> +    }
> +
> +  mach_port_deallocate (mach_task_self (), device_master);
> +
> +  irqdev = tryirq;
> +  return err;
> +}
> +
> +void *
> +irqhelp_server_loop(void *arg)
> +{
> +  struct irq *irq = (struct irq *)arg;
> +
> +  if (!irq)
> +    return NULL;
> +
> +  int interrupt_server (mach_msg_header_t *inp,
> +                     mach_msg_header_t *outp)
> +  {
> +    char interrupt[4];

This is unused?

> +
> +    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)
> +      {
> +        /* not an interrupt */
> +        return 0;
> +      }
> +
> +    /* FIXME: id <-> gsi now has an indirection, assuming 1:1 */
> +    if (n->id != irq->gsi)
> +      {
> +        /* interrupt not for us */
> +        return 0;
> +      }
> +
> +    /* enable pci interrupt if we know the device B/D/F and it is disabled */
> +    if ((irq->bus >= 0) && (irq->dev >= 0) && (irq->fun >= 0))
> +      {
> +        unsigned int val;
> +        pci_confread (irq->bus, irq->dev, irq->fun, PCI_COMMAND, &val);
> +        if (val & PCI_COMMAND_INT_DISABLE)
> +          {
> +            val &= ~PCI_COMMAND_INT_DISABLE;
> +            pci_confwrite (irq->bus, irq->dev, irq->fun, PCI_COMMAND, val);
> +          }
> +      }

Mmmm, is that not supposed to be rather done by the driver? Otherwise
you'll have all kinds of races between the reception of data in the
handler and allowing a newer interrupt.

> +    /* call handler */
> +    irq->handler(irq->context);
> +
> +    /* ACK interrupt */
> +    device_intr_ack (irqdev, irq->port, MACH_MSG_TYPE_MAKE_SEND);
> +
> +    return 1;
> +  }
> +
> +  /* Server loop */
> +  mach_msg_server (interrupt_server, 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);
> +
> +  irq->handler = handler;
> +  irq->context = context;
> +  irq->bus = bus;
> +  irq->dev = dev;
> +  irq->fun = fun;
> +  irq->gsi = gsi;
> +  irq->cookie = ++refcnt;

You may be called from thread context, so this needs to be made atomic.

> +  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;
> +
> +  return irq;
> +
> +fail:
> +  free(irq);
> +  --refcnt;

You can't really revert it, since another thread might have registered
another irq in the meanwhile. Better just leave as it is, we don't
really need cookies to be contiguous.

> +  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 all -1, will not use PCI
> +   to clear possibly disabled pci interrupt.
> +   Accepts a pointer to a cookie to be used to
> +   deregister the handler later.  */
> +void *
> +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_privileged_ports (&master_host, 0);
> +  if (err)
> +    return NULL;
> +
> +  err = get_irq();
> +  if (err)
> +    return NULL;
> +
> +  if (gsi < 0)
> +    {
> +      if ((bus < 0) && (dev < 0) && (fun < 0))

Are these not || ?

> +        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;
> +    }
> +
> +  if ((bus >= 0) && (dev >= 0) && (fun >= 0))
> +    {
> +      err = pci_init();
> +      if (err)
> +        return NULL;
> +    }
> +
> +  irq = interrupt_register(gsi, bus, dev, fun, handler, context, cookie);
> +
> +  return (void *)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..f7947630
> --- /dev/null
> +++ b/libirqhelp/irqhelp.h
> @@ -0,0 +1,31 @@
> +/* 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, int *cookie);
> +error_t irqhelp_remove_interrupt_handler(int gsi, int bus, int dev, int fun, 
> int cookie);
> +void * irqhelp_server_loop(void *arg);
> +
> +#endif
> --
> 2.34.1