Re: [PATCH 4/4] DynamicTablesPkg: AcpiSsdtPcieLibArm: Add support for override protocol

["PierreGondois" <pierre.gondois@arm.com>]

On 7/11/22 17:32, Jeff Brasen wrote:
> We also on some PCIe devices add a device entry under DOO_ w/ it's own _RST and _DSD. If you see the v3 version of the patch where I add a support library that we can override allows us to customize the device as we need.

Ok thanks! GeneratePciSlots() should allow you to add any platform specific
device, and should ideally not modify the parent 'PCI0' object (even if
it is possible in practice). So it should be ok.

A one last question:

[PATCH v3 2/3] DynamicTablesPkg: AcpiSsdtPcieLibArm: Support UID > 0xF
Is is still needed ?

Regards,
Pierre

> 
> -Jeff
> 
> 
>> -----Original Message-----
>> From: Pierre Gondois <pierre.gondois@arm.com>
>> Sent: Monday, July 11, 2022 9:02 AM
>> To: Jeff Brasen <jbrasen@nvidia.com>; devel@edk2.groups.io
>> Cc: Sami.Mujawar@arm.com; Alexei.Fedorov@arm.com
>> Subject: Re: [PATCH 4/4] DynamicTablesPkg: AcpiSsdtPcieLibArm: Add
>> support for override protocol
>>
>> External email: Use caution opening links or attachments
>>
>>
>> Hello Jeff,
>> To be sure I understand correctly, the SsdtPcieGenerator currently allows to
>> generate something like this:
>>
>> Scope(_SB) {
>>     Device(PCI0) {
>>       Device (D00_) { // Device 0
>>         Name(_ADR, 0x0000FFFF)
>>       }
>>       Method(_OSC,4) {
>>         [...] // Generic _OSC method
>>       }
>>     }
>> }
>>
>> What you want to do is:
>> 1. Use another _OSC method
>> 2. Add more information in the D00_ object, i.e.:
>>     - a _DSD object
>>     - a _RST method
>> 3. As _RST is added in 2., also add a _RST method to PCI0
>>
>> Is it correct ? Or is there some other information you want to add ?
>>
>> Regards,
>> Pierre
>>
>> On 7/8/22 17:36, Jeff Brasen wrote:
>>> I think this would work. Instead of GeneratePciDeviceInfo the function
>>> we would want to break out would be GeneratePciSlots. I'll work on
>>> changing my patch series to this. Unless you have a better name will
>>> call this library SsdtPciSupportLib and place in under Library/Common
>>>
>>> Going to also pass PciInfo into AddOscMethod in this new approach in case
>> client needs to have different _OSC per controller (And to GeneratePciSlots
>> as well).
>>>
>>> Thanks,
>>> Jeff
>>>
>>>> -----Original Message-----
>>>> From: Pierre Gondois <pierre.gondois@arm.com>
>>>> Sent: Monday, July 4, 2022 2:48 AM
>>>> To: Jeff Brasen <jbrasen@nvidia.com>; devel@edk2.groups.io
>>>> Cc: Sami.Mujawar@arm.com; Alexei.Fedorov@arm.com
>>>> Subject: Re: [PATCH 4/4] DynamicTablesPkg: AcpiSsdtPcieLibArm: Add
>>>> support for override protocol
>>>>
>>>> External email: Use caution opening links or attachments
>>>>
>>>>
>>>> Hello Jeff,
>>>> I think it would ideally be better to have the code adding/replacing
>>>> the methods/objects you noted inside the edk2/edk2-platfoms
>>>> repositories. The methods/objects that you want to replace seem to only
>> concern:
>>>> -the objects available in the 'Device (PCIx)' node -the _OSC method
>>>>
>>>> If a library with the following two methods (extracted from
>>>> SsdtPcieLibArm.inf) was created, would it be sufficient for all the
>>>> replacements you want to do ?
>>>> Like this we would have a generic implementation and specific ones.
>>>>
>>>> EFI_STATUS
>>>> EFIAPI
>>>> GeneratePciDeviceInfo (
>>>>      IN      CONST CM_ARM_PCI_CONFIG_SPACE_INFO  *PciInfo,
>>>>      IN            UINT32                        Uid,
>>>>      IN  OUT       AML_OBJECT_NODE_HANDLE        PciNode
>>>>      )
>>>>
>>>> EFI_STATUS
>>>> EFIAPI
>>>> AddOscMethod (
>>>>      IN  OUT   AML_OBJECT_NODE_HANDLE  PciNode
>>>>      )
>>>>
>>>> Regards,
>>>> Pierre
>>>>
>>>> On 7/1/22 17:52, Jeff Brasen wrote:
>>>>> Currently we do the following in this call.
>>>>>
>>>>> Remove and replace the _OSC method on certain targets. I originally
>>>>> had this pass the template over but removed that when I added this
>>>>> more generic override support Add a _RST method to the root port
>>>>> device sub node Add a _DSD for device properties Conditionally add
>>>>> an entry for the device attached to the PCIe bus if we need to add
>>>>> properties or _RST methods. This will likely eventually move to
>>>>> another driver (which is the purpose of patch 2/4 in this series)
>>>>>
>>>>> I figured trying to get the generator to handle that would be more
>>>>> difficult
>>>> as these would be hard to generalize.
>>>>>
>>>>>
>>>>> Thanks,
>>>>> Jeff
>>>>>
>>>>>> -----Original Message-----
>>>>>> From: Pierre Gondois <pierre.gondois@arm.com>
>>>>>> Sent: Friday, July 1, 2022 6:40 AM
>>>>>> To: Jeff Brasen <jbrasen@nvidia.com>; devel@edk2.groups.io
>>>>>> Cc: Sami.Mujawar@arm.com; Alexei.Fedorov@arm.com
>>>>>> Subject: Re: [PATCH 4/4] DynamicTablesPkg: AcpiSsdtPcieLibArm: Add
>>>>>> support for override protocol
>>>>>>
>>>>>> External email: Use caution opening links or attachments
>>>>>>
>>>>>>
>>>>>> Hello Jeff,
>>>>>> Is it possible to know what the UpdateTable() function would do ?
>>>>>> Maybe it would be possible to integrate the alternative
>>>>>> implementation without adding a new protocol.
>>>>>>
>>>>>> Regards,
>>>>>> Pierre
>>>>>>
>>>>>> On 6/30/22 17:48, Jeff Brasen wrote:
>>>>>>> Some platfoms may want to modify the ACPI table created.
>>>>>>> Add support for protocol that can provide an alternative
>>>> implementation.
>>>>>>>
>>>>>>> Signed-off-by: Jeff Brasen <jbrasen@nvidia.com>
>>>>>>> ---
>>>>>>>      DynamicTablesPkg/DynamicTablesPkg.dec         |  3 +
>>>>>>>      .../Protocol/SsdtPcieOverrideProtocol.h       | 63
>>>> +++++++++++++++++++
>>>>>>>      .../AcpiSsdtPcieLibArm/SsdtPcieGenerator.c    | 31 ++++++++-
>>>>>>>      .../Arm/AcpiSsdtPcieLibArm/SsdtPcieLibArm.inf |  4 ++
>>>>>>>      4 files changed, 98 insertions(+), 3 deletions(-)
>>>>>>>      create mode 100644
>>>>>>> DynamicTablesPkg/Include/Protocol/SsdtPcieOverrideProtocol.h
>>>>>>>
>>>>>>> diff --git a/DynamicTablesPkg/DynamicTablesPkg.dec
>>>>>>> b/DynamicTablesPkg/DynamicTablesPkg.dec
>>>>>>> index a890a048be..bb66bdaf14 100644
>>>>>>> --- a/DynamicTablesPkg/DynamicTablesPkg.dec
>>>>>>> +++ b/DynamicTablesPkg/DynamicTablesPkg.dec
>>>>>>> @@ -43,6 +43,9 @@
>>>>>>>        # Dynamic Table Factory Protocol GUID
>>>>>>>        gEdkiiDynamicTableFactoryProtocolGuid = { 0x91d1e327,
>>>>>>> 0xfe5a, 0x49b8, { 0xab, 0x65, 0xe, 0xce, 0x2d, 0xdb, 0x45, 0xec }
>>>>>>> }
>>>>>>>
>>>>>>> +  # Protocol to override PCI SSDT table generation
>>>>>>> + gEdkiiDynamicTableSsdtPcieOverrideProtocolGuid = { 0x962e8b44,
>>>>>>> + 0x23b3, 0x41da, { 0x9f, 0x36, 0xca, 0xde, 0x68, 0x49, 0xfb, 0xf6
>>>>>>> + } }
>>>>>>> +
>>>>>>>      [PcdsFixedAtBuild]
>>>>>>>
>>>>>>>        # Maximum number of Custom ACPI Generators diff --git
>>>>>>> a/DynamicTablesPkg/Include/Protocol/SsdtPcieOverrideProtocol.h
>>>>>>> b/DynamicTablesPkg/Include/Protocol/SsdtPcieOverrideProtocol.h
>>>>>>> new file mode 100644
>>>>>>> index 0000000000..29568a0159
>>>>>>> --- /dev/null
>>>>>>> +++
>> b/DynamicTablesPkg/Include/Protocol/SsdtPcieOverrideProtocol.h
>>>>>>> @@ -0,0 +1,63 @@
>>>>>>> +/** @file
>>>>>>> +
>>>>>>> +  Copyright (c) 2022, NVIDIA CORPORATION. All rights reserved.
>>>>>>> +
>>>>>>> +  SPDX-License-Identifier: BSD-2-Clause-Patent
>>>>>>> +
>>>>>>> +**/
>>>>>>> +
>>>>>>> +#ifndef SSDT_PCIE_OVERRIDE_PROTOCOL_H_ #define
>>>>>>> +SSDT_PCIE_OVERRIDE_PROTOCOL_H_
>>>>>>> +
>>>>>>> +#include <ArmNameSpaceObjects.h>
>>>>>>> +#include <Library/AmlLib/AmlLib.h>
>>>>>>> +
>>>>>>> +/** This macro defines the SSDT PCI Override Protocol GUID.
>>>>>>> +
>>>>>>> +  GUID: {D85A4835-5A82-4894-AC02-706F43D5978E}
>>>>>>> +*/
>>>>>>> +#define EDKII_SSDT_PCI_OVERRIDE_PROTOCOL_GUID           \
>>>>>>> +  { 0x962e8b44, 0x23b3, 0x41da,                         \
>>>>>>> +    { 0x9f, 0x36, 0xca, 0xde, 0x68, 0x49, 0xfb, 0xf6 }  \
>>>>>>> +  };
>>>>>>> +
>>>>>>> +/**
>>>>>>> +  Forward declarations:
>>>>>>> +*/
>>>>>>> +typedef struct SsdtOverridePciProtocol
>>>>>>> +EDKII_SSDT_PCI_OVERRIDE_PROTOCOL;
>>>>>>> +
>>>>>>> +/** The UpdateTable function allows the override protocol to
>>>>>>> +update
>>>> the
>>>>>>> + *   PCIe SSDT table prior to being created.
>>>>>>> +
>>>>>>> +  @param [in]      This    Pointer to the SSDT PCI Override Protocol.
>>>>>>> +  @param [in]      PciInfo The PCIe configuration info for this node.
>>>>>>> +  @param [in]      Uid     UID that was selected for this PCIe node.
>>>>>>> +  @param [in, out] PciNode Pointer to the PCI node of this ACPI
>> table.
>>>>>>> +
>>>>>>> +  @retval EFI_SUCCESS           Success.
>>>>>>> +  @retval EFI_INVALID_PARAMETER A parameter is invalid.
>>>>>>> +  @retval EFI_DEVICE_ERROR      Failed to update the table.
>>>>>>> +**/
>>>>>>> +typedef
>>>>>>> +EFI_STATUS
>>>>>>> +(EFIAPI *EDKII_SSDT_PCI_OVERRIDE_PROTOCOL_UPDATE_TABLE)(
>>>>>>> +  IN      CONST EDKII_SSDT_PCI_OVERRIDE_PROTOCOL  *CONST  This,
>>>>>>> +  IN      CONST CM_ARM_PCI_CONFIG_SPACE_INFO              *PciInfo,
>>>>>>> +  IN            UINT32                                    Uid,
>>>>>>> +  IN  OUT       AML_ROOT_NODE_HANDLE                      *PciNode
>>>>>>> +  );
>>>>>>> +
>>>>>>> +/** The EDKII_CONFIGURATION_MANAGER_PROTOCOL structure
>>>>>> describes the
>>>>>>> +    Configuration Manager Protocol interface.
>>>>>>> +*/
>>>>>>> +typedef struct SsdtOverridePciProtocol {
>>>>>>> +  /** The interface used to update the ACPI table for PCI.
>>>>>>> +  */
>>>>>>> +  EDKII_SSDT_PCI_OVERRIDE_PROTOCOL_UPDATE_TABLE
>>>> UpdateTable;
>>>>>>> +} EDKII_SSDT_PCI_OVERRIDE_PROTOCOL;
>>>>>>> +
>>>>>>> +/** The SSDT PCI Override Protocol GUID.
>>>>>>> +*/
>>>>>>> +extern EFI_GUID  gEdkiiDynamicTableSsdtPcieOverrideProtocolGuid;
>>>>>>> +
>>>>>>> +#endif // SSDT_PCIE_OVERRIDE_PROTOCOL_H_
>>>>>>> diff --git
>>>>>>>
>>>>>>
>>>>
>> a/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtPcieLibArm/SsdtPcieGenera
>>>>>> t
>>>>>>> or.c
>>>>>>>
>>>>>>
>>>>
>> b/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtPcieLibArm/SsdtPcieGenera
>>>>>> t
>>>>>>> or.c
>>>>>>> index c5b23d91d0..d5982c24ff 100644
>>>>>>> ---
>>>>>>>
>>>>>>
>>>>
>> a/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtPcieLibArm/SsdtPcieGenera
>>>>>> t
>>>>>>> or.c
>>>>>>> +++
>>>>>>
>> b/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtPcieLibArm/SsdtPcieGen
>>>>>>> +++ erator.c
>>>>>>> @@ -20,6 +20,7 @@
>>>>>>>      #include <Library/BaseMemoryLib.h>
>>>>>>>      #include <Library/DebugLib.h>
>>>>>>>      #include <Library/MemoryAllocationLib.h>
>>>>>>> +#include <Library/UefiBootServicesTableLib.h>
>>>>>>>      #include <Protocol/AcpiTable.h>
>>>>>>>
>>>>>>>      // Module specific include files.
>>>>>>> @@ -30,6 +31,7 @@
>>>>>>>      #include <Library/TableHelperLib.h>
>>>>>>>      #include <Library/AmlLib/AmlLib.h>
>>>>>>>      #include <Protocol/ConfigurationManagerProtocol.h>
>>>>>>> +#include <Protocol/SsdtPcieOverrideProtocol.h>
>>>>>>>
>>>>>>>      #include "SsdtPcieGenerator.h"
>>>>>>>
>>>>>>> @@ -798,9 +800,10 @@ GeneratePciDevice (
>>>>>>>      {
>>>>>>>        EFI_STATUS  Status;
>>>>>>>
>>>>>>> -  CHAR8                   AslName[AML_NAME_SEG_SIZE + 1];
>>>>>>> -  AML_OBJECT_NODE_HANDLE  ScopeNode;
>>>>>>> -  AML_OBJECT_NODE_HANDLE  PciNode;
>>>>>>> +  CHAR8                             AslName[AML_NAME_SEG_SIZE + 1];
>>>>>>> +  AML_OBJECT_NODE_HANDLE            ScopeNode;
>>>>>>> +  AML_OBJECT_NODE_HANDLE            PciNode;
>>>>>>> +  EDKII_SSDT_PCI_OVERRIDE_PROTOCOL  *OverrideProtocol;
>>>>>>>
>>>>>>>        ASSERT (Generator != NULL);
>>>>>>>        ASSERT (CfgMgrProtocol != NULL); @@ -860,6 +863,28 @@
>>>>>>> GeneratePciDevice (
>>>>>>>
>>>>>>>        // Add the template _OSC method.
>>>>>>>        Status = AddOscMethod (PciNode);
>>>>>>> +  if (EFI_ERROR (Status)) {
>>>>>>> +    ASSERT (0);
>>>>>>> +    return Status;
>>>>>>> +  }
>>>>>>> +
>>>>>>> +  Status = gBS->LocateProtocol (
>>>>>>> +                  &gEdkiiDynamicTableSsdtPcieOverrideProtocolGuid,
>>>>>>> +                  NULL,
>>>>>>> +                  (VOID **)&OverrideProtocol
>>>>>>> +                  );
>>>>>>> +  if (!EFI_ERROR (Status)) {
>>>>>>> +    Status = OverrideProtocol->UpdateTable (
>>>>>>> +                                 OverrideProtocol,
>>>>>>> +                                 PciInfo,
>>>>>>> +                                 Uid,
>>>>>>> +                                 PciNode
>>>>>>> +                                 );  } else {
>>>>>>> +    // Not an error if override protocol is not found
>>>>>>> +    Status = EFI_SUCCESS;
>>>>>>> +  }
>>>>>>> +
>>>>>>>        ASSERT_EFI_ERROR (Status);
>>>>>>>        return Status;
>>>>>>>      }
>>>>>>> diff --git
>>>>>>>
>>>>>>
>>>>
>> a/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtPcieLibArm/SsdtPcieLibArm.
>>>>>>> inf
>>>>>>>
>>>>>>
>>>>
>> b/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtPcieLibArm/SsdtPcieLibArm.
>>>>>>> inf
>>>>>>> index 431e32a777..8e916f15e9 100644
>>>>>>> ---
>>>>>>>
>>>>>>
>>>>
>> a/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtPcieLibArm/SsdtPcieLibArm.
>>>>>>> inf
>>>>>>> +++
>>>>>>
>> b/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtPcieLibArm/SsdtPcieLib
>>>>>>> +++ Arm.inf
>>>>>>> @@ -30,6 +30,10 @@
>>>>>>>        AcpiHelperLib
>>>>>>>        AmlLib
>>>>>>>        BaseLib
>>>>>>> +  UefiBootServicesTableLib
>>>>>>>
>>>>>>>      [Pcd]
>>>>>>>        gEdkiiDynamicTablesPkgTokenSpaceGuid.PcdPciUseSegmentAsUid
>>>>>>> +
>>>>>>> +[Protocols]
>>>>>>> +  gEdkiiDynamicTableSsdtPcieOverrideProtocolGuid