[PATCH] lwip: Allocate the loopback netif by default

[Joan Lledó]

From: Joan Lledó 

The translator received a null `netif_list` during initialization, this
caused a few bugs.

When started without parameters, the translator didn't add any new
interface to `netif_list`, and that broke any subsequent fsysopts over
the translator, as the stack was being initialized again instead of
being reconfigured.

DHCP was broken because the translator is usually installed without
parameters, which are supposed to be added by the DHCP client through
fsysopts.

The absence of an allocated `netif_list` also prevented configuring a
loopback interface.

After these changes, starting the translator always allocates one
interface and configures it as loopback.
---
 lwip/lwip-util.c | 14 +++---
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/lwip/lwip-util.c b/lwip/lwip-util.c
index fc4cb137..77d2c233 100644
--- a/lwip/lwip-util.c
+++ b/lwip/lwip-util.c
@@ -149,13 +149,13 @@ init_ifs (void *arg)
   ip6_addr_t *address6;
   int i;
 
-  if (netif_list != 0)
-{
-  if (netif_list->next == 0)
-   init_loopback ();
-  else
-   remove_ifs ();
-}
+  if (netif_list == 0)
+netif_list = calloc (1, sizeof (struct netif));
+
+  if (netif_list->next == 0)
+init_loopback ();
+  else
+remove_ifs ();
 
   /*
* Go through the list backwards. For LwIP
-- 
2.40.1

lwip: Allocate the loopback netif by default

[Joan Lledó]

Hi,

This patch fixes a few bugs. The translator was assuming one interface was 
already allocated in `netif_list` when calling `init_ifs` during startup, and 
used it to configure the loopback interface [1]. That was possibly true in the 
past but after upgrading liblwip I found `netif_list` is always null at the 
first call to `init_ifs`. That breaks fsysopts, the loopback interface and DHCP.

fsysopts is broken only when the translator is started without parameters. When 
that happens, `netif_list` remains null after `init_ifs` finished. Because of 
that, a call to fsysopts tries to initialize the stack again instead of 
reconfiguring it [2].

That's linked to the second problem, the lack of loopback interface. 
`parse_opt` at [2] assumes that a correctly initialized stack will at least 
have one configured interface, the loopback one. And `init_ifs` only configures 
it when there's a single netif allocated at `netif_list`, that condition is not 
met if `netif_list` arrives null.

Finally, DHCP fails on lwip if the translator is installed without parameters, 
like pfinet is:

$ showtrans /servers/socket/2
/hurd/pfinet -6 /servers/socket/26

The DHCP client calls fsysopts on the translator [3]. So when installed without 
parameters, this call to fsysopts tries to initialize the stack again and 
crashes.

This simple patch fixes the problems.

---
[1] https://git.savannah.gnu.org/cgit/hurd/hurd.git/tree/lwip/lwip-util.c#n155
[2] https://git.savannah.gnu.org/cgit/hurd/hurd.git/tree/lwip/options.c#n266
[3] 
https://salsa.debian.org/debian/isc-dhcp/-/blob/master/debian/dhclient-script.hurd#L184

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

[Joan Lledó]

Hi,

Thank you Damien and Samuel for your explanations.

On 22/7/23 3:31, Damien Zammit wrote:

diff --git a/pci-arbiter/netfs_impl.c b/pci-arbiter/netfs_impl.c
index 4bb5c97a..6b3d6918 100644
--- a/pci-arbiter/netfs_impl.c
+++ b/pci-arbiter/netfs_impl.c
@@ -522,6 +522,14 @@ netfs_attempt_read (struct iouser * cred, struct node * 
node,
/* Update atime */
UPDATE_TIMES (node->nn->ln, TOUCH_ATIME);
  }
+  else if (!strncmp
+  (node->nn->ln->name, FILE_IO_NAME, strlen (FILE_IO_NAME)))
+{
+  err = io_region_file (node->nn->ln, offset, len, data, 1);
+  if (!err)
+   /* Update atime */
+   UPDATE_TIMES (node->nn->ln, TOUCH_ATIME);
+}


This block code is repeated from the condition above. Why not modifying 
the condition to:


else if (
!strncmp (node->nn->ln->name, FILE_REGION_NAME, strlen 
(FILE_REGION_NAME)) ||

!strncmp (node->nn->ln->name, FILE_IO_NAME, strlen (FILE_IO_NAME))
)

And avoid the code duplication?


@@ -556,6 +564,14 @@ netfs_attempt_write (struct iouser * cred, struct node * 
node,
/* Update atime */
UPDATE_TIMES (node->nn->ln, TOUCH_MTIME | TOUCH_CTIME);
  }
+  else if (!strncmp
+  (node->nn->ln->name, FILE_IO_NAME, strlen (FILE_IO_NAME)))
+{
+  err = io_region_file (node->nn->ln, offset, len, (void*) data, 0);
+  if (!err)
+   /* Update atime */
+   UPDATE_TIMES (node->nn->ln, TOUCH_MTIME | TOUCH_CTIME);
+}


Same code duplication here.


else
  return EOPNOTSUPP;
  
diff --git a/pci-arbiter/pci-ops.c b/pci-arbiter/pci-ops.c

index 2f9f5296..aa92f991 100644
--- a/pci-arbiter/pci-ops.c
+++ b/pci-arbiter/pci-ops.c
@@ -273,3 +274,61 @@ 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, io_perm_t *io_perm)
+{
+  int bar;
+  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;


Correct me if I'm wrong: nested arbiters would fail here, right? An 
arbiter launched from a user (non-root) having permissions over the IO 
file should be able to get the port and handle it to it's clients. Then 
we need the fallback mechanism to RPC the master arbiter when this is a 
nested arbiter. Just mentioning, no need to be in this patch, though.



+
+  bar = strtol(&e->name[2], NULL, 10);


Better:

&e->name[strlen (FILE_IO_NAME)]

WDYT?

The rest of the patch looks good to me.

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 
  #include 
  #include 
+#include 
+#include 
  
  #include 

  #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 & ~0x) || (iosize & ~0x))
+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);
+}

pci-arbiter: Prevent mapping IO region files

[Joan Lledó]

Hello,

Time ago I sent some patches to implement mapping region and ROM files using
mmap(). However, a BAR region can represent either memory or I/O space, and
only the former should be allowed to be mapped, since I/O BARs don't contain
physical memory addresses, but I/O addresses. I attached a small patch to
prevent mapping I/O region files.

On the other hand, in the past we discussed how to make IO spaces available
for users through the arbiter [1]. It seems the way to go is adding a new RPC
that checks for permissions, calls i386_io_perm_create() and returns the
resulting port. I could work on that. After all changes in the arbiter and the 
Hurd these last years, would still be useful to have such RPC?

---
[1] https://lists.gnu.org/archive/html/bug-hurd/2017-12/msg00048.html

[PATCH] pci-arbiter: Prevent mapping IO regions

[Joan Lledó]

From: Joan Lledó 

* pci-arbiter/netfs_impl.c:
  * get_filemap_region(): Return MACH_PORT_NULL and set errno to EOPNOTSUPP
when the client tries to map a IO region file.
---
 pci-arbiter/netfs_impl.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/pci-arbiter/netfs_impl.c b/pci-arbiter/netfs_impl.c
index b66f0019..4bb5c97a 100644
--- a/pci-arbiter/netfs_impl.c
+++ b/pci-arbiter/netfs_impl.c
@@ -583,6 +583,9 @@ get_filemap_region (struct node *node, vm_prot_t prot)
 strtol (&node->nn->ln->name[strlen (node->nn->ln->name) - 1], 0, 16);
   region = &node->nn->ln->device->regions[reg_num];
 
+  if (region->is_IO)
+goto error;
+
   /* Ensure the region is mapped */
   err = device_map_region (node->nn->ln->device, region,
   &node->nn->ln->region_maps[reg_num]);
-- 
2.40.1

Re: [PATCH libpciaccess] hurd: Clients or nested arbiters don't touch ioports

[Joan Lledó]

Hi,

El 23/8/22 a les 10:36, Damien Zammit ha escrit:

@@ -600,14 +599,14 @@ static const struct pci_system_methods hurd_pci_methods = 
{
  .read = pci_device_hurd_read,
  .write = pci_device_hurd_write,
  .fill_capabilities = pci_fill_capabilities_generic,
-.open_legacy_io = pci_device_x86_open_legacy_io,
-.close_io = pci_device_x86_close_io,
-.read32 = pci_device_x86_read32,
-.read16 = pci_device_x86_read16,
-.read8 = pci_device_x86_read8,
-.write32 = pci_device_x86_write32,
-.write16 = pci_device_x86_write16,
-.write8 = pci_device_x86_write8,
+.open_legacy_io = NULL,
+.close_io = NULL,
+.read32 = NULL,
+.read16 = NULL,
+.read8 = NULL,
+.write32 = NULL,
+.write16 = NULL,
+.write8 = NULL,
  .map_legacy = pci_device_hurd_map_legacy,
  .unmap_legacy = pci_device_hurd_unmap_legacy,
  };


Those functions are declared at x86_pci.h precisely b/c they need to be 
known for hurd_pci.c. If you stop calling them you should remove them 
also from x86_pci.h and make them static in x86_pci.c

Re: pci-arbiter: Implement mapping for ROM files

[Joan Lledó]

Hi,

El 16/8/22 a les 18:10, Samuel Thibault ha escrit:

? Any memory allocated by a process will be freed when the process shuts
down.



Yes, I know, just wondering if there was a good practice about 
malloc/free in the Hurd.

Re: pci-arbiter: Implement mapping for ROM files

[Joan Lledó]

Hi,

I just though... shouldn't this allocated memory be freed somewhere when 
the arbiter shuts down?


El 15/8/22 a les 20:33, Samuel Thibault ha escrit:

Samuel Thibault, le lun. 15 août 2022 20:20:16 +0200, a ecrit:

Joan Lledó, le lun. 15 août 2022 20:07:09 +0200, a ecrit:

El 15/8/22 a les 19:37, Samuel Thibault ha escrit:

Mmm, but doesn't libpciaccess allow to map the BAR?Then pci-arbiter
could create a read-only memory proxy of the mapping?


Yes, the arbiter exposes the BAR regions as files called "region0",
"region1", etc. And they can be mapped through a proxy, that's already
working. But this is about the "rom" files which expose the content of the
expansion ROM for each device.


Sorry, I meant: why can't we do that for ROMs? Does libpciaccess not
provide a way to map ROMs?


Ah, no it doesn't.

Applied, thanks!

Samuel

Re: pci-arbiter: Implement mapping for ROM files

[Joan Lledó]

Hi,

El 15/8/22 a les 20:20, Samuel Thibault ha escrit:

Sorry, I meant: why can't we do that for ROMs? Does libpciaccess not
provide a way to map ROMs?



That's the problem: it doesn't. It only provides the function 
pci_device_read_rom() which copies the entire ROM to a given pointer. 
But it doesn't provide the ROM physical address, which I would need to 
map it in the arbiter space though pci_device_map_range(), like in


http://git.savannah.gnu.org/cgit/hurd/hurd.git/tree/pci-arbiter/device_map.c#n41

Re: pci-arbiter: Implement mapping for ROM files

[Joan Lledó]

Hi,

El 15/8/22 a les 19:37, Samuel Thibault ha escrit:

Mmm, but doesn't libpciaccess allow to map the BAR?Then pci-arbiter
could create a read-only memory proxy of the mapping?


Yes, the arbiter exposes the BAR regions as files called "region0", 
"region1", etc. And they can be mapped through a proxy, that's already 
working. But this is about the "rom" files which expose the content of 
the expansion ROM for each device.

[PATCH] Implement mapping for ROM files

[Joan Lledó]

From: Joan Lledó 

* pci-arbiter/device_map.h:
* pci-arbiter/device_map.c:
  * New function: device_map_rom
* Copies the whole rom in the arbiter space.
* pci-arbiter/pcifs.h:
  * struct pcifs_dirent:
* New field to store the mapping address for each device rom.
o pci-arbiter/func_files.h:
* pci-arbiter/func_files.c:
  * read_rom_file:
* Retrieves the rom contents from the local space instead of
  libpciaccess.
* pci-arbiter/netfs_impl.c:
  * netfs_attempt_read:get_filemap_region
* Update call to read_rom_file.
  * get_filemap_region:
* Uses the old code at netfs_get_filemap to get a proxy to
  the device memory region.
  * get_filemap_rom:
* Returns a proxy to the locally mapped device rom.
  * netfs_get_filemap:
* Checks the requested file to map and calls get_filemap_rom,
  get_filemap_region or returns en error.
---
 pci-arbiter/device_map.c | 23 ++
 pci-arbiter/device_map.h |  1 +
 pci-arbiter/func_files.c | 24 +++---
 pci-arbiter/func_files.h |  2 +-
 pci-arbiter/netfs_impl.c | 68 +++-
 pci-arbiter/pcifs.h  |  7 +
 6 files changed, 98 insertions(+), 27 deletions(-)

diff --git a/pci-arbiter/device_map.c b/pci-arbiter/device_map.c
index 1627746d..284d6e93 100644
--- a/pci-arbiter/device_map.c
+++ b/pci-arbiter/device_map.c
@@ -47,3 +47,26 @@ device_map_region (struct pci_device *device, struct 
pci_mem_region *region,
 
   return err;
 }
+
+error_t
+device_map_rom (struct pci_device *device, void **addr)
+{
+  error_t err = 0;
+  vm_address_t fullrom;
+
+  if (*addr == 0)
+{
+  err = vm_allocate (mach_task_self (), &fullrom, device->rom_size, 1);
+  if (err)
+   return ENOMEM;
+
+  err = pci_device_read_rom (device, (void *) fullrom);
+  if (err)
+   return err;
+
+  /* Return */
+  *addr = (void *) fullrom;
+}
+
+  return err;
+}
diff --git a/pci-arbiter/device_map.h b/pci-arbiter/device_map.h
index 0d92650c..ccdf482e 100644
--- a/pci-arbiter/device_map.h
+++ b/pci-arbiter/device_map.h
@@ -28,5 +28,6 @@
 
 error_t device_map_region (struct pci_device *device,
   struct pci_mem_region *region, void **addr);
+error_t device_map_rom (struct pci_device *device, void **addr);
 
 #endif /* DEVICE_MAP_H */
