[edk2-devel] [PATCH v2 5/6] DynamicTablesPkg: AcpiSsdtPcieLibArm: Added function to reserve ECAM space

[Kun Qin]

Hi Pierre,

Thank you for the suggestions. I will update the patch per your feedback and
send for review in v3.

Regards,
Kun

On 7/28/2022 6:07 AM, Pierre Gondois wrote:
> Hello Kun,
> With the changes below:
> Reviewed-by: Pierre Gondois <pierre.gondois at arm.com>
>
> Thanks!
>
>
> On 7/28/22 06:31, Kun Qin via groups.io wrote:
>> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3998
>>
>> Certain OSes will complain if the ECAM config space is not reserved in
>> the ACPI namespace.
>>
>> This change adds a function to reserve PNP motherboard resources for a
>> given PCI node.
>>
>> Co-authored-by: Joe Lopez <joelopez at microsoft.com>
>> Signed-off-by: Kun Qin <kuqin12 at gmail.com>
>> ---
>>
>> Notes:
>>      v2:
>>      - Only create RES0 after config space checking [Pierre]
>>
>> DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtPcieLibArm/SsdtPcieGenerator.c 
>> | 169 ++++++++++++++++++++
>>   1 file changed, 169 insertions(+)
>>
>> diff --git 
>> a/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtPcieLibArm/SsdtPcieGenerator.c 
>> b/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtPcieLibArm/SsdtPcieGenerator.c 
>>
>> index ceffe2838c03..c03550baabf2 100644
>> --- 
>> a/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtPcieLibArm/SsdtPcieGenerator.c
>> +++ 
>> b/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtPcieLibArm/SsdtPcieGenerator.c
>> @@ -616,6 +616,167 @@ GeneratePciCrs (
>>     return Status;
>>   }
>>   +/** Generate a Pci Resource Template to hold Address Space Info
>> +
>> +  @param [in]       PciNode       RootNode of the AML tree.
>> +  @param [in, out]  CrsNode       CRS node of the AML tree to populate.
>
> I think CrsNode is an 'out' only parameter. Also the description of 
> PciNode
> seems incorrect.
>
>> +
>> +  @retval EFI_SUCCESS             The function completed successfully.
>> +  @retval EFI_INVALID_PARAMETER   Invalid input parameter.
>> +  @retval EFI_OUT_OF_RESOURCES    Could not allocate memory.
>> +**/
>> +STATIC
>> +EFI_STATUS
>> +EFIAPI
>> +PopulateBasicPciResObjects (
>
> Would it be possible to rename it:
>    GenerateMotherboardDevice()
> or with a name related to 'motherboard' ?
>
>> +  IN        AML_OBJECT_NODE_HANDLE PciNode,
>> +  IN  OUT   AML_OBJECT_NODE_HANDLE  *CrsNode
>> +  )
>> +{
>> +  EFI_STATUS              Status;
>> +  UINT32                  EisaId;
>> +  AML_OBJECT_NODE_HANDLE  ResNode;
>> +
>> +  if (CrsNode == NULL) {
>> +    ASSERT (0);
>> +    return EFI_INVALID_PARAMETER;
>> +  }
>> +
>> +  // ASL: Device (PCIx) {}
>> +  Status = AmlCodeGenDevice ("RES0", PciNode, &ResNode);
>> +  if (EFI_ERROR (Status)) {
>> +    ASSERT (0);
>> +    return Status;
>> +  }
>> +
>> +  // ASL: Name (_HID, EISAID ("PNP0C02"))
>> +  Status = AmlGetEisaIdFromString ("PNP0C02", &EisaId); /* PNP 
>> Motherboard Resources */
>> +  if (EFI_ERROR (Status)) {
>> +    ASSERT (0);
>> +    return Status;
>> +  }
>> +
>> +  Status = AmlCodeGenNameInteger ("_HID", EisaId, ResNode, NULL);
>> +  if (EFI_ERROR (Status)) {
>> +    ASSERT (0);
>> +    return Status;
>> +  }
>> +
>> +  // ASL: Name (_CRS, ResourceTemplate () {})
>> +  Status = AmlCodeGenNameResourceTemplate ("_CRS", ResNode, CrsNode);
>> +  if (EFI_ERROR (Status)) {
>> +    ASSERT (0);
>> +    return Status;
>> +  }
>> +
>> +  return Status;
>> +}
>> +
>> +/** Generate a Pci Resource Template to hold Address Space Info
>> +
>> +  @param [in]       Generator       The SSDT Pci generator.
>> +  @param [in]       CfgMgrProtocol  Pointer to the Configuration 
>> Manager
>> +                                    Protocol interface.
>> +  @param [in]       PciInfo         Pci device information.
>> +  @param [in, out]  PciNode         RootNode of the AML tree to 
>> populate.
>> +
>> +  @retval EFI_SUCCESS             The function completed successfully.
>> +  @retval EFI_INVALID_PARAMETER   Invalid parameter.
>> +  @retval EFI_OUT_OF_RESOURCES    Could not allocate memory.
>> +**/
>> +STATIC
>> +EFI_STATUS
>> +EFIAPI
>> +GeneratePciRes (
>
> Would it be possible to rename it:
>    ReserveEcamSpace()
> or with a name related to 'ecam' ?
>
>
>> +  IN ACPI_PCI_GENERATOR                            *Generator,
>> +  IN      CONST EDKII_CONFIGURATION_MANAGER_PROTOCOL  *CONST 
>> CfgMgrProtocol,
>> +  IN      CONST CM_ARM_PCI_CONFIG_SPACE_INFO *PciInfo,
>> +  IN  OUT       AML_OBJECT_NODE_HANDLE PciNode
>> +  )
>> +{
>> +  EFI_STATUS                   Status;
>> +  AML_OBJECT_NODE_HANDLE       CrsNode;
>> +  BOOLEAN                      Translation;
>> +  UINT32                       Index;
>> +  CM_ARM_OBJ_REF               *RefInfo;
>> +  UINT32                       RefCount;
>> +  CM_ARM_PCI_ADDRESS_MAP_INFO  *AddrMapInfo;
>> +  BOOLEAN                      IsPosDecode;
>> +
>> +  // Get the array of CM_ARM_OBJ_REF referencing the
>> +  // CM_ARM_PCI_ADDRESS_MAP_INFO objects.
>> +  Status = GetEArmObjCmRef (
>> +             CfgMgrProtocol,
>> +             PciInfo->AddressMapToken,
>> +             &RefInfo,
>> +             &RefCount
>> +             );
>> +  if (EFI_ERROR (Status)) {
>> +    ASSERT (0);
>> +    return Status;
>> +  }
>> +
>> +  for (Index = 0; Index < RefCount; Index++) {
>> +    // Get CM_ARM_PCI_ADDRESS_MAP_INFO structures one by one.
>> +    Status = GetEArmObjPciAddressMapInfo (
>> +               CfgMgrProtocol,
>> +               RefInfo[Index].ReferenceToken,
>> +               &AddrMapInfo,
>> +               NULL
>> +               );
>> +    if (EFI_ERROR (Status)) {
>> +      ASSERT (0);
>> +      return Status;
>> +    }
>> +
>> +    Translation = (AddrMapInfo->CpuAddress != AddrMapInfo->PciAddress);
>> +    if (AddrMapInfo->CpuAddress >= AddrMapInfo->PciAddress) {
>> +      IsPosDecode = TRUE;
>> +    } else {
>> +      IsPosDecode = FALSE;
>> +    }
>
> I think this should be done in:
>   case PCI_SS_CONFIG:
> to only do it when necessary.
>
>> +
>> +    switch (AddrMapInfo->SpaceCode) {
>> +      case PCI_SS_CONFIG:
>> +        Status = PopulateBasicPciResObjects (PciNode, &CrsNode);
>> +        if (EFI_ERROR (Status)) {
>> +          ASSERT (0);
>> +          break;
>> +        }
>> +
>> +        Status = AmlCodeGenRdQWordMemory (
>> +                   FALSE,
>> +                   IsPosDecode,
>> +                   TRUE,
>> +                   TRUE,
>> +                   FALSE, // non-cacheable
>> +                   TRUE,
>> +                   0,
>> +                   AddrMapInfo->PciAddress,
>> +                   AddrMapInfo->PciAddress + 
>> AddrMapInfo->AddressSize - 1,
>> +                   Translation ? AddrMapInfo->CpuAddress - 
>> AddrMapInfo->PciAddress : 0,
>> +                   AddrMapInfo->AddressSize,
>> +                   0,
>> +                   NULL,
>> +                   0,
>> +                   TRUE,
>> +                   CrsNode,
>> +                   NULL
>> +                   );
>> +        break;
>> +      default:
>> +        break;
>> +    } // switch
>> +
>> +    if (EFI_ERROR (Status)) {
>> +      ASSERT (0);
>> +      return Status;
>> +    }
>> +  }
>> +
>> +  return Status;
>> +}
>> +
>>   /** Generate a Pci device.
>>       @param [in]       Generator       The SSDT Pci generator.
>> @@ -702,9 +863,17 @@ GeneratePciDevice (
>>       return Status;
>>     }
>>   +  // Add the PNP Motherboard Resources Device to reserve ECAM space
>> +  Status = GeneratePciRes (Generator, CfgMgrProtocol, PciInfo, 
>> PciNode);
>> +  if (EFI_ERROR (Status)) {
>> +    ASSERT (0);
>> +    return Status;
>> +  }
>> +
>>     // Add the template _OSC method.
>>     Status = AddOscMethod (PciInfo, PciNode);
>>     ASSERT_EFI_ERROR (Status);
>> +
>>     return Status;
>>   }


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#91963): https://edk2.groups.io/g/devel/message/91963
Mute This Topic: https://groups.io/mt/92664649/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-