Re: [PATCH v2 hurd] pci: Add RPCs for taking and freeing io ports by BAR

[Samuel Thibault]

Damien Zammit, le jeu. 20 juil. 2023 10:13:10 +0000, a ecrit:
> diff --git a/hurd/pci.defs b/hurd/pci.defs
> index e258f5ce..8542e602 100644
> --- a/hurd/pci.defs
> +++ b/hurd/pci.defs
> @@ -71,3 +71,13 @@ routine pci_get_dev_rom(
>       master: pci_t;
>       out data: data_t, dealloc
>  );
> +
> +/*
> + * Request io ports for BAR 0-5 for a given device.

Please explain that this can then be used with the gnumach RPCs

> + * Only works on bars that correspond to IO ports.
> + */
> +routine pci_request_io_ports(
> +     master: pci_t;
> +     bar: int;
> +     out io_perm: mach_port_t
> +);
> diff --git a/pci-arbiter/pci-ops.c b/pci-arbiter/pci-ops.c
> index 2f9f5296..2762f140 100644
> --- a/pci-arbiter/pci-ops.c
> +++ b/pci-arbiter/pci-ops.c
> @@ -24,6 +24,8 @@
>  #include <fcntl.h>
>  #include <hurd/netfs.h>
>  #include <sys/mman.h>
> +#include <sys/io.h>
> +#include <mach/machine/mach_i386.h>
>  
>  #include <pciaccess.h>
>  #include "pcifs.h"
> @@ -273,3 +275,59 @@ S_pci_get_dev_rom (struct protid * master, char **data, 
> mach_msg_type_number_t *
>  
>    return 0;
>  }
> +
> +error_t
> +request_io_ports (struct pcifs_dirent *e, int bar, io_perm_t *io_perm)
> +{
> +  uint64_t iobase, iosize;
> +  io_port_t from, to;
> +  error_t err;
> +  mach_port_t device_master;
> +
> +  if (get_privileged_ports (0, &device_master))
> +    return EPERM;
> +
> +  if ((bar < 0) || (bar > 5))
> +    return EINVAL;
> +
> +  if (!e->device->regions[bar].is_IO)
> +    /* This operation only works on IO bars */
> +    return EINVAL;
> +
> +  iobase = e->device->regions[bar].base_addr;
> +  iosize = e->device->regions[bar].size;
> +  if ((iobase & ~0xffff)

Rather test iobase >= 0x10000, which is more understandable (and the
compiler will probably produce the same code).

> || (iosize & ~0xffff))

Also rather iosize >= 0x10000.

and that was missing the case when iobase + iosize >= 0x10000.

> +    return EINVAL;
> +
> +  from = iobase;
> +  to = from + iosize - 1;
> +
> +  if ((err = i386_io_perm_create (device_master, from, to, io_perm)))
> +    return err;
> +
> +  /* Update atime */
> +  UPDATE_TIMES (e, TOUCH_ATIME);
> +
> +  return 0;
> +}
> +
> +error_t
> +S_pci_request_io_ports (struct protid * master, int bar, io_perm_t *io_perm)
> +{
> +  error_t err;
> +  struct pcifs_dirent *e;
> +
> +  if (!master)
> +    return EOPNOTSUPP;
> +
> +  e = master->po->np->nn->ln;
> +  if (strncmp (e->name, FILE_CONFIG_NAME, NAME_SIZE))
> +    /* This operation may only be addressed to the config file */
> +    return EINVAL;
> +
> +  err = check_permissions (master, O_READ);

I'm wondering about this: don't we want to require some write access?
(since the i/o ports will allow basically anything with the device).
Not actually to the PCI config since we don't want to let the caller
change it. But it can rather be the region file on which we'd want to
run the RPC? (and thus not need the bar parameter).

> +  if (err)
> +    return err;
> +
> +  return request_io_ports (e, bar, io_perm);
> +}