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

["Sami Mujawar" <sami.mujawar@arm.com>]

[-- Attachment #1: Type: text/plain, Size: 9799 bytes --]

Hi Kun,

I have just tried testing your patch with Kvmtool guest firmware and 
think this patch may need some modifications.

Also, the patch 4/6 may need some adjustment, which I will reply back on 
that patch separately.

Regards,

Sami Mujawar

On 08/08/2022 02:22 pm, Sami Mujawar wrote:
> Hi Kun,
>
> Thank you for this patch.
>
> These changes look good to me.
>
> Reviewed-by: Sami Mujawar <sami.mujawar@arm.com>
>
> Regards,
>
> Sami Mujawar
>
> On 31/07/2022 06:37 am, 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@microsoft.com>
>> Signed-off-by: Kun Qin <kuqin12@gmail.com>
>> Reviewed-by: Pierre Gondois <pierre.gondois@arm.com>
>> ---
>>
>> Notes:
>>      v2:
>>      - Only create RES0 after config space checking [Pierre]
>>           v3:
>>      - Updated function names and descriptions [Pierre]
>>      - Moved translation calculation to CONFIG case [Pierre]
>>
>> DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtPcieLibArm/SsdtPcieGenerator.c 
>> | 171 ++++++++++++++++++++
>>   1 file changed, 171 insertions(+)
>>
>> diff --git 
>> a/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtPcieLibArm/SsdtPcieGenerator.c 
>> b/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtPcieLibArm/SsdtPcieGenerator.c 
>>
>> index ceffe2838c03..658a089c8f1f 100644
>> --- 
>> a/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtPcieLibArm/SsdtPcieGenerator.c
>> +++ 
>> b/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtPcieLibArm/SsdtPcieGenerator.c
>> @@ -616,6 +616,169 @@ GeneratePciCrs (
>>     return Status;
>>
>>   }
>>
>>
>> +/** Generate a RES0 device node to reserve PNP motherboard resources
>>
>> +  for a given PCI node.
>>
>> +
>>
>> +  @param [in]   PciNode       Parent PCI node handle of the generated
>>
>> +                              resource object.
>>
>> +  @param [out]  CrsNode       CRS node of the AML tree to populate.
>>
>> +
>>
>> +  @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
>>
>> +GenerateMotherboardDevice (
>>
>> +  IN  AML_OBJECT_NODE_HANDLE  PciNode,
>>
>> +  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 (RES0) {}
>>
>> +  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;
>>
>> +}
>>
>> +
>>
>> +/** Reserves ECAM space for PCI config space
>>
>> +
>>
>> +  @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
>>
>> +ReserveEcamSpace (
>>
>> +  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;
>>
>> +    }
>>
[SAMI] Sorry for missing this earlier in the review. However, the ECAM 
memory space is described byCM_ARM_PCI_CONFIG_SPACE_INFO. So, I think 
that needs to be used here.

The CM_ARM_PCI_CONFIG_SPACE_INFO structure does not include the length 
of the configuration space and would probably need to be updated.

Which platform are you testing these changes on? I would like to 
understand more about your use case. Is it possible to share some more 
details, please?

[/SAMI]

>> +
>>
>> +    switch (AddrMapInfo->SpaceCode) {
>>
>> +      case PCI_SS_CONFIG:
>>
>> +        Translation = (AddrMapInfo->CpuAddress != 
>> AddrMapInfo->PciAddress);
>>
>> +        if (AddrMapInfo->CpuAddress >= AddrMapInfo->PciAddress) {
>>
>> +          IsPosDecode = TRUE;
>>
>> +        } else {
>>
>> +          IsPosDecode = FALSE;
>>
>> +        }
>>
>> +
>>
>> +        Status = GenerateMotherboardDevice (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 +865,17 @@ GeneratePciDevice (
>>       return Status;
>>
>>     }
>>
>>
>> +  // Add the PNP Motherboard Resources Device to reserve ECAM space
>>
>> +  Status = ReserveEcamSpace (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;
>>
>>   }
>>
>>

[-- Attachment #2: Type: text/html, Size: 22164 bytes --]