On 11/14/14 14:31, Gabriel Somlo wrote:
> On Fri, Nov 14, 2014 at 10:26:55AM +0100, Laszlo Ersek wrote:
>> One question.
>>
>> On 11/14/14 04:01, Gabriel L. Somlo wrote:
>>> +
>>> + Status = EFI_SUCCESS;
>>> +
>>> + if (PciHdr->Device.InterruptPin != 0) {
>>
>> so here InterruptPin >= 1
>
> Same as in SeaBIOS pci_bios_init_device() under "/* map the interrupt */":
>
> if (pin != 0)
> pci_config_writeb(bdf, PCI_INTERRUPT_LINE, pci_slot_get_irq(pci, pin));
>
>
>>> +
>>> + DevPathNode = DevicePathFromHandle (Handle);
>>> + ASSERT (DevPathNode != NULL);
>>> +
>>> + //
>>> + // Compute index into PciHostIrqs[] table by walking
>>> + // the device path and adding up all device numbers
>>> + //
>>> + Status = EFI_NOT_FOUND;
>>> + Idx = PciHdr->Device.InterruptPin - 1;
>>
>> Hence Idx >= 0 (in the mathematical sense -- I'm just saying there's no
>> unsigned underflow)
>
> So here it gets a bit fuzzy, because piix_pci_slot_get_irq uses
> "slot_addend", and mch_pci_slot_get_irq uses "pin_addend", so I had to
> massage them both a bit before I could factor out what's common and
> what differs :) But let's roll with piix_pci_slot_get_irq():
>
> static int piix_pci_slot_get_irq(struct pci_device *pci, int pin)
> {
> int slot_addend = 0;
>
> while (pci->parent != NULL) {
> slot_addend += pci_bdf_to_dev(pci->bdf);
> pci = pci->parent;
> }
> slot_addend += pci_bdf_to_dev(pci->bdf) - 1;
> return pci_irqs[(pin - 1 + slot_addend) & 3];
> }
>
>
>>> + RootSlot = 0; // Suppress gcc uninitialized use warning
>>> + while (!IsDevicePathEnd (DevPathNode)) {
>>> + if (DevicePathType (DevPathNode) == HARDWARE_DEVICE_PATH &&
>>> + DevicePathSubType (DevPathNode) == HW_PCI_DP) {
>>> +
>>> + Idx += ((PCI_DEVICE_PATH *)DevPathNode)->Device;
>>
>> Assume the loop body is entered precisely once, and that the addend here
>> on the right side is zero.
>
> pin == 1.
> slot_addend == 0 after the while loop (since parent == NULL).
> pci_bdf_to_dev(pci->bdf) might be 0, so slot_addend might become -1.
>
> We then return pci_irqs[( 1 - 1 + (-1) ) & 3].
>
>> That means Idx may equal zero after this increment is done.
>>
>>> +
>>> + //
>>> + // Unlike SeaBIOS, which starts climbing from the leaf device
>>> + // up toward the root, we traverse the device path starting at
>>> + // the root moving toward the leaf node.
>>> + // The slot number of the top-level parent bridge is needed for
>>> + // Q35 cases with more than 24 slots on the root bus.
>>> + //
>>> + if (Status != EFI_SUCCESS) {
>>> + RootSlot = ((PCI_DEVICE_PATH *)DevPathNode)->Device;
>>> + Status = EFI_SUCCESS;
>>> + }
>>> + }
>>> +
>>> + DevPathNode = NextDevicePathNode (DevPathNode);
>>> + }
>>> + if (EFI_ERROR (Status)) {
>>> + return Status;
>>> + }
>>
>> since we ran the loop body more than zero times (in this case, exactly
>> once), we don't return here.
>>
>>> +
>>> + //
>>> + // Final PciHostIrqs[] index calculation depends on the platform
>>> + // and should match SeaBIOS src/fw/pciinit.c *_pci_slot_get_irq()
>>> + //
>>> + switch (mHostBridgeDevId) {
>>> + case INTEL_82441_DEVICE_ID:
>>> + Idx -= 1;
>>
>> whoops, Idx becomes MAX_UINTN.
>
> Which is the same as (-1) above. We then go %= 4 (a.k.a &= 3), and
> pull an irq value from the table :)
>
>> Now where is my thought process wrong? You might want to add an assert
>> somewhere.
>
> I don't think your thought process is wrong. I think I'm being exactly
> as right (or as wrong) as SeaBIOS under all cases, which is what I was
> aiming for :)
Sounds fair enough.
Reviewed-by: Laszlo Ersek <[email protected]>
Thanks!
Laszlo
>
>> *Awesome* job, otherwise. Thank you very much!
>
> And thank *you* (again) for all the help navigating the edk2/OvmfPkg
> environment !
>
> --Gabriel
>
------------------------------------------------------------------------------
Comprehensive Server Monitoring with Site24x7.
Monitor 10 servers for $9/Month.
Get alerted through email, SMS, voice calls or mobile push notifications.
Take corrective actions from your mobile device.
http://pubads.g.doubleclick.net/gampad/clk?id=154624111&iu=/4140/ostg.clktrk
_______________________________________________
edk2-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/edk2-devel
