Re: [PATCH] pci-arbiter: Remove embedded pciaccess code
Hello, El 27/10/19 a les 16:32, Samuel Thibault ha escrit:> Hello, > Could you try to split your changes in separate patches? I splitted the patch into four patches, the first one is the Damien's original patch adapted to the current head + a changelog. The other three patches are my changes. > I don't remember: can we apply this patch in Debian Hurd already? (i.e. > does the libpciaccess there (possibly in unreleased) work fine enough > for our needs?) I've been taking a look at the big picture and I'd say the problem is the GNU Mach restriction to only one process accessing the cfg io ports. Any release where that is enabled, then libpciaccess must include Damien's patches so all clients are redirected to the arbiter. But for the arbiter, the commits we apply won't change anything I'd say, since the arbiter will be just another client trying to access the io ports, no matter if it does it directly or through a library. > This looks a bit complex, I would say we should rather just cast: > > typedef int (*pci_op_t) (struct pci_device * dev, void *data, >pciaddr_t reg, pciaddr_t width, >pciaddr_t * bytes); > > io_config_file (node->nn->ln->device, offset, len, data, > - pci_sys->write); > + (pci_op_t) pci_sys->write); > > Because it is completely compatible to pass a void* to a function that > takes a const void*. That will simplify the whole thing a lot. Def. I made an unnecessary mess. The attached patch just makes the cast. >>/* Copy the regions info */ >> - rom.base_addr = e->device->rom_base; >> + rom.base_addr = 0; // pci_device_private only >>rom.size = e->device->rom_size; >>memcpy (*data, &rom, size); > > The change is because rom_base is a private field in libpciaccess, we'd > need to get libpciaccess to expose it so we can properly take it into > account. For the time being, putting FIXME here would be enough. Understood! I filled that line in the changelog.
[PATCH 4/4] pci-arbiter: Fix warning on passing incompatible pointer type
From: Joan Lledó
* pci-arbiter/netfs_impl.c:
* netfs_attempt_write: Cast op function to pci_io_op_t
---
pci-arbiter/netfs_impl.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/pci-arbiter/netfs_impl.c b/pci-arbiter/netfs_impl.c
index 0be8c370..b987a0bc 100644
--- a/pci-arbiter/netfs_impl.c
+++ b/pci-arbiter/netfs_impl.c
@@ -539,7 +539,7 @@ netfs_attempt_write (struct iouser * cred, struct node *
node,
{
err =
io_config_file (node->nn->ln->device, offset, len, data,
- pci_device_cfg_write);
+ (pci_io_op_t) pci_device_cfg_write);
if (!err)
{
/* Update mtime and ctime */
--
2.20.1
[PATCH 2/4] pci-arbiter: Call libpciaccess cleanup on shutdown
From: Joan Lledó * pci-arbiter/startup-ops.c: * S_startup_dosync: Call pci_system_cleanup(). --- pci-arbiter/startup-ops.c | 4 1 file changed, 4 insertions(+) diff --git a/pci-arbiter/startup-ops.c b/pci-arbiter/startup-ops.c index f3506c42..eb387fd9 100644 --- a/pci-arbiter/startup-ops.c +++ b/pci-arbiter/startup-ops.c @@ -20,6 +20,7 @@ #include +#include #include #include "startup.h" @@ -34,6 +35,9 @@ S_startup_dosync (mach_port_t handle) if (!inpi) return EOPNOTSUPP; + // Free all libpciaccess resources + pci_system_cleanup (); + ports_port_deref (inpi); return netfs_shutdown (FSYS_GOAWAY_FORCE); -- 2.20.1
Re: [PATCH] pci-arbiter: Remove embedded pciaccess code
Strange, the patches were not attached... I send them again.
[PATCH 3/4] pci-arbiter: Fix a -Wstringop-truncation warning
From: Joan Lledó
* pci-arbiter/pcifs.c:
* create_dir_entry:
Limit to NAME_SIZE-1 when calling strncpy().
Finish entry->name with '\0'.
* create_fs_tree:
memset() to 0 the directory entry.
Limit to NAME_SIZE-1 all calls to
snprintf() and strncpy().
---
pci-arbiter/pcifs.c | 18 ++
1 file changed, 10 insertions(+), 8 deletions(-)
diff --git a/pci-arbiter/pcifs.c b/pci-arbiter/pcifs.c
index e68e4b8f..0ff1851c 100644
--- a/pci-arbiter/pcifs.c
+++ b/pci-arbiter/pcifs.c
@@ -45,7 +45,8 @@ create_dir_entry (int32_t domain, int16_t bus, int16_t dev,
entry->dev = dev;
entry->func = func;
entry->device_class = device_class;
- strncpy (entry->name, name, NAME_SIZE);
+ strncpy (entry->name, name, NAME_SIZE - 1);
+ entry->name[NAME_SIZE - 1] = '\0';
entry->parent = parent;
entry->stat = stat;
entry->dir = 0;
@@ -193,6 +194,7 @@ create_fs_tree (struct pcifs * fs)
return ENOMEM;
e = list + 1;
+ memset (e, 0, sizeof (struct pcifs_dirent));
c_domain = c_bus = c_dev = -1;
domain_parent = bus_parent = dev_parent = func_parent = 0;
iter = pci_slot_match_iterator_create(&match);
@@ -206,7 +208,7 @@ create_fs_tree (struct pcifs * fs)
e_stat = list->stat;
e_stat.st_mode &= ~S_IROOT; /* Remove the root mode */
memset (entry_name, 0, NAME_SIZE);
- snprintf (entry_name, NAME_SIZE, "%04x", device->domain);
+ snprintf (entry_name, NAME_SIZE - 1, "%04x", device->domain);
err =
create_dir_entry (device->domain, -1, -1, -1, -1, entry_name,
list, e_stat, 0, 0, e);
@@ -224,7 +226,7 @@ create_fs_tree (struct pcifs * fs)
{
/* We've found a new bus. Add an entry for it */
memset (entry_name, 0, NAME_SIZE);
- snprintf (entry_name, NAME_SIZE, "%02x", device->bus);
+ snprintf (entry_name, NAME_SIZE - 1, "%02x", device->bus);
err =
create_dir_entry (device->domain, device->bus, -1, -1, -1,
entry_name, domain_parent, domain_parent->stat,
@@ -242,7 +244,7 @@ create_fs_tree (struct pcifs * fs)
{
/* We've found a new dev. Add an entry for it */
memset (entry_name, 0, NAME_SIZE);
- snprintf (entry_name, NAME_SIZE, "%02x", device->dev);
+ snprintf (entry_name, NAME_SIZE - 1, "%02x", device->dev);
err =
create_dir_entry (device->domain, device->bus, device->dev, -1,
-1, entry_name, bus_parent, bus_parent->stat, 0,
@@ -261,7 +263,7 @@ create_fs_tree (struct pcifs * fs)
/* Add func entry */
memset (entry_name, 0, NAME_SIZE);
- snprintf (entry_name, NAME_SIZE, "%01u", device->func);
+ snprintf (entry_name, NAME_SIZE - 1, "%01u", device->func);
err =
create_dir_entry (device->domain, device->bus, device->dev,
device->func, device->device_class, entry_name,
@@ -279,7 +281,7 @@ create_fs_tree (struct pcifs * fs)
e_stat.st_size = PCI_CONFIG_SIZE; // FIXME: Hardcoded
/* Create config entry */
- strncpy (entry_name, FILE_CONFIG_NAME, NAME_SIZE);
+ strncpy (entry_name, FILE_CONFIG_NAME, NAME_SIZE - 1);
err =
create_dir_entry (device->domain, device->bus, device->dev,
device->func, device->device_class, entry_name,
@@ -293,7 +295,7 @@ create_fs_tree (struct pcifs * fs)
if (device->regions[j].size > 0)
{
e_stat.st_size = device->regions[j].size;
- snprintf (entry_name, NAME_SIZE, "%s%01u", FILE_REGION_NAME, j);
+ snprintf (entry_name, NAME_SIZE - 1, "%s%01u", FILE_REGION_NAME,
j);
err =
create_dir_entry (device->domain, device->bus, device->dev,
device->func, device->device_class,
@@ -310,7 +312,7 @@ create_fs_tree (struct pcifs * fs)
/* Make rom is read only */
e_stat.st_mode &= ~(S_IWUSR | S_IWGRP);
e_stat.st_size = device->rom_size;
- strncpy (entry_name, FILE_ROM_NAME, NAME_SIZE);
+ strncpy (entry_name, FILE_ROM_NAME, NAME_SIZE - 1);
err =
create_dir_entry (device->domain, device->bus, device->dev,
device->func, device->device_class, entry_name,
--
2.20.1
[PATCH 1/4] pci-arbiter: Remove embedded pciaccess code
From: Damien Zammit
This patch removes all embedded pciaccess code from the arbiter
and instead uses the external pciaccess library.
* pci-arbiter/Makefile:
* Remove pci_access.c and x86_pci.c from the sources.
* pci-arbiter/func_files.c:
* io_config_file: Use a harcoded PCI config size.
* read_rom_file:
Grab the full rom first, then return the
requested amount.
* pci-arbiter/main.c:
* main: Call create_fs_tree() w/o pci_sys.
Since it's not part of the translator anymore.
* pci-arbiter/netfs_impl.c:
* netfs_attempt_read:
Send pci_device_cfg_read() as the read op.
* netfs_attempt_write:
Send pci_device_cfg_write() as the write op.
* pci-arbiter/pci-ops.c:
* S_pci_conf_read: Call libpciaccess' pci_device_cfg_read().
* S_pci_conf_write: Call libpciaccess' pci_device_cfg_write().
* S_pci_get_dev_rom:
Set rom.base_addr to zero for the moment, until
libpciaccess esposes it properly.
* pci-arbiter/pci_access.c: Deleted
* pci-arbiter/pci_access.h: Deleted
* pci-arbiter/pcifs.c:
* create_fs_tree:
Remove the pci_sys parameter.
Use libpciaccess' iterator.
Use a hardcoded config space size.
* pci-arbiter/pcifs.h: Definitions for changes in pcifs.c.
* pci-arbiter/x86_pci.c: Deleted.
* pci-arbiter/x86_pci.h: Deleted.
---
pci-arbiter/Makefile | 4 +-
pci-arbiter/TODO | 9 +-
pci-arbiter/func_files.c | 34 +-
pci-arbiter/func_files.h | 4 +
pci-arbiter/main.c | 6 +-
pci-arbiter/netfs_impl.c | 22 +-
pci-arbiter/pci-ops.c| 8 +-
pci-arbiter/pci_access.c | 51 ---
pci-arbiter/pci_access.h | 187 -
pci-arbiter/pcifs.c | 24 +-
pci-arbiter/pcifs.h | 11 +-
pci-arbiter/x86_pci.c| 843 ---
pci-arbiter/x86_pci.h| 34 --
13 files changed, 68 insertions(+), 1169 deletions(-)
delete mode 100644 pci-arbiter/pci_access.c
delete mode 100644 pci-arbiter/pci_access.h
delete mode 100644 pci-arbiter/x86_pci.c
delete mode 100644 pci-arbiter/x86_pci.h
diff --git a/pci-arbiter/Makefile b/pci-arbiter/Makefile
index 357bb081..b13aefa8 100644
--- a/pci-arbiter/Makefile
+++ b/pci-arbiter/Makefile
@@ -20,14 +20,14 @@ makemode= server
PORTDIR = $(srcdir)/port
-SRCS = main.c pci-ops.c pci_access.c x86_pci.c netfs_impl.c \
+SRCS = main.c pci-ops.c netfs_impl.c \
pcifs.c ncache.c options.c func_files.c startup.c \
startup-ops.c
MIGSRCS= pciServer.c startup_notifyServer.c
OBJS = $(patsubst %.S,%.o,$(patsubst %.c,%.o, $(SRCS) $(MIGSRCS)))
HURDLIBS= fshelp ports shouldbeinlibc netfs iohelp ihash
-LDLIBS = -lpthread
+LDLIBS = -lpthread -lpciaccess
target = pci-arbiter
diff --git a/pci-arbiter/TODO b/pci-arbiter/TODO
index 22ae72a8..20060842 100644
--- a/pci-arbiter/TODO
+++ b/pci-arbiter/TODO
@@ -5,11 +5,10 @@
- pci_get_ndevs should be deprecated, applications shouldn't be relying on this
-- we shouldn't duplicate pci_access.[ch] x86_pci.[ch] from libpciaccess, we
- should get libpciaccess to expose pci_system_x86_create() to keep the
- maintenance of x86 port knocking there.
+- In pci-ops.c - config_block_op:
+ Update len with remaining allowed size once op() returns EIO
+ so that we get short reads/writes implemented by leaving it to pciaccess
- At least one difference with libpciaccess is the refresh operation. Perhaps
- we'd need to extend libpciaccess to fix that.
+- Upstream hurdish access method + x86 fixes to libpciaccess
BTW we could also support libpci.
diff --git a/pci-arbiter/func_files.c b/pci-arbiter/func_files.c
index 7df94d2f..c7da6978 100644
--- a/pci-arbiter/func_files.c
+++ b/pci-arbiter/func_files.c
@@ -35,10 +35,11 @@ config_block_op (struct pci_device *dev, off_t offset,
size_t * len,
{
error_t err;
size_t pendent = *len;
+ pciaddr_t actual = 0;
while (pendent >= 4)
{
- err = op (dev->bus, dev->dev, dev->func, offset, data, 4);
+ err = op (dev, data, offset, 4, &actual);
if (err)
return err;
@@ -49,7 +50,7 @@ config_block_op (struct pci_device *dev, off_t offset, size_t
* len,
if (pendent >= 2)
{
- err = op (dev->bus, dev->dev, dev->func, offset, data, 2);
+ err = op (dev, data, offset, 2, &actual);
if (err)
return err;
@@ -60,7 +61,7 @@ config_block_op (struct pci_device *dev, off_t offset, size_t
* len,
if (pendent)
{
- err = op (dev->bus, dev->dev, dev->func, offset, data, 1);
+ err = op (dev, data, offset, 1, &actual);
if (err)
return err;
@@ -85,10 +86,10 @@ io_config_file (struct pci_device * dev, off_t offset,
size_t * len,
assert_backtrace (dev != 0);
/* Don't exceed the config space size */
- if (offset > dev->config_size)
+ if (of
[PATCH] Hurd: avoid using the deprecated RPC pci_get_ndevs()
From: Joan Lledó
---
src/hurd_pci.c | 83 +-
1 file changed, 34 insertions(+), 49 deletions(-)
diff --git a/src/hurd_pci.c b/src/hurd_pci.c
index 28bef16..98bf83e 100644
--- a/src/hurd_pci.c
+++ b/src/hurd_pci.c
@@ -304,8 +304,8 @@ pci_device_hurd_destroy(struct pci_device *dev)
/* Walk through the FS tree to see what is allowed for us */
static int
-enum_devices(const char *parent, struct pci_device_private **device,
-int domain, int bus, int dev, int func, tree_level lev)
+enum_devices(const char *parent, int domain,
+int bus, int dev, int func, tree_level lev)
{
int err, ret;
DIR *dir;
@@ -315,6 +315,7 @@ enum_devices(const char *parent, struct pci_device_private
**device,
uint32_t reg;
size_t toread;
mach_port_t device_port;
+struct pci_device_private *d, *devices;
dir = opendir(parent);
if (!dir)
@@ -353,11 +354,10 @@ enum_devices(const char *parent, struct
pci_device_private **device,
return -1;
}
-err = enum_devices(path, device, domain, bus, dev, func, lev+1);
-if (err == EPERM)
-continue;
-}
-else {
+err = enum_devices(path, domain, bus, dev, func, lev+1);
+if (err && err != EPERM && err != EACCES)
+return err;
+} else {
if (strncmp(entry->d_name, FILE_CONFIG_NAME, NAME_MAX))
/* We are looking for the config file */
continue;
@@ -378,12 +378,20 @@ enum_devices(const char *parent, struct
pci_device_private **device,
if (toread != sizeof(reg))
return -1;
-(*device)->base.domain = domain;
-(*device)->base.bus = bus;
-(*device)->base.dev = dev;
-(*device)->base.func = func;
-(*device)->base.vendor_id = PCI_VENDOR(reg);
-(*device)->base.device_id = PCI_DEVICE(reg);
+devices = realloc(pci_sys->devices, (pci_sys->num_devices + 1)
+ * sizeof(struct pci_device_private));
+if (!devices)
+return ENOMEM;
+
+d = devices + pci_sys->num_devices;
+memset(d, 0, sizeof(struct pci_device_private));
+
+d->base.domain = domain;
+d->base.bus = bus;
+d->base.dev = dev;
+d->base.func = func;
+d->base.vendor_id = PCI_VENDOR(reg);
+d->base.device_id = PCI_DEVICE(reg);
toread = sizeof(reg);
err = pciclient_cfg_read(device_port, PCI_CLASS, (char*)®,
@@ -393,8 +401,8 @@ enum_devices(const char *parent, struct pci_device_private
**device,
if (toread != sizeof(reg))
return -1;
-(*device)->base.device_class = reg >> 8;
-(*device)->base.revision = reg & 0xFF;
+d->base.device_class = reg >> 8;
+d->base.revision = reg & 0xFF;
toread = sizeof(reg);
err = pciclient_cfg_read(device_port, PCI_SUB_VENDOR_ID,
@@ -404,12 +412,13 @@ enum_devices(const char *parent, struct
pci_device_private **device,
if (toread != sizeof(reg))
return -1;
-(*device)->base.subvendor_id = PCI_VENDOR(reg);
-(*device)->base.subdevice_id = PCI_DEVICE(reg);
+d->base.subvendor_id = PCI_VENDOR(reg);
+d->base.subdevice_id = PCI_DEVICE(reg);
-(*device)->device_port = device_port;
+d->device_port = device_port;
-(*device)++;
+pci_sys->devices = devices;
+pci_sys->num_devices++;
}
}
@@ -441,11 +450,8 @@ static const struct pci_system_methods hurd_pci_methods = {
_pci_hidden int
pci_system_hurd_create(void)
{
-struct pci_device_private *device;
int err;
struct pci_system_hurd *pci_sys_hurd;
-size_t ndevs;
-mach_port_t pci_server_port;
/* If we can open pci cfg io ports on hurd,
* we are the arbiter, therefore try x86 method first */
@@ -462,35 +468,14 @@ pci_system_hurd_create(void)
pci_sys->methods = &hurd_pci_methods;
-pci_server_port = file_name_lookup(_SERVERS_BUS_PCI, 0, 0);
-if (!pci_server_port) {
-/* Fall back to x86 access method */
-return pci_system_x86_create();
-}
-
-/* The server gives us the number of available devices for us */
-err = pci_get_ndevs (pci_server_port, &ndevs);
+pci_sys->num_devices = 0;
+err = enum_devices(_SERVERS_BUS_PCI, -1, -1, -1, -1, LEVEL_DOMAIN);
if (err) {
-mach_port_deallocate (mach_task_self (), pci_server_port);
-/* Fall back to x86 access method */
-return pci_system_x86_create();
+ /* There was an error, but we don't know which devices have been
+ * initialized correctly, so call cleanup to free whatever is allocated
*/
+ pci_system_clea
[PATCH] libpciaccess: avoid using pci_get_ndevs()
Hello Hurd, In order to deprecate pci_get_ndevs(), I think we only need to ensure neither pciaccess nor pciutil call it. I wrote this patch for libpciaccess which removes the call, I'm sending it here for your comments, and will send it to upstream if you think it's OK.
Re: [PATCH] Hurd: avoid using the deprecated RPC pci_get_ndevs()
Hello,
Joan Lledó via Bug reports for the GNU Hurd, le dim. 03 nov. 2019 11:00:56
+0100, a ecrit:
> +devices = realloc(pci_sys->devices, (pci_sys->num_devices + 1)
> + * sizeof(struct pci_device_private));
Yes, this looks like a simpler approach. Sure realloc() has some cost,
but that's not much compared to the RPCs etc. and we do that at
initialization only.
> +if (!devices)
> +return ENOMEM;
That makes me realize: we need to closedir(dir).
> if (err) {
> -mach_port_deallocate (mach_task_self (), pci_server_port);
> -/* Fall back to x86 access method */
> -return pci_system_x86_create();
> + /* There was an error, but we don't know which devices have been
> + * initialized correctly, so call cleanup to free whatever is
> allocated */
> + pci_system_cleanup();
> + return err;
We also need to call x86_disable_io();
Also, don't wee need to keep a way to fallback to the x86 access method?
Otherwise how does the pciarbiter manage to trigger calling the x86
access method?
Samuel
Re: [PATCH 3/4] pci-arbiter: Fix a -Wstringop-truncation warning
Hello, Joan Lledó via Bug reports for the GNU Hurd, le dim. 03 nov. 2019 10:38:28 +0100, a ecrit: > * pci-arbiter/pcifs.c: > * create_dir_entry: > Limit to NAME_SIZE-1 when calling strncpy(). > Finish entry->name with '\0'. > * create_fs_tree: > memset() to 0 the directory entry. > Limit to NAME_SIZE-1 all calls to >snprintf() and strncpy(). Applied, thanks! > @@ -206,7 +208,7 @@ create_fs_tree (struct pcifs * fs) > e_stat = list->stat; > e_stat.st_mode &= ~S_IROOT; /* Remove the root mode */ > memset (entry_name, 0, NAME_SIZE); > - snprintf (entry_name, NAME_SIZE, "%04x", device->domain); > + snprintf (entry_name, NAME_SIZE - 1, "%04x", device->domain); Perhaps replace the whole memset with just setting entry_name[NAME_SIZE-1] = 0 ? and ditto below. Samuel
Re: [PATCH 3/4] pci-arbiter: Fix a -Wstringop-truncation warning
On 3 Nov 2019, at 16:20, Samuel Thibault wrote: > > Hello, > > Joan Lledó via Bug reports for the GNU Hurd, le dim. 03 nov. 2019 10:38:28 > +0100, a ecrit: >> * pci-arbiter/pcifs.c: >>* create_dir_entry: >> Limit to NAME_SIZE-1 when calling strncpy(). >> Finish entry->name with '\0'. >>* create_fs_tree: >> memset() to 0 the directory entry. >> Limit to NAME_SIZE-1 all calls to >> snprintf() and strncpy(). > > Applied, thanks! > >> @@ -206,7 +208,7 @@ create_fs_tree (struct pcifs * fs) >>e_stat = list->stat; >>e_stat.st_mode &= ~S_IROOT; /* Remove the root mode */ >>memset (entry_name, 0, NAME_SIZE); >> - snprintf (entry_name, NAME_SIZE, "%04x", device->domain); >> + snprintf (entry_name, NAME_SIZE - 1, "%04x", device->domain); > > Perhaps replace the whole memset with just setting > entry_name[NAME_SIZE-1] = 0 > ? and ditto below. snprintf guarantees NUL termination, so this now over-truncates. James
Re: [PATCH 3/4] pci-arbiter: Fix a -Wstringop-truncation warning
James Clarke, le dim. 03 nov. 2019 16:38:50 +, a ecrit: > On 3 Nov 2019, at 16:20, Samuel Thibault wrote: > > Joan Lledó via Bug reports for the GNU Hurd, le dim. 03 nov. 2019 > > 10:38:28 +0100, a ecrit: > >> @@ -206,7 +208,7 @@ create_fs_tree (struct pcifs * fs) > >> e_stat = list->stat; > >> e_stat.st_mode &= ~S_IROOT; /* Remove the root mode */ > >> memset (entry_name, 0, NAME_SIZE); > >> -snprintf (entry_name, NAME_SIZE, "%04x", device->domain); > >> +snprintf (entry_name, NAME_SIZE - 1, "%04x", device->domain); > > > > Perhaps replace the whole memset with just setting > > entry_name[NAME_SIZE-1] = 0 > > ? and ditto below. > > snprintf guarantees NUL termination, so this now over-truncates. Ah, indeed. I reverted the snprintf ones. The memset still seems spurious to me, isn't it? Samuel
Re: [PATCH 1/4] pci-arbiter: Remove embedded pciaccess code
Joan Lledó via Bug reports for the GNU Hurd, le dim. 03 nov. 2019 10:38:25 +0100, a ecrit: > This patch removes all embedded pciaccess code from the arbiter > and instead uses the external pciaccess library. After applying the patch, my hurd box is stuck at Configuring network interfaces... I have libpciaccess0:hurd-i386 0.14-1+hurd.2 I guess the pci-arbiter is here not managing to revert to x86 access method. We have to do something about that. Samuel
Re: [PATCH 1/4] pci-arbiter: Remove embedded pciaccess code
Hi, El 3/11/19 a les 17:56, Samuel Thibault ha escrit: > After applying the patch, my hurd box is stuck at > > Configuring network interfaces... Could it be because GNU Mach is restricting the access to io ports to one process, and then when netdde tries to access, it can't b/c the arbiter arrived before? > I have libpciaccess0:hurd-i386 0.14-1+hurd.2 Does that version include Damien patches? Otherwise the library will ignore the arbiter and try to access the ports directly.
Re: [PATCH 1/4] pci-arbiter: Remove embedded pciaccess code
Joan Lledó, le dim. 03 nov. 2019 18:32:31 +0100, a ecrit: > El 3/11/19 a les 17:56, Samuel Thibault ha escrit: > > After applying the patch, my hurd box is stuck at > > > > Configuring network interfaces... > > Could it be because GNU Mach is restricting the access to io ports to > one process, and then when netdde tries to access, it can't b/c the > arbiter arrived before? netdde uses libpciaccess, which is supposed to use the pci arbiter. > > I have libpciaccess0:hurd-i386 0.14-1+hurd.2 > > Does that version include Damien patches? Yes, see http://ftp.ports.debian.org/debian-ports/pool-hurd-i386/main/libp/libpciaccess/libpciaccess_0.14-1+hurd.2.diff.gz the 99-pci-arbiter* patches. It only reverts to x86 ports if it can't connect to the arbiter. And with the patch you have sent today it would not, and thus the arbiter will only try to connect to itself. Samuel
Re: [PATCH] Hurd: avoid using the deprecated RPC pci_get_ndevs()
Hi,
El 3/11/19 a les 17:10, Samuel Thibault ha escrit:
>> +if (!devices)
>> +return ENOMEM;
>
> That makes me realize: we need to closedir(dir).
What do you mean?
>> if (err) {
>> -mach_port_deallocate (mach_task_self (), pci_server_port);
>> -/* Fall back to x86 access method */
>> -return pci_system_x86_create();
>> + /* There was an error, but we don't know which devices have been
>> + * initialized correctly, so call cleanup to free whatever is
>> allocated */
>> + pci_system_cleanup();
>> + return err;
>
> We also need to call x86_disable_io();
>
> Also, don't wee need to keep a way to fallback to the x86 access method?
> Otherwise how does the pciarbiter manage to trigger calling the x86
> access method?
I removed that code b/c I thought it was always failing, maybe I'm
missing something, let me explain. Take a look at hurd_pci.c:452 [1]
/* If we can open pci cfg io ports on hurd,
* we are the arbiter, therefore try x86 method first */
err = pci_system_x86_create();
if (!err)
return 0;
So, if !err, we are the arbiter and the function returns here, else, we
are a function trying to connect to the arbiter, no failback is
possible, since calling pci_system_x86_create() again will fail again.
On the other hand, if there's not arbiter at all and we want all
requests to fail back to the x86 method, then the call at line 452 is
enough, no need to run the rest of the function which tries to reach the
arbiter to end up calling pci_system_x86_create() again and failing again.
> We also need to call x86_disable_io();
I don't think so, b/c I think if the execution reached this point is
because we are not the arbiter, and GNU Mach don't allow us to access
the ports.
---
[1]
https://gitlab.freedesktop.org/xorg/lib/libpciaccess/blob/master/src/hurd_pci.c#L452
Re: [PATCH] Hurd: avoid using the deprecated RPC pci_get_ndevs()
Joan Lledó via Bug reports for the GNU Hurd, le dim. 03 nov. 2019 18:49:08 +0100, a ecrit: > El 3/11/19 a les 17:10, Samuel Thibault ha escrit: > >> +if (!devices) > >> +return ENOMEM; > > > > That makes me realize: we need to closedir(dir). > > What do you mean? The function currently calls opendir(), but never calls the corresponding closedir(), so it leaks memory. > Take a look at hurd_pci.c:452 [1] > > /* If we can open pci cfg io ports on hurd, > * we are the arbiter, therefore try x86 method first */ > err = pci_system_x86_create(); Uh, that is not what we have in the current Debian package. Somehow that version didn't get submitted here, only upstream. That is why there was a misunderstanding. That being said, how are we sure that the pci arbiter gets to run before netdde? Otherwise netdde will manage to open the PCI ports. I was thinking we wanted to aim for the converse: instead of making the first process to manage to open PCI ports, make all processes try to open the pci arbiter, and let the pci arbiter notice that it is connecting to itself, and then revert to the x86 method. Samuel
Re: [PATCH] Hurd: avoid using the deprecated RPC pci_get_ndevs()
Samuel Thibault, le dim. 03 nov. 2019 18:58:57 +0100, a ecrit: > let the pci arbiter notice that it is connecting to itself, Except that this is not an easy task, since the translator is starting and doesn't process messages yet. A hackish way would be to have pci-arbiter define a variable that the hurd driver of libpciaccess would weak-reference (e.g. checking for strcmp(netfs_server_name, "pci-arbiter") ), to determine whether the process using libpciaccess is a pci arbiter (and should thus try x86) or not. Samuel
Re: [PATCH] Hurd: avoid using the deprecated RPC pci_get_ndevs()
Samuel Thibault, le dim. 03 nov. 2019 18:58:57 +0100, a ecrit:
> Joan Lledó via Bug reports for the GNU Hurd, le dim. 03 nov. 2019 18:49:08
> +0100, a ecrit:
> > Take a look at hurd_pci.c:452 [1]
> >
> > /* If we can open pci cfg io ports on hurd,
> > * we are the arbiter, therefore try x86 method first */
> > err = pci_system_x86_create();
>
> Uh, that is not what we have in the current Debian package. Somehow that
> version didn't get submitted here, only upstream. That is why there was
> a misunderstanding.
Ok, with that change, the pci-arbiter cleanup change does let hurd boot.
I'm however seeing lspci successfully using the x86 method, and thus not
use the pci arbiter, that's not what we want :)
(it seems the PCI port exclusion does not actually work, is the PCI
arbiter really keeping them reserved in GNU Mach?)
So I guess that it boots just because libpciaccess succeeds to use the
x86 method, whether the pci arbiter is there or not.
I tried the hack I mentioned previously (attached), to properly detect
that we are the PCI arbiter. Now my system boots, but that's because
netdde crashes instead of just hanging. It looks like it does not manage
to use the PCI arbiter.
Samuel
Use netfs_server_name to detect whether we are the PCI arbiter.
Index: libpciaccess-0.14/src/hurd_pci.c
===
--- libpciaccess-0.14.orig/src/hurd_pci.c
+++ libpciaccess-0.14/src/hurd_pci.c
@@ -447,11 +447,14 @@ pci_system_hurd_create(void)
size_t ndevs;
mach_port_t pci_server_port;
-/* If we can open pci cfg io ports on hurd,
- * we are the arbiter, therefore try x86 method first */
-err = pci_system_x86_create();
-if (!err)
-return 0;
+extern char *netfs_server_name;
+#pragma weak netfs_server_name
+if (netfs_server_name && !strcmp(netfs_server_name, "pci-arbiter")) {
+ /* We are a PCI arbiter, try the x86 way */
+ err = pci_system_x86_create();
+ if (!err)
+ return 0;
+}
pci_sys_hurd = calloc(1, sizeof(struct pci_system_hurd));
if (pci_sys_hurd == NULL) {
Re: [PATCH] Hurd: avoid using the deprecated RPC pci_get_ndevs()
Samuel Thibault, le dim. 03 nov. 2019 20:07:46 +0100, a ecrit:
> I tried the hack I mentioned previously (attached), to properly detect
> that we are the PCI arbiter. Now my system boots, but that's because
> netdde crashes instead of just hanging. It looks like it does not manage
> to use the PCI arbiter.
My bad, my patch wasn't enough, here is a fixed version. I'm now getting
a netdde SIGILL crash inside the atp_init function.
Samuel
Use netfs_server_name to detect whether we are the PCI arbiter.
Index: libpciaccess-0.14/src/hurd_pci.c
===
--- libpciaccess-0.14.orig/src/hurd_pci.c
+++ libpciaccess-0.14/src/hurd_pci.c
@@ -447,11 +447,14 @@ pci_system_hurd_create(void)
size_t ndevs;
mach_port_t pci_server_port;
-/* If we can open pci cfg io ports on hurd,
- * we are the arbiter, therefore try x86 method first */
-err = pci_system_x86_create();
-if (!err)
-return 0;
+extern char *netfs_server_name;
+#pragma weak netfs_server_name
+if (&netfs_server_name && netfs_server_name && !strcmp(netfs_server_name,
"pci-arbiter")) {
+ /* We are a PCI arbiter, try the x86 way */
+ err = pci_system_x86_create();
+ if (!err)
+ return 0;
+}
pci_sys_hurd = calloc(1, sizeof(struct pci_system_hurd));
if (pci_sys_hurd == NULL) {
Re: [PATCH] Hurd: avoid using the deprecated RPC pci_get_ndevs()
Samuel Thibault, le dim. 03 nov. 2019 20:42:27 +0100, a ecrit: > Samuel Thibault, le dim. 03 nov. 2019 20:07:46 +0100, a ecrit: > > I tried the hack I mentioned previously (attached), to properly detect > > that we are the PCI arbiter. Now my system boots, but that's because > > netdde crashes instead of just hanging. It looks like it does not manage > > to use the PCI arbiter. > > My bad, my patch wasn't enough, here is a fixed version. I'm now getting > a netdde SIGILL crash inside the atp_init function. Ok, atp is a dumb driver on parallel ports, we don't want that anyway, I'll upload a fixed netdde. And then I can upload the updated libpciaccess and commit the pci-arbiter cleanup. Samuel
Re: [PATCH] pci-arbiter: Remove embedded pciaccess code
Hello, Uploaded fixed netdde, pciutils, libpciaccess, and commited this pci-arbiter cleanup! Samuel
