Patchwork pci: add standard bridge device

login
register
mail settings
Submitter Gerd Hoffmann
Date Sept. 8, 2011, 9:43 a.m.
Message ID <4E688E32.7060207@redhat.com>
Download mbox | patch
Permalink /patch/113879/
State New
Headers show

Comments

Gerd Hoffmann - Sept. 8, 2011, 9:43 a.m.
Hi,

> I modify the code like this, and the PCI_INTERRUPT_LINE register is set, and I can bind
> it to uio_pci_generic:

> --- a/src/pciinit.c
> +++ b/src/pciinit.c
> @@ -575,6 +575,8 @@ static int pci_bios_init_root_regions(u32 start, u32 end)

>       pci_bios_init_bus_bases(&busses[0]);
> -    pci_bios_map_device_in_bus(0 /* host bus */);
> +    for (bus = 0; bus<= MaxPCIBus; bus++) {
> +        pci_bios_map_device_in_bus(bus /* host bus */);

No.  pci_bios_map_device_in_bus goes down recursively when it finds a 
bridge, so it should cover all devices already.

> -    pci_bios_init_device_in_bus(0 /* host bus */);
> +        pci_bios_init_device_in_bus(bus /* host bus */);
> +    }

That is correct.  Can be done easier though by just not limiting device 
initialization to a specific bus like in the attached patch.  Does that 
one work for you?

cheers,
   Gerd
Wen Congyang - Sept. 8, 2011, 9:58 a.m.
At 09/08/2011 05:43 PM, Gerd Hoffmann Write:
>   Hi,
> 
>> I modify the code like this, and the PCI_INTERRUPT_LINE register is
>> set, and I can bind
>> it to uio_pci_generic:
> 
>> --- a/src/pciinit.c
>> +++ b/src/pciinit.c
>> @@ -575,6 +575,8 @@ static int pci_bios_init_root_regions(u32 start,
>> u32 end)
> 
>>       pci_bios_init_bus_bases(&busses[0]);
>> -    pci_bios_map_device_in_bus(0 /* host bus */);
>> +    for (bus = 0; bus<= MaxPCIBus; bus++) {
>> +        pci_bios_map_device_in_bus(bus /* host bus */);
> 
> No.  pci_bios_map_device_in_bus goes down recursively when it finds a
> bridge, so it should cover all devices already.

Yes, pci_bios_map_device() goes down recursively.

> 
>> -    pci_bios_init_device_in_bus(0 /* host bus */);
>> +        pci_bios_init_device_in_bus(bus /* host bus */);
>> +    }
> 
> That is correct.  Can be done easier though by just not limiting device
> initialization to a specific bus like in the attached patch.  Does that
> one work for you?

I test it, and it works for me.

Thanks
Wen Congyang

