Re: [PATCH] - hurd pci-arbiter: remove embedded pciaccess code

2019-10-27 Thread Joan Lledó via Bug reports for the GNU Hurd 
Hello Hurd,

I made some changes to this patch:

1- I added a call to pci_system_cleanup(); in the shutdown RPC,
   so libpciaccess is shutdown correctly.

2- As now we're using libpciaccess functions to read/write
   from netfs_attempt_read/write, and these two libpciaccess
   functions have different prototypes, we cannot use one single
   pointer to function like we did until now, so I added some
   changes to use always the proper prototype.
   I hope it's not too much tricky.

3- I solved a warning Fix a -Wstringop-truncation warning.

4- I wrote a  changelog for the whole patch including Damien's work,
   except the change in S_pci_get_dev_rom() in pci-ops.c.
   I don't know what's this change for, but if we're always setting base_addr
   to 0, then probably we don't need that field anymore and can remove it from
   struct pci_xrom_bar.

Re: [PATCH] - hurd pci-arbiter: remove embedded pciaccess code

2018-12-05 Thread Thomas Schwinge 
Hi!

On Sun, 11 Nov 2018 13:14:59 +1100, Damien Zammit  wrote:
> On 11/11/18 13:05, Samuel Thibault wrote:
> > Do you know how your copyright assignment is going, so I can commit it?
> 
> Haven't heard anything since I sent through my request.

This has yesterday been reported as completed in email with "Subject:
[gnu.org #1332514] Damien Zammit HURD MACH LIBC GRUB".

Thanks!


Grüße
 Thomas

Re: [PATCH] - hurd pci-arbiter: remove embedded pciaccess code

2018-11-10 Thread Damien Zammit 
On 11/11/18 13:05, Samuel Thibault wrote:
> Do you know how your copyright assignment is going, so I can commit it?

Haven't heard anything since I sent through my request.

Damien

Re: [PATCH] - hurd pci-arbiter: remove embedded pciaccess code

2018-11-10 Thread Samuel Thibault 
Hello,

Thanks for the revised patch :)

Do you know how your copyright assignment is going, so I can commit it?

Samuel

Re: [PATCH] - hurd pci-arbiter: remove embedded pciaccess code

2018-11-10 Thread Damien Zammit 
On 10/11/18 20:42, Samuel Thibault wrote:
> Damien Zammit, le sam. 10 nov. 2018 18:58:35 +1100, a ecrit:
>> +CPPFLAGS += -imacros $(srcdir)/config.h
> 
> Why is this needed?

I have now removed config.h and this line.

> So it seems that libpciaccess doesn't provide an interface for this.
> Please leave as TODO in config_block_op to just return what could be
> read once op() returns EIO, so that we do get the short reads
> implemented by just leaving it up to libpciaccess. Limiting to 256 bytes
> will be fine for now.

I added this in the TODO file

>> @@ -85,7 +85,7 @@ S_pci_conf_read (struct protid * master, int reg, char 
>> **data,
>> -
>> +  
> Please avoid such unneeded hunk :)

Fixed

>> @@ -266,7 +276,7 @@ create_fs_tree (struct pcifs * fs, struct pci_system * 
>> pci_sys)
>> -  e_stat.st_size = device->config_size;
>> +  e_stat.st_size = PCI_CONFIG_SIZE; // FIXME: Hardcoded

> Mmm. This will also be a TODO. I'd say on the long run libpciaccess
> should really provide a way to get the config size instead of having to
> just get EIO.

Yes, it would be nice if libpciaccess provided the size of the config
space.  I have just left this as is for now.

See attached for revised patch.

Damien
>From 1fadfd21f52bfec127d88f9c60dea9f7d99e47b0 Mon Sep 17 00:00:00 2001
From: Damien Zammit 
Date: Sun, 4 Nov 2018 21:33:50 -0500
Subject: [PATCH] pci-arbiter: Remove embedded pciaccess code

This patch removes all embedded pciaccess code from the arbiter
and instead uses the external pciaccess library.

There is another patch for pciaccess to add region and rom
support, as well as the hurdish access method, but the arbiter
will still work with this patch until the others are upstreamed.
---
 pci-arbiter/Makefile |  12 +-
 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, 72 insertions(+), 1173 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 3fc3010b..e42ab72e 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 \
-		  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)))
+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
-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

Re: [PATCH] - hurd pci-arbiter: remove embedded pciaccess code

2018-11-10 Thread Samuel Thibault 
Hello,

Thanks for this :)

Damien Zammit, le sam. 10 nov. 2018 18:58:35 +1100, a ecrit:
> @@ -35,6 +35,8 @@ include ../Makeconf
>  
>  CFLAGS += -I$(PORTDIR)/include
>  
> +CPPFLAGS += -imacros $(srcdir)/config.h

Why is this needed?

