[PATCH 0/4] DynamicTablesPkg: Pcie generation updates

[PATCH 0/4] DynamicTablesPkg: Pcie generation updates

[Jeff Brasen]

Add fixes/features to dynamic PCIe support

- Correct issue with translation in generated ACPI tables.
- Allow for more than 16 controllers to be generated.
- Allow optional use of segment number as UID for cases where ACPI path is needed in other places.
- Add support for override protocol that allows platform specific modification of node prior to creation.

Jeff Brasen (4):
  DynamicTablesPkg: AcpiSsdtPcieLibArm: Correct translation value
  DynamicTablesPkg: AcpiSsdtPcieLibArm: Allow use of segment number as
    UID
  DynamicTablesPkg: AcpiSsdtPcieLibArm: Support UID > 0xF
  DynamicTablesPkg: AcpiSsdtPcieLibArm: Add support for override
    protocol

 DynamicTablesPkg/DynamicTablesPkg.dec         |  6 ++
 .../Protocol/SsdtPcieOverrideProtocol.h       | 63 +++++++++++++++++++
 .../AcpiSsdtPcieLibArm/SsdtPcieGenerator.c    | 61 +++++++++++++++---
 .../AcpiSsdtPcieLibArm/SsdtPcieGenerator.h    |  2 +-
 .../Arm/AcpiSsdtPcieLibArm/SsdtPcieLibArm.inf |  7 +++
 5 files changed, 130 insertions(+), 9 deletions(-)
 create mode 100644 DynamicTablesPkg/Include/Protocol/SsdtPcieOverrideProtocol.h

-- 
2.25.1

[PATCH 1/4] DynamicTablesPkg: AcpiSsdtPcieLibArm: Correct translation value

[Jeff Brasen]

The translation value in ACPI should be the difference between the CPU and PCIe address.

Signed-off-by: Jeff Brasen <jbrasen@nvidia.com>
---
 .../Library/Acpi/Arm/AcpiSsdtPcieLibArm/SsdtPcieGenerator.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtPcieLibArm/SsdtPcieGenerator.c b/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtPcieLibArm/SsdtPcieGenerator.c
