Re: PCI arbiter crash on last qemu image

[Damien Zammit]

On 22/8/20 8:38 pm, Joan Lledó wrote:
> 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.

OK, so we need to add this patch upstream:
---
 src/x86_pci.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/src/x86_pci.c b/src/x86_pci.c
index ac598b1..1614729 100644
--- a/src/x86_pci.c
+++ b/src/x86_pci.c
@@ -814,10 +814,6 @@ pci_system_x86_scan_bus (uint8_t bus)
 
             d->base.device_class = reg >> 8;
 
-            err = pci_device_x86_probe (&d->base);
-            if (err)
-                return err;
-
             pci_sys->devices = devices;
             pci_sys->num_devices++;
 
-- 
2.25.1

 
> 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.

And so my original patch for this part was correct:
---
 src/x86_pci.c | 13 +++----------
 1 file changed, 3 insertions(+), 10 deletions(-)

diff --git a/src/x86_pci.c b/src/x86_pci.c
index 0d9f2e7..ac598b1 100644
--- a/src/x86_pci.c
+++ b/src/x86_pci.c
@@ -630,9 +630,6 @@ pci_device_x86_region_probe (struct pci_device *dev, int 
reg_num)
             if (err)
                 return err;
         }
-
-        /* Clear the map pointer */
-        dev->regions[reg_num].memory = 0;
     }
     else if (dev->regions[reg_num].size > 0)
     {
@@ -649,15 +646,11 @@ pci_device_x86_region_probe (struct pci_device *dev, int 
reg_num)
             if (err)
                 return err;
         }
-
-        /* Map the region in our space */
-       if ( (err = map_dev_mem(&dev->regions[reg_num].memory,
-                                dev->regions[reg_num].base_addr,
-                                dev->regions[reg_num].size,
-                                1)) )
-            return err;
     }
 
+    /* Clear the map pointer */
+    dev->regions[reg_num].memory = 0;
+
     return 0;
 }
 
-- 
2.25.1


> 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.

I see now.  Sorry, I think I added the probe() in create()!

> Would you like to fix the backend and I fix the arbiter?
Can you confirm I have it correct this time?  If so I will alter my merge 
request upstream.

Thanks,
Damien