> 
> cheers,
>   Gerd
Michael S. Tsirkin - Sept. 8, 2011, 10:42 a.m.
On Thu, Sep 08, 2011 at 05:58:12PM +0800, Wen Congyang wrote:
> At 09/08/2011 05:43 PM, Gerd Hoffmann Write:
> >   Hi,
> > 
> >> I modify the code like this, and the PCI_INTERRUPT_LINE register is
> >> set, and I can bind
> >> it to uio_pci_generic:
> > 
> >> --- a/src/pciinit.c
> >> +++ b/src/pciinit.c
> >> @@ -575,6 +575,8 @@ static int pci_bios_init_root_regions(u32 start,
> >> u32 end)
> > 
> >>       pci_bios_init_bus_bases(&busses[0]);
> >> -    pci_bios_map_device_in_bus(0 /* host bus */);
> >> +    for (bus = 0; bus<= MaxPCIBus; bus++) {
> >> +        pci_bios_map_device_in_bus(bus /* host bus */);
> > 
> > No.  pci_bios_map_device_in_bus goes down recursively when it finds a
> > bridge, so it should cover all devices already.
> 
> Yes, pci_bios_map_device() goes down recursively.

The value seems to be wrong though, I think.
It seems to simply use the interrupt pin as array index.
Instead, each bridge should interrupts as follows:

/* Mapping mandated by PCI-to-PCI Bridge architecture specification,
 * revision 1.2 */
/* Table 9-1: Interrupt Binding for Devices Behind a Bridge */
static int pci_bridge_dev_map_irq_fn(PCIDevice *dev, int irq_num)
{
    return (irq_num + PCI_SLOT(dev->devfn)) % PCI_NUM_PINS;
}

until we get to the host bridge.


> > 
> >> -    pci_bios_init_device_in_bus(0 /* host bus */);
> >> +        pci_bios_init_device_in_bus(bus /* host bus */);
> >> +    }
> > 
> > That is correct.  Can be done easier though by just not limiting device
> > initialization to a specific bus like in the attached patch.  Does that
> > one work for you?
> 
> I test it, and it works for me.
> 
> Thanks
> Wen Congyang
> 
> > 
> > cheers,
> >   Gerd
Wen Congyang - Sept. 8, 2011, 11:03 a.m.
At 09/08/2011 06:42 PM, Michael S. Tsirkin Write:
> On Thu, Sep 08, 2011 at 05:58:12PM +0800, Wen Congyang wrote:
>> At 09/08/2011 05:43 PM, Gerd Hoffmann Write:
>>>   Hi,
>>>
>>>> I modify the code like this, and the PCI_INTERRUPT_LINE register is
>>>> set, and I can bind
>>>> it to uio_pci_generic:
>>>
>>>> --- a/src/pciinit.c
>>>> +++ b/src/pciinit.c
>>>> @@ -575,6 +575,8 @@ static int pci_bios_init_root_regions(u32 start,
>>>> u32 end)
>>>
>>>>       pci_bios_init_bus_bases(&busses[0]);
>>>> -    pci_bios_map_device_in_bus(0 /* host bus */);
>>>> +    for (bus = 0; bus<= MaxPCIBus; bus++) {
>>>> +        pci_bios_map_device_in_bus(bus /* host bus */);
>>>
>>> No.  pci_bios_map_device_in_bus goes down recursively when it finds a
>>> bridge, so it should cover all devices already.
>>
>> Yes, pci_bios_map_device() goes down recursively.
> 
> The value seems to be wrong though, I think.
> It seems to simply use the interrupt pin as array index.
> Instead, each bridge should interrupts as follows:
> 
> /* Mapping mandated by PCI-to-PCI Bridge architecture specification,
>  * revision 1.2 */
> /* Table 9-1: Interrupt Binding for Devices Behind a Bridge */
> static int pci_bridge_dev_map_irq_fn(PCIDevice *dev, int irq_num)
> {
>     return (irq_num + PCI_SLOT(dev->devfn)) % PCI_NUM_PINS;
> }
> 
> until we get to the host bridge.

I use gdb to debug, and find that this function is never called.

Thanks
Wen Congyang

> 
> 
>>>
>>>> -    pci_bios_init_device_in_bus(0 /* host bus */);
>>>> +        pci_bios_init_device_in_bus(bus /* host bus */);
>>>> +    }
>>>
>>> That is correct.  Can be done easier though by just not limiting device
>>> initialization to a specific bus like in the attached patch.  Does that
>>> one work for you?
>>
>> I test it, and it works for me.
>>
>> Thanks
>> Wen Congyang
>>
>>>
>>> cheers,
>>>   Gerd
>
Michael S. Tsirkin - Sept. 8, 2011, 11:13 a.m.
On Thu, Sep 08, 2011 at 07:03:10PM +0800, Wen Congyang wrote:
> At 09/08/2011 06:42 PM, Michael S. Tsirkin Write:
> > On Thu, Sep 08, 2011 at 05:58:12PM +0800, Wen Congyang wrote:
> >> At 09/08/2011 05:43 PM, Gerd Hoffmann Write:
> >>>   Hi,
> >>>
> >>>> I modify the code like this, and the PCI_INTERRUPT_LINE register is
> >>>> set, and I can bind
> >>>> it to uio_pci_generic:
> >>>
> >>>> --- a/src/pciinit.c
> >>>> +++ b/src/pciinit.c
> >>>> @@ -575,6 +575,8 @@ static int pci_bios_init_root_regions(u32 start,
> >>>> u32 end)
> >>>
> >>>>       pci_bios_init_bus_bases(&busses[0]);
> >>>> -    pci_bios_map_device_in_bus(0 /* host bus */);
> >>>> +    for (bus = 0; bus<= MaxPCIBus; bus++) {
> >>>> +        pci_bios_map_device_in_bus(bus /* host bus */);
> >>>
> >>> No.  pci_bios_map_device_in_bus goes down recursively when it finds a
> >>> bridge, so it should cover all devices already.
> >>
> >> Yes, pci_bios_map_device() goes down recursively.
> > 
> > The value seems to be wrong though, I think.
> > It seems to simply use the interrupt pin as array index.
> > Instead, each bridge should interrupts as follows:
> > 
> > /* Mapping mandated by PCI-to-PCI Bridge architecture specification,
> >  * revision 1.2 */
> > /* Table 9-1: Interrupt Binding for Devices Behind a Bridge */
> > static int pci_bridge_dev_map_irq_fn(PCIDevice *dev, int irq_num)
> > {
> >     return (irq_num + PCI_SLOT(dev->devfn)) % PCI_NUM_PINS;
> > }
> > 
> > until we get to the host bridge.
> 
> I use gdb to debug, and find that this function is never called.
> 
> Thanks
> Wen Congyang

No, I mean that bios must implement this logic.
You don't see it called probably because ivshmem
does not cause interrupts for you.

> > 
> > 
> >>>
> >>>> -    pci_bios_init_device_in_bus(0 /* host bus */);
> >>>> +        pci_bios_init_device_in_bus(bus /* host bus */);
> >>>> +    }
> >>>
> >>> That is correct.  Can be done easier though by just not limiting device
> >>> initialization to a specific bus like in the attached patch.  Does that
> >>> one work for you?
> >>
> >> I test it, and it works for me.
> >>
> >> Thanks
> >> Wen Congyang
> >>
> >>>
> >>> cheers,
> >>>   Gerd
> >

