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

["PierreGondois" <pierre.gondois@arm.com>]

On 8/8/22 17:39, Sami Mujawar wrote:
> 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]

[Pierre]

Yes indeed, CM_ARM_PCI_CONFIG_SPACE_INFO should contain the configuration
address space, it should not be described in CM_ARM_PCI_ADDRESS_MAP_INFO.
So there should be no need to search through the CM_ARM_PCI_ADDRESS_MAP_INFO
objects. Sorry for missing this earlier.

The length of the address space could be computed as:
length = (end_bus - start_bus + 1) × 32 devices × 8 functions × 4 KB

[/Pierre]

> 
>>> +
>>>
>>> +    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;
>>>
>>>   }
>>>
>>>