index a34018151f..f0d15f69a4 100644
--- a/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtPcieLibArm/SsdtPcieGenerator.c
+++ b/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtPcieLibArm/SsdtPcieGenerator.c
@@ -621,7 +621,7 @@ GeneratePciCrs (
                    0,
                    AddrMapInfo->PciAddress,
                    AddrMapInfo->PciAddress + AddrMapInfo->AddressSize - 1,
-                   Translation ? AddrMapInfo->CpuAddress : 0,
+                   Translation ? AddrMapInfo->CpuAddress - AddrMapInfo->PciAddress : 0,
                    AddrMapInfo->AddressSize,
                    0,
                    NULL,
@@ -643,7 +643,7 @@ GeneratePciCrs (
                    0,
                    AddrMapInfo->PciAddress,
                    AddrMapInfo->PciAddress + AddrMapInfo->AddressSize - 1,
-                   Translation ? AddrMapInfo->CpuAddress : 0,
+                   Translation ? AddrMapInfo->CpuAddress - AddrMapInfo->PciAddress : 0,
                    AddrMapInfo->AddressSize,
                    0,
                    NULL,
@@ -665,7 +665,7 @@ GeneratePciCrs (
                    0,
                    AddrMapInfo->PciAddress,
                    AddrMapInfo->PciAddress + AddrMapInfo->AddressSize - 1,
-                   Translation ? AddrMapInfo->CpuAddress : 0,
+                   Translation ? AddrMapInfo->CpuAddress - AddrMapInfo->PciAddress : 0,
                    AddrMapInfo->AddressSize,
                    0,
                    NULL,
-- 
2.25.1

Re: [PATCH 1/4] DynamicTablesPkg: AcpiSsdtPcieLibArm: Correct translation value

[PierreGondois]

Hello Jeff,
The patch is correct, but there will be an issue if
AddrMapInfo->CpuAddress - AddrMapInfo->PciAddress < 0

AmlCodeGenRdDWordIo() takes an 'IsPosDecode' argment,
would it be possible to set IsPosDecode to FALSE in such case ?

Regards,
Pierre

On 6/30/22 17:48, Jeff Brasen wrote:
> The translation value in ACPI should be the difference between the CPU and PCIe address.
> 
> Signed-off-by: Jeff Brasen <jbrasen@nvidia.com>
> ---
>   .../Library/Acpi/Arm/AcpiSsdtPcieLibArm/SsdtPcieGenerator.c | 6 +++---
>   1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtPcieLibArm/SsdtPcieGenerator.c b/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtPcieLibArm/SsdtPcieGenerator.c
> index a34018151f..f0d15f69a4 100644
> --- a/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtPcieLibArm/SsdtPcieGenerator.c
> +++ b/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtPcieLibArm/SsdtPcieGenerator.c
> @@ -621,7 +621,7 @@ GeneratePciCrs (
>                      0,
>                      AddrMapInfo->PciAddress,
>                      AddrMapInfo->PciAddress + AddrMapInfo->AddressSize - 1,
> -                   Translation ? AddrMapInfo->CpuAddress : 0,
> +                   Translation ? AddrMapInfo->CpuAddress - AddrMapInfo->PciAddress : 0,
>                      AddrMapInfo->AddressSize,
>                      0,
>                      NULL,
> @@ -643,7 +643,7 @@ GeneratePciCrs (
>                      0,
>                      AddrMapInfo->PciAddress,
>                      AddrMapInfo->PciAddress + AddrMapInfo->AddressSize - 1,
> -                   Translation ? AddrMapInfo->CpuAddress : 0,
> +                   Translation ? AddrMapInfo->CpuAddress - AddrMapInfo->PciAddress : 0,
>                      AddrMapInfo->AddressSize,
>                      0,
>                      NULL,
> @@ -665,7 +665,7 @@ GeneratePciCrs (
>                      0,
>                      AddrMapInfo->PciAddress,
>                      AddrMapInfo->PciAddress + AddrMapInfo->AddressSize - 1,
> -                   Translation ? AddrMapInfo->CpuAddress : 0,
> +                   Translation ? AddrMapInfo->CpuAddress - AddrMapInfo->PciAddress : 0,
>                      AddrMapInfo->AddressSize,
>                      0,
>                      NULL,

Re: [PATCH 1/4] DynamicTablesPkg: AcpiSsdtPcieLibArm: Correct translation value

[Jeff Brasen]

Yes, I'll try to add a v2 patch for this shortly.

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 1/4] DynamicTablesPkg: AcpiSsdtPcieLibArm: Correct
> translation value
> 
> External email: Use caution opening links or attachments
> 
> 
> Hello Jeff,
> The patch is correct, but there will be an issue if
> AddrMapInfo->CpuAddress - AddrMapInfo->PciAddress < 0
> 
> AmlCodeGenRdDWordIo() takes an 'IsPosDecode' argment, would it be
> possible to set IsPosDecode to FALSE in such case ?
> 
> Regards,
> Pierre
> 
> On 6/30/22 17:48, Jeff Brasen wrote:
> > The translation value in ACPI should be the difference between the CPU
> and PCIe address.
> >
> > Signed-off-by: Jeff Brasen <jbrasen@nvidia.com>
> > ---
> >   .../Library/Acpi/Arm/AcpiSsdtPcieLibArm/SsdtPcieGenerator.c | 6 +++---
> >   1 file changed, 3 insertions(+), 3 deletions(-)
> >
> > diff --git
> >
> a/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtPcieLibArm/SsdtPcieGenera
> t
> > or.c
> >
> b/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtPcieLibArm/SsdtPcieGenera
> t
> > or.c
> > index a34018151f..f0d15f69a4 100644
> > ---
> >
> a/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtPcieLibArm/SsdtPcieGenera
> t
> > or.c
> > +++
> b/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtPcieLibArm/SsdtPcieGen
> > +++ erator.c
> > @@ -621,7 +621,7 @@ GeneratePciCrs (
> >                      0,
> >                      AddrMapInfo->PciAddress,
> >                      AddrMapInfo->PciAddress + AddrMapInfo->AddressSize - 1,
> > -                   Translation ? AddrMapInfo->CpuAddress : 0,
> > +                   Translation ? AddrMapInfo->CpuAddress -
> > + AddrMapInfo->PciAddress : 0,
> >                      AddrMapInfo->AddressSize,
> >                      0,
> >                      NULL,
> > @@ -643,7 +643,7 @@ GeneratePciCrs (
> >                      0,
> >                      AddrMapInfo->PciAddress,
> >                      AddrMapInfo->PciAddress + AddrMapInfo->AddressSize - 1,
> > -                   Translation ? AddrMapInfo->CpuAddress : 0,
> > +                   Translation ? AddrMapInfo->CpuAddress -
> > + AddrMapInfo->PciAddress : 0,
> >                      AddrMapInfo->AddressSize,
> >                      0,
> >                      NULL,
> > @@ -665,7 +665,7 @@ GeneratePciCrs (
> >                      0,
> >                      AddrMapInfo->PciAddress,
> >                      AddrMapInfo->PciAddress + AddrMapInfo->AddressSize - 1,
> > -                   Translation ? AddrMapInfo->CpuAddress : 0,
> > +                   Translation ? AddrMapInfo->CpuAddress -
> > + AddrMapInfo->PciAddress : 0,
> >                      AddrMapInfo->AddressSize,
> >                      0,
> >                      NULL,

[PATCH 2/4] DynamicTablesPkg: AcpiSsdtPcieLibArm: Allow use of segment number as UID

[Jeff Brasen]

Add support for selecting to use index or segment number as UID and name.
This allows the path of the nodes to be well known.

Signed-off-by: Jeff Brasen <jbrasen@nvidia.com>
---
 DynamicTablesPkg/DynamicTablesPkg.dec         |  3 +++
 .../AcpiSsdtPcieLibArm/SsdtPcieGenerator.c    | 19 ++++++++++++++++++-
 .../Arm/AcpiSsdtPcieLibArm/SsdtPcieLibArm.inf |  3 +++
 3 files changed, 24 insertions(+), 1 deletion(-)

diff --git a/DynamicTablesPkg/DynamicTablesPkg.dec b/DynamicTablesPkg/DynamicTablesPkg.dec
index 9b74c5a671..a890a048be 100644
--- a/DynamicTablesPkg/DynamicTablesPkg.dec
+++ b/DynamicTablesPkg/DynamicTablesPkg.dec
@@ -57,5 +57,8 @@
   # Non BSA Compliant 16550 Serial HID
   gEdkiiDynamicTablesPkgTokenSpaceGuid.PcdNonBsaCompliant16550SerialHid|""|VOID*|0x40000008
 
+  # Use PCI segment numbers as UID
+  gEdkiiDynamicTablesPkgTokenSpaceGuid.PcdPciUseSegmentAsUid|FALSE|BOOLEAN|0x40000009
+
 [Guids]
   gEdkiiDynamicTablesPkgTokenSpaceGuid = { 0xab226e66, 0x31d8, 0x4613, { 0x87, 0x9d, 0xd2, 0xfa, 0xb6, 0x10, 0x26, 0x3c } }
diff --git a/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtPcieLibArm/SsdtPcieGenerator.c b/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtPcieLibArm/SsdtPcieGenerator.c
index f0d15f69a4..85782af380 100644
--- a/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtPcieLibArm/SsdtPcieGenerator.c
+++ b/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtPcieLibArm/SsdtPcieGenerator.c
@@ -996,6 +996,7 @@ BuildSsdtPciTableEx (
   UINTN                         Index;
   EFI_ACPI_DESCRIPTION_HEADER   **TableList;
   ACPI_PCI_GENERATOR            *Generator;
+  UINT32                        Uid;
 
   ASSERT (This != NULL);
   ASSERT (AcpiTableInfo != NULL);
@@ -1051,13 +1052,29 @@ BuildSsdtPciTableEx (
   *Table = TableList;
 
   for (Index = 0; Index < PciCount; Index++) {
+    if (PcdGetBool (PcdPciUseSegmentAsUid)) {
+      Uid = PciInfo[Index].PciSegmentGroupNumber;
+      if (Uid > MAX_PCI_ROOT_COMPLEXES_SUPPORTED) {
+        DEBUG ((
+          DEBUG_ERROR,
+          "ERROR: SSDT-PCI: Pci root complexes segment number: %d."
+          " Greater than maximum number of Pci root complexes supported = %d.\n",
+          Uid,
+          MAX_PCI_ROOT_COMPLEXES_SUPPORTED
+          ));
+        return EFI_INVALID_PARAMETER;
+      }
+    } else {
+      Uid = Index;
+    }
+
     // Build a SSDT table describing the Pci devices.
     Status = BuildSsdtPciTable (
                Generator,
                CfgMgrProtocol,
                AcpiTableInfo,
                &PciInfo[Index],
-               Index,
+               Uid,
                &TableList[Index]
                );
     if (EFI_ERROR (Status)) {
diff --git a/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtPcieLibArm/SsdtPcieLibArm.inf b/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtPcieLibArm/SsdtPcieLibArm.inf
index 283b564801..431e32a777 100644
--- a/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtPcieLibArm/SsdtPcieLibArm.inf
+++ b/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtPcieLibArm/SsdtPcieLibArm.inf
@@ -30,3 +30,6 @@
   AcpiHelperLib
   AmlLib
   BaseLib
+
+[Pcd]
+  gEdkiiDynamicTablesPkgTokenSpaceGuid.PcdPciUseSegmentAsUid
-- 
2.25.1

Re: [PATCH 2/4] DynamicTablesPkg: AcpiSsdtPcieLibArm: Allow use of segment number as UID

[PierreGondois]

Hello Jeff,

 From "PCI Firmware Specification Revision 3.3", s4.1.2.
"MCFG Table Description", the "PCI Segment Group Number" field of the
MCFG table must match the value returned by the _SEG object in the
corresponding object.

I didn't find any constraint about the _UID value. Would it be possible
to know why this is required ?

Regards,
Pierre

On 6/30/22 17:48, Jeff Brasen wrote:
> Add support for selecting to use index or segment number as UID and name.
> This allows the path of the nodes to be well known.
> 
> Signed-off-by: Jeff Brasen <jbrasen@nvidia.com>
> ---
>   DynamicTablesPkg/DynamicTablesPkg.dec         |  3 +++
>   .../AcpiSsdtPcieLibArm/SsdtPcieGenerator.c    | 19 ++++++++++++++++++-
>   .../Arm/AcpiSsdtPcieLibArm/SsdtPcieLibArm.inf |  3 +++
>   3 files changed, 24 insertions(+), 1 deletion(-)
> 
> diff --git a/DynamicTablesPkg/DynamicTablesPkg.dec b/DynamicTablesPkg/DynamicTablesPkg.dec
> index 9b74c5a671..a890a048be 100644
> --- a/DynamicTablesPkg/DynamicTablesPkg.dec
> +++ b/DynamicTablesPkg/DynamicTablesPkg.dec
> @@ -57,5 +57,8 @@
>     # Non BSA Compliant 16550 Serial HID
>     gEdkiiDynamicTablesPkgTokenSpaceGuid.PcdNonBsaCompliant16550SerialHid|""|VOID*|0x40000008
>   
> +  # Use PCI segment numbers as UID
> +  gEdkiiDynamicTablesPkgTokenSpaceGuid.PcdPciUseSegmentAsUid|FALSE|BOOLEAN|0x40000009
> +
>   [Guids]
>     gEdkiiDynamicTablesPkgTokenSpaceGuid = { 0xab226e66, 0x31d8, 0x4613, { 0x87, 0x9d, 0xd2, 0xfa, 0xb6, 0x10, 0x26, 0x3c } }
> diff --git a/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtPcieLibArm/SsdtPcieGenerator.c b/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtPcieLibArm/SsdtPcieGenerator.c
> index f0d15f69a4..85782af380 100644
> --- a/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtPcieLibArm/SsdtPcieGenerator.c
> +++ b/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtPcieLibArm/SsdtPcieGenerator.c
> @@ -996,6 +996,7 @@ BuildSsdtPciTableEx (
>     UINTN                         Index;
>     EFI_ACPI_DESCRIPTION_HEADER   **TableList;
>     ACPI_PCI_GENERATOR            *Generator;
> +  UINT32                        Uid;
>   
>     ASSERT (This != NULL);
>     ASSERT (AcpiTableInfo != NULL);
> @@ -1051,13 +1052,29 @@ BuildSsdtPciTableEx (
>     *Table = TableList;
>   
>     for (Index = 0; Index < PciCount; Index++) {
> +    if (PcdGetBool (PcdPciUseSegmentAsUid)) {
> +      Uid = PciInfo[Index].PciSegmentGroupNumber;
> +      if (Uid > MAX_PCI_ROOT_COMPLEXES_SUPPORTED) {
> +        DEBUG ((
> +          DEBUG_ERROR,
> +          "ERROR: SSDT-PCI: Pci root complexes segment number: %d."
> +          " Greater than maximum number of Pci root complexes supported = %d.\n",
> +          Uid,
> +          MAX_PCI_ROOT_COMPLEXES_SUPPORTED
> +          ));
> +        return EFI_INVALID_PARAMETER;
> +      }
> +    } else {
> +      Uid = Index;
> +    }
> +
>       // Build a SSDT table describing the Pci devices.
>       Status = BuildSsdtPciTable (
>                  Generator,
>                  CfgMgrProtocol,
>                  AcpiTableInfo,
>                  &PciInfo[Index],
> -               Index,
> +               Uid,
>                  &TableList[Index]
>                  );
>       if (EFI_ERROR (Status)) {
> diff --git a/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtPcieLibArm/SsdtPcieLibArm.inf b/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtPcieLibArm/SsdtPcieLibArm.inf
> index 283b564801..431e32a777 100644
> --- a/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtPcieLibArm/SsdtPcieLibArm.inf
> +++ b/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtPcieLibArm/SsdtPcieLibArm.inf
> @@ -30,3 +30,6 @@
>     AcpiHelperLib
>     AmlLib
>     BaseLib
> +
> +[Pcd]
> +  gEdkiiDynamicTablesPkgTokenSpaceGuid.PcdPciUseSegmentAsUid

Re: [PATCH 2/4] DynamicTablesPkg: AcpiSsdtPcieLibArm: Allow use of segment number as UID

[Jeff Brasen]

I don't think there are any requirements that this should be mapped this way which is why I added this under a PCD to enable it. We have a use case where it would be helpful to know the ACPI path of the PCIe controllers externally and using the segment number for that would allow us to do this without having to have matching logic that calculates the PCIe UID from the CM object list again.

Thanks,
Jeff


> -----Original Message-----
> From: Pierre Gondois <pierre.gondois@arm.com>
> Sent: Friday, July 1, 2022 6:42 AM
> To: Jeff Brasen <jbrasen@nvidia.com>; devel@edk2.groups.io
> Cc: Sami.Mujawar@arm.com; Alexei.Fedorov@arm.com
> Subject: Re: [PATCH 2/4] DynamicTablesPkg: AcpiSsdtPcieLibArm: Allow use
> of segment number as UID
> 
> External email: Use caution opening links or attachments
> 
> 
> Hello Jeff,
> 
>  From "PCI Firmware Specification Revision 3.3", s4.1.2.
> "MCFG Table Description", the "PCI Segment Group Number" field of the
> MCFG table must match the value returned by the _SEG object in the
> corresponding object.
> 
> I didn't find any constraint about the _UID value. Would it be possible to
> know why this is required ?
> 
> Regards,
> Pierre
> 
> On 6/30/22 17:48, Jeff Brasen wrote:
> > Add support for selecting to use index or segment number as UID and
> name.
> > This allows the path of the nodes to be well known.
> >
> > Signed-off-by: Jeff Brasen <jbrasen@nvidia.com>
> > ---
> >   DynamicTablesPkg/DynamicTablesPkg.dec         |  3 +++
> >   .../AcpiSsdtPcieLibArm/SsdtPcieGenerator.c    | 19
> ++++++++++++++++++-
> >   .../Arm/AcpiSsdtPcieLibArm/SsdtPcieLibArm.inf |  3 +++
> >   3 files changed, 24 insertions(+), 1 deletion(-)
> >
> > diff --git a/DynamicTablesPkg/DynamicTablesPkg.dec
> > b/DynamicTablesPkg/DynamicTablesPkg.dec
> > index 9b74c5a671..a890a048be 100644
> > --- a/DynamicTablesPkg/DynamicTablesPkg.dec
> > +++ b/DynamicTablesPkg/DynamicTablesPkg.dec
> > @@ -57,5 +57,8 @@
> >     # Non BSA Compliant 16550 Serial HID
> >
> >
> gEdkiiDynamicTablesPkgTokenSpaceGuid.PcdNonBsaCompliant16550SerialHi
> d|
> > ""|VOID*|0x40000008
> >
> > +  # Use PCI segment numbers as UID
> > +
> > +
> gEdkiiDynamicTablesPkgTokenSpaceGuid.PcdPciUseSegmentAsUid|FALSE|B
> OO
> > + LEAN|0x40000009
> > +
> >   [Guids]
> >     gEdkiiDynamicTablesPkgTokenSpaceGuid = { 0xab226e66, 0x31d8,
> > 0x4613, { 0x87, 0x9d, 0xd2, 0xfa, 0xb6, 0x10, 0x26, 0x3c } } diff
> > --git
> >
> a/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtPcieLibArm/SsdtPcieGenera
> t
> > or.c
> >
> b/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtPcieLibArm/SsdtPcieGenera
> t
> > or.c
> > index f0d15f69a4..85782af380 100644
> > ---
> >
> a/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtPcieLibArm/SsdtPcieGenera
> t
> > or.c
> > +++
> b/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtPcieLibArm/SsdtPcieGen
> > +++ erator.c
> > @@ -996,6 +996,7 @@ BuildSsdtPciTableEx (
> >     UINTN                         Index;
> >     EFI_ACPI_DESCRIPTION_HEADER   **TableList;
> >     ACPI_PCI_GENERATOR            *Generator;
> > +  UINT32                        Uid;
> >
> >     ASSERT (This != NULL);
> >     ASSERT (AcpiTableInfo != NULL);
> > @@ -1051,13 +1052,29 @@ BuildSsdtPciTableEx (
> >     *Table = TableList;
> >
> >     for (Index = 0; Index < PciCount; Index++) {
> > +    if (PcdGetBool (PcdPciUseSegmentAsUid)) {
> > +      Uid = PciInfo[Index].PciSegmentGroupNumber;
> > +      if (Uid > MAX_PCI_ROOT_COMPLEXES_SUPPORTED) {
> > +        DEBUG ((
> > +          DEBUG_ERROR,
> > +          "ERROR: SSDT-PCI: Pci root complexes segment number: %d."
> > +          " Greater than maximum number of Pci root complexes supported =
> %d.\n",
> > +          Uid,
> > +          MAX_PCI_ROOT_COMPLEXES_SUPPORTED
> > +          ));
> > +        return EFI_INVALID_PARAMETER;
> > +      }
> > +    } else {
> > +      Uid = Index;
> > +    }
> > +
> >       // Build a SSDT table describing the Pci devices.
> >       Status = BuildSsdtPciTable (
> >                  Generator,
> >                  CfgMgrProtocol,
> >                  AcpiTableInfo,
> >                  &PciInfo[Index],
> > -               Index,
> > +               Uid,
> >                  &TableList[Index]
> >                  );
> >       if (EFI_ERROR (Status)) {
> > diff --git
> >
> a/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtPcieLibArm/SsdtPcieLibArm.
> > inf
> >
> b/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtPcieLibArm/SsdtPcieLibArm.
> > inf
> > index 283b564801..431e32a777 100644
> > ---
> >
> a/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtPcieLibArm/SsdtPcieLibArm.
> > inf
> > +++
> b/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtPcieLibArm/SsdtPcieLib
> > +++ Arm.inf
> > @@ -30,3 +30,6 @@
> >     AcpiHelperLib
> >     AmlLib
> >     BaseLib
> > +
> > +[Pcd]
> > +  gEdkiiDynamicTablesPkgTokenSpaceGuid.PcdPciUseSegmentAsUid

Re: [PATCH 2/4] DynamicTablesPkg: AcpiSsdtPcieLibArm: Allow use of segment number as UID

[PierreGondois]

Hello Jeff,
I understand that since the _UID value is generated, it is not possible for
an external module to guess its value.
However, the pcd (and the case you detailed) seem to be really specific.
If you need an interface in the AmlLib to locate an AML node based on custom
callback, we can help.
Would this fit your need ?
Also I don't understand how having a known _UID would ease locating the
PCI device node.

Regards,
Pierre

On 7/1/22 17:54, Jeff Brasen wrote:
> I don't think there are any requirements that this should be mapped this way which is why I added this under a PCD to enable it. We have a use case where it would be helpful to know the ACPI path of the PCIe controllers externally and using the segment number for that would allow us to do this without having to have matching logic that calculates the PCIe UID from the CM object list again.
> 
> Thanks,
> Jeff
> 
> 
>> -----Original Message-----
>> From: Pierre Gondois <pierre.gondois@arm.com>
>> Sent: Friday, July 1, 2022 6:42 AM
>> To: Jeff Brasen <jbrasen@nvidia.com>; devel@edk2.groups.io
>> Cc: Sami.Mujawar@arm.com; Alexei.Fedorov@arm.com
>> Subject: Re: [PATCH 2/4] DynamicTablesPkg: AcpiSsdtPcieLibArm: Allow use
>> of segment number as UID
>>
>> External email: Use caution opening links or attachments
>>
>>
>> Hello Jeff,
>>
>>   From "PCI Firmware Specification Revision 3.3", s4.1.2.
>> "MCFG Table Description", the "PCI Segment Group Number" field of the
>> MCFG table must match the value returned by the _SEG object in the
>> corresponding object.
>>
>> I didn't find any constraint about the _UID value. Would it be possible to
>> know why this is required ?
>>
>> Regards,
>> Pierre
>>
>> On 6/30/22 17:48, Jeff Brasen wrote:
>>> Add support for selecting to use index or segment number as UID and
>> name.
>>> This allows the path of the nodes to be well known.
>>>
>>> Signed-off-by: Jeff Brasen <jbrasen@nvidia.com>
>>> ---
>>>    DynamicTablesPkg/DynamicTablesPkg.dec         |  3 +++
>>>    .../AcpiSsdtPcieLibArm/SsdtPcieGenerator.c    | 19
>> ++++++++++++++++++-
>>>    .../Arm/AcpiSsdtPcieLibArm/SsdtPcieLibArm.inf |  3 +++
>>>    3 files changed, 24 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/DynamicTablesPkg/DynamicTablesPkg.dec
>>> b/DynamicTablesPkg/DynamicTablesPkg.dec
>>> index 9b74c5a671..a890a048be 100644
>>> --- a/DynamicTablesPkg/DynamicTablesPkg.dec
>>> +++ b/DynamicTablesPkg/DynamicTablesPkg.dec
>>> @@ -57,5 +57,8 @@
>>>      # Non BSA Compliant 16550 Serial HID
>>>
>>>
>> gEdkiiDynamicTablesPkgTokenSpaceGuid.PcdNonBsaCompliant16550SerialHi
>> d|
>>> ""|VOID*|0x40000008
>>>
>>> +  # Use PCI segment numbers as UID
>>> +
>>> +
>> gEdkiiDynamicTablesPkgTokenSpaceGuid.PcdPciUseSegmentAsUid|FALSE|B
>> OO
>>> + LEAN|0x40000009
>>> +
>>>    [Guids]
>>>      gEdkiiDynamicTablesPkgTokenSpaceGuid = { 0xab226e66, 0x31d8,
>>> 0x4613, { 0x87, 0x9d, 0xd2, 0xfa, 0xb6, 0x10, 0x26, 0x3c } } diff
>>> --git
>>>
>> a/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtPcieLibArm/SsdtPcieGenera
>> t
>>> or.c
>>>
>> b/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtPcieLibArm/SsdtPcieGenera
>> t
>>> or.c
>>> index f0d15f69a4..85782af380 100644
>>> ---
>>>
>> a/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtPcieLibArm/SsdtPcieGenera
>> t
>>> or.c
>>> +++
>> b/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtPcieLibArm/SsdtPcieGen
>>> +++ erator.c
>>> @@ -996,6 +996,7 @@ BuildSsdtPciTableEx (
>>>      UINTN                         Index;
>>>      EFI_ACPI_DESCRIPTION_HEADER   **TableList;
>>>      ACPI_PCI_GENERATOR            *Generator;
>>> +  UINT32                        Uid;
>>>
>>>      ASSERT (This != NULL);
>>>      ASSERT (AcpiTableInfo != NULL);
>>> @@ -1051,13 +1052,29 @@ BuildSsdtPciTableEx (
>>>      *Table = TableList;
>>>
>>>      for (Index = 0; Index < PciCount; Index++) {
>>> +    if (PcdGetBool (PcdPciUseSegmentAsUid)) {
>>> +      Uid = PciInfo[Index].PciSegmentGroupNumber;
>>> +      if (Uid > MAX_PCI_ROOT_COMPLEXES_SUPPORTED) {
>>> +        DEBUG ((
>>> +          DEBUG_ERROR,
>>> +          "ERROR: SSDT-PCI: Pci root complexes segment number: %d."
>>> +          " Greater than maximum number of Pci root complexes supported =
>> %d.\n",
>>> +          Uid,
>>> +          MAX_PCI_ROOT_COMPLEXES_SUPPORTED
>>> +          ));
>>> +        return EFI_INVALID_PARAMETER;
>>> +      }
>>> +    } else {
>>> +      Uid = Index;
>>> +    }
>>> +
>>>        // Build a SSDT table describing the Pci devices.
>>>        Status = BuildSsdtPciTable (
>>>                   Generator,
>>>                   CfgMgrProtocol,
>>>                   AcpiTableInfo,
>>>                   &PciInfo[Index],
>>> -               Index,
>>> +               Uid,
>>>                   &TableList[Index]
>>>                   );
>>>        if (EFI_ERROR (Status)) {
>>> diff --git
>>>
>> a/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtPcieLibArm/SsdtPcieLibArm.
>>> inf
>>>
>> b/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtPcieLibArm/SsdtPcieLibArm.
>>> inf
>>> index 283b564801..431e32a777 100644
>>> ---
>>>
>> a/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtPcieLibArm/SsdtPcieLibArm.
>>> inf
>>> +++
>> b/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtPcieLibArm/SsdtPcieLib
>>> +++ Arm.inf
>>> @@ -30,3 +30,6 @@
>>>      AcpiHelperLib
>>>      AmlLib
>>>      BaseLib
>>> +
>>> +[Pcd]
>>> +  gEdkiiDynamicTablesPkgTokenSpaceGuid.PcdPciUseSegmentAsUid

Re: [PATCH 2/4] DynamicTablesPkg: AcpiSsdtPcieLibArm: Allow use of segment number as UID

[Jeff Brasen]

So to detail the use case we have an embedded device on a known controller (we know the segment number) by forcing the autogeneration to the segment number we could in another SSDT create the subdevice node right. With just \SB\PCI9\D0x as the scope right? If we had a callback I suppose we don't need that. I think we could handle that with the proposed change you had in [4/4] if we just chain the implementation of GeneratePciDeviceInfo to our second place we would implement this. I think we can drop this patch and do it that way. Will move additional thoughts to the other patch.

Thanks,
Jeff


> -----Original Message-----
> From: Pierre Gondois <pierre.gondois@arm.com>
> Sent: Monday, July 4, 2022 2:38 AM
> To: Jeff Brasen <jbrasen@nvidia.com>; devel@edk2.groups.io
> Cc: Sami.Mujawar@arm.com; Alexei.Fedorov@arm.com
> Subject: Re: [PATCH 2/4] DynamicTablesPkg: AcpiSsdtPcieLibArm: Allow use
> of segment number as UID
> 
> External email: Use caution opening links or attachments
> 
> 
> Hello Jeff,
> I understand that since the _UID value is generated, it is not possible for an
> external module to guess its value.
> However, the pcd (and the case you detailed) seem to be really specific.
> If you need an interface in the AmlLib to locate an AML node based on
> custom callback, we can help.
> Would this fit your need ?
> Also I don't understand how having a known _UID would ease locating the
> PCI device node.
> 
> Regards,
> Pierre
> 
> On 7/1/22 17:54, Jeff Brasen wrote:
> > I don't think there are any requirements that this should be mapped this
> way which is why I added this under a PCD to enable it. We have a use case
> where it would be helpful to know the ACPI path of the PCIe controllers
> externally and using the segment number for that would allow us to do this
> without having to have matching logic that calculates the PCIe UID from the
> CM object list again.
> >
> > Thanks,
> > Jeff
> >
> >
> >> -----Original Message-----
> >> From: Pierre Gondois <pierre.gondois@arm.com>
> >> Sent: Friday, July 1, 2022 6:42 AM
> >> To: Jeff Brasen <jbrasen@nvidia.com>; devel@edk2.groups.io
> >> Cc: Sami.Mujawar@arm.com; Alexei.Fedorov@arm.com
> >> Subject: Re: [PATCH 2/4] DynamicTablesPkg: AcpiSsdtPcieLibArm: Allow
> >> use of segment number as UID
> >>
> >> External email: Use caution opening links or attachments
> >>
> >>
> >> Hello Jeff,
> >>
> >>   From "PCI Firmware Specification Revision 3.3", s4.1.2.
> >> "MCFG Table Description", the "PCI Segment Group Number" field of the
> >> MCFG table must match the value returned by the _SEG object in the
> >> corresponding object.
> >>
> >> I didn't find any constraint about the _UID value. Would it be
> >> possible to know why this is required ?
> >>
> >> Regards,
> >> Pierre
> >>
> >> On 6/30/22 17:48, Jeff Brasen wrote:
> >>> Add support for selecting to use index or segment number as UID and
> >> name.
> >>> This allows the path of the nodes to be well known.
> >>>
> >>> Signed-off-by: Jeff Brasen <jbrasen@nvidia.com>
> >>> ---
> >>>    DynamicTablesPkg/DynamicTablesPkg.dec         |  3 +++
> >>>    .../AcpiSsdtPcieLibArm/SsdtPcieGenerator.c    | 19
> >> ++++++++++++++++++-
> >>>    .../Arm/AcpiSsdtPcieLibArm/SsdtPcieLibArm.inf |  3 +++
> >>>    3 files changed, 24 insertions(+), 1 deletion(-)
> >>>
> >>> diff --git a/DynamicTablesPkg/DynamicTablesPkg.dec
> >>> b/DynamicTablesPkg/DynamicTablesPkg.dec
> >>> index 9b74c5a671..a890a048be 100644
> >>> --- a/DynamicTablesPkg/DynamicTablesPkg.dec
> >>> +++ b/DynamicTablesPkg/DynamicTablesPkg.dec
> >>> @@ -57,5 +57,8 @@
> >>>      # Non BSA Compliant 16550 Serial HID
> >>>
> >>>
> >>
> gEdkiiDynamicTablesPkgTokenSpaceGuid.PcdNonBsaCompliant16550SerialHi
> >> d|
> >>> ""|VOID*|0x40000008
> >>>
> >>> +  # Use PCI segment numbers as UID
> >>> +
> >>> +
> >>
> gEdkiiDynamicTablesPkgTokenSpaceGuid.PcdPciUseSegmentAsUid|FALSE|B
> >> OO
> >>> + LEAN|0x40000009
> >>> +
> >>>    [Guids]
> >>>      gEdkiiDynamicTablesPkgTokenSpaceGuid = { 0xab226e66, 0x31d8,
> >>> 0x4613, { 0x87, 0x9d, 0xd2, 0xfa, 0xb6, 0x10, 0x26, 0x3c } } diff
> >>> --git
> >>>
> >>
> a/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtPcieLibArm/SsdtPcieGenera
> >> t
> >>> or.c
> >>>
> >>
> b/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtPcieLibArm/SsdtPcieGenera
> >> t
> >>> or.c
> >>> index f0d15f69a4..85782af380 100644
> >>> ---
> >>>
> >>
> a/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtPcieLibArm/SsdtPcieGenera
> >> t
> >>> or.c
> >>> +++
> >> b/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtPcieLibArm/SsdtPcieGen
> >>> +++ erator.c
> >>> @@ -996,6 +996,7 @@ BuildSsdtPciTableEx (
> >>>      UINTN                         Index;
> >>>      EFI_ACPI_DESCRIPTION_HEADER   **TableList;
> >>>      ACPI_PCI_GENERATOR            *Generator;
> >>> +  UINT32                        Uid;
> >>>
> >>>      ASSERT (This != NULL);
> >>>      ASSERT (AcpiTableInfo != NULL); @@ -1051,13 +1052,29 @@
> >>> BuildSsdtPciTableEx (
> >>>      *Table = TableList;
> >>>
> >>>      for (Index = 0; Index < PciCount; Index++) {
> >>> +    if (PcdGetBool (PcdPciUseSegmentAsUid)) {
> >>> +      Uid = PciInfo[Index].PciSegmentGroupNumber;
> >>> +      if (Uid > MAX_PCI_ROOT_COMPLEXES_SUPPORTED) {
> >>> +        DEBUG ((
> >>> +          DEBUG_ERROR,
> >>> +          "ERROR: SSDT-PCI: Pci root complexes segment number: %d."
> >>> +          " Greater than maximum number of Pci root complexes
> >>> + supported =
> >> %d.\n",
> >>> +          Uid,
> >>> +          MAX_PCI_ROOT_COMPLEXES_SUPPORTED
> >>> +          ));
> >>> +        return EFI_INVALID_PARAMETER;
> >>> +      }
> >>> +    } else {
> >>> +      Uid = Index;
> >>> +    }
> >>> +
> >>>        // Build a SSDT table describing the Pci devices.
> >>>        Status = BuildSsdtPciTable (
> >>>                   Generator,
> >>>                   CfgMgrProtocol,
> >>>                   AcpiTableInfo,
> >>>                   &PciInfo[Index],
> >>> -               Index,
> >>> +               Uid,
> >>>                   &TableList[Index]
> >>>                   );
> >>>        if (EFI_ERROR (Status)) {
> >>> diff --git
> >>>
> >>
> a/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtPcieLibArm/SsdtPcieLibArm.
> >>> inf
> >>>
> >>
> b/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtPcieLibArm/SsdtPcieLibArm.
> >>> inf
> >>> index 283b564801..431e32a777 100644
> >>> ---
> >>>
> >>
> a/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtPcieLibArm/SsdtPcieLibArm.
> >>> inf
> >>> +++
> >> b/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtPcieLibArm/SsdtPcieLib
> >>> +++ Arm.inf
> >>> @@ -30,3 +30,6 @@
> >>>      AcpiHelperLib
> >>>      AmlLib
> >>>      BaseLib
> >>> +
> >>> +[Pcd]
> >>> +  gEdkiiDynamicTablesPkgTokenSpaceGuid.PcdPciUseSegmentAsUid

[PATCH 3/4] DynamicTablesPkg: AcpiSsdtPcieLibArm: Support UID > 0xF

[Jeff Brasen]

Add support for PCIe devices with UID > 0xF.
This is done by using the next value in the name so
PCI5, PC26, etc

Signed-off-by: Jeff Brasen <jbrasen@nvidia.com>
---
 .../Acpi/Arm/AcpiSsdtPcieLibArm/SsdtPcieGenerator.c   | 11 +++++++----
 .../Acpi/Arm/AcpiSsdtPcieLibArm/SsdtPcieGenerator.h   |  2 +-
 2 files changed, 8 insertions(+), 5 deletions(-)

diff --git a/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtPcieLibArm/SsdtPcieGenerator.c b/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtPcieLibArm/SsdtPcieGenerator.c
index 85782af380..c5b23d91d0 100644
--- a/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtPcieLibArm/SsdtPcieGenerator.c
+++ b/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtPcieLibArm/SsdtPcieGenerator.c
@@ -818,7 +818,10 @@ GeneratePciDevice (
 
   // Write the name of the PCI device.
   CopyMem (AslName, "PCIx", AML_NAME_SEG_SIZE + 1);
-  AslName[AML_NAME_SEG_SIZE - 1] = AsciiFromHex (Uid);
+  AslName[AML_NAME_SEG_SIZE - 1] = AsciiFromHex (Uid & 0xF);
+  if (Uid > 0xF) {
+    AslName[AML_NAME_SEG_SIZE - 2] = AsciiFromHex ((Uid >> 4) & 0xF);
+  }
 
   // ASL: Device (PCIx) {}
   Status = AmlCodeGenDevice (AslName, ScopeNode, &PciNode);
@@ -1054,13 +1057,13 @@ BuildSsdtPciTableEx (
   for (Index = 0; Index < PciCount; Index++) {
     if (PcdGetBool (PcdPciUseSegmentAsUid)) {
       Uid = PciInfo[Index].PciSegmentGroupNumber;
-      if (Uid > MAX_PCI_ROOT_COMPLEXES_SUPPORTED) {
+      if (Uid >= MAX_PCI_ROOT_COMPLEXES_SUPPORTED) {
         DEBUG ((
           DEBUG_ERROR,
           "ERROR: SSDT-PCI: Pci root complexes segment number: %d."
-          " Greater than maximum number of Pci root complexes supported = %d.\n",
+          " Greater than maximum supported value = %d.\n",
           Uid,
-          MAX_PCI_ROOT_COMPLEXES_SUPPORTED
+          MAX_PCI_ROOT_COMPLEXES_SUPPORTED - 1
           ));
         return EFI_INVALID_PARAMETER;
       }
diff --git a/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtPcieLibArm/SsdtPcieGenerator.h b/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtPcieLibArm/SsdtPcieGenerator.h
index 59a0d601a3..515a3e1785 100644
--- a/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtPcieLibArm/SsdtPcieGenerator.h
+++ b/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtPcieLibArm/SsdtPcieGenerator.h
@@ -31,7 +31,7 @@
         Corresponding changes would be needed to support the Name and
         UID fields describing the Pci root complexes.
 */
-#define MAX_PCI_ROOT_COMPLEXES_SUPPORTED  16
+#define MAX_PCI_ROOT_COMPLEXES_SUPPORTED  256
 
 // _SB scope of the AML namespace.
 #define SB_SCOPE  "\\_SB_"
-- 
2.25.1

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

[Jeff Brasen]

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/SsdtPcieGenerator.c b/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtPcieLibArm/SsdtPcieGenerator.c
index c5b23d91d0..d5982c24ff 100644
--- a/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtPcieLibArm/SsdtPcieGenerator.c
+++ b/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtPcieLibArm/SsdtPcieGenerator.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/SsdtPcieLibArm.inf
@@ -30,6 +30,10 @@
   AcpiHelperLib
   AmlLib
   BaseLib
+  UefiBootServicesTableLib
 
 [Pcd]
   gEdkiiDynamicTablesPkgTokenSpaceGuid.PcdPciUseSegmentAsUid
+
+[Protocols]
+  gEdkiiDynamicTableSsdtPcieOverrideProtocolGuid
-- 
2.25.1

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

[PierreGondois]

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/SsdtPcieGenerator.c b/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtPcieLibArm/SsdtPcieGenerator.c
> index c5b23d91d0..d5982c24ff 100644
> --- a/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtPcieLibArm/SsdtPcieGenerator.c
> +++ b/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtPcieLibArm/SsdtPcieGenerator.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/SsdtPcieLibArm.inf
> @@ -30,6 +30,10 @@
>     AcpiHelperLib
>     AmlLib
>     BaseLib
> +  UefiBootServicesTableLib
>   
>   [Pcd]
>     gEdkiiDynamicTablesPkgTokenSpaceGuid.PcdPciUseSegmentAsUid
> +
> +[Protocols]
> +  gEdkiiDynamicTableSsdtPcieOverrideProtocolGuid

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

[Jeff Brasen]

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

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

[PierreGondois]

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

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

[Jeff Brasen]

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

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

[PierreGondois]

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

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

[Jeff Brasen]

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. 

-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

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

[PierreGondois]

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

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

[Jeff Brasen]

Yes we have systems where we will have more than 16 controllers/segments.

> -----Original Message-----
> From: Pierre Gondois <pierre.gondois@arm.com>
> Sent: Monday, July 11, 2022 11:15 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
> 
> 
> 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