Re: PCI arbiter crash on last qemu image

[Joan Lledó]

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
>