diff --git a/pci-arbiter/func_files.c b/pci-arbiter/func_files.c
index 40706135..27a72209 100644
--- a/pci-arbiter/func_files.c
+++ b/pci-arbiter/func_files.c
@@ -104,28 +104,28 @@ io_config_file (struct pci_device * dev, off_t offset, 
size_t * len,
 
 /* Read the mapped ROM */
 error_t
-read_rom_file (struct pci_device * dev, off_t offset, size_t * len,
+read_rom_file (struct pcifs_dirent * e, off_t offset, size_t * len,
   void *data)
 {
-  void *fullrom;
+  error_t err;
 
   /* This should never happen */
-  assert_backtrace (dev != 0);
+  assert_backtrace (e->device != 0);
 
   /* Don't exceed the ROM size */
-  if (offset > dev->rom_size)
+  if (offset > e->device->rom_size)
 return EINVAL;
-  if ((offset + *len) > dev->rom_size)
-*len = dev->rom_size - offset;
+  if ((offset + *len) > e->device->rom_size)
+*len = e->device->rom_size - offset;
 
-  /* Grab the full rom first */
-  fullrom = calloc(1, dev->rom_size);
-  pci_device_read_rom(dev, fullrom);
+  /* Ensure the rom is mapped */
+  err = device_map_rom (e->device, &e->rom_map);
+  if (err)
+return err;
 
-  /* Return the requested amount */
-  memcpy (data, fullrom + offset, *len);
+  /* Return the requested tange */
+  memcpy (data, e->rom_map + offset, *len);
 
-  free(fullrom);
   return 0;
 }
 
diff --git a/pci-arbiter/func_files.h b/pci-arbiter/func_files.h
index 03cafee1..cf77374a 100644
--- a/pci-arbiter/func_files.h
+++ b/pci-arbiter/func_files.h
@@ -40,7 +40,7 @@ typedef int (*pci_io_op_t) (struct pci_device *dev, void 
*data,
 error_t io_config_file (struct pci_device * dev, off_t offset, size_t * len,
void *data, pci_io_op_t op);
 
-error_t read_rom_file (struct pci_device *dev, off_t offset, size_t * len,
+error_t read_rom_file (struct pcifs_dirent * e, off_t offset, size_t * len,
   void *data);
 
 error_t io_region_file (struct pcifs_dirent *e, off_t offset, size_t * len,
diff --git a/pci-arbiter/netfs_impl.c b/pci-arbiter/netfs_impl.c
index b2630f01..b66f0019 100644
--- a/pci-arbiter/netfs_impl.c
+++ b/pci-arbiter/netfs_impl.c
@@ -509,7 +509,7 @@ netfs_attempt_read (struct iouser * cred, struct node * 
node,
 }
   else if (!strncmp (node->nn->ln->name, FILE_ROM_NAME, NAME_SIZE))
 {
-  err = read_rom_file (node->nn->ln->device, offset, len, data);
+  err = read_rom_file (node->nn->ln, offset, len, data);
   if (!err)
/* Update atime */
UPDATE_TIMES (node->nn->ln, TOUCH_ATIME);
@@ -569,8 +569,8 @@ netfs_node_norefs (struct node *node)
   destroy_node 

pci-arbiter: Implement mapping for ROM files

[Joan Lledó]

Helllo Hurd,

I wrote a patch to allow mapping ROM files from the arbiter file system.

In order to achieve this, the arbiter first allocates and copies the entire 
device ROM in its space the first time it's accessed, and returns a proxy to 
that memory region when a client requests a ROM filemap.

This has the obvious drawback of each arbiter instance having to copy all 
devices ROMs into its space. It would be better to get the ROM physical address 
somehow and map it into the arbiter space the same way we do for BAR regions, 
but (AFAIK) libpciaccess doesn't provide a way for a client to get the ROM 
physical address, only the ROM size is available.

Re: gnumach: bug in dev_pager.c

[Joan Lledó]

Since it was a tiny patch I pushed the changes myself.

El 8/8/22 a les 2:16, Samuel Thibault ha escrit:

It seems to me that the correct condition at lines 238 and 316 should be:

if (!queue_end(bucket, &entry->links))


That should be it indeed!

gnumach: bug in dev_pager.c

[Joan Lledó]

Hi,

I think there's a bug in dev_pager.c, at methods dev_pager_hash_delete() 
and dev_device_hash_delete(), lines 238 and 316:


https://git.savannah.gnu.org/cgit/hurd/gnumach.git/tree/device/dev_pager.c#n238
https://git.savannah.gnu.org/cgit/hurd/gnumach.git/tree/device/dev_pager.c#n316

It's calling kmem_cache_free() when entry is true, assuming that 
entry==true means entry found and entry==false means entry not found. 
But empty will always be true because queues are initialized as:


(q)->next = (q)->prev = q

AIUI, when an entry is not found, then then entry will point to the head 
of the queue (bucket == &entry->links). And it will call 
kmem_cache_free() to try to remove the head from the cache, when it's 
not in the cache. I'm surprised this is not crashing somehow. I tried to 
make dev_pager_hash_delete() get called with a non-existent entry but I 
don't know how to cause that situation.


It seems to me that the correct condition at lines 238 and 316 should be:

if (!queue_end(bucket, &entry->links))

Am I missing something?

[PATCH] Hurd: Fix initialization order

[Joan Lledó]

From: Joan Lledó 

---
 src/hurd_pci.c | 22 --
 1 file changed, 12 insertions(+), 10 deletions(-)

diff --git a/src/hurd_pci.c b/src/hurd_pci.c
index 70a9f89..653faa5 100644
--- a/src/hurd_pci.c
+++ b/src/hurd_pci.c
@@ -610,7 +610,8 @@ pci_system_hurd_create(void)
 {
 int err;
 struct pci_system_hurd *pci_sys_hurd;
-mach_port_t device_master, pci_port;
+mach_port_t device_master;
+mach_port_t pci_port = MACH_PORT_NULL;
 mach_port_t root = MACH_PORT_NULL;
 
 if (&netfs_server_name && netfs_server_name
@@ -637,23 +638,24 @@ pci_system_hurd_create(void)
 
 pci_sys->num_devices = 0;
 
-if ((err = get_privileged_ports (NULL, &device_master)) || (device_master 
== MACH_PORT_NULL)) {
-pci_system_cleanup();
-return err;
+err = get_privileged_ports (NULL, &device_master);
+
+if(!err && device_master != MACH_PORT_NULL) {
+err = device_open (device_master, D_READ, "pci", &pci_port);
+   mach_port_deallocate (mach_task_self (), device_master);
 }
 
-err = device_open (device_master, D_READ|D_WRITE, "pci", &pci_port);
-if (!err) {
-root = file_name_lookup_under (pci_port, ".", O_DIRECTORY | O_RDWR | 
O_EXEC, 0);
+if (!err && pci_port != MACH_PORT_NULL) {
+root = file_name_lookup_under (pci_port, ".", O_DIRECTORY | O_RDONLY | 
O_EXEC, 0);
 device_close (pci_port);
 mach_port_deallocate (mach_task_self (), pci_port);
 }
 
-if (!root) {
-root = file_name_lookup (_SERVERS_BUS_PCI, O_RDWR, 0);
+if (root == MACH_PORT_NULL) {
+root = file_name_lookup (_SERVERS_BUS_PCI, O_RDONLY, 0);
 }
 
-if (!root) {
+if (root == MACH_PORT_NULL) {
 pci_system_cleanup();
 return errno;
 }
-- 
2.31.1

Re: [PATCH] hurd: Implement device memory mapping

[Joan Lledó]

El 12/3/22 a les 12:30, Samuel Thibault ha escrit:
> Is netdde using your modified libpciaccess?

That was it, netdde was using the original libpciaccess.

I'm attaching a patch to fix the problem.

Re: [PATCH] hurd: Implement device memory mapping

[Joan Lledó]

El 23/1/22 a les 18:44, Samuel Thibault ha escrit:

With a normal hurd system, i.e. no bootstrap pci-arbiter, and using
netdde, which is thus supposed to eventually use the translator sitting
on /servers/bus/pci



I still don't know how to reproduce this, if I run a normal Hurd with an 
e1000 network card, is it not already using netdde? All seems to work 
fine for me.

Re: Recent libpciaccess changes

[Joan Lledó]

Hi,

El 7/2/22 a les 11:42, Damien Zammit ha escrit:

libpciaccess:
I have not been able to prove it yet, but I think the function call
pci_device_hurd_map_range() attempts to look up _SERVERS_BUS_PCI during
bootstrap and fails because it does not exist (no root filesystem exists).


Yes, the hurd module is to be used only by nested arbiters and 
libpciaccess clients, and it assumes there's a fs with a master arbiter 
running at /servers/bus/pci




Joan, I heard we are not supposed to use the hurd access method during 
bootstrap,
only the x86 access method.  Is this the problem, and if so, how can we fix 
this?
How did you envisage your mapping of regions to work during bootstrap when there
is no filesystem access?


Regrettably I didn't think on that, because I'm not familiar with your 
work on rump (I'll watch your fosdem talk soon :)). And yes, it should 
use the x86 backend, but it calls pci_system_x86_create() at [1], does 
that call fail in your scenario? In that case it will take the hurd 
methods at [2] which includes the hurd_map_range() function, which needs 
a fs. That probably worked before my mapping implementation because 
there were no hurd mapping methods and the ones in the x86 module were 
being used in all cases.


Is there a way to detect, from the hurd_create() function, when is your 
scenario running? If so, the solution would be reassign 
pci_sys->methods->[map_range, unmap_range] back to the x86 ones in that 
case.



Samuel suggested on IRC that we could make it try the x86 map range function 
first
and then fall back to hurd method, is there any problem with that?


I don't have the time now to study it, but I'd say that would bypass the 
protection set from the parent arbiter, since it would test the 
permissions the current user has over "mem", before testing the 
permissions in the file system created by the parent arbiter.



Perhaps there is a better solution to make the access method selectable via a 
parameter,
instead of implementing fallback mechanisms and making assumptions about what 
is running?
Would that work better?

In any case, we do need better testing of changes in different scenarios
to not put a burden on Samuel to fix everything we break.


How can I reproduce your scenario?



[1] 
https://gitlab.freedesktop.org/xorg/lib/libpciaccess/-/blob/master/src/hurd_pci.c#L619
[2] 
https://gitlab.freedesktop.org/xorg/lib/libpciaccess/-/blob/master/src/hurd_pci.c#L634

Re: [PATCH] hurd: Implement device memory mapping

[Joan Lledó]

Hi,

El 9/1/22 a les 1:02, Samuel Thibault ha escrit:

Err, this is breaking everything when pci-arbiter is not running as a
bootstrap translator. In that case e.g. netdde succeeds getting the
privileged port, and thus tries to open pci, but fails, and thus the
whole thing aborts. What problem the older approach had?

Samuel, not amused having to debug such thing.



How can I reproduce the bug?

Re: [PATCH] hurd: Implement device memory mapping

[Joan Lledó]

El 9/1/22 a les 1:15, Samuel Thibault ha escrit:

Err, this is breaking everything when pci-arbiter is not running as a
bootstrap translator. In that case e.g. netdde succeeds getting the
privileged port, and thus tries to open pci, but fails, and thus the
whole thing aborts. What problem the older approach had?


I uploaded a version with that ordering fixed back to debian-ports, to
be seen on mirrors within about 6h.

Samuel


That was because I wanted to allow non-root users to install nested 
arbiters. Non-root users can't pass this condition:


https://gitlab.freedesktop.org/xorg/lib/libpciaccess/-/blob/master/src/hurd_pci.c#L640

I also removed the need to read/write permissions, device scanning only 
needs read permissions actually.

Re: [PATCH] pci-arbiter: Stop using deprecated region memory pointer

[Joan Lledó]

I updated the comment

[PATCH] pci-arbiter: Stop using deprecated region memory pointer

[Joan Lledó]

From: Joan Lledó 

Use a internal array of pointers instead

* pci-arbiter/device_map.h:
  * Update device_map_region() prototype
* Now it receives an output address as parameter
* pci-arbiter/device_map.c:
  * Update device_map_region() definition to match the new prototype
  * Support for legacy mappings
* When the base address is lower than 1 mb
* pci-arbiter/func_files.c:
* pci-arbiter/netfs_impl.c:
  * Update calls to device_map_region to match the new prototype
  * Use the internal array of pointers instead of region->memory
* pci-arbiter/pcifs.h:
  * struct pcifs_dirent: Declare the internal array of pointers
---
 pci-arbiter/device_map.c | 19 +++
 pci-arbiter/device_map.h |  2 +-
 pci-arbiter/func_files.c |  6 +++---
 pci-arbiter/netfs_impl.c |  8 +---
 pci-arbiter/pcifs.h  |  7 +++
 5 files changed, 31 insertions(+), 11 deletions(-)

diff --git a/pci-arbiter/device_map.c b/pci-arbiter/device_map.c
index 216adfb9..1627746d 100644
--- a/pci-arbiter/device_map.c
+++ b/pci-arbiter/device_map.c
@@ -24,14 +24,25 @@
 #include "device_map.h"
 
 error_t
-device_map_region (struct pci_device *device, struct pci_mem_region *region)
+device_map_region (struct pci_device *device, struct pci_mem_region *region,
+  void **addr)
 {
   error_t err = 0;
 
-  if (region->memory == 0)
+  if (*addr == 0)
 {
-  err = pci_device_map_range (device, region->base_addr, region->size,
- PCI_DEV_MAP_FLAG_WRITABLE, ®ion->memory);
+  /*
+   * We could use the non-legacy call for all ranges, but libpciaccess
+   * offers a call for ranges under 1Mb. We call it for those cases, even
+   * when there's no difference for us.
+   */
+  if (region->base_addr > 0x10
+ || region->base_addr + region->size > 0x10)
+err = pci_device_map_range (device, region->base_addr, region->size,
+   PCI_DEV_MAP_FLAG_WRITABLE, addr);
+  else
+err = pci_device_map_legacy (device, region->base_addr, region->size,
+   PCI_DEV_MAP_FLAG_WRITABLE, addr);
 }
 
   return err;
diff --git a/pci-arbiter/device_map.h b/pci-arbiter/device_map.h
index 9062e901..0d92650c 100644
--- a/pci-arbiter/device_map.h
+++ b/pci-arbiter/device_map.h
@@ -27,6 +27,6 @@
 #include 
 
 error_t device_map_region (struct pci_device *device,
-  struct pci_mem_region *region);
+  struct pci_mem_region *region, void **addr);
 
 #endif /* DEVICE_MAP_H */
diff --git a/pci-arbiter/func_files.c b/pci-arbiter/func_files.c
index 81ebfded..40706135 100644
--- a/pci-arbiter/func_files.c
+++ b/pci-arbiter/func_files.c
@@ -205,13 +205,13 @@ io_region_file (struct pcifs_dirent * e, off_t offset, 
size_t * len,
   else
 {
   /* Ensure the region is mapped */
-  err = device_map_region (e->device, region);
+  err = device_map_region (e->device, region, &e->region_maps[reg_num]);
   if (err)
return err;
   if (read)
-   memcpy (data, region->memory + offset, *len);
+   memcpy (data, e->region_maps[reg_num] + offset, *len);
   else
-   memcpy (region->memory + offset, data, *len);
+   memcpy (e->region_maps[reg_num] + offset, data, *len);
 }
 
   return err;
diff --git a/pci-arbiter/netfs_impl.c b/pci-arbiter/netfs_impl.c
index 8b4bd22b..63f8354e 100644
--- a/pci-arbiter/netfs_impl.c
+++ b/pci-arbiter/netfs_impl.c
@@ -591,15 +591,17 @@ netfs_get_filemap (struct node *node, vm_prot_t prot)
   region = &node->nn->ln->device->regions[reg_num];
 
   /* Ensure the region is mapped */
-  err = device_map_region (node->nn->ln->device, region);
+  err = device_map_region (node->nn->ln->device, region,
+  &node->nn->ln->region_maps[reg_num]);
   if (err)
 return err;
 
   /* Create a new memory object proxy with the required protection */
   max_prot = (VM_PROT_READ | VM_PROT_WRITE) & prot;
   err =
-vm_region_create_proxy(mach_task_self (), (vm_address_t)region->memory,
-   max_prot, region->size, &proxy);
+vm_region_create_proxy(mach_task_self (),
+  (vm_address_t)node->nn->ln->region_maps[reg_num],
+  max_prot, region->size, &proxy);
   if (err)
 goto error;
 
diff --git a/pci-arbiter/pcifs.h b/pci-arbiter/pcifs.h
index 18f2141c..050c9e32 100644
--- a/pci-arbiter/pcifs.h
+++ b/pci-arbiter/pcifs.h
@@ -91,6 +91,13 @@ struct pcifs_dirent
* Only for entries having a full B/D/F address.
*/
   struct pci_device *device;
+
+  /*
+   * Array of addresses where regions are mapped
+   *
+   * Only when a device is present
+   */
+  void *region_maps[6];
 };
 
 /*
-- 
2.31.1

[PATCH] pci-arbiter: Stop using deprecated region memory pointer

[Joan Lledó]

From: Joan Lledó 

Use a internal array of pointers instead

* pci-arbiter/device_map.h:
  * Update device_map_region() prototype
* Now it receives an output address as parameter
* pci-arbiter/device_map.c:
  * Update device_map_region() definition to match the new prototype
  * Support for legacy mappings
* When the base address is lower than 1 mb
* pci-arbiter/func_files.c:
* pci-arbiter/netfs_impl.c:
  * Update calls to device_map_region to match the new prototype
  * Use the internal array of pointers instead of region->memory
* pci-arbiter/pcifs.h:
  * struct pcifs_dirent: Declare the internal array of pointers
---
 pci-arbiter/device_map.c | 15 +++
 pci-arbiter/device_map.h |  2 +-
 pci-arbiter/func_files.c |  6 +++---
 pci-arbiter/netfs_impl.c |  8 +---
 pci-arbiter/pcifs.h  |  7 +++
 5 files changed, 27 insertions(+), 11 deletions(-)

diff --git a/pci-arbiter/device_map.c b/pci-arbiter/device_map.c
index 216adfb9..b5a19074 100644
--- a/pci-arbiter/device_map.c
+++ b/pci-arbiter/device_map.c
@@ -24,14 +24,21 @@
 #include "device_map.h"
 
 error_t
-device_map_region (struct pci_device *device, struct pci_mem_region *region)
+device_map_region (struct pci_device *device, struct pci_mem_region *region,
+  void **addr)
 {
   error_t err = 0;
 
-  if (region->memory == 0)
+  if (*addr == 0)
 {
-  err = pci_device_map_range (device, region->base_addr, region->size,
- PCI_DEV_MAP_FLAG_WRITABLE, ®ion->memory);
+  /* Use the legacy call for regions under 1MB */
+  if (region->base_addr > 0x10
+ || region->base_addr + region->size > 0x10)
+err = pci_device_map_range (device, region->base_addr, region->size,
+   PCI_DEV_MAP_FLAG_WRITABLE, addr);
+  else
+err = pci_device_map_legacy (device, region->base_addr, region->size,
+   PCI_DEV_MAP_FLAG_WRITABLE, addr);
 }
 
   return err;
diff --git a/pci-arbiter/device_map.h b/pci-arbiter/device_map.h
index 9062e901..0d92650c 100644
--- a/pci-arbiter/device_map.h
+++ b/pci-arbiter/device_map.h
@@ -27,6 +27,6 @@
 #include 
 
 error_t device_map_region (struct pci_device *device,
-  struct pci_mem_region *region);
+  struct pci_mem_region *region, void **addr);
 
 #endif /* DEVICE_MAP_H */
diff --git a/pci-arbiter/func_files.c b/pci-arbiter/func_files.c
index 81ebfded..40706135 100644
--- a/pci-arbiter/func_files.c
+++ b/pci-arbiter/func_files.c
@@ -205,13 +205,13 @@ io_region_file (struct pcifs_dirent * e, off_t offset, 
size_t * len,
   else
 {
   /* Ensure the region is mapped */
-  err = device_map_region (e->device, region);
+  err = device_map_region (e->device, region, &e->region_maps[reg_num]);
   if (err)
return err;
   if (read)
-   memcpy (data, region->memory + offset, *len);
+   memcpy (data, e->region_maps[reg_num] + offset, *len);
   else
-   memcpy (region->memory + offset, data, *len);
+   memcpy (e->region_maps[reg_num] + offset, data, *len);
 }
 
   return err;
diff --git a/pci-arbiter/netfs_impl.c b/pci-arbiter/netfs_impl.c
index 8b4bd22b..63f8354e 100644
--- a/pci-arbiter/netfs_impl.c
+++ b/pci-arbiter/netfs_impl.c
@@ -591,15 +591,17 @@ netfs_get_filemap (struct node *node, vm_prot_t prot)
   region = &node->nn->ln->device->regions[reg_num];
 
   /* Ensure the region is mapped */
-  err = device_map_region (node->nn->ln->device, region);
+  err = device_map_region (node->nn->ln->device, region,
+  &node->nn->ln->region_maps[reg_num]);
   if (err)
 return err;
 
   /* Create a new memory object proxy with the required protection */
   max_prot = (VM_PROT_READ | VM_PROT_WRITE) & prot;
   err =
-vm_region_create_proxy(mach_task_self (), (vm_address_t)region->memory,
-   max_prot, region->size, &proxy);
+vm_region_create_proxy(mach_task_self (),
+  (vm_address_t)node->nn->ln->region_maps[reg_num],
+  max_prot, region->size, &proxy);
   if (err)
 goto error;
 
diff --git a/pci-arbiter/pcifs.h b/pci-arbiter/pcifs.h
index 18f2141c..050c9e32 100644
--- a/pci-arbiter/pcifs.h
+++ b/pci-arbiter/pcifs.h
@@ -91,6 +91,13 @@ struct pcifs_dirent
* Only for entries having a full B/D/F address.
*/
   struct pci_device *device;
+
+  /*
+   * Array of addresses where regions are mapped
+   *
+   * Only when a device is present
+   */
+  void *region_maps[6];
 };
 
 /*
-- 
2.31.1

Re: [PATCH] pci-arbiter: Stop using deprecated region memory pointer

[Joan Lledó]

Hi,

> Rather >= ?

No, libpciaccess accepts using the legacy method for addrress up to 0x10 
included.

> Also, why this address?  Putting a comment there would be welcome to explain 
> the magic number.

I'm taking the condition from here:

https://gitlab.freedesktop.org/xorg/lib/libpciaccess/-/blob/master/src/common_interface.c#L677

I updated the patch to add a comment, and also updated the condition to match 
the one in libpciaccess, including not only the base address but also the size

[PATCH] hurd: Implement device memory mapping

[Joan Lledó]

From: Joan Lledó 

* src/hurd_pci.c:
* Implement device memory mapping functions
* pci_device_hurd_map_range
* pci_device_hurd_unmap_range
* pci_device_hurd_map_legacy
* pci_device_hurd_unmap_legacy
* src/x86_pci.h:
* Remove unused declarations
* pci_device_x86_map_range()
* pci_device_x86_unmap_range()
* pci_device_x86_map_legacy()
* pci_device_x86_unmap_legacy()
* src/x86_pci.c:
* Fix port leaks
* Make mapping function static again
* map_dev_mem(): use device_map() support for offsets
---
 src/hurd_pci.c | 153 +++--
 src/x86_pci.c  |  23 +---
 src/x86_pci.h  |   8 ---
 3 files changed, 148 insertions(+), 36 deletions(-)

diff --git a/src/hurd_pci.c b/src/hurd_pci.c
index 40c9925..ce96cbe 100644
--- a/src/hurd_pci.c
+++ b/src/hurd_pci.c
@@ -55,6 +55,7 @@
 
 /* File names */
 #define FILE_CONFIG_NAME "config"
+#define FILE_REGION_NAME "region"
 #define FILE_ROM_NAME "rom"
 
 /* Level in the fs tree */
@@ -149,8 +150,119 @@ pci_device_hurd_probe(struct pci_device *dev)
 return 0;
 }
 
+static int
+pci_device_hurd_map_range(struct pci_device *dev,
+struct pci_device_mapping *map)
+{
+int err = 0;
+file_t file = MACH_PORT_NULL;
+memory_object_t robj, wobj, pager;
+vm_offset_t offset;
+vm_prot_t prot = VM_PROT_READ;
+const struct pci_mem_region * const region = &dev->regions[map->region];
+int flags = O_RDONLY;
+char server[NAME_MAX];
+
+if (map->flags & PCI_DEV_MAP_FLAG_WRITABLE) {
+prot |= VM_PROT_WRITE;
+flags = O_RDWR;
+}
+
+snprintf(server, NAME_MAX, "%s/%04x/%02x/%02x/%01u/%s%01u",
+_SERVERS_BUS_PCI, dev->domain, dev->bus, dev->dev, dev->func,
+FILE_REGION_NAME, map->region);
+
+file = file_name_lookup (server, flags, 0);
+if (! MACH_PORT_VALID (file)) {
+return errno;
+}
+
+err = io_map (file, &robj, &wobj);
+mach_port_deallocate (mach_task_self(), file);
+if (err)
+return err;
+
+switch (prot & (VM_PROT_READ|VM_PROT_WRITE)) {
+case VM_PROT_READ:
+pager = robj;
+if (wobj != MACH_PORT_NULL)
+mach_port_deallocate (mach_task_self(), wobj);
+break;
+case VM_PROT_READ|VM_PROT_WRITE:
+if (robj == wobj) {
+if (robj == MACH_PORT_NULL)
+return EPERM;
+
+pager = wobj;
+/* Remove extra reference.  */
+mach_port_deallocate (mach_task_self (), pager);
+}
+else {
+if (robj != MACH_PORT_NULL)
+mach_port_deallocate (mach_task_self (), robj);
+if (wobj != MACH_PORT_NULL)
+mach_port_deallocate (mach_task_self (), wobj);
+
+return EPERM;
+}
+break;
+default:
+return EINVAL;
+}
+
+offset = map->base - region->base_addr;
+err = vm_map (mach_task_self (), (vm_address_t *)&map->memory, map->size,
+  0, 1,
+  pager, /* a memory object proxy containing only the region */
+  offset, /* offset from region start */
+  0, prot, VM_PROT_ALL, VM_INHERIT_SHARE);
+mach_port_deallocate (mach_task_self(), pager);
+
+return err;
+}
+
+static int
+pci_device_hurd_unmap_range(struct pci_device *dev,
+struct pci_device_mapping *map)
+{
+int err;
+err = pci_device_generic_unmap_range(dev, map);
+map->memory = NULL;
+
+return err;
+}
+
+static int
+pci_device_hurd_map_legacy(struct pci_device *dev, pciaddr_t base,
+pciaddr_t size, unsigned map_flags, void **addr)
+{
+struct pci_device_mapping map;
+int err;
+
+map.base = base;
+map.size = size;
+map.flags = map_flags;
+err = pci_device_hurd_map_range(dev, &map);
+*addr = map.memory;
+
+return err;
+}
+
+static int
+pci_device_hurd_unmap_legacy(struct pci_device *dev, void *addr,
+pciaddr_t size)
+{
+struct pci_device_mapping map;
+
+map.size = size;
+map.flags = 0;
+map.memory = addr;
+
+return pci_device_hurd_unmap_range(dev, &map);
+}
+
 /*
- * Read `nbytes' bytes from `reg' in device's configuretion space
+ * Read `nbytes' bytes from `reg' in device's configuration space
  * and store them in `buf'.
  *
  * It's assumed that `nbytes' bytes are allocated in `buf'
@@ -339,7 +451,8 @@ enum_devices(mach_port_t pci_port, const char *parent, int 
domain,
 if (lev > LEVEL_FUNC + 1) {
 return 0;
 }
-cwd_port = file_name_lookup_under (pci_port, parent, O_DIRECTORY | O_RDWR 
| O_EXEC, 0);
+cwd_port = file_name_lookup_under (pci_port, parent,
+   O_D

Re: [PATCH] hurd: Implement device memory mapping

[Joan Lledó]

Hi,

> This is not using map.base as set by pci_device_hurd_map_legacy?

You're right, I checked it out and found that libpciaccess has a set of old and 
deprecated memory mapping functions to map/unmap entire BARs (regions). Later, 
they added new functions to allow manny mappings in the same region. Which are 
the ones we are using, tough we use them to map entire regions anyway.

I updated the libpciaccess patch to allow offsets inside the region. I also 
created a new patch for the arbiter to stop using libpciaccess region->memory 
pointer, which is deprecated.

[PATCH] pci-arbiter: Stop using deprecated region memory pointer

[Joan Lledó]

From: Joan Lledó 

Use a internal array of pointers instead

* pci-arbiter/device_map.h:
  * Update device_map_region() prototype
* Now it receives an output address as parameter
* pci-arbiter/device_map.c:
  * Update device_map_region() definition to match the new prototype
  * Support for legacy mappings
* When the base address is lower than 1 mb
* pci-arbiter/func_files.c:
* pci-arbiter/netfs_impl.c:
  * Update calls to device_map_region to match the new prototype
  * Use the internal array of pointers instead of region->memory
* pci-arbiter/pcifs.h:
  * struct pcifs_dirent: Declare the internal array of pointers
---
 pci-arbiter/device_map.c | 13 +
 pci-arbiter/device_map.h |  2 +-
 pci-arbiter/func_files.c |  6 +++---
 pci-arbiter/netfs_impl.c |  8 +---
 pci-arbiter/pcifs.h  |  7 +++
 5 files changed, 25 insertions(+), 11 deletions(-)

diff --git a/pci-arbiter/device_map.c b/pci-arbiter/device_map.c
index 216adfb9..aa7fed0b 100644
--- a/pci-arbiter/device_map.c
+++ b/pci-arbiter/device_map.c
@@ -24,14 +24,19 @@
 #include "device_map.h"
 
 error_t
-device_map_region (struct pci_device *device, struct pci_mem_region *region)
+device_map_region (struct pci_device *device, struct pci_mem_region *region,
+  void **addr)
 {
   error_t err = 0;
 
-  if (region->memory == 0)
+  if (*addr == 0)
 {
-  err = pci_device_map_range (device, region->base_addr, region->size,
- PCI_DEV_MAP_FLAG_WRITABLE, ®ion->memory);
+  if (region->base_addr > 0x10)
+err = pci_device_map_range (device, region->base_addr, region->size,
+   PCI_DEV_MAP_FLAG_WRITABLE, addr);
+  else
+err = pci_device_map_legacy (device, region->base_addr, region->size,
+   PCI_DEV_MAP_FLAG_WRITABLE, addr);
 }
 
   return err;
diff --git a/pci-arbiter/device_map.h b/pci-arbiter/device_map.h
index 9062e901..0d92650c 100644
--- a/pci-arbiter/device_map.h
+++ b/pci-arbiter/device_map.h
@@ -27,6 +27,6 @@
 #include 
 
 error_t device_map_region (struct pci_device *device,
-  struct pci_mem_region *region);
+  struct pci_mem_region *region, void **addr);
 
 #endif /* DEVICE_MAP_H */
diff --git a/pci-arbiter/func_files.c b/pci-arbiter/func_files.c
index 81ebfded..40706135 100644
--- a/pci-arbiter/func_files.c
+++ b/pci-arbiter/func_files.c
@@ -205,13 +205,13 @@ io_region_file (struct pcifs_dirent * e, off_t offset, 
size_t * len,
   else
 {
   /* Ensure the region is mapped */
-  err = device_map_region (e->device, region);
+  err = device_map_region (e->device, region, &e->region_maps[reg_num]);
   if (err)
return err;
   if (read)
-   memcpy (data, region->memory + offset, *len);
+   memcpy (data, e->region_maps[reg_num] + offset, *len);
   else
-   memcpy (region->memory + offset, data, *len);
+   memcpy (e->region_maps[reg_num] + offset, data, *len);
 }
 
   return err;
diff --git a/pci-arbiter/netfs_impl.c b/pci-arbiter/netfs_impl.c
index 8b4bd22b..63f8354e 100644
--- a/pci-arbiter/netfs_impl.c
+++ b/pci-arbiter/netfs_impl.c
@@ -591,15 +591,17 @@ netfs_get_filemap (struct node *node, vm_prot_t prot)
   region = &node->nn->ln->device->regions[reg_num];
 
   /* Ensure the region is mapped */
-  err = device_map_region (node->nn->ln->device, region);
+  err = device_map_region (node->nn->ln->device, region,
+  &node->nn->ln->region_maps[reg_num]);
   if (err)
 return err;
 
   /* Create a new memory object proxy with the required protection */
   max_prot = (VM_PROT_READ | VM_PROT_WRITE) & prot;
   err =
-vm_region_create_proxy(mach_task_self (), (vm_address_t)region->memory,
-   max_prot, region->size, &proxy);
+vm_region_create_proxy(mach_task_self (),
+  (vm_address_t)node->nn->ln->region_maps[reg_num],
+  max_prot, region->size, &proxy);
   if (err)
 goto error;
 
diff --git a/pci-arbiter/pcifs.h b/pci-arbiter/pcifs.h
index 18f2141c..050c9e32 100644
--- a/pci-arbiter/pcifs.h
+++ b/pci-arbiter/pcifs.h
@@ -91,6 +91,13 @@ struct pcifs_dirent
* Only for entries having a full B/D/F address.
*/
   struct pci_device *device;
+
+  /*
+   * Array of addresses where regions are mapped
+   *
+   * Only when a device is present
+   */
+  void *region_maps[6];
 };
 
 /*
-- 
2.31.1

Re: [PATCH] hurd: Implement device memory mapping

[Joan Lledó]

Hi,

I attached a patch with the changes

[PATCH] hurd: Implement device memory mapping

[Joan Lledó]

From: Joan Lledó 

* src/hurd_pci.c:
* Implement device memory mapping functions
* pci_device_hurd_map_range
* pci_device_hurd_unmap_range
* pci_device_hurd_map_legacy
* pci_device_hurd_unmap_legacy
* src/x86_pci.h:
* Remove unused declarations
* pci_device_x86_map_range()
* pci_device_x86_unmap_range()
* pci_device_x86_map_legacy()
* pci_device_x86_unmap_legacy()
* src/x86_pci.c:
* Fix port leaks
* Make mapping function static again
* map_dev_mem(): use device_map() support for offsets
---
 src/hurd_pci.c | 150 ++---
 src/x86_pci.c  |  23 +---
 src/x86_pci.h  |   8 ---
 3 files changed, 145 insertions(+), 36 deletions(-)

diff --git a/src/hurd_pci.c b/src/hurd_pci.c
index 40c9925..ed0e89a 100644
--- a/src/hurd_pci.c
+++ b/src/hurd_pci.c
@@ -55,6 +55,7 @@
 
 /* File names */
 #define FILE_CONFIG_NAME "config"
+#define FILE_REGION_NAME "region"
 #define FILE_ROM_NAME "rom"
 
 /* Level in the fs tree */
@@ -149,8 +150,116 @@ pci_device_hurd_probe(struct pci_device *dev)
 return 0;
 }
 
+static int
+pci_device_hurd_map_range(struct pci_device *dev,
+struct pci_device_mapping *map)
+{
+int err = 0;
+file_t file = MACH_PORT_NULL;
+memory_object_t robj, wobj, pager;
+vm_prot_t prot = VM_PROT_READ;
+int flags = O_RDONLY;
+char server[NAME_MAX];
+
+if (map->flags & PCI_DEV_MAP_FLAG_WRITABLE) {
+prot |= VM_PROT_WRITE;
+flags = O_RDWR;
+}
+
+snprintf(server, NAME_MAX, "%s/%04x/%02x/%02x/%01u/%s%01u",
+_SERVERS_BUS_PCI, dev->domain, dev->bus, dev->dev, dev->func,
+FILE_REGION_NAME, map->region);
+
+file = file_name_lookup (server, flags, 0);
+if (! MACH_PORT_VALID (file)) {
+return errno;
+}
+
+err = io_map (file, &robj, &wobj);
+mach_port_deallocate (mach_task_self(), file);
+if (err)
+return err;
+
+switch (prot & (VM_PROT_READ|VM_PROT_WRITE)) {
+case VM_PROT_READ:
+pager = robj;
+if (wobj != MACH_PORT_NULL)
+mach_port_deallocate (mach_task_self(), wobj);
+break;
+case VM_PROT_READ|VM_PROT_WRITE:
+if (robj == wobj) {
+if (robj == MACH_PORT_NULL)
+return EPERM;
+
+pager = wobj;
+/* Remove extra reference.  */
+mach_port_deallocate (mach_task_self (), pager);
+}
+else {
+if (robj != MACH_PORT_NULL)
+mach_port_deallocate (mach_task_self (), robj);
+if (wobj != MACH_PORT_NULL)
+mach_port_deallocate (mach_task_self (), wobj);
+
+return EPERM;
+}
+break;
+default:
+return EINVAL;
+}
+
+err = vm_map (mach_task_self (), (vm_address_t *)&map->memory, map->size,
+  0, 1,
+  pager, /* a memory object proxy containing only the region */
+  0, /* offset from region start */
+  0, prot, VM_PROT_ALL, VM_INHERIT_SHARE);
+mach_port_deallocate (mach_task_self(), pager);
+
+return err;
+}
+
+static int
+pci_device_hurd_unmap_range(struct pci_device *dev,
+struct pci_device_mapping *map)
+{
+int err;
+err = pci_device_generic_unmap_range(dev, map);
+map->memory = NULL;
+
+return err;
+}
+
+static int
+pci_device_hurd_map_legacy(struct pci_device *dev, pciaddr_t base,
+pciaddr_t size, unsigned map_flags, void **addr)
+{
+struct pci_device_mapping map;
+int err;
+
+map.base = base;
+map.size = size;
+map.flags = map_flags;
+err = pci_device_hurd_map_range(dev, &map);
+*addr = map.memory;
+
+return err;
+}
+
+static int
+pci_device_hurd_unmap_legacy(struct pci_device *dev, void *addr,
+pciaddr_t size)
+{
+struct pci_device_mapping map;
+
+map.size = size;
+map.flags = 0;
+map.memory = addr;
+
+return pci_device_hurd_unmap_range(dev, &map);
+}
+
 /*
- * Read `nbytes' bytes from `reg' in device's configuretion space
+ * Read `nbytes' bytes from `reg' in device's configuration space
  * and store them in `buf'.
  *
  * It's assumed that `nbytes' bytes are allocated in `buf'
@@ -339,7 +448,8 @@ enum_devices(mach_port_t pci_port, const char *parent, int 
domain,
 if (lev > LEVEL_FUNC + 1) {
 return 0;
 }
-cwd_port = file_name_lookup_under (pci_port, parent, O_DIRECTORY | O_RDWR 
| O_EXEC, 0);
+cwd_port = file_name_lookup_under (pci_port, parent,
+   O_DIRECTORY | O_RDONLY | O_EXEC, 0);
 if (cwd_port == MACH_PORT_NULL) {
 return 0;
 }
@@ -391,7 +501,7 @@ enum_devices(mach_port_t pci_por

Re: [PATCH] hurd: Implement device memory mapping

[Joan Lledó]

Hi,

El 28/12/21 a les 13:55, Samuel Thibault ha escrit:


? The interface explicitly says they can be different.

Samuel



Sorry, I was confused. I meant the function pci_device_hurd_map_range at 
libpciaccess. If io_map returns different ports for robj and wobj, we 
can only assign one of them to the variable pager, which we send to 
vm_map. Since we can only pass one of them to vm_map, when io_map 
returns different values for robj and wobj we could return an error and 
don't map. Something like this:


case VM_PROT_READ|VM_PROT_WRITE:
if (robj == wobj) {
if (robj == MACH_PORT_NULL)
return EPERM;

pager = wobj;
/* Remove extra reference.  */
mach_port_deallocate (mach_task_self (), pager);
}
else {
if (robj != MACH_PORT_NULL)
mach_port_deallocate (mach_task_self (), robj);
if (wobj != MACH_PORT_NULL)
mach_port_deallocate (mach_task_self (), wobj);

return EPERM;
}
break;

Re: [PATCH] hurd: Implement device memory mapping

[Joan Lledó]

Hi,

El 19/12/21 a les 16:40, Samuel Thibault ha escrit:

“
For objects where read data and write data are the same, these objects
will be equal, otherwise they will be disjoint.
”



In that case, which one should we return as pager? Probably the right 
thing is to return an error, since a caller asking for RW permissions is 
expecting to receive a pager with both permissions. Either robj & wobj 
are equal and not null, or return an error.

[PATCH 1/3] libnetfs: new user callback: netfs_get_filemap()

[Joan Lledó]

From: Joan Lledó 

Provide the user with a new callback so they can implement file
mapping over file system nodes.

* libnetfs/netfs.h: Add prototype for netfs_get_filemap
* libnetfs/file-map.c: netfs_get_filemap definition
* libnetfs/Makefile: add file-map.c to sources
---
 libnetfs/Makefile   |  2 +-
 libnetfs/file-map.c | 31 +++
 libnetfs/netfs.h|  5 +
 3 files changed, 37 insertions(+), 1 deletion(-)
 create mode 100644 libnetfs/file-map.c

diff --git a/libnetfs/Makefile b/libnetfs/Makefile
index 0d4205d5..24606ff9 100644
--- a/libnetfs/Makefile
+++ b/libnetfs/Makefile
@@ -31,7 +31,7 @@ FSSRCS= dir-link.c dir-lookup.c dir-mkdir.c dir-mkfile.c \
file-check-access.c file-chflags.c file-chmod.c file-chown.c \
file-exec.c file-get-fs-options.c file-get-storage-info.c \
file-get-translator.c file-getcontrol.c file-getlinknode.c \
-   file-lock-stat.c file-lock.c file-set-size.c \
+   file-lock-stat.c file-lock.c file-map.c file-set-size.c \
file-set-translator.c file-statfs.c file-sync.c file-syncfs.c \
file-utimes.c file-record-lock.c file-reparent.c fsstubs.c \
file-get-transcntl.c
diff --git a/libnetfs/file-map.c b/libnetfs/file-map.c
new file mode 100644
index ..85fe93f2
--- /dev/null
+++ b/libnetfs/file-map.c
@@ -0,0 +1,31 @@
+/* Default version of netfs_get_filemap
+
+   Copyright (C) 2021 Free Software Foundation, Inc.
+
+   This file is part of the GNU Hurd.
+
+   The GNU Hurd 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.
+
+   The GNU Hurd 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., 59 Temple Place - Suite 330, Boston, MA  02111, USA. */
+
+#include "netfs.h"
+
+/* The user may define this function. Return a memory object proxy port (send
+   right) for the file contents of NP. PROT is the maximum allowable
+   access. On errors, return MACH_PORT_NULL and set errno.  */
+mach_port_t __attribute__ ((weak))
+netfs_get_filemap (struct node *np, vm_prot_t prot)
+{
+  errno = EOPNOTSUPP;
+  return MACH_PORT_NULL;
+}
diff --git a/libnetfs/netfs.h b/libnetfs/netfs.h
index 38182ab7..ca506747 100644
--- a/libnetfs/netfs.h
+++ b/libnetfs/netfs.h
@@ -303,6 +303,11 @@ error_t netfs_get_dirents (struct iouser *cred, struct 
node *dir,
   mach_msg_type_number_t *datacnt,
   vm_size_t bufsize, int *amt);
 
+/* The user may define this function. Return a memory object proxy port (send
+   right) for the file contents of NP. PROT is the maximum allowable
+   access. On errors, return MACH_PORT_NULL and set errno.  */
+mach_port_t netfs_get_filemap (struct node *np, vm_prot_t prot);
+
 /* The user may define this function.  For a full description,
see hurd/hurd_types.h.  The default response indicates a network
store.  If the supplied buffers are not large enough, they should
-- 
2.31.1

Re: [PATCH 1/3] libnetfs: new user callback: netfs_get_filemap()

[Joan Lledó]

> This only provides the declaration.

Sorry, I hope it's correct now

[PATCH 2/3] libnetfs: Implement RPC: io_map

[Joan Lledó]

From: Marcus Brinkmann 

* libnetfs/iostubs.c: implement io_map
---
 libnetfs/iostubs.c | 46 +++---
 1 file changed, 43 insertions(+), 3 deletions(-)

diff --git a/libnetfs/iostubs.c b/libnetfs/iostubs.c
index a5ff4504..df48f8b7 100644
--- a/libnetfs/iostubs.c
+++ b/libnetfs/iostubs.c
@@ -1,4 +1,4 @@
-/* 
+/*
Copyright (C) 1995 Free Software Foundation, Inc.
Written by Michael I. Bushnell, p/BSG.
 
@@ -23,11 +23,51 @@
 #include "io_S.h"
 
 error_t __attribute__((weak))
-netfs_S_io_map (struct protid *user, 
+netfs_S_io_map (struct protid *user,
mach_port_t *rdobj, mach_msg_type_name_t *rdobjtype,
mach_port_t *wrobj, mach_msg_type_name_t *wrobjtype)
 {
-  return EOPNOTSUPP;
+  int flags;
+  struct node *node;
+
+  if (!user)
+return EOPNOTSUPP;
+
+  *wrobj = *rdobj = MACH_PORT_NULL;
+
+  node = user->po->np;
+  flags = user->po->openstat & (O_READ | O_WRITE);
+
+  pthread_mutex_lock (&node->lock);
+  switch (flags)
+{
+case O_READ | O_WRITE:
+  *wrobj = *rdobj = netfs_get_filemap (node, VM_PROT_READ |VM_PROT_WRITE);
+  if (*wrobj == MACH_PORT_NULL)
+   goto error;
+  mach_port_mod_refs (mach_task_self (), *rdobj, MACH_PORT_RIGHT_SEND, 1);
+  break;
+case O_READ:
+  *rdobj = netfs_get_filemap (node, VM_PROT_READ);
+  if (*rdobj == MACH_PORT_NULL)
+   goto error;
+  break;
+case O_WRITE:
+  *wrobj = netfs_get_filemap (node, VM_PROT_WRITE);
+  if (*wrobj == MACH_PORT_NULL)
+   goto error;
+  break;
+}
+  pthread_mutex_unlock (&node->lock);
+
+  *rdobjtype = MACH_MSG_TYPE_MOVE_SEND;
+  *wrobjtype = MACH_MSG_TYPE_MOVE_SEND;
+
+  return 0;
+
+error:
+  pthread_mutex_unlock (&node->lock);
+  return errno;
 }
 
 error_t __attribute__((weak))
-- 
2.31.1

[PATCH 3/3] pci-arbiter: Implement memory mapping over region files

[Joan Lledó]

From: Joan Lledó 

* pci-arbiter/Makefile:
* Add device_map.c to sources
* pci-arbiter/device_map.c:
* pci-arbiter/device_map.h:
* New module for device mapping
* Relies on libpciaccess mapping methods
* pci-arbiter/func_files.c:
* io_region_file(): Use the new device mapping module
* pci-arbiter/netfs_impl.c:
* Implements netfs_get_filemap():
* Uses the device mapping module to map the region to the
  arbiter space
* Calls the kernel RPC vm_region_create_proxy() to obtain the
  memory object proxy
* Only region files are mapped for now
---
 pci-arbiter/Makefile |  2 +-
 pci-arbiter/device_map.c | 38 +++
 pci-arbiter/device_map.h | 32 +++
 pci-arbiter/func_files.c | 16 +-
 pci-arbiter/netfs_impl.c | 48 +++-
 5 files changed, 124 insertions(+), 12 deletions(-)
 create mode 100644 pci-arbiter/device_map.c
 create mode 100644 pci-arbiter/device_map.h

diff --git a/pci-arbiter/Makefile b/pci-arbiter/Makefile
index d3d205ec..821c3ca9 100644
--- a/pci-arbiter/Makefile
+++ b/pci-arbiter/Makefile
@@ -22,7 +22,7 @@ PORTDIR = $(srcdir)/port
 
 SRCS   = main.c pci-ops.c netfs_impl.c \
  pcifs.c ncache.c options.c func_files.c \
- pciServer.c startup_notifyServer.c
+ device_map.c pciServer.c startup_notifyServer.c
 OBJS   = $(SRCS:.c=.o) $(MIGSTUBS)
 
 HURDLIBS= fshelp ports shouldbeinlibc netfs iohelp ihash trivfs machdev
diff --git a/pci-arbiter/device_map.c b/pci-arbiter/device_map.c
new file mode 100644
index ..216adfb9
--- /dev/null
+++ b/pci-arbiter/device_map.c
@@ -0,0 +1,38 @@
+/*
+   Copyright (C) 2021 Free Software Foundation, Inc.
+
+   This file is part of the GNU Hurd.
+
+   The GNU Hurd 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.
+
+   The GNU Hurd 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 the GNU Hurd.  If not, see <http://www.gnu.org/licenses/>.
+*/
+
+/* Implementation for device memory mapping functions */
+
+#include 
+
+#include "device_map.h"
+
+error_t
+device_map_region (struct pci_device *device, struct pci_mem_region *region)
+{
+  error_t err = 0;
+
+  if (region->memory == 0)
+{
+  err = pci_device_map_range (device, region->base_addr, region->size,
+ PCI_DEV_MAP_FLAG_WRITABLE, ®ion->memory);
+}
+
+  return err;
+}
diff --git a/pci-arbiter/device_map.h b/pci-arbiter/device_map.h
new file mode 100644
index ..9062e901
--- /dev/null
+++ b/pci-arbiter/device_map.h
@@ -0,0 +1,32 @@
+/*
+   Copyright (C) 2021 Free Software Foundation, Inc.
+
+   This file is part of the GNU Hurd.
+
+   The GNU Hurd 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.
+
+   The GNU Hurd 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 the GNU Hurd.  If not, see <http://www.gnu.org/licenses/>.
+*/
+
+/* Header for device memory mapping functions */
+
+#ifndef DEVICE_MAP_H
+#define DEVICE_MAP_H
+
+#include 
+
+#include 
+
+error_t device_map_region (struct pci_device *device,
+  struct pci_mem_region *region);
+
+#endif /* DEVICE_MAP_H */
diff --git a/pci-arbiter/func_files.c b/pci-arbiter/func_files.c
index d7d8c5d5..81ebfded 100644
--- a/pci-arbiter/func_files.c
+++ b/pci-arbiter/func_files.c
@@ -30,6 +30,8 @@
 
 #include 
 
+#include "device_map.h"
+
 /* Read or write a block of data from/to the configuration space */
 static error_t
 config_block_op (struct pci_device *dev, off_t offset, size_t * len,
@@ -202,16 +204,10 @@ io_region_file (struct pcifs_dirent * e, off_t offset, 
size_t * len,
 region_block_ioport_op (region->base_addr, offset, len, data, read);
   else
 {
-  /* First check whether the region is already mapped */
-  if (region->memory == 0)
-   {
- /* Not mapped, try to map it now */
- err =
-   pci_device_map

[PATCH 1/3] libnetfs: new user callback: netfs_get_filemap()

[Joan Lledó]

From: Joan Lledó 

Provide the user with a new callback so they can implement file
mapping over file system nodes.

* libnetfs/netfs.h: Add prototype for netfs_get_filemap
---
 libnetfs/netfs.h | 6 ++
 1 file changed, 6 insertions(+)

diff --git a/libnetfs/netfs.h b/libnetfs/netfs.h
index 38182ab7..96d1eea8 100644
--- a/libnetfs/netfs.h
+++ b/libnetfs/netfs.h
@@ -303,6 +303,12 @@ error_t netfs_get_dirents (struct iouser *cred, struct 
node *dir,
   mach_msg_type_number_t *datacnt,
   vm_size_t bufsize, int *amt);
 
+/* The user may define this function. Return a memory object proxy port (send
+   right) for the file contents of NP. PROT is the maximum allowable
+   access. On errors, return MACH_PORT_NULL and set errno.  */
+mach_port_t __attribute__ ((weak))
+netfs_get_filemap (struct node *np, vm_prot_t prot);
+
 /* The user may define this function.  For a full description,
see hurd/hurd_types.h.  The default response indicates a network
store.  If the supplied buffers are not large enough, they should
-- 
2.31.1

Re: [PATCH 1/3] libnetfs: new user callback: netfs_get_filemap()

[Joan Lledó]

> Rather than requiring the translator to provide a netfs_get_filemap
> (which breaks compatibility for any existing translator using
> libnetfs), you can add a weak definition in libnetfs, see for instance
> libnetfs/set-get-trans.c.

Like this?

I'm also attaching the other two patches, since there were some conflicts.

Re: [PATCH] hurd: Implement device memory mapping

[Joan Lledó]

El 12/12/21 a les 16:19, Samuel Thibault ha escrit:


Don't we need

else { pager = wobj; } ?

Otherwise pager would be undefined.


In the case when the two previous conditions are false, what about robj? 
can we be sure it's null and not leaked?


When could the two previous conditions be false, when the user asked for 
RW permissions but only write was granted?

[PATCH 3/3] pci-arbiter: Implement memory mapping over region files

[Joan Lledó]

From: Joan Lledó 

* pci-arbiter/Makefile:
* Add device_map.c to sources
* pci-arbiter/device_map.c:
* pci-arbiter/device_map.h:
* New module for device mapping
* Relies on libpciaccess mapping methods
* pci-arbiter/func_files.c:
* io_region_file(): Use the new device mapping module
* pci-arbiter/netfs_impl.c:
* Implements netfs_get_filemap():
* Uses the device mapping module to map the region to the
  arbiter space
* Calls the kernel RPC vm_region_create_proxy() to obtain the
  memory object proxy
* Only region files are mapped for now
---
 pci-arbiter/Makefile |  2 +-
 pci-arbiter/device_map.c | 38 ++
 pci-arbiter/device_map.h | 32 +
 pci-arbiter/func_files.c | 16 ++-
 pci-arbiter/netfs_impl.c | 44 
 5 files changed, 117 insertions(+), 15 deletions(-)
 create mode 100644 pci-arbiter/device_map.c
 create mode 100644 pci-arbiter/device_map.h

diff --git a/pci-arbiter/Makefile b/pci-arbiter/Makefile
index d3d205ec..821c3ca9 100644
--- a/pci-arbiter/Makefile
+++ b/pci-arbiter/Makefile
@@ -22,7 +22,7 @@ PORTDIR = $(srcdir)/port
 
 SRCS   = main.c pci-ops.c netfs_impl.c \
  pcifs.c ncache.c options.c func_files.c \
- pciServer.c startup_notifyServer.c
+ device_map.c pciServer.c startup_notifyServer.c
 OBJS   = $(SRCS:.c=.o) $(MIGSTUBS)
 
 HURDLIBS= fshelp ports shouldbeinlibc netfs iohelp ihash trivfs machdev
diff --git a/pci-arbiter/device_map.c b/pci-arbiter/device_map.c
new file mode 100644
index ..216adfb9
--- /dev/null
+++ b/pci-arbiter/device_map.c
@@ -0,0 +1,38 @@
+/*
+   Copyright (C) 2021 Free Software Foundation, Inc.
+
+   This file is part of the GNU Hurd.
+
+   The GNU Hurd 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.
+
+   The GNU Hurd 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 the GNU Hurd.  If not, see <http://www.gnu.org/licenses/>.
+*/
+
+/* Implementation for device memory mapping functions */
+
+#include 
+
+#include "device_map.h"
+
+error_t
+device_map_region (struct pci_device *device, struct pci_mem_region *region)
+{
+  error_t err = 0;
+
+  if (region->memory == 0)
+{
+  err = pci_device_map_range (device, region->base_addr, region->size,
+ PCI_DEV_MAP_FLAG_WRITABLE, ®ion->memory);
+}
+
+  return err;
+}
diff --git a/pci-arbiter/device_map.h b/pci-arbiter/device_map.h
new file mode 100644
index ..9062e901
--- /dev/null
+++ b/pci-arbiter/device_map.h
@@ -0,0 +1,32 @@
+/*
+   Copyright (C) 2021 Free Software Foundation, Inc.
+
+   This file is part of the GNU Hurd.
+
+   The GNU Hurd 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.
+
+   The GNU Hurd 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 the GNU Hurd.  If not, see <http://www.gnu.org/licenses/>.
+*/
+
+/* Header for device memory mapping functions */
+
+#ifndef DEVICE_MAP_H
+#define DEVICE_MAP_H
+
+#include 
+
+#include 
+
+error_t device_map_region (struct pci_device *device,
+  struct pci_mem_region *region);
+
+#endif /* DEVICE_MAP_H */
diff --git a/pci-arbiter/func_files.c b/pci-arbiter/func_files.c
index d7d8c5d5..81ebfded 100644
--- a/pci-arbiter/func_files.c
+++ b/pci-arbiter/func_files.c
@@ -30,6 +30,8 @@
 
 #include 
 
+#include "device_map.h"
+
 /* Read or write a block of data from/to the configuration space */
 static error_t
 config_block_op (struct pci_device *dev, off_t offset, size_t * len,
@@ -202,16 +204,10 @@ io_region_file (struct pcifs_dirent * e, off_t offset, 
size_t * len,
 region_block_ioport_op (region->base_addr, offset, len, data, read);
   else
 {
-  /* First check whether the region is already mapped */
-  if (region->memory == 0)
-   {
- /* Not mapped, try to map it now */
- err =
-   pci_device_map

Re: PCI arbiter memory mapping

[Joan Lledó]

Hi,

These patches implement device memory mapping in the arbiter by using the new 
gnumach RPC

[PATCH 2/3] libnetfs: Implement RPC: io_map

[Joan Lledó]

From: Marcus Brinkmann 

* libnetfs/iostubs.c: implement io_map
---
 libnetfs/iostubs.c | 46 +++---
 1 file changed, 43 insertions(+), 3 deletions(-)

diff --git a/libnetfs/iostubs.c b/libnetfs/iostubs.c
index a5ff4504..df48f8b7 100644
--- a/libnetfs/iostubs.c
+++ b/libnetfs/iostubs.c
@@ -1,4 +1,4 @@
-/* 
+/*
Copyright (C) 1995 Free Software Foundation, Inc.
Written by Michael I. Bushnell, p/BSG.
 
@@ -23,11 +23,51 @@
 #include "io_S.h"
 
 error_t __attribute__((weak))
-netfs_S_io_map (struct protid *user, 
+netfs_S_io_map (struct protid *user,
mach_port_t *rdobj, mach_msg_type_name_t *rdobjtype,
mach_port_t *wrobj, mach_msg_type_name_t *wrobjtype)
 {
-  return EOPNOTSUPP;
+  int flags;
+  struct node *node;
+
+  if (!user)
+return EOPNOTSUPP;
+
+  *wrobj = *rdobj = MACH_PORT_NULL;
+
+  node = user->po->np;
+  flags = user->po->openstat & (O_READ | O_WRITE);
+
+  pthread_mutex_lock (&node->lock);
+  switch (flags)
+{
+case O_READ | O_WRITE:
+  *wrobj = *rdobj = netfs_get_filemap (node, VM_PROT_READ |VM_PROT_WRITE);
+  if (*wrobj == MACH_PORT_NULL)
+   goto error;
+  mach_port_mod_refs (mach_task_self (), *rdobj, MACH_PORT_RIGHT_SEND, 1);
+  break;
+case O_READ:
+  *rdobj = netfs_get_filemap (node, VM_PROT_READ);
+  if (*rdobj == MACH_PORT_NULL)
+   goto error;
+  break;
+case O_WRITE:
+  *wrobj = netfs_get_filemap (node, VM_PROT_WRITE);
+  if (*wrobj == MACH_PORT_NULL)
+   goto error;
+  break;
+}
+  pthread_mutex_unlock (&node->lock);
+
+  *rdobjtype = MACH_MSG_TYPE_MOVE_SEND;
+  *wrobjtype = MACH_MSG_TYPE_MOVE_SEND;
+
+  return 0;
+
+error:
+  pthread_mutex_unlock (&node->lock);
+  return errno;
 }
 
 error_t __attribute__((weak))
-- 
2.31.1

Re: About consuming/releaseing ports in libpciaccess

[Joan Lledó]

Hi,

> These rules are different between the server and client sides of MIG.
> I was talking about the server side; but you are on the client side
> here. On the client side, whether or not the call consumes a port
> right is determined by the "disposition" specified in .defs (or
> specified at runtime, for polymorphic types). Basically, if the
> disposition is move-send, then the call does consume the right, and if
> it's copy-send, then it doesn't. And copy-send is by far the most
> common.

I think I'll need some explanations here... I never heard about "disposition" 
before, do you mean whether the parameter in the .defs has the "out" keyword?

> Hello! You're lucky to have caught me on my very last day 

Then I'm a bit late now :)

I attached the patch for libpciaccess, I'd like to send it to upstream if you 
guys agree.

[PATCH] hurd: Implement device memory mapping

[Joan Lledó]

From: Joan Lledó 

* src/hurd_pci.c:
* Implement device memory mapping functions
* pci_device_hurd_map_range
* pci_device_hurd_unmap_range
* pci_device_hurd_map_legacy
* pci_device_hurd_unmap_legacy
* src/x86_pci.h:
* Remove unused declarations
* pci_device_x86_map_range()
* pci_device_x86_unmap_range()
* pci_device_x86_map_legacy()
* pci_device_x86_unmap_legacy()
* src/x86_pci.c:
* Fix port leaks
* Make mapping function static again
* map_dev_mem(): use device_map() support for offsets
---
 src/hurd_pci.c | 146 ++---
 src/x86_pci.c  |  23 +---
 src/x86_pci.h  |   8 ---
 3 files changed, 141 insertions(+), 36 deletions(-)

diff --git a/src/hurd_pci.c b/src/hurd_pci.c
index 40c9925..4a07e4e 100644
--- a/src/hurd_pci.c
+++ b/src/hurd_pci.c
@@ -55,6 +55,7 @@
 
 /* File names */
 #define FILE_CONFIG_NAME "config"
+#define FILE_REGION_NAME "region"
 #define FILE_ROM_NAME "rom"
 
 /* Level in the fs tree */
@@ -149,8 +150,112 @@ pci_device_hurd_probe(struct pci_device *dev)
 return 0;
 }
 
+static int
+pci_device_hurd_map_range(struct pci_device *dev,
+struct pci_device_mapping *map)
+{
+int err = 0;
+file_t file = MACH_PORT_NULL;
+memory_object_t robj = MACH_PORT_NULL;
+memory_object_t wobj = MACH_PORT_NULL;
+memory_object_t pager = MACH_PORT_NULL;
+vm_prot_t prot = VM_PROT_READ;
+int flags = O_RDONLY;
+char server[NAME_MAX];
+
+if (map->flags & PCI_DEV_MAP_FLAG_WRITABLE) {
+prot |= VM_PROT_WRITE;
+flags = O_RDWR;
+}
+
+snprintf(server, NAME_MAX, "%s/%04x/%02x/%02x/%01u/%s%01u",
+_SERVERS_BUS_PCI, dev->domain, dev->bus, dev->dev, dev->func,
+FILE_REGION_NAME, map->region);
+
+file = file_name_lookup (server, flags, 0);
+if (! MACH_PORT_VALID (file)) {
+return errno;
+}
+
+err = io_map (file, &robj, &wobj);
+mach_port_deallocate (mach_task_self(), file);
+if (err)
+return err;
+
+switch (prot & (VM_PROT_READ|VM_PROT_WRITE)) {
+case VM_PROT_READ:
+pager = robj;
+if (wobj != MACH_PORT_NULL)
+mach_port_deallocate (mach_task_self(), wobj);
+break;
+case VM_PROT_READ|VM_PROT_WRITE:
+if (robj == wobj) {
+pager = wobj;
+/* Remove extra reference.  */
+mach_port_deallocate (mach_task_self (), pager);
+}
+else if (wobj == MACH_PORT_NULL) {
+/* We asked for write permissions but they weren't granted.  */
+mach_port_deallocate (mach_task_self (), robj);
+return EPERM;
+}
+break;
+default:
+return EINVAL;
+}
+
+err = vm_map (mach_task_self (), (vm_address_t *)&map->memory, map->size,
+  0, 1,
+  pager, /* a memory object proxy containing only the region */
+  0, /* offset from region start */
+  0, prot, VM_PROT_ALL, VM_INHERIT_SHARE);
+mach_port_deallocate (mach_task_self(), pager);
+
+return err;
+}
+
+static int
+pci_device_hurd_unmap_range(struct pci_device *dev,
+struct pci_device_mapping *map)
+{
+int err;
+err = pci_device_generic_unmap_range(dev, map);
+map->memory = NULL;
+
+return err;
+}
+
+static int
+pci_device_hurd_map_legacy(struct pci_device *dev, pciaddr_t base,
+pciaddr_t size, unsigned map_flags, void **addr)
+{
+struct pci_device_mapping map;
+int err;
+
+map.base = base;
+map.size = size;
+map.flags = map_flags;
+err = pci_device_hurd_map_range(dev, &map);
+*addr = map.memory;
+
+return err;
+}
+
+static int
+pci_device_hurd_unmap_legacy(struct pci_device *dev, void *addr,
+pciaddr_t size)
+{
+struct pci_device_mapping map;
+
+map.size = size;
+map.flags = 0;
+map.memory = addr;
+
+return pci_device_hurd_unmap_range(dev, &map);
+}
+
 /*
- * Read `nbytes' bytes from `reg' in device's configuretion space
+ * Read `nbytes' bytes from `reg' in device's configuration space
  * and store them in `buf'.
  *
  * It's assumed that `nbytes' bytes are allocated in `buf'
@@ -339,7 +444,8 @@ enum_devices(mach_port_t pci_port, const char *parent, int 
domain,
 if (lev > LEVEL_FUNC + 1) {
 return 0;
 }
-cwd_port = file_name_lookup_under (pci_port, parent, O_DIRECTORY | O_RDWR 
| O_EXEC, 0);
+cwd_port = file_name_lookup_under (pci_port, parent,
+   O_DIRECTORY | O_RDONLY | O_EXEC, 0);
 if (cwd_port == MACH_PORT_NULL) {
 return 0;
 }
@@ -391,7 +497,7 @@ enum_devices(mach_port_t pci_port, const char *parent, int 
doma

About consuming/releaseing ports in libpciaccess

[Joan Lledó]

Hi,

I'm trying to implement a correct release of resources in libpciaccess 
x86 module. I have this text from Sergey as reference:


> Consuming, as in taking ownership of, or "eating" the passed-in 
reference.

>
> If a call consuming something, the callee takes ownership of the
> passed-in reference:
>
> some_resource_t global_resource;
>
> void
> callee (some_resource_t *argument)
> {
>/* Take ownership.  */
>global_resource = argument;
> }
>
> void
> caller ()
> {
>some_resource_t *res = make_resource ();
>callee (res);
>/* No need to release the resource -- the callee took ownership of 
it.  */

> }
>
> If the call is *not* consuming something, the caller keeps ownership
> of the reference, and the callee has to add another reference if it
> wants to keep the resource for longer:
>
> some_resource_t global_resource;
>
> void
> callee (some_resource_t *argument)
> {
>/* Add a reference since we're storing the resource for longer.  */
>some_resource_ref (argument);
>global_resource = argument;
> }
>
> void
> caller ()
> {
>some_resource_t *res = make_resource ();
>callee (res);
>some_resource_rele (res);
> }
>
> For MIG routines, the convention is that the routine implementation
> takes ownership of all passed-in resources (except the request port)
> if and only if it returns KERN_SUCCESS (aka 0) or MIG_NO_REPLY. In
> other cases (so, for error returns) MIG runtime (or technically not
> even MIG but rather mach_msg_server ()) will destroy any resources in
> the message.

Please take a look at map_dev_mem() implementation in my branch at [1].

So, as I understood it, if the callee is supposed to take ownership of 
all given resources except the request port, then in the call to 
device_open(), the routine is not consuming the master_device and I 
should release it after the call, is that right?


a few lines below, it's the same situation when calling device_map(), 
it's not consuming devmem, so should I call device_close and release it 
after the call?


And what about the pager after calling vm_map? since it's not the 
request port I assume vm_map() takes ownership and I don't have to care



[1] 
https://gitlab.freedesktop.org/jlledom/libpciaccess/-/blob/hurd-device-map/src/x86_pci.c#L244

Re: [PATCH] new interface: memory_object_get_proxy

[Joan Lledó]

Hi,

I tested it and everything works fine. I'm attaching the two patches for 
convenience.

About the name, feel free to change it if you want, although I think if we 
rename it then the function should be moved to vm_map.c, maybe just below 
vm_region()

[PATCH 1/2] new interface: memory_object_get_proxy

[Joan Lledó]

From: Joan Lledó 

To get a proxy to the region a given address belongs to,
with protection and range limited to the region ones.

* include/mach/mach4.defs: memory_object_get_proxy RPC declaration
* vm/memory_object_proxy.c: memory_object_get_proxy implementation
---
 include/mach/mach4.defs  | 10 
 vm/memory_object_proxy.c | 50 
 2 files changed, 60 insertions(+)

diff --git a/include/mach/mach4.defs b/include/mach/mach4.defs
index 98af5905..e1641146 100644
--- a/include/mach/mach4.defs
+++ b/include/mach/mach4.defs
@@ -110,3 +110,13 @@ routine memory_object_create_proxy(
start   : vm_offset_array_t;
len : vm_offset_array_t;
out proxy   : mach_port_t);
+
+/* Gets a proxy to the region that ADDRESS belongs to, starting at the region
+   start, with MAX_PROTECTION and LEN limited by the region ones, and returns
+   it in *PORT.  */
+routine memory_object_get_proxy(
+   task: task_t;
+   address : vm_address_t;
+   max_protection  : vm_prot_t;
+   len : vm_offset_t;
+   out proxy   : mach_port_t);
diff --git a/vm/memory_object_proxy.c b/vm/memory_object_proxy.c
index b55a17f1..b6c73576 100644
--- a/vm/memory_object_proxy.c
+++ b/vm/memory_object_proxy.c
@@ -191,6 +191,56 @@ memory_object_create_proxy (const ipc_space_t space, 
vm_prot_t max_protection,
   return KERN_SUCCESS;
 }
 
+/* Gets a proxy to the region that ADDRESS belongs to, starting at the region
+   start, with MAX_PROTECTION and LEN limited by the region ones, and returns
+   it in *PORT.  */
+kern_return_t
+memory_object_get_proxy (task_t task, const vm_offset_t address,
+vm_prot_t max_protection, vm_offset_t len,
+ipc_port_t *port)
+{
+  kern_return_t ret;
+  vm_map_entry_t entry, tmp_entry;
+  vm_offset_t offset, start;
+  ipc_port_t pager;
+
+  if (task == TASK_NULL)
+return(KERN_INVALID_ARGUMENT);
+
+  vm_map_lock_read(task->map);
+  if (!vm_map_lookup_entry(task->map, address, &tmp_entry)) {
+if ((entry = tmp_entry->vme_next) == vm_map_to_entry(task->map)) {
+  vm_map_unlock_read(task->map);
+  return(KERN_NO_SPACE);
+}
+  } else {
+entry = tmp_entry;
+  }
+
+  /* Limit the allowed protection and range to the entry ones */
+  if (len > entry->vme_end - entry->vme_start) {
+vm_map_unlock_read(task->map);
+return(KERN_INVALID_ARGUMENT);
+  }
+
+  max_protection &= entry->max_protection;
+  pager = ipc_port_copy_send(entry->object.vm_object->pager);
+  offset = entry->offset;
+  start = 0;
+
+  vm_map_unlock_read(task->map);
+
+  ret = memory_object_create_proxy(task->itk_space, max_protection,
+   &pager, 1,
+   &offset, 1,
+   &start, 1,
+   &len, 1, port);
+  if (ret)
+ipc_port_release_send(pager);
+
+  return ret;
+}
+
 /* Lookup the real memory object and maximum protection for the proxy
memory object port PORT, for which the caller holds a reference.
*OBJECT is only guaranteed to be valid as long as the caller holds
-- 
2.31.1

[PATCH 2/2] Memory proxies: Add support for anonymous mappings

[Joan Lledó]

From: Sergey Bugaev 

* vm/memory_object_proxy.c:
  * memory_object_get_proxy():
* Return KERN_INVALID_ARGUMENT when the entry is a submap.
* Create a pager for the vm_object when the entry doesn't
  have any yet, since it's an anonymous mapping.
---
 vm/memory_object_proxy.c | 21 +
 1 file changed, 17 insertions(+), 4 deletions(-)

diff --git a/vm/memory_object_proxy.c b/vm/memory_object_proxy.c
index b6c73576..13a124ba 100644
--- a/vm/memory_object_proxy.c
+++ b/vm/memory_object_proxy.c
@@ -201,6 +201,7 @@ memory_object_get_proxy (task_t task, const vm_offset_t 
address,
 {
   kern_return_t ret;
   vm_map_entry_t entry, tmp_entry;
+  vm_object_t object;
   vm_offset_t offset, start;
   ipc_port_t pager;
 
@@ -217,16 +218,28 @@ memory_object_get_proxy (task_t task, const vm_offset_t 
address,
 entry = tmp_entry;
   }
 
+  if (entry->is_sub_map) {
+vm_map_unlock_read(task->map);
+return(KERN_INVALID_ARGUMENT);
+  }
+
   /* Limit the allowed protection and range to the entry ones */
   if (len > entry->vme_end - entry->vme_start) {
 vm_map_unlock_read(task->map);
 return(KERN_INVALID_ARGUMENT);
   }
-
   max_protection &= entry->max_protection;
-  pager = ipc_port_copy_send(entry->object.vm_object->pager);
-  offset = entry->offset;
-  start = 0;
+
+  object = entry->object.vm_object;
+  vm_object_lock(object);
+  /* Create a pager in case this is an internal object that does
+ not yet have one. */
+  vm_object_pager_create(object);
+  pager = ipc_port_copy_send(object->pager);
+  vm_object_unlock(object);
+
+  start = (address - entry->vme_start) + entry->offset;
+  offset = 0;
 
   vm_map_unlock_read(task->map);
 
-- 
2.31.1

Re: [VULN 0/4] Hurd vulnerability details

[Joan Lledó]

Hi,

El 2/11/21 a les 17:35, Samuel Thibault ha escrit:

Hello,

Thanks a lot for this writing! That'll surely be an interesting read for
whoever wants to look a bit at the details of how the Hurd works. And of
course thanks for finding and fixing the vulnerabilities :)



Yes, I'm gonna read it carefully. Thanks Sergey!

Re: [PATCH] new interface: memory_object_get_proxy

[Joan Lledó]

Hi,

El 1/11/21 a les 17:47, Sergey Bugaev ha escrit:

With this diff (on top of your patch), it finally works sanely for me:


Cool, great work. I'd like to try it myself but I won't have the time 
until next weekend. I'll merge your changes with mine and make my tests.

Re: [PATCH] new interface: memory_object_get_proxy

[Joan Lledó]

Hi,

El 1/11/21 a les 14:36, Sergey Bugaev ha escrit:

* "Anonymous" mappings (created with a null memory object)


If they provide no object when calling vm_map, then the kernel uses the 
default pager, isn't it? Can't you send a reference to the default pager 
to memory_object_create_proxy()?


The RPC is to return a proxy, and proxies always need an original object 
to proxy, if there's no object, I'm not sure how this RPC should behave

[PATCH] new interface: memory_object_get_proxy

[Joan Lledó]

From: Joan Lledó 

To get a proxy to the region a given address belongs to,
with protection and range limited to the region ones.

* include/mach/mach4.defs: memory_object_get_proxy RPC declaration
* vm/memory_object_proxy.c: memory_object_get_proxy implementation
---
 include/mach/mach4.defs  | 10 
 vm/memory_object_proxy.c | 50 
 2 files changed, 60 insertions(+)

diff --git a/include/mach/mach4.defs b/include/mach/mach4.defs
index 98af5905..e1641146 100644
--- a/include/mach/mach4.defs
+++ b/include/mach/mach4.defs
@@ -110,3 +110,13 @@ routine memory_object_create_proxy(
start   : vm_offset_array_t;
len : vm_offset_array_t;
out proxy   : mach_port_t);
+
+/* Gets a proxy to the region that ADDRESS belongs to, starting at the region
+   start, with MAX_PROTECTION and LEN limited by the region ones, and returns
+   it in *PORT.  */
+routine memory_object_get_proxy(
+   task: task_t;
+   address : vm_address_t;
+   max_protection  : vm_prot_t;
+   len : vm_offset_t;
+   out proxy   : mach_port_t);
diff --git a/vm/memory_object_proxy.c b/vm/memory_object_proxy.c
index b55a17f1..b6c73576 100644
--- a/vm/memory_object_proxy.c
+++ b/vm/memory_object_proxy.c
@@ -191,6 +191,56 @@ memory_object_create_proxy (const ipc_space_t space, 
vm_prot_t max_protection,
   return KERN_SUCCESS;
 }
 
+/* Gets a proxy to the region that ADDRESS belongs to, starting at the region
+   start, with MAX_PROTECTION and LEN limited by the region ones, and returns
+   it in *PORT.  */
+kern_return_t
+memory_object_get_proxy (task_t task, const vm_offset_t address,
+vm_prot_t max_protection, vm_offset_t len,
+ipc_port_t *port)
+{
+  kern_return_t ret;
+  vm_map_entry_t entry, tmp_entry;
+  vm_offset_t offset, start;
+  ipc_port_t pager;
+
+  if (task == TASK_NULL)
+return(KERN_INVALID_ARGUMENT);
+
+  vm_map_lock_read(task->map);
+  if (!vm_map_lookup_entry(task->map, address, &tmp_entry)) {
+if ((entry = tmp_entry->vme_next) == vm_map_to_entry(task->map)) {
+  vm_map_unlock_read(task->map);
+  return(KERN_NO_SPACE);
+}
+  } else {
+entry = tmp_entry;
+  }
+
+  /* Limit the allowed protection and range to the entry ones */
+  if (len > entry->vme_end - entry->vme_start) {
+vm_map_unlock_read(task->map);
+return(KERN_INVALID_ARGUMENT);
+  }
+
+  max_protection &= entry->max_protection;
+  pager = ipc_port_copy_send(entry->object.vm_object->pager);
+  offset = entry->offset;
+  start = 0;
+
+  vm_map_unlock_read(task->map);
+
+  ret = memory_object_create_proxy(task->itk_space, max_protection,
+   &pager, 1,
+   &offset, 1,
+   &start, 1,
+   &len, 1, port);
+  if (ret)
+ipc_port_release_send(pager);
+
+  return ret;
+}
+
 /* Lookup the real memory object and maximum protection for the proxy
memory object port PORT, for which the caller holds a reference.
*OBJECT is only guaranteed to be valid as long as the caller holds
-- 
2.31.1

Re: [PATCH] new interface: memory_object_get_proxy

[Joan Lledó]

Here you go

Re: [PATCH] new interface: memory_object_get_proxy

[Joan Lledó]

Hi,

El 30/10/21 a les 14:06, Sergey Bugaev ha escrit:
> I hope this makes sense; I'd be happy to expand if not.

Thanks for your explanations, it makes sense but there's still one thing I 
don't understand: if memory_object_create_proxy() is the owner of the pager it 
receives as parameter, and the caller doesn't care about releasing it, then 
where is it released?

I modified my patch to met all your points, I assumed the function to release 
the reference is ipc_port_release_send(). Tell me if I'm wrong.

Thanks

[PATCH] new interface: memory_object_get_proxy

[Joan Lledó]

From: Joan Lledó 

To get a proxy to the region a given address belongs to,
with protection and range limited to the region ones.

* include/mach/mach4.defs: memory_object_get_proxy RPC declaration
* vm/memory_object_proxy.c: memory_object_get_proxy implementation
---
 include/mach/mach4.defs  | 10 +
 vm/memory_object_proxy.c | 48 
 2 files changed, 58 insertions(+)

diff --git a/include/mach/mach4.defs b/include/mach/mach4.defs
index 98af5905..e1641146 100644
--- a/include/mach/mach4.defs
+++ b/include/mach/mach4.defs
@@ -110,3 +110,13 @@ routine memory_object_create_proxy(
start   : vm_offset_array_t;
len : vm_offset_array_t;
out proxy   : mach_port_t);
+
+/* Gets a proxy to the region that ADDRESS belongs to, starting at the region
+   start, with MAX_PROTECTION and LEN limited by the region ones, and returns
+   it in *PORT.  */
+routine memory_object_get_proxy(
+   task: task_t;
+   address : vm_address_t;
+   max_protection  : vm_prot_t;
+   len : vm_offset_t;
+   out proxy   : mach_port_t);
diff --git a/vm/memory_object_proxy.c b/vm/memory_object_proxy.c
index b55a17f1..984a247b 100644
--- a/vm/memory_object_proxy.c
+++ b/vm/memory_object_proxy.c
@@ -191,6 +191,54 @@ memory_object_create_proxy (const ipc_space_t space, 
vm_prot_t max_protection,
   return KERN_SUCCESS;
 }
 
+/* Gets a proxy to the region that ADDRESS belongs to, starting at the region
+   start, with MAX_PROTECTION and LEN limited by the region ones, and returns
+   it in *PORT.  */
+kern_return_t
+memory_object_get_proxy (task_t task, const vm_offset_t address,
+vm_prot_t max_protection, vm_offset_t len,
+ipc_port_t *port)
+{
+  kern_return_t err;
+  vm_map_entry_t entry, tmp_entry;
+  vm_offset_t offset, start;
+  ipc_port_t pager;
+
+  if (task == TASK_NULL)
+return(KERN_INVALID_ARGUMENT);
+
+  vm_map_lock_read(task->map);
+  if (!vm_map_lookup_entry(task->map, address, &tmp_entry)) {
+if ((entry = tmp_entry->vme_next) == vm_map_to_entry(task->map)) {
+  vm_map_unlock_read(task->map);
+  return(KERN_NO_SPACE);
+}
+  } else {
+entry = tmp_entry;
+  }
+
+  /* Limit the allowed protection and range to the entry ones */
+  if (len > entry->vme_end - entry->vme_start)
+return(KERN_INVALID_ARGUMENT);
+
+  max_protection &= entry->max_protection;
+  pager = ipc_port_copy_send(entry->object.vm_object->pager);
+  offset = entry->offset;
+  start = 0;
+
+  vm_map_unlock_read(task->map);
+
+  err = memory_object_create_proxy(task->itk_space, max_protection,
+   &pager, 1,
+   &offset, 1,
+   &start, 1,
+   &len, 1, port);
+  if (err)
+ipc_port_release_send(pager);
+
+  return err;
+}
+
 /* Lookup the real memory object and maximum protection for the proxy
memory object port PORT, for which the caller holds a reference.
*OBJECT is only guaranteed to be valid as long as the caller holds
-- 
2.31.1

Re: [PATCH] new interface: memory_object_get_proxy

[Joan Lledó]

Hi,

El 24/10/21 a les 19:50, Sergey Bugaev ha escrit:

Naming: perhaps memory_object_create_vm_proxy ()? or even
memory_object_create_task_vm_proxy ()?


I don't care about the name, you guys decide.


I would expect the request port argument to be a vm_task_t (i.e. a
vm_map), not a full task. But I see that you need to pass
task->itk_space to memory_object_create_proxy (). But
memory_object_create_proxy () doesn't actually need the task either,
it just checks it against NULL (as usual) and never uses it again. So
maybe it'd be cleaner to make both calls accept a vm_task_t? Not that
this matters.


yes, that's true, but if we remove the parameter from 
memory_object_create_proxy() then we must remove it from all calls in 
the code, and in the documentation if there's any. So that's beyond the 
scope of this patch, it's something we can do later on.




I don't think you can access the entry once you've unlocked the map.



You're probably right b/c it doesn't seem the entry is being accessed 
after the lock release in any other place of the code.



Should the implementation cap the length to that of the entry
silently, or should it return an error if called with an overly long
len argument?



I don't know, Samuel, what do you think?



I'm... not sure if this is wrong, but just to bring some attention to
it: isn't memory_object_create_proxy () supposed to *consume* the
'objects' ports array in the successful case? I see it uses
ipc_port_copy_send (object[0]), why does that not cause a leak? For a
reference point, mach_ports_register (), which is another kernel RPC
that accepts a port array, sure does consume it.

If it *is* the case that memory_object_create_proxy () should consume
the ports, then your new routine should make an extra ref to the pager
port before passing it to memory_object_create_proxy ().


I'm not familiar with the concept of consuming objects, could you 
explain it to me?

[PATCH] new interface: memory_object_get_proxy

[Joan Lledó]

From: Joan Lledó 

To get a proxy to the region a given address belongs to,
with protection and range limited to the region ones.

* include/mach/mach4.defs: memory_object_get_proxy RPC declaration
* vm/memory_object_proxy.c: memory_object_get_proxy implementation
---
 include/mach/mach4.defs  | 10 ++
 vm/memory_object_proxy.c | 39 +++
 2 files changed, 49 insertions(+)

diff --git a/include/mach/mach4.defs b/include/mach/mach4.defs
index 98af5905..e1641146 100644
--- a/include/mach/mach4.defs
+++ b/include/mach/mach4.defs
@@ -110,3 +110,13 @@ routine memory_object_create_proxy(
start   : vm_offset_array_t;
len : vm_offset_array_t;
out proxy   : mach_port_t);
+
+/* Gets a proxy to the region that ADDRESS belongs to, starting at the region
+   start, with MAX_PROTECTION and LEN limited by the region ones, and returns
+   it in *PORT.  */
+routine memory_object_get_proxy(
+   task: task_t;
+   address : vm_address_t;
+   max_protection  : vm_prot_t;
+   len : vm_offset_t;
+   out proxy   : mach_port_t);
diff --git a/vm/memory_object_proxy.c b/vm/memory_object_proxy.c
index b55a17f1..5b13e749 100644
--- a/vm/memory_object_proxy.c
+++ b/vm/memory_object_proxy.c
@@ -191,6 +191,45 @@ memory_object_create_proxy (const ipc_space_t space, 
vm_prot_t max_protection,
   return KERN_SUCCESS;
 }
 
+/* Gets a proxy to the region that ADDRESS belongs to, starting at the region
+   start, with MAX_PROTECTION and LEN limited by the region ones, and returns
+   it in *PORT.  */
+kern_return_t
+memory_object_get_proxy (task_t task, const vm_offset_t address,
+vm_prot_t max_protection, vm_offset_t len,
+ipc_port_t *port)
+{
+  vm_map_entry_t entry, tmp_entry;
+  vm_offset_t offset, start;
+
+  if (task == TASK_NULL)
+return(KERN_INVALID_ARGUMENT);
+
+  vm_map_lock_read(task->map);
+  if (!vm_map_lookup_entry(task->map, address, &tmp_entry)) {
+if ((entry = tmp_entry->vme_next) == vm_map_to_entry(task->map)) {
+  vm_map_unlock_read(task->map);
+  return(KERN_NO_SPACE);
+}
+  } else {
+entry = tmp_entry;
+  }
+  vm_map_unlock_read(task->map);
+
+  /* Limit the allowed protection and range to the entry ones */
+  max_protection &= entry->max_protection;
+  if (len > entry->vme_end - entry->vme_start)
+len = entry->vme_end - entry->vme_start;
+  offset = entry->offset;
+  start = 0;
+
+  return memory_object_create_proxy(task->itk_space, max_protection,
+   &entry->object.vm_object->pager, 1,
+   &offset, 1,
+   &start, 1,
+   &len, 1, port);
+}
+
 /* Lookup the real memory object and maximum protection for the proxy
memory object port PORT, for which the caller holds a reference.
*OBJECT is only guaranteed to be valid as long as the caller holds
-- 
2.31.1

Re: gnumach RPC: get info about the calling task

[Joan Lledó]

Hi,

I wrote a patch with the RPC as you guys asked. Please tell me if it fits your 
plans for mremap()

Re: gnumach RPC: get info about the calling task

[Joan Lledó]

El 17/10/21 a les 18:32, Samuel Thibault ha escrit> It will be useful to 
implement mremap with the same call.

In your case you know the size, don't you?



Yes



That's almost the same, isn't it? (there is just the max_prot parameter,
which you can indeed add to the RPC above).



It's similar but I think the RPC should belong to the 
memory_object_proxy_* family intead of vm_* and be implemented in 
memory_object_proxy.c, so all logic related to proxies is in that file.

Re: gnumach RPC: get info about the calling task

[Joan Lledó]

Hi,

El 16/10/21 a les 13:27, Sergey Bugaev ha escrit:


routine vm_make_proxy (
 target_task : vm_task_t;
 address : vm_address_t;
 size : vm_size_t;
 out proxy : memory_object_t);



Why the "size" parameter? I'd rather see a new wrapper for 
memory_object_create_proxy() which receives the same params but with the 
address instead of the original pager, which internally gets the pager 
from the address and calls memory_object_create_proxy(). After all, the 
only reason why I need the pager is to send it to 
memory_object_create_proxy() at netfs_impl.c:617 [1]. So why not skip 
one step?


-
[1] 
http://git.savannah.gnu.org/cgit/hurd/hurd.git/tree/pci-arbiter/netfs_impl.c?h=jlledom-pci-memory-map#n616

Re: gnumach RPC: get info about the calling task

[Joan Lledó]

OK, I'll try with your design

El 16/10/21 a les 18:06, Sergey Bugaev ha escrit:

On Sat, Oct 16, 2021 at 6:54 PM Samuel Thibault  wrote:

Indeed, since it's the region that remembers which protection was
allowed, we need a proxy there to enforce them.


Right, that's also a good point. max_proection can be less than 7 even
if there never was a proxy. That is, one can do

vm_map (some_other_task, memobj, VM_PROT_READ, VM_PROT_READ);

and expect some_other_task to never get write access to the object.

Sergey

[PATCH] new interface: vm_pager

[Joan Lledó]

From: Joan Lledó 

Given a task and an address, it returns the pager used to map that
address in that task.

* include/mach/mach4.defs: new interface declaration: vm_pager
* vm/memory_object_proxy.h: add declaration for proxy hash functions
* vm/memory_object_proxy.c: implement proxy hash functions
* To track the proxies so they can be looked up when needed
* vm/vm_map.c: implementation for the vm_pager RPC
* vm/vm_object.c: implement vm_object_pager
* new util function to get the pager from a vm_object
* vm/vm_object.h: declare vm_object_pager
* vm/vm_user.c: update functions to work with the proxies hash
* vm_map: inserts the given proxy in the hash when mapping
* vm_deallocate: removes the proxy from the hash when unmapping
---
 include/mach/mach4.defs  |   6 ++
 vm/memory_object_proxy.c | 120 +++
 vm/memory_object_proxy.h |   6 ++
 vm/vm_map.c  |  46 +++
 vm/vm_object.c   |  31 ++
 vm/vm_object.h   |   1 +
 vm/vm_user.c |  13 -
 7 files changed, 222 insertions(+), 1 deletion(-)

diff --git a/include/mach/mach4.defs b/include/mach/mach4.defs
index 98af5905..195d6292 100644
--- a/include/mach/mach4.defs
+++ b/include/mach/mach4.defs
@@ -110,3 +110,9 @@ routine memory_object_create_proxy(
start   : vm_offset_array_t;
len : vm_offset_array_t;
out proxy   : mach_port_t);
+
+/* Get the pager where the given address is mapped to  */
+routine vm_pager(
+   target_task : vm_task_t;
+   address : vm_address_t;
+   out pager   : mach_port_t);
diff --git a/vm/memory_object_proxy.c b/vm/memory_object_proxy.c
index b55a17f1..fa09aa47 100644
--- a/vm/memory_object_proxy.c
+++ b/vm/memory_object_proxy.c
@@ -41,6 +41,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -62,12 +63,131 @@ struct memory_object_proxy
 };
 typedef struct memory_object_proxy *memory_object_proxy_t;
 
+/*
+ * A hash table of ports for struct memory_object_proxy backed objects.
+ */
+
+#defineMEMORY_OBJECT_PROXY_HASH_COUNT  127
+
+struct memory_object_proxy_entry {
+   queue_chain_t   links;
+   vm_map_tmap;
+   vm_offset_t address;
+   ipc_port_t  proxy;
+};
+typedef struct memory_object_proxy_entry *memory_object_proxy_entry_t;
+
+/*
+ * Indexed by port name, each element contains a queue of all
+ * memory_object_proxy_entry_t which name shares the same hash
+ */
+queue_head_t   memory_object_proxy_hashtable[MEMORY_OBJECT_PROXY_HASH_COUNT];
+struct kmem_cache  memory_object_proxy_hash_cache;
+decl_simple_lock_data(,
+   memory_object_proxy_hash_lock)
+
+#definememory_object_proxy_hash(name_port) \
+   (((vm_offset_t)(name_port) & 0xff) % MEMORY_OBJECT_PROXY_HASH_COUNT)
+
+static
+void memory_object_proxy_hash_init(void)
+{
+   int i;
+   vm_size_t   size;
+
+   size = sizeof(struct memory_object_proxy_entry);
+   kmem_cache_init(&memory_object_proxy_hash_cache,
+   "memory_object_proxy_entry", size, 0, NULL, 0);
+   for (i = 0; i < MEMORY_OBJECT_PROXY_HASH_COUNT; i++)
+   queue_init(&memory_object_proxy_hashtable[i]);
+   simple_lock_init(&memory_object_proxy_hash_lock);
+}
+
+void memory_object_proxy_hash_insert(
+   const vm_map_t  map,
+   const vm_offset_t   address,
+   const ipc_port_tproxy)
+{
+   memory_object_proxy_entry_t new_entry;
+
+   new_entry = (memory_object_proxy_entry_t) kmem_cache_alloc(
+   &memory_object_proxy_hash_cache
+   );
+   new_entry->map = map;
+   new_entry->address = address;
+   new_entry->proxy = proxy;
+
+   simple_lock(&memory_object_proxy_hash_lock);
+   queue_enter(&memory_object_proxy_hashtable[
+   memory_object_proxy_hash(map + address)
+   ], new_entry, memory_object_proxy_entry_t, links);
+   simple_unlock(&memory_object_proxy_hash_lock);
+}
+
+void memory_object_proxy_hash_delete(
+   const vm_map_t  map,
+   const vm_offset_t   address)
+{
+   queue_t bucket;
+   memory_object_proxy_entry_t tmp_entry;
+   memory_object_proxy_entry_t entry = NULL;
+
+   bucket = &memory_object_proxy_hashtable[
+   memory_object_proxy_hash(map + address)
+   ];
+
+   simple_lock(&memory_object_proxy_hash_lock);
+   for (tmp_entry = (memory_object_proxy_entry_t)queue_first(bucket);
+!queue_end(bucket, &tmp_entry->links);
+tmp_entry =
+   (memory_object_proxy_entry_t)queue_next(&tmp_entry->links)) {
+   if (tmp_entry->map == map && tmp_entry->ad

Re: gnumach RPC: get info about the calling task

[Joan Lledó]

El 12/10/21 a les 20:32, Samuel Thibault ha escrit:
> Sergey Bugaev, le mar. 12 oct. 2021 16:22:48 +0300, a ecrit:
>> So in the case of vm_map-backed pager, it should matter whether you
>> have a task port to the target task, not whether you *are* the target
>> task. If someone has a task port to a task, it allows them to
>> completely control the task anyway (including making it invoke any
>> RPC); there's no additional security gains from checking who the
>> caller is, but there will be additional breakage.
> 
> Yes, making the caller pass the task port should be completely enough.

Thanks for your explanations, if that's the case then I basically have it 
already. I attached a patch with the changes.

The new interface needs to know about proxies, and if one range has been mapped 
using a proxy, it must return the proxy and not the original object, which 
could be used to bypass the proxy protection, that's why I needed a way to 
lookup for used proxies from a task and an address.

I implemented that using a hash, like I did in the my previous dev_pager patch, 
that seems ok and it works, but the only way I found to remove proxies from the 
hash once they are not needed is to lookup for a proxy every time vm_deallocate 
is called. I don't like that, since proxies are rarely used so rarely (if ever) 
a range being unmaped will be present in the hash.

Ideas?

gnumach RPC: get info about the calling task

[Joan Lledó]

Hi,

I'm working on the gnumach rpc to return a pager from a task and an 
address mapped in that task.


For security reasons I'd like to check if the calling task is the same 
as the one the given map belongs to. But I don't know how to do it.


In the rpc implementation, the function receives a vm_map_t parameter. 
How can I get the task from the vm_map_t? and how can I compare it with 
the calling task to check whether it's the same?

Re: Wrong MIG generated headers

[Joan Lledó]

Hi,

El 11/9/21 a les 13:39, Sergey Bugaev ha escrit:

This is a little change that Samuel has committed recently


Thanks a lot, I was spending a lot of time on this. Should have asked 
before.

Wrong MIG generated headers

[Joan Lledó]

Hi,

I'm having a weird issue trying to add a new interface to gnumach. I'm 
using this mach4.defs:


--

/*
 * Mach Operating System
 * Copyright (c) 1994,1993,1992 Carnegie Mellon University
 * All Rights Reserved.
 *
 * Permission to use, copy, modify and distribute this software and its
 * documentation is hereby granted, provided that both the copyright
 * notice and this permission notice appear in all copies of the
 * software, derivative works or modified versions, and any portions
 * thereof, and that both notices appear in supporting documentation.
 *
 * CARNEGIE MELLON ALLOWS FREE USE OF THIS SOFTWARE IN ITS "AS IS"
 * CONDITION.  CARNEGIE MELLON DISCLAIMS ANY LIABILITY OF ANY KIND FOR
 * ANY DAMAGES WHATSOEVER RESULTING FROM THE USE OF THIS SOFTWARE.
 *
 * Carnegie Mellon requests users of this software to return to
 *
 *  Software Distribution Coordinator  or  software.distribut...@cs.cmu.edu
 *  School of Computer Science
 *  Carnegie Mellon University
 *  Pittsburgh PA 15213-3890
 *
 * any improvements or extensions that they make and grant Carnegie Mellon
 * the rights to redistribute these changes.
 */
/*
 *  Matchmaker definitions file for Mach4 kernel interface.
 */

subsystem
#if KERNEL_SERVER
  KernelServer
#endif  /* KERNEL_SERVER */
#if KERNEL_USER
  KernelUser
#endif  /* KERNEL_USER */
   mach4 4000;

#include 
#include 


#ifdef MACH_PCSAMPLE
type sampled_pc_t   = struct[3]of natural_t;
type sampled_pc_array_t = array[*:512] of sampled_pc_t;
type sampled_pc_seqno_t = unsigned;
type sampled_pc_flavor_t = natural_t;

routine task_enable_pc_sampling(
host  : task_t;
out tick  : int; /* sample frequency in usecs   */
flavor: sampled_pc_flavor_t );

routine task_disable_pc_sampling(
host  : task_t;
out samplecnt : int);

routine task_get_sampled_pcs(
host: task_t;
inout seqno : sampled_pc_seqno_t;
out sampled_pcs : sampled_pc_array_t);

routine thread_enable_pc_sampling(
host  : thread_t;
out tick  : int; /* sample frequency in usecs*/
flavor: sampled_pc_flavor_t );

routine thread_disable_pc_sampling(
host  : thread_t;
out samplecnt : int);

routine thread_get_sampled_pcs(
host: thread_t;
inout seqno : sampled_pc_seqno_t;
out sampled_pcs : sampled_pc_array_t);


skip/* pc_sampling reserved 1*/;
skip/* pc_sampling reserved 2*/;
skip/* pc_sampling reserved 3*/;
skip/* pc_sampling reserved 4*/;

#else

skip;   /* task_enable_pc_sampling */
skip;   /* task_disable_pc_sampling */
skip;   /* task_get_sampled_pcs */
skip;   /* thread_enable_pc_sampling */
skip;   /* thread_disable_pc_sampling */
skip;   /* thread_get_sampled_pcs */

skip/* pc_sampling reserved 1*/;
skip/* pc_sampling reserved 2*/;
skip/* pc_sampling reserved 3*/;
skip/* pc_sampling reserved 4*/;

#endif


/* Create a new proxy memory object from [START;START+LEN) in the
   given memory object OBJECT at OFFSET in the new object with the maximum
   protection MAX_PROTECTION and return it in *PORT.  */
type vm_offset_array_t = array[*:1024] of vm_offset_t;
routine memory_object_create_proxy(
task: ipc_space_t;
max_protection  : vm_prot_t;
object  : memory_object_array_t =
  array[*:1024] of mach_port_send_t;
offset  : vm_offset_array_t;
start   : vm_offset_array_t;
len : vm_offset_array_t;
out proxy   : mach_port_t);

/* Get the pager where the given address is mapped to  */
routine vm_pager(
task: ipc_space_t;
address : vm_address_t;
out pager   : mach_port_t);


--

Which is the same but with my new interface at the end. But when I 
compile glibc to get my new lubmachuser, I find it has generated this 
mach4.h:


-

#ifndef _mach4_user_
#define _mach4_user_

/* Module mach4 */

#include 
#include 
#include 

#include 
#include 

/* Routine task_enable_pc_sampling */
#ifdef  mig_external
mig_external
#else
extern
#endif
kern_return_t __task_enable_pc_sampling
(
mach_port_t host,
int *tick,
sampled_pc_flavor_t flavor
);

/* Routine task_disable_pc_sampling */
#ifdef  mig_external
mig_external
#else
extern
#endif
kern_return_t __task_disable_pc_sampling
(
mach_port_t host,
int *samplecnt
);

/* Routine task_get_sampled_pcs */

[PATCH 2/2] dev_pager: rename hash macros

[Joan Lledó]

From: Joan Lledó 

Remove the reference to the pager hash since they are used
both in the pager and the device hashes.

* device/dev_pager.c:
* Rename DEV_PAGER_HASH_COUNT to DEV_HASH_COUNT
* Rename dev_pager_hash to dev_hash
---
 device/dev_pager.c | 26 +-
 1 file changed, 13 insertions(+), 13 deletions(-)

diff --git a/device/dev_pager.c b/device/dev_pager.c
index 6fd2d69a..2ec48d0f 100644
--- a/device/dev_pager.c
+++ b/device/dev_pager.c
@@ -151,7 +151,7 @@ void dev_pager_deallocate(dev_pager_t   ds)
  * A hash table of ports for device_pager backed objects.
  */
 
-#defineDEV_PAGER_HASH_COUNT127
+#defineDEV_HASH_COUNT  127
 
 struct dev_pager_entry {
queue_chain_t   links;
@@ -164,7 +164,7 @@ typedef struct dev_pager_entry *dev_pager_entry_t;
  * Indexed by port name, each element contains a queue of all dev_pager_entry_t
  * which name shares the same hash
  */
-queue_head_t   dev_pager_hashtable[DEV_PAGER_HASH_COUNT];
+queue_head_t   dev_pager_hashtable[DEV_HASH_COUNT];
 struct kmem_cache  dev_pager_hash_cache;
 decl_simple_lock_data(,
dev_pager_hash_lock)
@@ -181,13 +181,13 @@ typedef struct dev_device_entry *dev_device_entry_t;
  * Indexed by device + offset, each element contains a queue of all
  * dev_device_entry_t which device + offset shares the same hash
  */
-queue_head_t   dev_device_hashtable[DEV_PAGER_HASH_COUNT];
+queue_head_t   dev_device_hashtable[DEV_HASH_COUNT];
 struct kmem_cache  dev_device_hash_cache;
 decl_simple_lock_data(,
dev_device_hash_lock)
 
-#definedev_pager_hash(name_port) \
-   (((vm_offset_t)(name_port) & 0xff) % DEV_PAGER_HASH_COUNT)
+#definedev_hash(name_port) \
+   (((vm_offset_t)(name_port) & 0xff) % DEV_HASH_COUNT)
 
 void dev_pager_hash_init(void)
 {
@@ -197,7 +197,7 @@ void dev_pager_hash_init(void)
size = sizeof(struct dev_pager_entry);
kmem_cache_init(&dev_pager_hash_cache, "dev_pager_entry", size, 0,
NULL, 0);
-   for (i = 0; i < DEV_PAGER_HASH_COUNT; i++)
+   for (i = 0; i < DEV_HASH_COUNT; i++)
queue_init(&dev_pager_hashtable[i]);
simple_lock_init(&dev_pager_hash_lock);
 }
@@ -213,7 +213,7 @@ void dev_pager_hash_insert(
new_entry->pager_rec = rec;
 
simple_lock(&dev_pager_hash_lock);
-   queue_enter(&dev_pager_hashtable[dev_pager_hash(name_port)],
+   queue_enter(&dev_pager_hashtable[dev_hash(name_port)],
new_entry, dev_pager_entry_t, links);
simple_unlock(&dev_pager_hash_lock);
 }
@@ -223,7 +223,7 @@ void dev_pager_hash_delete(const ipc_port_t name_port)
queue_t bucket;
dev_pager_entry_t   entry;
 
-   bucket = &dev_pager_hashtable[dev_pager_hash(name_port)];
+   bucket = &dev_pager_hashtable[dev_hash(name_port)];
 
simple_lock(&dev_pager_hash_lock);
for (entry = (dev_pager_entry_t)queue_first(bucket);
@@ -245,7 +245,7 @@ dev_pager_t dev_pager_hash_lookup(const ipc_port_t 
name_port)
dev_pager_entry_t   entry;
dev_pager_t pager;
 
-   bucket = &dev_pager_hashtable[dev_pager_hash(name_port)];
+   bucket = &dev_pager_hashtable[dev_hash(name_port)];
 
simple_lock(&dev_pager_hash_lock);
for (entry = (dev_pager_entry_t)queue_first(bucket);
@@ -270,7 +270,7 @@ void dev_device_hash_init(void)
size = sizeof(struct dev_device_entry);
kmem_cache_init(&dev_device_hash_cache, "dev_device_entry", size, 0,
NULL, 0);
-   for (i = 0; i < DEV_PAGER_HASH_COUNT; i++) {
+   for (i = 0; i < DEV_HASH_COUNT; i++) {
queue_init(&dev_device_hashtable[i]);
}
simple_lock_init(&dev_device_hash_lock);
@@ -289,7 +289,7 @@ void dev_device_hash_insert(
new_entry->pager_rec = rec;
 
simple_lock(&dev_device_hash_lock);
-   queue_enter(&dev_device_hashtable[dev_pager_hash(device + offset)],
+   queue_enter(&dev_device_hashtable[dev_hash(device + offset)],
new_entry, dev_device_entry_t, links);
simple_unlock(&dev_device_hash_lock);
 }
@@ -301,7 +301,7 @@ void dev_device_hash_delete(
queue_t bucket;
dev_device_entry_t  entry;
 
-   bucket = &dev_device_hashtable[dev_pager_hash(device + offset)];
+   bucket = &dev_device_hashtable[dev_hash(device + offset)];
 
simple_lock(&dev_device_hash_lock);
for (entry = (dev_device_entry_t)queue_first(bucket);
@@ -325,7 +325,7 @@ dev_pager_t dev_device_hash_lookup(
dev_device_entry_t  entry;
dev_pager_t pager;
 
-   bucket = &dev_device_hashtable[dev_pager_hash(device +

Re: [PATCH] - gnumach: Implement offset in device map

[Joan Lledó]

Hi,

> all elements that have the same hash key end up in the same list

Ok, I missed that and the rest of the patch is wrong b/c of this.

I did what you said and created a second hash table indexed by device + offset, 
leaving all code for dev_pager_hashtable untouched.

I tried to insert the same dev_pager_entry in both lists to avoid duplication, 
but the links field can't be shared between two lists, so I created a new 
struct dev_device_entry for the devices hash.

I also created the equivalent to dev_device_hash[init,insert,delete,lookup] 
functions for the devices hash, so I don't have to make changes in any of the 
pagers hash functions, but I could merge these functions for both hashes to 
avoid the duplication if you think it's better.

Finally I renamed the hash macros since now they are not exclusive for the 
pagers hash.

There's another thing I don't understand: One task can call device_map on a 
device and offset for the first time, then dev_pager setup will allocate the 
pager and insert it on the hash. Later, another task can call device_map on the 
same device and offset, then dev_pager_setup will find it in the hash and 
return a name for the same pager, so two tasks are sharing the pager, is not 
that a security problem?

> adding to gnu mach a function that returns a memory proxy for a given range 
> of memory, that would also make a lot of sense

If you think it's safe to return a pager belonging to the same task who 
requested it, then I'll do it.

[PATCH 1/2] dev_pager: implement offset

[Joan Lledó]

From: Joan Lledó 

* device/dev_pager.c:
* struct dev_pager: add offset field
* new struct dev_device_entry: includes device and offset
* new hash table dev_device_hashtable
* index [device + offset]
* new functions dev_device_hash[init,insert,delete,lookup]
* do the same as their counterparts for
  dev_pager_hashtable
* dev_pager_setup(): record the offset
* device_map_page(): add the recorded offset on the fly
---
 device/dev_pager.c | 115 +++--
 1 file changed, 110 insertions(+), 5 deletions(-)

diff --git a/device/dev_pager.c b/device/dev_pager.c
index 37ec69fd..6fd2d69a 100644
--- a/device/dev_pager.c
+++ b/device/dev_pager.c
@@ -114,6 +114,7 @@ struct dev_pager {
ipc_port_t  pager_request;  /* Known request port */
ipc_port_t  pager_name; /* Known name port */
mach_device_t   device; /* Device handle */
+   vm_offset_t offset; /* offset within the pager, in bytes*/
int type;   /* to distinguish */
 #define DEV_PAGER_TYPE 0
 #define CHAR_PAGER_TYPE1
@@ -159,11 +160,32 @@ struct dev_pager_entry {
 };
 typedef struct dev_pager_entry *dev_pager_entry_t;
 
+/*
+ * Indexed by port name, each element contains a queue of all dev_pager_entry_t
+ * which name shares the same hash
+ */
 queue_head_t   dev_pager_hashtable[DEV_PAGER_HASH_COUNT];
 struct kmem_cache  dev_pager_hash_cache;
 decl_simple_lock_data(,
dev_pager_hash_lock)
 
+struct dev_device_entry {
+   queue_chain_t   links;
+   mach_device_t   device;
+   vm_offset_t offset;
+   dev_pager_t pager_rec;
+};
+typedef struct dev_device_entry *dev_device_entry_t;
+
+/*
+ * Indexed by device + offset, each element contains a queue of all
+ * dev_device_entry_t which device + offset shares the same hash
+ */
+queue_head_t   dev_device_hashtable[DEV_PAGER_HASH_COUNT];
+struct kmem_cache  dev_device_hash_cache;
+decl_simple_lock_data(,
+   dev_device_hash_lock)
+
 #definedev_pager_hash(name_port) \
(((vm_offset_t)(name_port) & 0xff) % DEV_PAGER_HASH_COUNT)
 
@@ -240,7 +262,86 @@ dev_pager_t dev_pager_hash_lookup(const ipc_port_t 
name_port)
return (DEV_PAGER_NULL);
 }
 
-/* FIXME: This is not recording offset! */
+void dev_device_hash_init(void)
+{
+   int i;
+   vm_size_t   size;
+
+   size = sizeof(struct dev_device_entry);
+   kmem_cache_init(&dev_device_hash_cache, "dev_device_entry", size, 0,
+   NULL, 0);
+   for (i = 0; i < DEV_PAGER_HASH_COUNT; i++) {
+   queue_init(&dev_device_hashtable[i]);
+   }
+   simple_lock_init(&dev_device_hash_lock);
+}
+
+void dev_device_hash_insert(
+   const mach_device_t device,
+   const vm_offset_t   offset,
+   const dev_pager_t   rec)
+{
+   dev_device_entry_t new_entry;
+
+   new_entry = (dev_device_entry_t) 
kmem_cache_alloc(&dev_device_hash_cache);
+   new_entry->device = device;
+   new_entry->offset = offset;
+   new_entry->pager_rec = rec;
+
+   simple_lock(&dev_device_hash_lock);
+   queue_enter(&dev_device_hashtable[dev_pager_hash(device + offset)],
+   new_entry, dev_device_entry_t, links);
+   simple_unlock(&dev_device_hash_lock);
+}
+
+void dev_device_hash_delete(
+   const mach_device_t device,
+   const vm_offset_t   offset)
+{
+   queue_t bucket;
+   dev_device_entry_t  entry;
+
+   bucket = &dev_device_hashtable[dev_pager_hash(device + offset)];
+
+   simple_lock(&dev_device_hash_lock);
+   for (entry = (dev_device_entry_t)queue_first(bucket);
+!queue_end(bucket, &entry->links);
+entry = (dev_device_entry_t)queue_next(&entry->links)) {
+   if (entry->device == device && entry->offset == offset) {
+   queue_remove(bucket, entry, dev_device_entry_t, links);
+   break;
+   }
+   }
+   simple_unlock(&dev_device_hash_lock);
+   if (entry)
+   kmem_cache_free(&dev_device_hash_cache, (vm_offset_t)entry);
+}
+
+dev_pager_t dev_device_hash_lookup(
+   const mach_device_t device,
+   const vm_offset_t   offset)
+{
+   queue_t bucket;
+   dev_device_entry_t  entry;
+   dev_pager_t pager;
+
+   bucket = &dev_device_hashtable[dev_pager_hash(device + offset)];
+
+   simple_lock(&dev_device_hash_lock);
+   for (entry = (dev_device_entry_t)queue_first(bucket);
+!queue_end(bucket, &entry->links);
+entry = (dev_device_entry_t)queue_next(&entry->links)) {
+   if (entry->device 

[PATCH] dev_pager: Implement offset

[Joan Lledó]

From: Joan Lledó 

* New structure for the internal hash table:
* 127 slots identified by the device address,
  each one containing a queue of pagers belonging
  to that device.
* New auxiliary index [pager: device] to quick search
  of pagers.

* dev_pager.c:
* struct dev_pager: add offset field
* struct dev_pager_entry: add offset field
* struct dev_pager_hashtable:
  * is now an index [pager: device]
* new hash table dev_device_hashtable
  * index [device: queue of pagers]
* dev_pager_setup(): record the offset
* device_map_page(): add the recorded offset on the fly
---
 device/dev_pager.c | 102 +++--
 1 file changed, 70 insertions(+), 32 deletions(-)

diff --git a/device/dev_pager.c b/device/dev_pager.c
index 37ec69fd..fe4a560c 100644
--- a/device/dev_pager.c
+++ b/device/dev_pager.c
@@ -113,6 +113,7 @@ struct dev_pager {
ipc_port_t  pager;  /* pager port */
ipc_port_t  pager_request;  /* Known request port */
ipc_port_t  pager_name; /* Known name port */
+   vm_offset_t offset; /* offset within the pager, in bytes*/
mach_device_t   device; /* Device handle */
int type;   /* to distinguish */
 #define DEV_PAGER_TYPE 0
@@ -150,24 +151,26 @@ void dev_pager_deallocate(dev_pager_t ds)
  * A hash table of ports for device_pager backed objects.
  */
 
-#defineDEV_PAGER_HASH_COUNT127
+#defineDEV_HASH_COUNT  127
 
 struct dev_pager_entry {
queue_chain_t   links;
ipc_port_t  name;
+   vm_offset_t offset;
dev_pager_t pager_rec;
 };
 typedef struct dev_pager_entry *dev_pager_entry_t;
 
-queue_head_t   dev_pager_hashtable[DEV_PAGER_HASH_COUNT];
+queue_head_t   dev_device_hashtable[DEV_HASH_COUNT];
 struct kmem_cache  dev_pager_hash_cache;
+mach_device_t  dev_pager_hashtable[DEV_HASH_COUNT];
 decl_simple_lock_data(,
-   dev_pager_hash_lock)
+   dev_hash_lock)
 
-#definedev_pager_hash(name_port) \
-   (((vm_offset_t)(name_port) & 0xff) % DEV_PAGER_HASH_COUNT)
+#definedev_hash(key) \
+   (((vm_offset_t)(key) & 0xff) % DEV_HASH_COUNT)
 
-void dev_pager_hash_init(void)
+void dev_hash_init(void)
 {
int i;
vm_size_t   size;
@@ -175,72 +178,106 @@ void dev_pager_hash_init(void)
size = sizeof(struct dev_pager_entry);
kmem_cache_init(&dev_pager_hash_cache, "dev_pager_entry", size, 0,
NULL, 0);
-   for (i = 0; i < DEV_PAGER_HASH_COUNT; i++)
-   queue_init(&dev_pager_hashtable[i]);
-   simple_lock_init(&dev_pager_hash_lock);
+   for (i = 0; i < DEV_HASH_COUNT; i++) {
+   queue_init(&dev_device_hashtable[i]);
+   dev_pager_hashtable[i] = MACH_DEVICE_NULL;
+   }
+   simple_lock_init(&dev_hash_lock);
 }
 
-void dev_pager_hash_insert(
-   const ipc_port_tname_port,
+void dev_hash_insert(
+   const mach_device_t device,
+   const vm_offset_t   offset,
const dev_pager_t   rec)
 {
dev_pager_entry_t new_entry;
 
new_entry = (dev_pager_entry_t) kmem_cache_alloc(&dev_pager_hash_cache);
-   new_entry->name = name_port;
+   new_entry->name = rec->pager;
+   new_entry->offset = offset;
new_entry->pager_rec = rec;
 
-   simple_lock(&dev_pager_hash_lock);
-   queue_enter(&dev_pager_hashtable[dev_pager_hash(name_port)],
+   simple_lock(&dev_hash_lock);
+   queue_enter(&dev_device_hashtable[dev_hash(device)],
new_entry, dev_pager_entry_t, links);
-   simple_unlock(&dev_pager_hash_lock);
+   dev_pager_hashtable[dev_hash(rec->pager)] = device;
+   simple_unlock(&dev_hash_lock);
 }
 
-void dev_pager_hash_delete(const ipc_port_t name_port)
+void dev_hash_delete(
+   const mach_device_t device,
+   const vm_offset_t   offset)
 {
queue_t bucket;
dev_pager_entry_t   entry;
 
-   bucket = &dev_pager_hashtable[dev_pager_hash(name_port)];
+   bucket = &dev_device_hashtable[dev_hash(device)];
 
-   simple_lock(&dev_pager_hash_lock);
+   simple_lock(&dev_hash_lock);
for (entry = (dev_pager_entry_t)queue_first(bucket);
 !queue_end(bucket, &entry->links);
 entry = (dev_pager_entry_t)queue_next(&entry->links)) {
-   if (entry->name == name_port) {
+   if (entry->offset == offset) {
queue_remove(bucket, entry, dev_pager_entry_t, links);
+   dev_pager_hashtable[dev_hash(entry->pager_rec->pager)] = 
MACH_DEVICE_NULL;

[PATCH] - gnumach: Implement offset in device map

[Joan Lledó]

Hi,

I made the changed in the device mapper to implement the offset, also updated 
my branches of hurd and libpciaccess accordingly.

It wasn't as trivial as it seemed, there were some /* HACK */ lines which in 
fact limited the number of pagers per device to 1, so once the system got a 
pager for "mem" at offset 0, I couldn't give it another offset without that 
affecting all other memory mappings for that device. I updated the structure of 
the hash table to get rid of the /* HACK */ lines and support many pagers per 
device, one pager per [device,offset]. (BTW, why is the the hash table limited 
to 127 slots?)

On the other hand, I couldn't think of any other way to get rid of the struct 
pci_user_data to get the pager from the arbiter. Any ideas? I'll work on the 
new libpciaccess interface for the Hurd to get the pager unless somebody has a 
better idea.

Re: PCI arbiter memory mapping

[Joan Lledó]

El 18/8/21 a les 0:02, Sergey Bugaev ha escrit:

To me it sounds like libpciaccess should have a Hurd-specific API
addition that would let the user get the memory object


That's a solution and can be done. But I'd like to know more about 
vm_region first. It seems it can return the object, and I don't see why 
is a security problem to allow a task to retrieve objects belonging to 
itself.

Re: PCI arbiter memory mapping

[Joan Lledó]

El 18/8/21 a les 0:13, Sergey Bugaev ha escrit:

you can no longer get the underlying memory object with vm_region ()


How so? reading the docs I understood it does:

https://www.gnu.org/software/hurd/gnumach-doc/Memory-Attributes.html

"The port object_name identifies the memory object associated with this 
region"



(Well in fact what actually changed is that gnumach at some point
allowed the use of "memory object name ports" in vm_map (), and now
once again doesn't.


vm_map does receive  the memory object as parameter:

https://www.gnu.org/software/hurd/gnumach-doc/Mapping-Memory-Objects.html

"memory_object is the port that represents the memory object: used by 
user tasks in vm_map"



It never allowed you to get the actual memory
object.)


vm_map receives a memory object, doesn't return it

Re: PCI arbiter memory mapping

[Joan Lledó]

Hi,

I'm sorry I can't follow your discussion, I only know about the small 
part of the kernel I worked on.


El 16/8/21 a les 23:07, Sergey Bugaev ha escrit:

I don't think I understand enough about the situation. It would help
if you or Joan were to kindly give me some more context :)


Basically, libpciaccess gets a memory object at [1]. And later I need it 
in the arbiter at [2], to create a proxy over it.


To do that, the current code stores it in a structure I created called 
pci_user_data [3], and then it reads that structure from the arbiter [4].


> What's the issue you're trying to solve?

We are looking for another way to get the pager at [4] and get rid of 
that structure.



As I understand it, there's the PCI arbiter, which is a translator
that arbitrates access to PCI, which is a hardware bus that various
devices can be connected to.


Yes, and the arbiter can play two roles: root arbiter, which uses x86 
module in libpciacces; and nested arbiter, which uses the hurd module in 
libpciaccess.



The hardware devices connected via PCI are available (to the PCI arbiter)
as Mach devices


Actually, the devices are available to the arbiter as libpciaccess devices


it's possible to use device_map () and then vm_map () to access the
device memory.


Yes, root arbiter uses device_map() on "mem" to get the memory object, 
nested arbiters use io_map() over the region files exposed by the root 
arbiter to get the memory object.


Both pass the memory object to vm_map() to map the range.


Then there's libpciaccess whose Hurd backend uses the
files exported by the PCI arbiter to get access to the PCI,


Only nested arbiters, as I said the root arbiter uses the x86 backend


Naturally its user can request read-only or
read-write mapping, but the PCI arbiter may decide to only return a
read-only memory object (a proxy to the device pager), in which case
libpciaccess should deallocate the port and return EPREM, or the PCI
arbiter may return the real device pager.


Yes, but that's not really relevant for our problem, I was talking about 
a bug I found.




[1] 
https://gitlab.freedesktop.org/jlledom/libpciaccess/-/blob/hurd-device-map/src/x86_pci.c#L275
[2] 
http://git.savannah.gnu.org/cgit/hurd/hurd.git/tree/pci-arbiter/netfs_impl.c?h=jlledom-pci-memory-map#n613
[3] 
https://gitlab.freedesktop.org/jlledom/libpciaccess/-/blob/hurd-device-map/src/x86_pci.c#L287
[4] 
http://git.savannah.gnu.org/cgit/hurd/hurd.git/tree/pci-arbiter/netfs_impl.c?h=jlledom-pci-memory-map#n605

Re: PCI arbiter memory mapping

[Joan Lledó]

Hi,

El 16/8/21 a les 20:16, Samuel Thibault ha escrit:

Ok but I meant that the device_map interface already has has an "offset"


Ok, now I got it, yes I think that's better. I'll do that.


Actually I'm thinking that this is just another case of mremap().


I need help on this part, why is mremap relevant here?


The underlying question is getting the memory object of a given memory
range, like vm_region does.


Yes, I see that could be useful. Is vm_region not workig for proxies?

> We need to be careful since we don't want any process to be able to
> get the memory object of any other process

Is that not being checked yet? can I call vm_region to get another 
task's memory object now?

Re: PCI arbiter memory mapping

[Joan Lledó]

Hi,

El 5/8/21 a les 1:26, Samuel Thibault ha escrit:


Is it not possible to avoid having to call memory_object_proxy_valid?
maybe better fix device_map into supporting non-zero offset,


I think it'd be a better solution to move the call to 
memory_object_proxy_valid() and the start value overwrite to 
memory_object_create_proxy(), that way all logic related to proxies is 
kept inside memory_object_proxy.c and other modules of the kernel don't 
need to be aware of proxies.




Does pci_device_hurd_unmap_range not need to close the pager?



Yes.


Also, map_dev_mem needs to release the pager when dev == NULL? otherwise
pci_device_x86_read_rom would leak the pager?



Yes. Or not passing a pager to device_map() if dev == NULL, is not going 
to be used anyway.



I'm a bit afraid of the struct pci_user_data passing between
libpciaccess and pci-arbiter


Me too.


I'd rather see a
hurd-specific libpciaccess function to get the pager.



That's better, but we'd have to add that function in both hurd_pci.c and 
x86_pci.c. I don't like the idea of adding Hurd specific code to 
x86_module but there's already a lot of it and we could avoid the 
existence of struct pci_user_data, which I like even less.


Apart from that, I also found a bug in hurd_pci.c:196 [1]. When the user 
asks for read/write permissions but only read is allowed, we should either:


- Deallocate robj and return EPERM
- Do nothing and map the range read-only which is not what the user 
asked for.


The current code deallocates wobj which is null, leaks robj and returns 
EPERM. That wrong, since it doesn't make much sense, but which of two 
above is correct?


I think pci_device_hurd_map_range() should behave the same way 
pci_device_x86_map_range() does in the x86 module. But the 
implementation of map_dev_mem() is not clear about what happens in that 
case, I guess vm_map() handles that.



BTW, you can directly push "fix indentation" commits, so that after
rebasing your branch only contains actual code changes, for easier
review.


I'll do.

--
[1] 
https://gitlab.freedesktop.org/jlledom/libpciaccess/-/blob/hurd-device-map/src/hurd_pci.c#L196

Re: PCI arbiter memory mapping

[Joan Lledó]

Hi

El 9/8/21 a les 19:45, Samuel Thibault ha escrit:

I pushed the start/len start, along with a fix for the length.  Could
you proofread that part?





I ran it and works fine

Re: Regarding copyright assignment to FSF

[Joan Lledó]

El 14/8/21 a les 23:26, Svante Signell ha escrit:

How to make lwip by default enabled instead of pfinet?



settrans -fgap /servers/socket/2 /hurd/lwip -6 /servers/socket/26

Re: PCI arbiter memory mapping

[Joan Lledó]

hi

El 9/8/21 a les 19:45, Samuel Thibault ha escrit:

I pushed the start/len start, along with a fix for the length.  Could
you proofread that part?



It seems all right for me. I'll test this and check you other comments 
next weekend or the following.

Re: PCI arbiter memory mapping

[Joan Lledó]

Any thoughts on this?

El 19/6/21 a les 11:50, Joan Lledó ha escrit:

Hi Hurd,

I finally got memory mapping working in the pci arbiter. That means any 
user with the proper permissions can map the device region files 
generated by an arbiter. This is also working for nested arbiters.


For this I made changes in libpciaccess, gnumach, and the Hurd: 
pci-arbiter and libnetfs folders.


I got the code in some branches in different repositories. How should I 
share the code for your review? do you prefer diff patches by email or 
should I clean the branches and share the links?

Re: PCI arbiter memory mapping

[Joan Lledó]

Hi,

El 20/6/21 a les 3:25, Damien Zammit ha escrit:

Hi Joan,

On 19/6/21 7:50 pm, Joan Lledó wrote:



How does that interact with existing pci access for example, I think the AHCI 
rump driver
works with DMA so do we need to also adjust the pci-userspace part of 
rumpkernel?


I couldn't say, I know very little of that, TBH. but I'd say if it was 
working before you shouldn't need to adjust anything, b/c my changes in 
the arbiter added new functionality, not breaking changes on components 
already working. At the end of the day what is did is only allowing 
region files to be mapped with mmap() and vm_map().


Anyway, I cleaned up the branches, you can test it yourself:

https://gitlab.freedesktop.org/jlledom/libpciaccess/-/tree/hurd-device-map

http://git.savannah.gnu.org/cgit/hurd/gnumach.git/log/?h=jlledom-memory-object-proxy

http://git.savannah.gnu.org/cgit/hurd/hurd.git/log/?h=jlledom-pci-memory-map

PCI arbiter memory mapping

[Joan Lledó]

Hi Hurd,

I finally got memory mapping working in the pci arbiter. That means any 
user with the proper permissions can map the device region files 
generated by an arbiter. This is also working for nested arbiters.


For this I made changes in libpciaccess, gnumach, and the Hurd: 
pci-arbiter and libnetfs folders.


I got the code in some branches in different repositories. How should I 
share the code for your review? do you prefer diff patches by email or 
should I clean the branches and share the links?

Re: Adding a new syscall to gnumach

[Joan Lledó]

Hi

El 6/6/21 a les 14:48, Samuel Thibault ha escrit:

AIUI it's not a system call that you are talking about, but an RPC.


yes, you're right


but it's not enough.


What do you mean by "it's not enough"?  What does not work *exactly*?


sorry, gnumach compilation fails with this error:

if test -s gnumach-undef-bad; \
then cat gnumach-undef-bad; exit 2; else true; fi
memory_object_proxy_valid
Makefile:9598: recipe for target 'clib-routines.o' failed
make: *** [clib-routines.o] Error 2
rm i386/i386/i386asm.symc.o i386/i386/i386asm.symc



Source-code-wise IIRC it's enough. But to get the function into
libmachuser you need to rebuild it against the new .defs file.


yes, what I really need is to rebuild libmachuser to include this 
function, I tried to:


- update mach4.defs to declare it
- replace the original mach4.defs by mine
- rebuild the glibc and install the generated *.debs

after that, the error continues

Adding a new syscall to gnumach

[Joan Lledó]

Hi Hurd,

In my work to extend the memory proxies implementation, I'm considering 
adding a new syscall which receives a memory object and returns whether 
is it a proxy or not. I need something like that from the arbiter to 
properly set ranges when creating a new proxy.


What are the steps to add a new syscall?, is there any docs? I wrote the 
function and declared its prototype in mach4.defs, but it's not enough.


Thanks

Re: Privileged ports to access the arbiter

[Joan Lledó]

El 5/5/21 a les 20:22, Samuel Thibault ha escrit:

Mmm, no, we only need it for the device_open case. We don't need it for
falling back on using file_name_lookup (_SERVERS_BUS_PCI)



And do we need write permissions to perform the scan?

Privileged ports to access the arbiter

[Joan Lledó]

Hi,

fortunately, after pulling last changes and updating hurd-libs0.3 I 
solved my problems with the arbiter being killed. Now I got my user 
clients getting the maps correctly, and wanted to go further and 
implement memory object proxy nesting and pci-arbiter nesting.


I found a problem there:

https://gitlab.freedesktop.org/xorg/lib/libpciaccess/-/blob/master/src/hurd_pci.c#L526

I can't run my nested arbiter as a non-privileged user b/c this line 
prevents me to connect to the main arbiter by lack of permissions. Why 
are why asking for privileges here? do we really need it?

Re: pci arbiter being killed

[Joan Lledó]

El 17/4/21 a les 12:44, Damien Zammit ha escrit:


Can you try rebasing on master and see if you have the same issues?



After merging the last changes the arbiter hangs at startup, so it 
doesn't live enough to be killed. I think it's probably another effect 
of the same bug in my version of the kernel.

Re: pci arbiter being killed

[Joan Lledó]

El 17/4/21 a les 14:14, Samuel Thibault ha escrit:



Do you mean that without your changes there is no pci arbiter kill?



Yes

Re: pci arbiter being killed

[Joan Lledó]

Hi,

El 7/3/21 a les 20:33, Samuel Thibault ha escrit:

You could e.g. put mach_prints in glibc's sysdeps/mach/hurd/kill.c's
SIGKILL case.

Possibly better also add a mach_print in hurd/hurdsig.c SIGKILL on
_hurd_orphaned.



I tried but I saw nothing, is it supposed to show the messages in the 
console?


I spent too much time on this and still haven't found the problem, need 
some help. I suspect the problem is in the kernel b/c it's the part I'm 
less competent. I'm attaching a patch with the changes I made on the 
memory object proxy logic. Could you guys take a look and tell me if you 
see any problem?


Thanks
diff --git a/vm/memory_object_proxy.c b/vm/memory_object_proxy.c
index 01bce2a5..912387f0 100644
--- a/vm/memory_object_proxy.c
+++ b/vm/memory_object_proxy.c
@@ -56,6 +56,8 @@ struct memory_object_proxy
 
   ipc_port_t object;
   vm_prot_t max_protection;
+  vm_offset_t start;
+  vm_offset_t len;
 };
 typedef struct memory_object_proxy *memory_object_proxy_t;
 
@@ -66,7 +68,7 @@ memory_object_proxy_init (void)
   kmem_cache_init (&memory_object_proxy_cache, "memory_object_proxy",
 		   sizeof (struct memory_object_proxy), 0, NULL, 0);
 }
-  
+
 /* Lookup a proxy memory object by its port.  */
 static memory_object_proxy_t
 memory_object_proxy_port_lookup (ipc_port_t port)
@@ -143,10 +145,6 @@ memory_object_create_proxy (const ipc_space_t space, vm_prot_t max_protection,
   if (offset[0] != 0)
 return KERN_INVALID_ARGUMENT;
 
-  /* FIXME: Support a different range from total.  */
-  if (start[0] != 0 || len[0] != (vm_offset_t) ~0)
-return KERN_INVALID_ARGUMENT;
-
   proxy = (memory_object_proxy_t) kmem_cache_alloc (&memory_object_proxy_cache);
 
   /* Allocate port, keeping a reference for it.  */
@@ -167,6 +165,8 @@ memory_object_create_proxy (const ipc_space_t space, vm_prot_t max_protection,
 
   proxy->object = ipc_port_copy_send (object[0]);
   proxy->max_protection = max_protection;
+  proxy->start = start[0];
+  proxy->len = len[0];
 
   *port = ipc_port_make_send (proxy->port);
   return KERN_SUCCESS;
@@ -181,7 +181,8 @@ memory_object_create_proxy (const ipc_space_t space, vm_prot_t max_protection,
KERN_INVALID_ARGUMENT.  */
 kern_return_t
 memory_object_proxy_lookup (ipc_port_t port, ipc_port_t *object,
-			vm_prot_t *max_protection)
+			vm_prot_t *max_protection, vm_offset_t *start,
+			vm_offset_t *len)
 {
   memory_object_proxy_t proxy;
 
@@ -191,6 +192,8 @@ memory_object_proxy_lookup (ipc_port_t port, ipc_port_t *object,
 
*object = proxy->object;
*max_protection = proxy->max_protection;
+   *start = proxy->start;
+   *len = proxy->len;
 
   return KERN_SUCCESS;
 }
diff --git a/vm/memory_object_proxy.h b/vm/memory_object_proxy.h
index dc0ea747..8b3f2025 100644
--- a/vm/memory_object_proxy.h
+++ b/vm/memory_object_proxy.h
@@ -32,6 +32,8 @@ extern void memory_object_proxy_init (void);
 extern boolean_t memory_object_proxy_notify (mach_msg_header_t *msg);
 extern kern_return_t memory_object_proxy_lookup (ipc_port_t port,
  ipc_port_t *object,
- vm_prot_t *max_protection);
+ vm_prot_t *max_protection,
+ vm_offset_t *start,
+ vm_offset_t *len);
 
 #endif /* _VM_MEMORY_OBJECT_PROXY_H_ */
diff --git a/vm/vm_user.c b/vm/vm_user.c
index 4d5728c8..6e82cc60 100644
--- a/vm/vm_user.c
+++ b/vm/vm_user.c
@@ -1,32 +1,32 @@
-/* 
+/*
  * Mach Operating System
  * Copyright (c) 1991,1990,1989,1988 Carnegie Mellon University
  * All Rights Reserved.
- * 
+ *
  * Permission to use, copy, modify and distribute this software and its
  * documentation is hereby granted, provided that both the copyright
  * notice and this permission notice appear in all copies of the
  * software, derivative works or modified versions, and any portions
  * thereof, and that both notices appear in supporting documentation.
- * 
+ *
  * CARNEGIE MELLON ALLOWS FREE USE OF THIS SOFTWARE IN ITS "AS IS"
  * CONDITION.  CARNEGIE MELLON DISCLAIMS ANY LIABILITY OF ANY KIND FOR
  * ANY DAMAGES WHATSOEVER RESULTING FROM THE USE OF THIS SOFTWARE.
- * 
+ *
  * Carnegie Mellon requests users of this software to return to
- * 
+ *
  *  Software Distribution Coordinator  or  software.distribut...@cs.cmu.edu
  *  School of Computer Science
  *  Carnegie Mellon University
  *  Pittsburgh PA 15213-3890
- * 
+ *
  * any improvements or extensions that they make and grant Carnegie Mellon
  * the rights to redistribute these changes.
  */
 /*
  *	File:	vm/vm_user.c
  *	Author:	Avadis Tevanian, Jr., Michael Wayne Young
- * 
+ *
  *	User-exported virtual memory functions.
  */
 
@@ -158,7 +158,7 @@ kern_return_t vm_protect(
 	boolean_t		set_maximum,
 	vm_prot_t		new_protection)
 {
-	if ((map == VM_MAP_NULL) || 
+	if ((map == VM_MAP_NULL) ||
 		(new_protection & ~(VM_PROT_ALL|V

Re: pci arbiter being killed

[Joan Lledó]

Hi

El 7/3/21 a les 20:33, Samuel Thibault ha escrit:

Joan Lledó, le dim. 07 mars 2021 20:08:21 +0100, a ecrit:

is not there any process that kills the arbiter after a while of not
being used?


That shouldn't be happening.


and how could I know who is sending the SIGKILL?


How do you notice that it's a SIGKILL?


I connected gdb to the arbiter and it showed a message saying the 
process has received a SIGKILL.




You could e.g. put mach_prints in glibc's sysdeps/mach/hurd/kill.c's
SIGKILL case.

Possibly better also add a mach_print in hurd/hurdsig.c SIGKILL on
_hurd_orphaned.



I'll try that, thanks.

pci arbiter being killed

[Joan Lledó]

Hi Hurd,

I've observed how my instance of the pci arbiter receives a SIGKILL 
about a minute after start, it could be my fault as I've being messing 
around with gnumach, libpciaccess and the arbiter itself, but just to 
confirm: is not there any process that kills the arbiter after a while 
of not being used? and how could I know who is sending the SIGKILL?

Re: Debugging gnumach

[Joan Lledó]

Hi,

El 25/12/20 a les 23:43, Almudena Garcia ha escrit:

My script is here
https://gist.github.com/AlmuHS/73bae6dadf19b0482a34eaab567bfdfa 



Thanks. This didn't worked for me. Question: what's the content of 
hurd_qemu/hurd.img? is it the image of a single partition or does it 
contain many partitions and GRUB installed on it?

Debugging gnumach

[Joan Lledó]

Hi Hurd,

recently I tried to implement ranges on memory object proxies[1]. I 
never worked on gnumach before, so as expected it failed. That's ok, but 
now I'd like to debug gnumach in order to find the issue.


The docs[2] say I can either use the built-in debugger or gdb over qemu. 
I tried kdb, but I don't know how to find the address of a local 
variable inside a function. I also tried the gdb approach, but I can't 
boot the kernel b/c I don't know how to load the modules from the qemu 
command-line ("panic: No bootstrap code loaded with the kernel!")


- How do you guys debug gnumach? gdb or kdb?

- If gdb, what command do you use?

- If kdb, how do you read the value for a local variable?


---
[1] 
http://git.savannah.gnu.org/cgit/hurd/gnumach.git/log/?h=jlledom-mem-obj-proxy
[2] 
https://www.gnu.org/software/hurd/microkernel/mach/gnumach/debugging.html

Re: Contributing - available projects?

[Joan Lledó]

Hi,

El 15/12/20 a les 21:40, Edward Haigh ha escrit:
Of course! I'll set a dev environment up and do a little research on 
existing frameworks, then come back to you.




I remember I used to use the Perl test suite to test the lwip 
translator, you may want to take a look.

Re: Implement paging on the pci arbiter

[Joan Lledó]

Hi,

El 3/11/20 a les 23:13, Samuel Thibault ha escrit:
> 
> That would probably work, yes.
> 
> 

I got something pushed to my branch at [1]. But I found the
implementation for pager proxies in gnu mach is incomplete. In
particular I can't restrict a range to be mapped. I think I could fix it
but need some help.

- Would it be enough to add the range info to the memory_object_proxy
  struct and then read them from vm_map and use them to restrict the
  values of the offset and size being sent to vm_map_enter?
  (vm_user.c:395)
- Why is the proxy interface designed to work with arrays? Is vm_map
  supposed to call vm_map_enter multiple times for proxies?

---
1. http://git.savannah.gnu.org/cgit/hurd/hurd.git/log/?h=jlledom-pci-mem

Re: Implement paging on the pci arbiter

[Joan Lledó]

Hi,

El 26/8/20 a les 17:01, Samuel Thibault ha escrit:
> I'm unsure if libpager will be useful actually, since all you need is
> to pass on a memory object clamped to the target physical memory. See
> gnumach's support for proxy memory object, which possibly is just
> enough.
> 
> Samuel
> 

I did a research on this and think I could implement file mapping in
libnetfs based on this old patch by Marcus Brinkmann:

https://lists.gnu.org/archive/html/bug-hurd/2001-10/msg00305.html

Thomas Bushnell BSG answered to that message saying

> You must promise the kernel that *any* change to the underlying data
> for the mapped object will never change except with you telling the
> kernel

Which makes sense, but it seems to me it should be OK to enable file
mapping in a net filesystem provided only memory proxy objects are used
for this, since each proxy will belong to a memory object that meets the
requirement of keeping the kernel updated on any change in the
underlying data.

So we can take the implementation of io_map from that patch, but adding
a check to ensure whatever returned by the user from netfs_get_filemap()
is a proxy object. Is there a way to check that?

With that, from the aribter side we only need to get the default pager
and create a proxy for each region file, with its boundaries and
permissions, and write an implementation of netfs_get_filemap which
returns the proper proxy for each request.

What do you think?



signature.asc
Description: OpenPGP digital signature

Re: PCI arbiter crash on last qemu image

[Joan Lledó]

El 13/9/20 a les 15:54, Samuel Thibault ha escrit:
> Hello,
> 
> Joan Lledó, le dim. 13 sept. 2020 08:38:48 +0200, a ecrit:
>> El 10/9/20 a les 0:29, Samuel Thibault ha escrit:
>>> Now fixed in libpciaccess 0.16-1+hurd.6 and upstream.
>>
>> Then should I merge jlledom-pciaccess-map into master?
>>
>> http://git.savannah.gnu.org/cgit/hurd/hurd.git/log/?h=jlledom-pciaccess-map
> 
> This looks so indeed.

Done, I've also removed my branch

Re: PCI arbiter crash on last qemu image

[Joan Lledó]

El 10/9/20 a les 0:29, Samuel Thibault ha escrit:
> Now fixed in libpciaccess 0.16-1+hurd.6 and upstream.
> 

Then should I merge jlledom-pciaccess-map into master?

http://git.savannah.gnu.org/cgit/hurd/hurd.git/log/?h=jlledom-pciaccess-map

Re: Implement paging on the pci arbiter

[Joan Lledó]

El 27/8/20 a les 0:30, Samuel Thibault ha escrit:
> libpager/demuxer.c:pager_demuxer (struct pager_requests *requests,

That's not the same pager_demuxer in the reference, it's a static
function and receives different parameters. It seems there were changes
in the libpager's interface and the documents are not updated.

Re: PCI arbiter crash on last qemu image

[Joan Lledó]

Hi,

El 26/8/20 a les 11:13, Damien Zammit ha escrit:
> If you think everything is okay with this, I will squash the last patch and 
> submit patches upstream.

Yes it's OK for me