On Sat, Nov 15, 2014 at 12:01:18PM -0600, Scott Duplichan wrote:
> I am still unclear on philosophy of ASSERT use in EDK2. A complicating
> factor is that the choice of whether or not to disable ASSERT for release
> builds is left up to the project. For example, OvmfPkg and ArmPkg disable
> ASSERT for release builds by defining MDEPKG_NDEBUG in the .dsc file. In
> addition, ASSERT can be turned off by use of a PCD. So depending on the
> project, the type of build, and the PcdDebugPropertyMask, ASSERT may or
> may not do anything. If the ASSERT is used purely as a debug aid, then
> it doesn't matter.
>
> Thanks,
> Scott
>
> + // Compute index into PciHostIrqs[] table by walking
> + // the device path and adding up all device numbers
> + //
> + Status = EFI_NOT_FOUND;
> + RootSlot = 0;
> + Idx = PciHdr->Device.InterruptPin - 1;
> + while (!IsDevicePathEnd (DevPathNode)) {
> + if (DevicePathType (DevPathNode) == HARDWARE_DEVICE_PATH &&
> + DevicePathSubType (DevPathNode) == HW_PCI_DP) {
> +
> + Idx += ((PCI_DEVICE_PATH *)DevPathNode)->Device;
> +
> + //
> + // 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) {
> + Status = EFI_SUCCESS;
> + RootSlot = ((PCI_DEVICE_PATH *)DevPathNode)->Device;
> + }
> + }
> +
> + DevPathNode = NextDevicePathNode (DevPathNode);
> + }
> + if (EFI_ERROR (Status)) {
> + return Status;
> + }
As Laszlo pointed out, at this point we've clearly found a PCI device
with a non-zero InterruptPin. If its slot on the root PCI bus is 0,
we're in "uncharted waters" -- as Paolo mentioned, device 0 on the
root bus (the host bridge) should never be in this situation. SeaBIOS
makes this assumption implicitly, but I agree it'd be nice if we
actually checked and "did something" if the unexpected happens.
> + if (RootSlot == 0) {
> + DEBUG((
> + EFI_D_ERROR,
> + "%a: PCI host bridge (00:00.0) should have no interrupts!\n",
> + __FUNCTION__
> + ));
> + ASSERT (FALSE);
> + }
I was thinking about maybe just replacing the above with
ASSERT (RootSlot > 0);
and relying on whatever macro ASSERT is written on top of to print out
something useful.
But now that you mention it, I prefer an explicit DEBUG printout if
ASSERT can be made a NOP during build...
If we continue instead of halting when RootSlot == 0, we just pick an
irq from the table and set it, which is no better or worse than
SeaBIOS, which is what I'm using as a reference.
OTOH, if ASSERT() is turned off with the assumption that "we tested it
and never ran into that case, so we assume it *really* can't ever
happen", then maybe my "if (RootSlot == 0) " block above really should
be replaced with "ASSERT (RootSlot > 0);" and nop-ed out in released
binaries once it's clear it never got triggered during testing.
I can send out v9 with the one-liner ASSERT, or Jordan/Laszlo could
swap out the block when applying the patch, or leave it as is, or
maybe we get more ideas from others ? I'm not well enough versed in
the edk2/ovmf environment to feel strongly either way :)
Thanks much,
--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
