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 > > 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 >> Signed-off-by: Kun Qin >> Reviewed-by: Pierre Gondois >> --- >> >> 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; >> >>   } >> >>