From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from foss.arm.com (foss.arm.com [217.140.110.172]) by mx.groups.io with SMTP id smtpd.web09.32382.1659013645408938875 for ; Thu, 28 Jul 2022 06:07:25 -0700 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: arm.com, ip: 217.140.110.172, mailfrom: pierre.gondois@arm.com) Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id A942D106F; Thu, 28 Jul 2022 06:07:25 -0700 (PDT) Received: from [10.34.100.102] (pierre123.nice.arm.com [10.34.100.102]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 4F83A3F70D; Thu, 28 Jul 2022 06:07:24 -0700 (PDT) Message-ID: Date: Thu, 28 Jul 2022 15:07:23 +0200 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.11.0 From: "PierreGondois" Subject: Re: [edk2-devel] [PATCH v2 5/6] DynamicTablesPkg: AcpiSsdtPcieLibArm: Added function to reserve ECAM space To: devel@edk2.groups.io, kuqin12@gmail.com Cc: Joe Lopez , Sami Mujawar References: <20220728043147.395-1-kuqin12@gmail.com> <20220728043147.395-6-kuqin12@gmail.com> In-Reply-To: <20220728043147.395-6-kuqin12@gmail.com> Content-Language: en-US Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit Hello Kun, With the changes below: Reviewed-by: Pierre Gondois Thanks! On 7/28/22 06:31, 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 > --- > > Notes: > v2: > - Only create RES0 after config space checking [Pierre] > > DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtPcieLibArm/SsdtPcieGenerator.c | 169 ++++++++++++++++++++ > 1 file changed, 169 insertions(+) > > diff --git a/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtPcieLibArm/SsdtPcieGenerator.c b/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtPcieLibArm/SsdtPcieGenerator.c > index ceffe2838c03..c03550baabf2 100644 > --- a/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtPcieLibArm/SsdtPcieGenerator.c > +++ b/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtPcieLibArm/SsdtPcieGenerator.c > @@ -616,6 +616,167 @@ GeneratePciCrs ( > return Status; > } > > +/** Generate a Pci Resource Template to hold Address Space Info > + > + @param [in] PciNode RootNode of the AML tree. > + @param [in, out] CrsNode CRS node of the AML tree to populate. I think CrsNode is an 'out' only parameter. Also the description of PciNode seems incorrect. > + > + @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 > +PopulateBasicPciResObjects ( Would it be possible to rename it: GenerateMotherboardDevice() or with a name related to 'motherboard' ? > + IN AML_OBJECT_NODE_HANDLE PciNode, > + IN 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 (PCIx) {} > + 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; > +} > + > +/** Generate a Pci Resource Template to hold Address Space Info > + > + @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 > +GeneratePciRes ( Would it be possible to rename it: ReserveEcamSpace() or with a name related to 'ecam' ? > + 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; > + } > + > + Translation = (AddrMapInfo->CpuAddress != AddrMapInfo->PciAddress); > + if (AddrMapInfo->CpuAddress >= AddrMapInfo->PciAddress) { > + IsPosDecode = TRUE; > + } else { > + IsPosDecode = FALSE; > + } I think this should be done in: case PCI_SS_CONFIG: to only do it when necessary. > + > + switch (AddrMapInfo->SpaceCode) { > + case PCI_SS_CONFIG: > + Status = PopulateBasicPciResObjects (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 +863,17 @@ GeneratePciDevice ( > return Status; > } > > + // Add the PNP Motherboard Resources Device to reserve ECAM space > + Status = GeneratePciRes (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; > } >