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 :)
> *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
edk2-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/edk2-devel
