Re: [edk2] [PATCH v7] OvmfPkg: PlatformBdsLib: Dynamic PCI Interrupt Line register setup

Subject: Re: [edk2] [PATCH v7] OvmfPkg: PlatformBdsLib: Dynamic PCI Interrupt Line register setup

From: Laszlo Ersek <lersek@redhat.com>

To: "Gabriel L. Somlo" <somlo@cmu.edu>, edk2-devel@lists.sourceforge.net

Date: 2014-11-14 18:26:55

One question.

On 11/14/14 04:01, Gabriel L. Somlo wrote:
> Remove hard-coded list of PCI devices for which the Interrupt Line
> register is initialized. Instead, provide a "visitor" function to
> initialize the register only for present and applicable PCI devices.
> 
> At this time, we match the behavior of SeaBIOS (file src/fw/pciinit.c,
> functions *_pci_slot_get_irq() and "map the interrupt" block from
> pci_bios_init_device()).
> 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Gabriel Somlo 
> ---
> 
> This used to be 9/9 out of a series, but 1-8 (v6) got applied already,
> so it's now a standalone patch.
> 
> New in version 7: correctly handle Q35 case with more than 24 slots
> on the root bus.
> 
> SeaBIOS traverses the device path starting at the leaf and climbing
> toward the root, so the last "slot" value in mch_pci_slot_get_irq()
> is that of the top-level parent device.
> 
> Our SetPciIntLine() visitor function uses DevicePathFromHandle()
> which starts at the root and moves toward the leaf node via
> NextDevicePathNode(), so we need to remember the *first* rather than
> the last device ID along the path to get the same result SeaBIOS does.
> 
> Thanks,
>   Gabriel 
> 
>  OvmfPkg/Library/PlatformBdsLib/BdsPlatform.c | 176 ++++++++++++++++++++-------
>  1 file changed, 132 insertions(+), 44 deletions(-)
> 
> diff --git a/OvmfPkg/Library/PlatformBdsLib/BdsPlatform.c b/OvmfPkg/Library/PlatformBdsLib/BdsPlatform.c
> index 6fc5a89..c537a93 100644
> --- a/OvmfPkg/Library/PlatformBdsLib/BdsPlatform.c
> +++ b/OvmfPkg/Library/PlatformBdsLib/BdsPlatform.c
> @@ -25,7 +25,20 @@ EFI_EVENT     mEfiDevPathEvent;
>  VOID          *mEmuVariableEventReg;
>  EFI_EVENT     mEmuVariableEvent;
>  BOOLEAN       mDetectVgaOnly;
> +UINT16        mHostBridgeDevId;
>  
> +//
> +// Table of host IRQs matching PCI IRQs A-D
> +// (for configuring PCI Interrupt Line register)
> +//
> +CONST UINT8 PciHostIrqs[] = {
> +  0x0a, 0x0a, 0x0b, 0x0b
> +};
> +
> +//
> +// Array Size macro
> +//
> +#define ARRAY_SIZE(array) (sizeof (array) / sizeof (array[0]))
>  
>  //
>  // Type definitions
> @@ -716,18 +729,128 @@ Returns:
>  }
>  
>  
> +/**
> +  Configure PCI Interrupt Line register for applicable devices
> +  Ported from SeaBIOS, src/fw/pciinit.c, *_pci_slot_get_irq()
> +
> +  @param[in]  Handle - Handle of PCI device instance
> +  @param[in]  PciIo - PCI IO protocol instance
> +  @param[in]  PciHdr - PCI Header register block
> +
> +  @retval EFI_SUCCESS - PCI Interrupt Line register configured successfully.
> +
> +**/
> +EFI_STATUS
> +EFIAPI
> +SetPciIntLine (
> +  IN EFI_HANDLE           Handle,
> +  IN EFI_PCI_IO_PROTOCOL  *PciIo,
> +  IN PCI_TYPE00           *PciHdr
> +  )
> +{
> +  EFI_DEVICE_PATH_PROTOCOL  *DevPathNode;
> +  UINTN                     RootSlot;
> +  UINTN                     Idx;
> +  UINT8                     IrqLine;
> +  EFI_STATUS                Status;
> +
> +  Status = EFI_SUCCESS;
> +
> +  if (PciHdr->Device.InterruptPin != 0) {

so here InterruptPin >= 1

> +
> +    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)

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

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.

Now where is my thought process wrong? You might want to add an assert
somewhere.

*Awesome* job, otherwise. Thank you very much!
Laszlo


> +        break;
> +      case INTEL_Q35_MCH_DEVICE_ID:
> +        //
> +        // SeaBIOS contains the following comment:
> +        // "Slots 0-24 rotate slot:pin mapping similar to piix above, but
> +        //  with a different starting index - see q35-acpi-dsdt.dsl.
> +        //
> +        //  Slots 25-31 all use LNKA mapping (or LNKE, but A:D = E:H)"
> +        //
> +        if (RootSlot > 24) {
> +          //
> +          // in this case, subtract back out RootSlot from Idx
> +          // (SeaBIOS never adds it to begin with, but that would make our
> +          //  device path traversal loop above too awkward)
> +          //
> +          Idx -= RootSlot;
> +        }
> +        break;
> +      default:
> +        ASSERT (FALSE); // should never get here
> +    }
> +    Idx %= ARRAY_SIZE (PciHostIrqs);
> +    IrqLine = PciHostIrqs[Idx];
> +
> +    //
> +    // Set PCI Interrupt Line register for this device to PciHostIrqs[Idx]
> +    //
> +    Status = PciIo->Pci.Write (
> +               PciIo,
> +               EfiPciIoWidthUint8,
> +               PCI_INT_LINE_OFFSET,
> +               1,
> +               &IrqLine
> +               );
> +  }
> +
> +  return Status;
> +}
> +
> +
>  VOID
>  PciAcpiInitialization (
>    )
>  {
> -  UINT16 HostBridgeDevId;
>    UINTN  Pmba;
>  
>    //
>    // Query Host Bridge DID to determine platform type
>    //
> -  HostBridgeDevId = PcdGet16 (PcdOvmfHostBridgePciDevId);
> -  switch (HostBridgeDevId) {
> +  mHostBridgeDevId = PcdGet16 (PcdOvmfHostBridgePciDevId);
> +  switch (mHostBridgeDevId) {
>      case INTEL_82441_DEVICE_ID:
>        Pmba = POWER_MGMT_REGISTER_PIIX4 (0x40);
>        //
> @@ -754,55 +877,20 @@ PciAcpiInitialization (
>        break;
>      default:
>        DEBUG ((EFI_D_ERROR, "%a: Unknown Host Bridge Device ID: 0x%04x\n",
> -        __FUNCTION__, HostBridgeDevId));
> +        __FUNCTION__, mHostBridgeDevId));
>        ASSERT (FALSE);
>        return;
>    }
>  
>    //
> +  // Initialize PCI_INTERRUPT_LINE for applicable present PCI devices
> +  //
> +  VisitAllPciInstances (SetPciIntLine);
> +
> +  //
>    // Set ACPI SCI_EN bit in PMCNTRL
>    //
>    IoOr16 ((PciRead32 (Pmba) & ~BIT0) + 4, BIT0);
> -
> -  //
> -  // Initialize PCI_INTERRUPT_LINE for commonly encountered devices and slots
> -  //
> -  // FIXME: This should instead be accomplished programmatically by
> -  //        ennumerating all PCI devices present in the system and
> -  //        computing PCI_INTERRUPT_LINE from PCI_INTERRUPT_PIN, the
> -  //        slot/position of the device, and the available host IRQs
> -  //        (for an example, see SeaBIOS pci_bios_init_devices() in
> -  //        src/fw/pciinit.c)
> -  //
> -  switch (HostBridgeDevId) {
> -    case INTEL_82441_DEVICE_ID:
> -      PciWrite8 (PCI_LIB_ADDRESS (0,    1, 2, 0x3c), 0x0b); // usb (northbr.)
> -      PciWrite8 (PCI_LIB_ADDRESS (0,    1, 3, 0x3c), 0x0a); // acpi (northbr.)
> -      PciWrite8 (PCI_LIB_ADDRESS (0,    3, 0, 0x3c), 0x0b);
> -      PciWrite8 (PCI_LIB_ADDRESS (0,    4, 0, 0x3c), 0x0b);
> -      PciWrite8 (PCI_LIB_ADDRESS (0,    5, 0, 0x3c), 0x0a);
> -      PciWrite8 (PCI_LIB_ADDRESS (0,    6, 0, 0x3c), 0x0a);
> -      PciWrite8 (PCI_LIB_ADDRESS (0,    7, 0, 0x3c), 0x0b);
> -      PciWrite8 (PCI_LIB_ADDRESS (0,    8, 0, 0x3c), 0x0b);
> -      break;
> -    case INTEL_Q35_MCH_DEVICE_ID:
> -      PciWrite8 (PCI_LIB_ADDRESS (0,    2, 0, 0x3c), 0x0b);
> -      PciWrite8 (PCI_LIB_ADDRESS (0,    3, 0, 0x3c), 0x0b);
> -      PciWrite8 (PCI_LIB_ADDRESS (0,    4, 0, 0x3c), 0x0a);
> -      PciWrite8 (PCI_LIB_ADDRESS (0,    5, 0, 0x3c), 0x0a);
> -      PciWrite8 (PCI_LIB_ADDRESS (0,    6, 0, 0x3c), 0x0b);
> -      PciWrite8 (PCI_LIB_ADDRESS (0,    7, 0, 0x3c), 0x0b);
> -      PciWrite8 (PCI_LIB_ADDRESS (0,    8, 0, 0x3c), 0x0a);
> -      PciWrite8 (PCI_LIB_ADDRESS (0, 0x1d, 0, 0x3c), 0x0a); // uhci1
> -      PciWrite8 (PCI_LIB_ADDRESS (0, 0x1d, 1, 0x3c), 0x0a); // uhci2
> -      PciWrite8 (PCI_LIB_ADDRESS (0, 0x1d, 2, 0x3c), 0x0b); // uhci3
> -      PciWrite8 (PCI_LIB_ADDRESS (0, 0x1d, 7, 0x3c), 0x0b); // ehci1
> -      PciWrite8 (PCI_LIB_ADDRESS (0, 0x1f, 2, 0x3c), 0x0a); // ahci (northbr.)
> -      PciWrite8 (PCI_LIB_ADDRESS (0, 0x1f, 3, 0x3c), 0x0a); // smbus (northbr.)
> -      break;
> -    default:
> -      ASSERT (FALSE); // should never be reached
> -  }
>  }
>  
>  
> 


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