Patch

diff --git a/src/pciinit.c b/src/pciinit.c
index 597c8ea..676e35e 100644
--- a/src/pciinit.c
+++ b/src/pciinit.c
@@ -45,7 +45,7 @@  static struct pci_bus {
 } *busses;
 static int busses_count;
 
-static void pci_bios_init_device_in_bus(int bus);
+static void pci_bios_init_device_all(void);
 static void pci_bios_check_device_in_bus(int bus);
 static void pci_bios_init_bus_bases(struct pci_bus *bus);
 static void pci_bios_map_device_in_bus(int bus);
@@ -254,15 +254,10 @@  static void pci_bios_init_device(struct pci_device *pci)
     pci_init_device(pci_device_tbl, pci, NULL);
 }
 
-static void pci_bios_init_device_in_bus(int bus)
+static void pci_bios_init_device_all(void)
 {
     struct pci_device *pci;
     foreachpci(pci) {
-        u8 pci_bus = pci_bdf_to_bus(pci->bdf);
-        if (pci_bus < bus)
-            continue;
-        if (pci_bus > bus)
-            break;
         pci_bios_init_device(pci);
     }
 }
@@ -605,7 +600,7 @@  pci_setup(void)
     pci_bios_init_bus_bases(&busses[0]);
     pci_bios_map_device_in_bus(0 /* host bus */);
 
-    pci_bios_init_device_in_bus(0 /* host bus */);
+    pci_bios_init_device_all();
 
     struct pci_device *pci;
     foreachpci(pci) {