Re: [PATCH] pci-arbiter: Remove embedded pciaccess code

[Joan Lledó]

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.