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;
+    }
+
+    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;
 }
 
-- 
2.37.1.windows.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#92000): https://edk2.groups.io/g/devel/message/92000
Mute This Topic: https://groups.io/mt/92722843/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

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


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#92201): https://edk2.groups.io/g/devel/message/92201
Mute This Topic: https://groups.io/mt/92722843/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

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


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#92204): https://edk2.groups.io/g/devel/message/92204
Mute This Topic: https://groups.io/mt/92722843/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

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


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#92298): https://edk2.groups.io/g/devel/message/92298
Mute This Topic: https://groups.io/mt/92722843/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

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


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#92320): https://edk2.groups.io/g/devel/message/92320
Mute This Topic: https://groups.io/mt/92722843/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-