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

[Joan Lledó]

Hi Damien,

I think your design is not compatible with nested arbiters. Correct me 
if I'm wrong, but AFAIK the arbiter doesn't know if it's a main or a 
nested arbiter, it's libpciaccess who handles that. So the arbiter code 
must be the same and work no matter if it's main or nested. In your 
design, the arbiter calls i386_io_perm_create(), that would work for the 
main arbiter but not for nested arbiters, because only one process can 
take the port, AFAIK.

I think it should be libpciaccess x86 module who calls 
i386_io_perm_create() once per device during the initial probe. And 
store the resulting port at some place the arbiter can get it. Then, the 
S_pci_request_io_ports() RPC should check the user permissions and 
return that port. This should work for both main an nested arbiters, 
since the logic to get the port would be implemented in libpciaccess.

On the other hand, the hurd module in libpciaccess should call this new 
RPC to get the port from the main arbiter when probing the devices. That 
way the nested arbiter can get the port too. Also clients using 
libpciaccess directly.

I saw that `struct pci_device` contains a pointer `*user_data`. We could 
use that to pass the port from libpciaccess to the arbiter. I wonder why 
aren't we using that already, for instance to make the ROM base address 
available for the arbiter.

Does this make any sense for you? I could be wrong because I don't 
remember all the details right now.

On 20/7/23 12:13, Damien Zammit wrote:
> This would allow pci-arbiter to control which io ports
> are accessed and pave the way to having granular access to io ports
> based on pci cards exposing their IO BARs.
>
> libpciaccess has convenient api for this kind of access, it allows
> opening and registering io ports by BAR.
>
> Therefore, we can first introduce this new RPC, then see if changing
> libpciaccess to only expose PCI_CFG1 ports on x86 method and granular access
> to other io ports on hurdish method still works with other servers.
>
> Finally, once this is done, we can check if locking the io ports down
> to exclusive regions that cant overlap will function correctly with
> gnumach and the rest of the hurd.
>
> I am looking for a way to have the io ports which are attached
> to a particular pci device released when the pci device port is deallocated.
>
> ---
>   hurd/pci.defs         | 10 ++++++++
>   pci-arbiter/pci-ops.c | 58 +++++++++++++++++++++++++++++++++++++++++++
>   2 files changed, 68 insertions(+)
>
> 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.
> + * 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) || (iosize & ~0xffff))
> +    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);
> +  if (err)
> +    return err;
> +
> +  return request_io_ports (e, bar, io_perm);
> +}