(that's the reason why you have to add a config.h file, but its content
is completely useless).

> @@ -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 (offset > PCI_CONFIG_SIZE)
>  return EINVAL;

> --- a/pci-arbiter/pcifs.h
> +++ b/pci-arbiter/pcifs.h
> @@ -27,8 +27,11 @@
> +// FIXME: Hardcoded PCI config size
> +#define PCI_CONFIG_SIZE 256

So it seems that libpciaccess doesn't provide an interface for this.
Please leave as TODO in config_block_op to just return what could be
read once op() returns EIO, so that we do get the short reads
implemented by just leaving it up to libpciaccess. Limiting to 256 bytes
will be fine for now.

> @@ -85,7 +85,7 @@ S_pci_conf_read (struct protid * master, int reg, char 
> **data,
>error_t err;
>pthread_mutex_t *lock;
>struct pcifs_dirent *e;
> -
> +  
>if (!master)
>  return EOPNOTSUPP;
>  

Please avoid such unneeded hunk :)

> @@ -266,7 +276,7 @@ create_fs_tree (struct pcifs * fs, struct pci_system * 
> pci_sys)
>e_stat = func_parent->stat;
>e_stat.st_mode &= ~(S_IFDIR | S_IXUSR | S_IXGRP);
>e_stat.st_mode |= S_IFREG | S_IWUSR | S_IWGRP;
> -  e_stat.st_size = device->config_size;
> +  e_stat.st_size = PCI_CONFIG_SIZE; // FIXME: Hardcoded

Mmm. This will also be a TODO. I'd say on the long run libpciaccess
should really provide a way to get the config size instead of having to
just get EIO.

Samuel

[PATCH] - hurd pci-arbiter: remove embedded pciaccess code

2018-11-10 Thread Damien Zammit 
Hi all,

This patch (see attached) removes all embedded pciaccess code from the
arbiter, and adds config.h which is currently missing from master.

NB: I have another patch for libpciaccess to add the hurd method and add
Joan's tweaks to the raw x86 access method, but is not strictly
essential to upstream that to have the arbiter in a working state.

The arbiter works ok with just this patch on top of master since
libpciaccess uses the raw x86 method for hurd currently.

However, to get regionX access and rom access, as well as the "real"
hurdish pci access method into pciaccess, I will need to upstream the
pciaccess stuff.

Also, to make everything work smoothly in the future, I have a patch for
gnumach to restrict the number of processes that can simultaneously
access pci io cfg range of ports down to 1 as per discussion on the xorg
mailing list.  I will make a new post with the gnumach patch.

Cheers,
Damien
>From 72f96ca9e17e838443cbbf64331d6acdd242ec29 Mon Sep 17 00:00:00 2001
From: Damien Zammit 
Date: Sun, 4 Nov 2018 21:33:50 -0500
Subject: [PATCH] pci-arbiter: Remove embedded pciaccess code

This patch removes all embedded pciaccess code from the arbiter
and instead uses the external pciaccess library.

There is another patch for pciaccess to add region and rom
support, as well as the hurdish access method, but the arbiter
will still work with this patch until the others are upstreamed.
---
 pci-arbiter/Makefile |  14 +-
 pci-arbiter/TODO |   7 -
 pci-arbiter/config.h |   4 +
 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|  12 +-
 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 --
 14 files changed, 76 insertions(+), 1177 deletions(-)
 create mode 100644 pci-arbiter/config.h
 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 3fc3010b..4e2b8a76 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 \
-		  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)))
+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
-LDLIBS = -lpthread
+LDLIBS = -lpthread -lpciaccess
 
 target = pci-arbiter
 
@@ -35,6 +35,8 @@ include ../Makeconf
 
 CFLAGS += -I$(PORTDIR)/include
 
+CPPFLAGS += -imacros $(srcdir)/config.h
+
 pci-MIGSFLAGS = -imacros $(srcdir)/mig-mutate.h
 
 # cpp doesn't automatically make dependencies for -imacros dependencies. argh.
diff --git a/pci-arbiter/TODO b/pci-arbiter/TODO
index 22ae72a8..972a36e4 100644
--- a/pci-arbiter/TODO
+++ b/pci-arbiter/TODO
@@ -5,11 +5,4 @@
 
 - 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.
-
-  At least one difference with libpciaccess is the refresh operation. Perhaps
-  we'd need to extend libpciaccess to fix that.
-
   BTW we could also support libpci.
diff --git a/pci-arbiter/config.h b/pci-arbiter/config.h
new file mode 100644
index ..8eb20cd1
--- /dev/null
+++ b/pci-arbiter/config.h
@@ -0,0 +1,4 @@
+#define __KERNEL__  1
+#undef  __SMP__
+
+#define _HURD_  1
diff --git a/pci-arbiter/func_files.c b/pci-arbiter/func_files.c
index 7df94d2f..3e97a732 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