[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

From: Joan Lledó
Subject: Re: [PATCH v2 hurd] pci: Add RPCs for taking and freeing io ports by BAR
Date: Thu, 20 Jul 2023 22:38:42 +0200
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.13.0

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;
+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 */
+  return 0;
+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);

reply via email to

[Prev in Thread] Current Thread [Next in Thread]