From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-pj1-f48.google.com (mail-pj1-f48.google.com [209.85.216.48]) by mx.groups.io with SMTP id smtpd.web11.21.1660171022710739184 for ; Wed, 10 Aug 2022 15:37:02 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@gmail.com header.s=20210112 header.b=JO0U8BZi; spf=pass (domain: gmail.com, ip: 209.85.216.48, mailfrom: kuqin12@gmail.com) Received: by mail-pj1-f48.google.com with SMTP id pm17so16108580pjb.3 for ; Wed, 10 Aug 2022 15:37:02 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112; h=content-transfer-encoding:in-reply-to:from:references:cc:to :content-language:subject:user-agent:mime-version:date:message-id :from:to:cc; bh=5oYdfWE0rqiXIXs4ogim9LJyHCaSDkKldLKjZIZ4XxA=; b=JO0U8BZih4nbAXOwC0q8VI7UetDuCeH4UZw2R3Y1e0PKluY1xxTooITUjr6sBzsTp9 ewSXdH5XF2Dw6xynZQxQlnY1yrau9E3mteySQ4XPvLj3LOiq/TkrJRk7m8rDxqHpyY+3 x2ZYs1iqFLkWwRS5uh3yYydUFkzeTeTSrXi5a+CHmcAy93mjKk7nQdhlh9gFMQcqnIGu JCJLEmyoBxlDFYA/Siw62FOxs/MpGSGIF3YVI+Fjmd9GfBxJ4GfCCsF2fIwrj69n+4LH VrHJLn8JnDG8IXesDgqMhozHkYiJ76P9ajWRWBUnTeGbS173weOQEoYBuJLNLg/1TT9H ATOA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=content-transfer-encoding:in-reply-to:from:references:cc:to :content-language:subject:user-agent:mime-version:date:message-id :x-gm-message-state:from:to:cc; bh=5oYdfWE0rqiXIXs4ogim9LJyHCaSDkKldLKjZIZ4XxA=; b=luImPSslnFw5LYhck6kmI5yzLMyAecI0EDTBqNKOpXYfWTfniADQejzFdrfOX4MdeO dPiXmjGRNzDXnGfk25/4an+H860uCLJ0FSGtDgHFi9aEPnaJYupvBM5stAaDatgj2wsc YVT5FGPpYKTdYdSQ7ogUHbunPwtvUd8pJr6vyKGX1xpk/F17cAVcwiTW+ZSL+A1R0cYv h/57GWrvO3N4hu/T53H62PcQrnxMg7mdT3hmMHxVZBjZI4NuLGKzziaFR81JAx2u2g/R +QiSeVzLmmXXiF3HlDN4z3SxdVB8F/ae3zZt9AmruG0lpgsXoxzYRALThKK0mgbcqm4J 6kpw== X-Gm-Message-State: ACgBeo0cFIvvwZxOkCGLGoKVGyp95iW3cM8nYqGR+XNX8vp/lrzLizn0 ulonwaVyqtqmRthmxQ1K9qA= X-Google-Smtp-Source: AA6agR4ResTW7rdvmgTAUOReMp7bUDCYbK3wx747AKNNVhH5aIoEVu6T5fsf5mkyAfmB1nzqOTUr8g== X-Received: by 2002:a17:90a:e58a:b0:1e2:fe75:dd5f with SMTP id g10-20020a17090ae58a00b001e2fe75dd5fmr5856740pjz.138.1660171022142; Wed, 10 Aug 2022 15:37:02 -0700 (PDT) Return-Path: Received: from ?IPV6:2001:4898:d8:33:352e:f1a8:aef2:7dc4? ([2001:4898:80e8:38:b518:f1a8:aef2:7dc4]) by smtp.gmail.com with ESMTPSA id t7-20020a1709027fc700b0016d7afee272sm13401936plb.153.2022.08.10.15.37.01 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Wed, 10 Aug 2022 15:37:01 -0700 (PDT) Message-ID: <68fdf6f0-3059-ea56-9e96-758cccb46c01@gmail.com> Date: Wed, 10 Aug 2022 15:37:01 -0700 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:91.0) Gecko/20100101 Thunderbird/91.12.0 Subject: Re: [edk2-devel] [PATCH v3 5/6] DynamicTablesPkg: AcpiSsdtPcieLibArm: Added function to reserve ECAM space To: Pierre Gondois , Sami Mujawar , devel@edk2.groups.io Cc: Joe Lopez , "nd@arm.com" References: <20220731053727.536-1-kuqin12@gmail.com> <20220731053727.536-6-kuqin12@gmail.com> <7b2fb1d3-6acf-9cc8-025d-2ef598976df4@arm.com> <8e9707a8-b200-5727-46bb-58f99c74f501@arm.com> From: "Kun Qin" In-Reply-To: <8e9707a8-b200-5727-46bb-58f99c74f501@arm.com> Content-Language: en-US Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit 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 >>> >>> 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] > > [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; >>>> >>>>   } >>>> >>>>