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

["Kun Qin" <kuqin12@gmail.com>]

Hi Pierre/Sami,

Thanks for your feedback. We modified the routine to be based on
`CM_ARM_PCI_CONFIG_SPACE_INFO` and sanity checked the table
outputs from UEFI shell and verified the system bootabilty on VDK
based ARM platform.

The new patch can be found here:
https://edk2.groups.io/g/devel/message/92317

Looking forward to your further feedback.

Thanks,
Kun

On 8/10/2022 1:51 AM, Pierre Gondois wrote:
>
>
> 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;
>>>>
>>>>   }
>>>>
>>>>