Hi, > I have removed my latest patch from my upstream merge request and replaced it > with a patch that fixes the problem:
I took a look at your patch. > mappings[devp->num_mappings].flags = map_flags; > mappings[devp->num_mappings].memory = NULL; > > - if (dev->regions[region].memory == NULL) { > - err = (*pci_sys->methods->map_range)(dev, > - &mappings[devp->num_mappings]); > - } > + err = (*pci_sys->methods->map_range)(dev, &mappings[devp->num_mappings]); > > if (err == 0) { > *addr = mappings[devp->num_mappings].memory; I've spent some time studying memory management in libpciaccess and haven't found a reason to make range mappings and region mappings mutually exclusive. This completely disables range mappings in the x86 backend, since a region mapping for each region is done during the bus scan. However, I think the problem here is the x86 backend, not the common interface. If we take a look at all other backends we'll see that: 1.- Neither of them call its probe() from its create(). So it's the client who must call pci_device_probe(), it's our arbiter who should do it and it doesn't. 2.- Neither of them map memory from its probe(). So again it's the client who must call either pci_device_map_range() or pci_device_map_region(), and again it's our arbiter who is not doing it. This is due to our transition from having a libpciaccess copy of x86 backend embedded in the arbiter to use libpciaccess. When it was embedded, I modified it to probe and map everything during the bus scan, but the original code in libpciaccess[1] didn't do it. So we are not using libpciaccess properly and we modified libpciaccess to fit us instead of the other way around. It's the commit in [2] that introduced the problem. And that affected to all clients who use the x86 backend and mapped memory, though it seems we are the only ones or at least the first ones to worry. I don't think modifying the common interface of libpciaccess is the solution, b/c that would affect all clients, not only those using the x86 backend, I don't see why range and region mappings are mutually exclusive but they are and clients assume that. So what we should do instead is modify the x86 backend to behave as others and adapt the arbiter to use libpciaccess right. Would you like to fix the backend and I fix the arbiter? ----- [1] https://gitlab.freedesktop.org/xorg/lib/libpciaccess/-/blob/a167bd6474522a709ff3cbb00476c0e4309cb66f/src/x86_pci.c [2] https://gitlab.freedesktop.org/xorg/lib/libpciaccess/-/commit/95fbfeeacfd054de1037d6a10dee03b2b2cbc290 El 22/8/20 a les 8:35, Damien Zammit ha escrit: > Joan, > > On 18/8/20 6:51 am, Joan Lledó wrote: >> El 17/8/20 a les 1:51, Damien Zammit ha escrit: >>> Perhaps a better way to fix the mapping problem I encountered >>> is by removing the check for previous mappings when trying to map regions, > > I have removed my latest patch from my upstream merge request and replaced it > with a patch that fixes the problem: > > https://gitlab.freedesktop.org/xorg/lib/libpciaccess/-/merge_requests/12/commits > > Samuel, would you be able to fix the debian package of libpciaccess based on > the above? > (ie, remove one patch and add one patch). > > I now get the following with my version of pciaccess (and rumpdisk still > works with x86 method): > > root@zamhurd:~# hexdump -C -n16 /servers/bus/pci/0000/00/02/0/region0 > 00000000 45 02 14 18 00 00 00 00 83 02 00 00 00 00 00 00 |E...............| > 00000010 > root@zamhurd:~# > > Damien >