[PATCH edk2-platforms 0/2] Platform/ARM/Juno: remove explicit PCIe enumeration

[PATCH edk2-platforms 0/2] Platform/ARM/Juno: remove explicit PCIe enumeration

[Ard Biesheuvel]

The platform DXE driver of the Juno platform enumerates the PCIe explicitly
by calling ConnectController on the PCIe root bridge, in order to ensure
that all PCI I/O protocols have been instantiated when we try to locate the
one describing the Marvel Yukon NIC, so that we can program its MAC address.

Taking control of core firmware behavior like this has already caused
confusion when reasoning about why and when PCIe option ROMs get dispatched,
and is likely to cause more confusion down the road.

So let's fix this, by triggering the MAC programming via a protocol notify
on the PCI I/O protocol.

This has been build tested only at the moment.

Cc: Sami Mujawar <sami.mujawar@arm.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Leif Lindholm <leif.lindholm@linaro.org>

Ard Biesheuvel (2):
  Platform/ARM/ArmJunoDxe: make mAcpiRegistration STATIC
  Platform/ARM/ArmJunoDxe: use PciIo protocol notify to program the MAC

 .../JunoPkg/Drivers/ArmJunoDxe/ArmJunoDxe.c   | 163 ++++--------------
 .../JunoPkg/Drivers/ArmJunoDxe/ArmJunoDxe.inf |   1 -
 2 files changed, 31 insertions(+), 133 deletions(-)

-- 
2.17.1

[PATCH edk2-platforms 1/2] Platform/ARM/ArmJunoDxe: make mAcpiRegistration STATIC

[Ard Biesheuvel]

Generally, variables should only have external linkage if needed,
so make mAcpiRegistration STATIC, given that static linkage suffices
for it.

Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 Platform/ARM/JunoPkg/Drivers/ArmJunoDxe/ArmJunoDxe.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Platform/ARM/JunoPkg/Drivers/ArmJunoDxe/ArmJunoDxe.c b/Platform/ARM/JunoPkg/Drivers/ArmJunoDxe/ArmJunoDxe.c
index ea7591d70443..c0ad7ced2959 100644
--- a/Platform/ARM/JunoPkg/Drivers/ArmJunoDxe/ArmJunoDxe.c
+++ b/Platform/ARM/JunoPkg/Drivers/ArmJunoDxe/ArmJunoDxe.c
@@ -64,7 +64,7 @@ STATIC CONST EFI_PCI_ROOT_BRIDGE_DEVICE_PATH mPciRootComplexDevicePath = {
     }
 };
 
-EFI_EVENT mAcpiRegistration = NULL;
+STATIC EFI_EVENT mAcpiRegistration = NULL;
 
 /**
   This function reads PCI ID of the controller.
-- 
2.17.1

[PATCH edk2-platforms 2/2] Platform/ARM/ArmJunoDxe: use PciIo protocol notify to program the MAC

[Ard Biesheuvel]

Instead of connecting and thus enumerating the PCIe topology in a
platform driver, just so that we can grab the PciIo protocol that
belongs to the Marvell Yukon NIC and program its MAC address, rely
on a protocol notification handler to do this whenever the core code
decides to enumerate the PCIe.

Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 Platform/ARM/JunoPkg/Drivers/ArmJunoDxe/ArmJunoDxe.c   | 161 ++++----------------
 Platform/ARM/JunoPkg/Drivers/ArmJunoDxe/ArmJunoDxe.inf |   1 -
 2 files changed, 30 insertions(+), 132 deletions(-)

diff --git a/Platform/ARM/JunoPkg/Drivers/ArmJunoDxe/ArmJunoDxe.c b/Platform/ARM/JunoPkg/Drivers/ArmJunoDxe/ArmJunoDxe.c
index c0ad7ced2959..ebaf2aa134da 100644
--- a/Platform/ARM/JunoPkg/Drivers/ArmJunoDxe/ArmJunoDxe.c
+++ b/Platform/ARM/JunoPkg/Drivers/ArmJunoDxe/ArmJunoDxe.c
@@ -32,39 +32,8 @@
 STATIC CONST EFI_GUID mJunoAcpiTableFile = { 0xa1dd808e, 0x1e95, 0x4399, { 0xab, 0xc0, 0x65, 0x3c, 0x82, 0xe8, 0x53, 0x0c } };
 #endif
 
-typedef struct {
-  ACPI_HID_DEVICE_PATH      AcpiDevicePath;
-  PCI_DEVICE_PATH           PciDevicePath;
-  EFI_DEVICE_PATH_PROTOCOL  EndDevicePath;
-} EFI_PCI_ROOT_BRIDGE_DEVICE_PATH;
-
-STATIC CONST EFI_PCI_ROOT_BRIDGE_DEVICE_PATH mPciRootComplexDevicePath = {
-    {
-      { ACPI_DEVICE_PATH,
-        ACPI_DP,
-        { (UINT8) (sizeof (ACPI_HID_DEVICE_PATH)),
-          (UINT8) ((sizeof (ACPI_HID_DEVICE_PATH)) >> 8) }
-      },
-      EISA_PNP_ID (0x0A03),
-      0
-    },
-    {
-      { HARDWARE_DEVICE_PATH,
-        HW_PCI_DP,
-        { (UINT8) (sizeof (PCI_DEVICE_PATH)),
-          (UINT8) ((sizeof (PCI_DEVICE_PATH)) >> 8) }
-      },
-      0,
-      0
-    },
-    {
-      END_DEVICE_PATH_TYPE,
-      END_ENTIRE_DEVICE_PATH_SUBTYPE,
-      { END_DEVICE_PATH_LENGTH, 0 }
-    }
-};
-
 STATIC EFI_EVENT mAcpiRegistration = NULL;
+STATIC EFI_EVENT mPciIoNotificationRegistration = NULL;
 
 /**
   This function reads PCI ID of the controller.
@@ -99,59 +68,6 @@ ReadMarvellYoukonPciId (
   return EFI_SUCCESS;
 }
 
-/**
-  This function searches for Marvell Yukon NIC on the Juno
-  platform and returns PCI IO protocol handle for the controller.
-
-  @param[out]  PciIo   PCI IO protocol handle
-**/
-STATIC
-EFI_STATUS
-GetMarvellYukonPciIoProtocol (
-  OUT EFI_PCI_IO_PROTOCOL  **PciIo
-  )
-{
-  UINTN       HandleCount;
-  EFI_HANDLE  *HandleBuffer;
-  UINTN       HIndex;
-  EFI_STATUS  Status;
-
-  Status = gBS->LocateHandleBuffer (
-                  ByProtocol,
-                  &gEfiPciIoProtocolGuid,
-                  NULL,
-                  &HandleCount,
-                  &HandleBuffer);
-  if (EFI_ERROR (Status)) {
-    return (Status);
-  }
-
-  for (HIndex = 0; HIndex < HandleCount; ++HIndex) {
-    // If PciIo opened with EFI_OPEN_PROTOCOL_GET_PROTOCOL, the CloseProtocol() is not required
-    Status = gBS->OpenProtocol (
-                    HandleBuffer[HIndex],
-                    &gEfiPciIoProtocolGuid,
-                    (VOID **) PciIo,
-                    NULL,
-                    NULL,
-                    EFI_OPEN_PROTOCOL_GET_PROTOCOL);
-    if (EFI_ERROR (Status)) {
-      continue;
-    }
-
-    Status = ReadMarvellYoukonPciId (*PciIo, JUNO_MARVELL_YUKON_ID);
-    if (EFI_ERROR (Status)) {
-      continue;
-    } else {
-      break;
-    }
-  }
-
-  gBS->FreePool (HandleBuffer);
-
-  return Status;
-}
-
 /**
  This function restore the original controller attributes
 
@@ -326,18 +242,14 @@ WriteMacAddress (
 **/
 STATIC
 EFI_STATUS
-ArmJunoSetNicMacAddress ()
+ArmJunoSetNicMacAddress (
+  IN  EFI_PCI_IO_PROTOCOL   *PciIo
+  )
 {
   UINT64                              OldPciAttr;
-  EFI_PCI_IO_PROTOCOL*                PciIo;
   UINT32                              PciRegBase;
   EFI_STATUS                          Status;
 
-  Status = GetMarvellYukonPciIoProtocol (&PciIo);
-  if (EFI_ERROR (Status)) {
-    return Status;
-  }
-
   PciRegBase = 0;
   Status = InitPciDev (PciIo, &PciRegBase, &OldPciAttr);
   if (EFI_ERROR (Status)) {
@@ -352,14 +264,8 @@ ArmJunoSetNicMacAddress ()
 }
 
 /**
-  Notification function of the event defined as belonging to the
-  EFI_END_OF_DXE_EVENT_GROUP_GUID event group that was created in
-  the entry point of the driver.
-
-  This function is called when an event belonging to the
-  EFI_END_OF_DXE_EVENT_GROUP_GUID event group is signalled. Such an
-  event is signalled once at the end of the dispatching of all
-  drivers (end of the so called DXE phase).
+  This function is called when a gEfiPciIoProtocolGuid protocol instance is
+  registered in the protocol database.
 
   @param[in]  Event    Event declared in the entry point of the driver whose
                        notification function is being invoked.
@@ -367,33 +273,30 @@ ArmJunoSetNicMacAddress ()
 **/
 STATIC
 VOID
-OnEndOfDxe (
+PciIoNotificationEvent (
   IN EFI_EVENT  Event,
   IN VOID       *Context
   )
 {
-  EFI_DEVICE_PATH_PROTOCOL* PciRootComplexDevicePath;
-  EFI_HANDLE                Handle;
-  EFI_STATUS                Status;
+  EFI_STATUS            Status;
+  EFI_PCI_IO_PROTOCOL   *PciIo;
 
-  //
-  // PCI Root Complex initialization
-  // At the end of the DXE phase, we should get all the driver dispatched.
-  // Force the PCI Root Complex to be initialized. It allows the OS to skip
-  // this step.
-  //
-  PciRootComplexDevicePath = (EFI_DEVICE_PATH_PROTOCOL*) &mPciRootComplexDevicePath;
-  Status = gBS->LocateDevicePath (&gEfiPciRootBridgeIoProtocolGuid,
-                                  &PciRootComplexDevicePath,
-                                  &Handle);
+  Status = gBS->LocateProtocol (&gEfiPciIoProtocolGuid,
+                  mPciIoNotificationRegistration, (VOID **)&PciIo);
+  if (EFI_ERROR (Status)) {
+    return;
+  }
 
-  Status = gBS->ConnectController (Handle, NULL, PciRootComplexDevicePath, FALSE);
-  ASSERT_EFI_ERROR (Status);
+  Status = ReadMarvellYoukonPciId (PciIo, JUNO_MARVELL_YUKON_ID);
+  if (EFI_ERROR (Status)) {
+    return;
+  }
 
-  Status = ArmJunoSetNicMacAddress ();
+  Status = ArmJunoSetNicMacAddress (PciIo);
   if (EFI_ERROR (Status)) {
     DEBUG ((DEBUG_ERROR, "ArmJunoDxe: Failed to set Marvell Yukon NIC MAC address\n"));
   }
+  gBS->CloseEvent (Event);
 }
 
 EFI_STATUS
@@ -408,7 +311,6 @@ ArmJunoEntryPoint (
   CHAR16                *TextDevicePath;
   UINTN                 TextDevicePathSize;
   UINT32                JunoRevision;
-  EFI_EVENT             EndOfDxeEvent;
 
   //
   // Register the OHCI and EHCI controllers as non-coherent
@@ -497,20 +399,17 @@ ArmJunoEntryPoint (
     PcdSetBoolS (PcdPciDisableBusEnumeration, FALSE);
 
     //
-    // Create an event belonging to the "gEfiEndOfDxeEventGroupGuid" group.
-    // The "OnEndOfDxe()" function is declared as the call back function.
-    // It will be called at the end of the DXE phase when an event of the
-    // same group is signalled to inform about the end of the DXE phase.
-    // Install the INSTALL_FDT_PROTOCOL protocol.
+    // Create a protocol notification event handler on the PciIo protocol
+    // so we can set the MAC address on the Marvell Yukon as soon as it
+    // appears.
     //
-    Status = gBS->CreateEventEx (
-                    EVT_NOTIFY_SIGNAL,
-                    TPL_CALLBACK,
-                    OnEndOfDxe,
-                    NULL,
-                    &gEfiEndOfDxeEventGroupGuid,
-                    &EndOfDxeEvent
-                    );
+    EfiCreateProtocolNotifyEvent (
+        &gEfiPciIoProtocolGuid,
+        TPL_NOTIFY,
+        PciIoNotificationEvent,
+        NULL,
+        &mPciIoNotificationRegistration
+        );
 
 #ifndef DYNAMIC_TABLES_FRAMEWORK
     // Declare the related ACPI Tables
diff --git a/Platform/ARM/JunoPkg/Drivers/ArmJunoDxe/ArmJunoDxe.inf b/Platform/ARM/JunoPkg/Drivers/ArmJunoDxe/ArmJunoDxe.inf
index 7c118d9c9c6b..d016967c3c37 100644
--- a/Platform/ARM/JunoPkg/Drivers/ArmJunoDxe/ArmJunoDxe.inf
+++ b/Platform/ARM/JunoPkg/Drivers/ArmJunoDxe/ArmJunoDxe.inf
@@ -44,7 +44,6 @@ [LibraryClasses]
   UefiDriverEntryPoint
 
 [Guids]
-  gEfiEndOfDxeEventGroupGuid
   gEfiFileInfoGuid
 
 [Protocols]
-- 
2.17.1

Re: [PATCH edk2-platforms 0/2] Platform/ARM/Juno: remove explicit PCIe enumeration

[Leif Lindholm]

On Fri, Dec 06, 2019 at 11:02:17 +0000, Ard Biesheuvel wrote:
> The platform DXE driver of the Juno platform enumerates the PCIe explicitly
> by calling ConnectController on the PCIe root bridge, in order to ensure
> that all PCI I/O protocols have been instantiated when we try to locate the
> one describing the Marvel Yukon NIC, so that we can program its MAC address.
> 
> Taking control of core firmware behavior like this has already caused
> confusion when reasoning about why and when PCIe option ROMs get dispatched,
> and is likely to cause more confusion down the road.
> 
> So let's fix this, by triggering the MAC programming via a protocol notify
> on the PCI I/O protocol.
> 
> This has been build tested only at the moment.
> 
> Cc: Sami Mujawar <sami.mujawar@arm.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Leif Lindholm <leif.lindholm@linaro.org>

This looks sensible to me (and way more can-of-wormish than I would
have expected) - so:
Acked-by: Leif Lindholm <leif.lindholm@linaro.org>
*but* we need tested-by from somewhere before merging 2/2.

1/2 could go in whenever.

/
    Leif

> Ard Biesheuvel (2):
>   Platform/ARM/ArmJunoDxe: make mAcpiRegistration STATIC
>   Platform/ARM/ArmJunoDxe: use PciIo protocol notify to program the MAC
> 
>  .../JunoPkg/Drivers/ArmJunoDxe/ArmJunoDxe.c   | 163 ++++--------------
>  .../JunoPkg/Drivers/ArmJunoDxe/ArmJunoDxe.inf |   1 -
>  2 files changed, 31 insertions(+), 133 deletions(-)
> 
> -- 
> 2.17.1
> 

Re: [PATCH edk2-platforms 0/2] Platform/ARM/Juno: remove explicit PCIe enumeration

[Sami Mujawar]

Tested on Juno R2, the MAC address for Marvel Yukon NIC is programmed correctly.

Tested-by: Sami Mujawar <sami.mujawar@arm.com>

Regards,

Sami Mujawar
-----Original Message-----
From: Leif Lindholm <leif.lindholm@linaro.org>
Sent: 06 December 2019 11:13 AM
To: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: devel@edk2.groups.io; Sami Mujawar <Sami.Mujawar@arm.com>; Laszlo Ersek <lersek@redhat.com>
Subject: Re: [PATCH edk2-platforms 0/2] Platform/ARM/Juno: remove explicit PCIe enumeration

On Fri, Dec 06, 2019 at 11:02:17 +0000, Ard Biesheuvel wrote:
> The platform DXE driver of the Juno platform enumerates the PCIe
> explicitly by calling ConnectController on the PCIe root bridge, in
> order to ensure that all PCI I/O protocols have been instantiated when
> we try to locate the one describing the Marvel Yukon NIC, so that we can program its MAC address.
>
> Taking control of core firmware behavior like this has already caused
> confusion when reasoning about why and when PCIe option ROMs get
> dispatched, and is likely to cause more confusion down the road.
>
> So let's fix this, by triggering the MAC programming via a protocol
> notify on the PCI I/O protocol.
>
> This has been build tested only at the moment.
>
> Cc: Sami Mujawar <sami.mujawar@arm.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Leif Lindholm <leif.lindholm@linaro.org>

This looks sensible to me (and way more can-of-wormish than I would have expected) - so:
Acked-by: Leif Lindholm <leif.lindholm@linaro.org>
*but* we need tested-by from somewhere before merging 2/2.

1/2 could go in whenever.

/
    Leif

> Ard Biesheuvel (2):
>   Platform/ARM/ArmJunoDxe: make mAcpiRegistration STATIC
>   Platform/ARM/ArmJunoDxe: use PciIo protocol notify to program the
> MAC
>
>  .../JunoPkg/Drivers/ArmJunoDxe/ArmJunoDxe.c   | 163 ++++--------------
>  .../JunoPkg/Drivers/ArmJunoDxe/ArmJunoDxe.inf |   1 -
>  2 files changed, 31 insertions(+), 133 deletions(-)
>
> --
> 2.17.1
>
IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.

Re: [PATCH edk2-platforms 2/2] Platform/ARM/ArmJunoDxe: use PciIo protocol notify to program the MAC

[Laszlo Ersek]

On 12/06/19 12:02, Ard Biesheuvel wrote:
> Instead of connecting and thus enumerating the PCIe topology in a
> platform driver, just so that we can grab the PciIo protocol that
> belongs to the Marvell Yukon NIC and program its MAC address, rely
> on a protocol notification handler to do this whenever the core code
> decides to enumerate the PCIe.
> 
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
>  Platform/ARM/JunoPkg/Drivers/ArmJunoDxe/ArmJunoDxe.c   | 161 ++++----------------
>  Platform/ARM/JunoPkg/Drivers/ArmJunoDxe/ArmJunoDxe.inf |   1 -
>  2 files changed, 30 insertions(+), 132 deletions(-)
> 
> diff --git a/Platform/ARM/JunoPkg/Drivers/ArmJunoDxe/ArmJunoDxe.c b/Platform/ARM/JunoPkg/Drivers/ArmJunoDxe/ArmJunoDxe.c
> index c0ad7ced2959..ebaf2aa134da 100644
> --- a/Platform/ARM/JunoPkg/Drivers/ArmJunoDxe/ArmJunoDxe.c
> +++ b/Platform/ARM/JunoPkg/Drivers/ArmJunoDxe/ArmJunoDxe.c
> @@ -32,39 +32,8 @@
>  STATIC CONST EFI_GUID mJunoAcpiTableFile = { 0xa1dd808e, 0x1e95, 0x4399, { 0xab, 0xc0, 0x65, 0x3c, 0x82, 0xe8, 0x53, 0x0c } };
>  #endif
>  
> -typedef struct {
> -  ACPI_HID_DEVICE_PATH      AcpiDevicePath;
> -  PCI_DEVICE_PATH           PciDevicePath;
> -  EFI_DEVICE_PATH_PROTOCOL  EndDevicePath;
> -} EFI_PCI_ROOT_BRIDGE_DEVICE_PATH;
> -
> -STATIC CONST EFI_PCI_ROOT_BRIDGE_DEVICE_PATH mPciRootComplexDevicePath = {
> -    {
> -      { ACPI_DEVICE_PATH,
> -        ACPI_DP,
> -        { (UINT8) (sizeof (ACPI_HID_DEVICE_PATH)),
> -          (UINT8) ((sizeof (ACPI_HID_DEVICE_PATH)) >> 8) }
> -      },
> -      EISA_PNP_ID (0x0A03),
> -      0
> -    },
> -    {
> -      { HARDWARE_DEVICE_PATH,
> -        HW_PCI_DP,
> -        { (UINT8) (sizeof (PCI_DEVICE_PATH)),
> -          (UINT8) ((sizeof (PCI_DEVICE_PATH)) >> 8) }
> -      },
> -      0,
> -      0
> -    },
> -    {
> -      END_DEVICE_PATH_TYPE,
> -      END_ENTIRE_DEVICE_PATH_SUBTYPE,
> -      { END_DEVICE_PATH_LENGTH, 0 }
> -    }
> -};
> -
>  STATIC EFI_EVENT mAcpiRegistration = NULL;
> +STATIC EFI_EVENT mPciIoNotificationRegistration = NULL;

(1) Please use "VOID *" as the type of "mPciIoNotificationRegistration".

EFI_EVENT happens to work (because it is, regrettably, a typedef, per
spec, to "VOID *").

But semantically we need "VOID *" here -- please see the "Registration"
parameter of EFI_BOOT_SERVICES.RegisterProtocolNotify() and
EFI_BOOT_SERVICES.LocateProtocol().

>  
>  /**
>    This function reads PCI ID of the controller.
> @@ -99,59 +68,6 @@ ReadMarvellYoukonPciId (
>    return EFI_SUCCESS;
>  }
>  
> -/**
> -  This function searches for Marvell Yukon NIC on the Juno
> -  platform and returns PCI IO protocol handle for the controller.
> -
> -  @param[out]  PciIo   PCI IO protocol handle
> -**/
> -STATIC
> -EFI_STATUS
> -GetMarvellYukonPciIoProtocol (
> -  OUT EFI_PCI_IO_PROTOCOL  **PciIo
> -  )
> -{
> -  UINTN       HandleCount;
> -  EFI_HANDLE  *HandleBuffer;
> -  UINTN       HIndex;
> -  EFI_STATUS  Status;
> -
> -  Status = gBS->LocateHandleBuffer (
> -                  ByProtocol,
> -                  &gEfiPciIoProtocolGuid,
> -                  NULL,
> -                  &HandleCount,
> -                  &HandleBuffer);
> -  if (EFI_ERROR (Status)) {
> -    return (Status);
> -  }
> -
> -  for (HIndex = 0; HIndex < HandleCount; ++HIndex) {
> -    // If PciIo opened with EFI_OPEN_PROTOCOL_GET_PROTOCOL, the CloseProtocol() is not required
> -    Status = gBS->OpenProtocol (
> -                    HandleBuffer[HIndex],
> -                    &gEfiPciIoProtocolGuid,
> -                    (VOID **) PciIo,
> -                    NULL,
> -                    NULL,
> -                    EFI_OPEN_PROTOCOL_GET_PROTOCOL);
> -    if (EFI_ERROR (Status)) {
> -      continue;
> -    }
> -
> -    Status = ReadMarvellYoukonPciId (*PciIo, JUNO_MARVELL_YUKON_ID);
> -    if (EFI_ERROR (Status)) {
> -      continue;
> -    } else {
> -      break;
> -    }
> -  }
> -
> -  gBS->FreePool (HandleBuffer);
> -
> -  return Status;
> -}
> -
>  /**
>   This function restore the original controller attributes
>  
> @@ -326,18 +242,14 @@ WriteMacAddress (
>  **/
>  STATIC
>  EFI_STATUS
> -ArmJunoSetNicMacAddress ()
> +ArmJunoSetNicMacAddress (
> +  IN  EFI_PCI_IO_PROTOCOL   *PciIo
> +  )
>  {
>    UINT64                              OldPciAttr;
> -  EFI_PCI_IO_PROTOCOL*                PciIo;
>    UINT32                              PciRegBase;
>    EFI_STATUS                          Status;
>  
> -  Status = GetMarvellYukonPciIoProtocol (&PciIo);
> -  if (EFI_ERROR (Status)) {
> -    return Status;
> -  }
> -
>    PciRegBase = 0;
>    Status = InitPciDev (PciIo, &PciRegBase, &OldPciAttr);
>    if (EFI_ERROR (Status)) {
> @@ -352,14 +264,8 @@ ArmJunoSetNicMacAddress ()
>  }
>  
>  /**
> -  Notification function of the event defined as belonging to the
> -  EFI_END_OF_DXE_EVENT_GROUP_GUID event group that was created in
> -  the entry point of the driver.
> -
> -  This function is called when an event belonging to the
> -  EFI_END_OF_DXE_EVENT_GROUP_GUID event group is signalled. Such an
> -  event is signalled once at the end of the dispatching of all
> -  drivers (end of the so called DXE phase).
> +  This function is called when a gEfiPciIoProtocolGuid protocol instance is
> +  registered in the protocol database.
>  
>    @param[in]  Event    Event declared in the entry point of the driver whose
>                         notification function is being invoked.
> @@ -367,33 +273,30 @@ ArmJunoSetNicMacAddress ()
>  **/
>  STATIC
>  VOID
> -OnEndOfDxe (
> +PciIoNotificationEvent (
>    IN EFI_EVENT  Event,
>    IN VOID       *Context
>    )
>  {
> -  EFI_DEVICE_PATH_PROTOCOL* PciRootComplexDevicePath;
> -  EFI_HANDLE                Handle;
> -  EFI_STATUS                Status;
> +  EFI_STATUS            Status;
> +  EFI_PCI_IO_PROTOCOL   *PciIo;
>  
> -  //
> -  // PCI Root Complex initialization
> -  // At the end of the DXE phase, we should get all the driver dispatched.
> -  // Force the PCI Root Complex to be initialized. It allows the OS to skip
> -  // this step.
> -  //
> -  PciRootComplexDevicePath = (EFI_DEVICE_PATH_PROTOCOL*) &mPciRootComplexDevicePath;
> -  Status = gBS->LocateDevicePath (&gEfiPciRootBridgeIoProtocolGuid,
> -                                  &PciRootComplexDevicePath,
> -                                  &Handle);
> +  Status = gBS->LocateProtocol (&gEfiPciIoProtocolGuid,
> +                  mPciIoNotificationRegistration, (VOID **)&PciIo);
> +  if (EFI_ERROR (Status)) {
> +    return;
> +  }
>  
> -  Status = gBS->ConnectController (Handle, NULL, PciRootComplexDevicePath, FALSE);
> -  ASSERT_EFI_ERROR (Status);
> +  Status = ReadMarvellYoukonPciId (PciIo, JUNO_MARVELL_YUKON_ID);
> +  if (EFI_ERROR (Status)) {
> +    return;
> +  }
>  
> -  Status = ArmJunoSetNicMacAddress ();
> +  Status = ArmJunoSetNicMacAddress (PciIo);
>    if (EFI_ERROR (Status)) {
>      DEBUG ((DEBUG_ERROR, "ArmJunoDxe: Failed to set Marvell Yukon NIC MAC address\n"));
>    }
> +  gBS->CloseEvent (Event);
>  }
>  
>  EFI_STATUS
> @@ -408,7 +311,6 @@ ArmJunoEntryPoint (
>    CHAR16                *TextDevicePath;
>    UINTN                 TextDevicePathSize;
>    UINT32                JunoRevision;
> -  EFI_EVENT             EndOfDxeEvent;
>  
>    //
>    // Register the OHCI and EHCI controllers as non-coherent
> @@ -497,20 +399,17 @@ ArmJunoEntryPoint (
>      PcdSetBoolS (PcdPciDisableBusEnumeration, FALSE);
>  
>      //
> -    // Create an event belonging to the "gEfiEndOfDxeEventGroupGuid" group.
> -    // The "OnEndOfDxe()" function is declared as the call back function.
> -    // It will be called at the end of the DXE phase when an event of the
> -    // same group is signalled to inform about the end of the DXE phase.
> -    // Install the INSTALL_FDT_PROTOCOL protocol.
> +    // Create a protocol notification event handler on the PciIo protocol
> +    // so we can set the MAC address on the Marvell Yukon as soon as it
> +    // appears.
>      //
> -    Status = gBS->CreateEventEx (
> -                    EVT_NOTIFY_SIGNAL,
> -                    TPL_CALLBACK,
> -                    OnEndOfDxe,
> -                    NULL,
> -                    &gEfiEndOfDxeEventGroupGuid,
> -                    &EndOfDxeEvent
> -                    );
> +    EfiCreateProtocolNotifyEvent (
> +        &gEfiPciIoProtocolGuid,
> +        TPL_NOTIFY,

(2) I generally prefer the lowest TPL that works, so I'd suggest
TPL_CALLBACK here.

With (1) updated, and (2) optionally updated (if you agree):

Reviewed-by: Laszlo Ersek <lersek@redhat.com>

Thanks!
Laszlo


> +        PciIoNotificationEvent,
> +        NULL,
> +        &mPciIoNotificationRegistration
> +        );
>  
>  #ifndef DYNAMIC_TABLES_FRAMEWORK
>      // Declare the related ACPI Tables
> diff --git a/Platform/ARM/JunoPkg/Drivers/ArmJunoDxe/ArmJunoDxe.inf b/Platform/ARM/JunoPkg/Drivers/ArmJunoDxe/ArmJunoDxe.inf
> index 7c118d9c9c6b..d016967c3c37 100644
> --- a/Platform/ARM/JunoPkg/Drivers/ArmJunoDxe/ArmJunoDxe.inf
> +++ b/Platform/ARM/JunoPkg/Drivers/ArmJunoDxe/ArmJunoDxe.inf
> @@ -44,7 +44,6 @@ [LibraryClasses]
>    UefiDriverEntryPoint
>  
>  [Guids]
> -  gEfiEndOfDxeEventGroupGuid
>    gEfiFileInfoGuid
>  
>  [Protocols]
> 

Re: [PATCH edk2-platforms 2/2] Platform/ARM/ArmJunoDxe: use PciIo protocol notify to program the MAC

[Ard Biesheuvel]

On Fri, 6 Dec 2019 at 12:12, Laszlo Ersek <lersek@redhat.com> wrote:
>
> On 12/06/19 12:02, Ard Biesheuvel wrote:
> > Instead of connecting and thus enumerating the PCIe topology in a
> > platform driver, just so that we can grab the PciIo protocol that
> > belongs to the Marvell Yukon NIC and program its MAC address, rely
> > on a protocol notification handler to do this whenever the core code
> > decides to enumerate the PCIe.
> >
> > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> > ---
> >  Platform/ARM/JunoPkg/Drivers/ArmJunoDxe/ArmJunoDxe.c   | 161 ++++----------------
> >  Platform/ARM/JunoPkg/Drivers/ArmJunoDxe/ArmJunoDxe.inf |   1 -
> >  2 files changed, 30 insertions(+), 132 deletions(-)
> >
> > diff --git a/Platform/ARM/JunoPkg/Drivers/ArmJunoDxe/ArmJunoDxe.c b/Platform/ARM/JunoPkg/Drivers/ArmJunoDxe/ArmJunoDxe.c
> > index c0ad7ced2959..ebaf2aa134da 100644
> > --- a/Platform/ARM/JunoPkg/Drivers/ArmJunoDxe/ArmJunoDxe.c
> > +++ b/Platform/ARM/JunoPkg/Drivers/ArmJunoDxe/ArmJunoDxe.c
> > @@ -32,39 +32,8 @@
> >  STATIC CONST EFI_GUID mJunoAcpiTableFile = { 0xa1dd808e, 0x1e95, 0x4399, { 0xab, 0xc0, 0x65, 0x3c, 0x82, 0xe8, 0x53, 0x0c } };
> >  #endif
> >
> > -typedef struct {
> > -  ACPI_HID_DEVICE_PATH      AcpiDevicePath;
> > -  PCI_DEVICE_PATH           PciDevicePath;
> > -  EFI_DEVICE_PATH_PROTOCOL  EndDevicePath;
> > -} EFI_PCI_ROOT_BRIDGE_DEVICE_PATH;
> > -
> > -STATIC CONST EFI_PCI_ROOT_BRIDGE_DEVICE_PATH mPciRootComplexDevicePath = {
> > -    {
> > -      { ACPI_DEVICE_PATH,
> > -        ACPI_DP,
> > -        { (UINT8) (sizeof (ACPI_HID_DEVICE_PATH)),
> > -          (UINT8) ((sizeof (ACPI_HID_DEVICE_PATH)) >> 8) }
> > -      },
> > -      EISA_PNP_ID (0x0A03),
> > -      0
> > -    },
> > -    {
> > -      { HARDWARE_DEVICE_PATH,
> > -        HW_PCI_DP,
> > -        { (UINT8) (sizeof (PCI_DEVICE_PATH)),
> > -          (UINT8) ((sizeof (PCI_DEVICE_PATH)) >> 8) }
> > -      },
> > -      0,
> > -      0
> > -    },
> > -    {
> > -      END_DEVICE_PATH_TYPE,
> > -      END_ENTIRE_DEVICE_PATH_SUBTYPE,
> > -      { END_DEVICE_PATH_LENGTH, 0 }
> > -    }
> > -};
> > -
> >  STATIC EFI_EVENT mAcpiRegistration = NULL;
> > +STATIC EFI_EVENT mPciIoNotificationRegistration = NULL;
>
> (1) Please use "VOID *" as the type of "mPciIoNotificationRegistration".
>
> EFI_EVENT happens to work (because it is, regrettably, a typedef, per
> spec, to "VOID *").
>
> But semantically we need "VOID *" here -- please see the "Registration"
> parameter of EFI_BOOT_SERVICES.RegisterProtocolNotify() and
> EFI_BOOT_SERVICES.LocateProtocol().
>

OK, thanks for spotting that.

> >
> >  /**
> >    This function reads PCI ID of the controller.
> > @@ -99,59 +68,6 @@ ReadMarvellYoukonPciId (
> >    return EFI_SUCCESS;
> >  }
> >
> > -/**
> > -  This function searches for Marvell Yukon NIC on the Juno
> > -  platform and returns PCI IO protocol handle for the controller.
> > -
> > -  @param[out]  PciIo   PCI IO protocol handle
> > -**/
> > -STATIC
> > -EFI_STATUS
> > -GetMarvellYukonPciIoProtocol (
> > -  OUT EFI_PCI_IO_PROTOCOL  **PciIo
> > -  )
> > -{
> > -  UINTN       HandleCount;
> > -  EFI_HANDLE  *HandleBuffer;
> > -  UINTN       HIndex;
> > -  EFI_STATUS  Status;
> > -
> > -  Status = gBS->LocateHandleBuffer (
> > -                  ByProtocol,
> > -                  &gEfiPciIoProtocolGuid,
> > -                  NULL,
> > -                  &HandleCount,
> > -                  &HandleBuffer);
> > -  if (EFI_ERROR (Status)) {
> > -    return (Status);
> > -  }
> > -
> > -  for (HIndex = 0; HIndex < HandleCount; ++HIndex) {
> > -    // If PciIo opened with EFI_OPEN_PROTOCOL_GET_PROTOCOL, the CloseProtocol() is not required
> > -    Status = gBS->OpenProtocol (
> > -                    HandleBuffer[HIndex],
> > -                    &gEfiPciIoProtocolGuid,
> > -                    (VOID **) PciIo,
> > -                    NULL,
> > -                    NULL,
> > -                    EFI_OPEN_PROTOCOL_GET_PROTOCOL);
> > -    if (EFI_ERROR (Status)) {
> > -      continue;
> > -    }
> > -
> > -    Status = ReadMarvellYoukonPciId (*PciIo, JUNO_MARVELL_YUKON_ID);
> > -    if (EFI_ERROR (Status)) {
> > -      continue;
> > -    } else {
> > -      break;
> > -    }
> > -  }
> > -
> > -  gBS->FreePool (HandleBuffer);
> > -
> > -  return Status;
> > -}
> > -
> >  /**
> >   This function restore the original controller attributes
> >
> > @@ -326,18 +242,14 @@ WriteMacAddress (
> >  **/
> >  STATIC
> >  EFI_STATUS
> > -ArmJunoSetNicMacAddress ()
> > +ArmJunoSetNicMacAddress (
> > +  IN  EFI_PCI_IO_PROTOCOL   *PciIo
> > +  )
> >  {
> >    UINT64                              OldPciAttr;
> > -  EFI_PCI_IO_PROTOCOL*                PciIo;
> >    UINT32                              PciRegBase;
> >    EFI_STATUS                          Status;
> >
> > -  Status = GetMarvellYukonPciIoProtocol (&PciIo);
> > -  if (EFI_ERROR (Status)) {
> > -    return Status;
> > -  }
> > -
> >    PciRegBase = 0;
> >    Status = InitPciDev (PciIo, &PciRegBase, &OldPciAttr);
> >    if (EFI_ERROR (Status)) {
> > @@ -352,14 +264,8 @@ ArmJunoSetNicMacAddress ()
> >  }
> >
> >  /**
> > -  Notification function of the event defined as belonging to the
> > -  EFI_END_OF_DXE_EVENT_GROUP_GUID event group that was created in
> > -  the entry point of the driver.
> > -
> > -  This function is called when an event belonging to the
> > -  EFI_END_OF_DXE_EVENT_GROUP_GUID event group is signalled. Such an
> > -  event is signalled once at the end of the dispatching of all
> > -  drivers (end of the so called DXE phase).
> > +  This function is called when a gEfiPciIoProtocolGuid protocol instance is
> > +  registered in the protocol database.
> >
> >    @param[in]  Event    Event declared in the entry point of the driver whose
> >                         notification function is being invoked.
> > @@ -367,33 +273,30 @@ ArmJunoSetNicMacAddress ()
> >  **/
> >  STATIC
> >  VOID
> > -OnEndOfDxe (
> > +PciIoNotificationEvent (
> >    IN EFI_EVENT  Event,
> >    IN VOID       *Context
> >    )
> >  {
> > -  EFI_DEVICE_PATH_PROTOCOL* PciRootComplexDevicePath;
> > -  EFI_HANDLE                Handle;
> > -  EFI_STATUS                Status;
> > +  EFI_STATUS            Status;
> > +  EFI_PCI_IO_PROTOCOL   *PciIo;
> >
> > -  //
> > -  // PCI Root Complex initialization
> > -  // At the end of the DXE phase, we should get all the driver dispatched.
> > -  // Force the PCI Root Complex to be initialized. It allows the OS to skip
> > -  // this step.
> > -  //
> > -  PciRootComplexDevicePath = (EFI_DEVICE_PATH_PROTOCOL*) &mPciRootComplexDevicePath;
> > -  Status = gBS->LocateDevicePath (&gEfiPciRootBridgeIoProtocolGuid,
> > -                                  &PciRootComplexDevicePath,
> > -                                  &Handle);
> > +  Status = gBS->LocateProtocol (&gEfiPciIoProtocolGuid,
> > +                  mPciIoNotificationRegistration, (VOID **)&PciIo);
> > +  if (EFI_ERROR (Status)) {
> > +    return;
> > +  }
> >
> > -  Status = gBS->ConnectController (Handle, NULL, PciRootComplexDevicePath, FALSE);
> > -  ASSERT_EFI_ERROR (Status);
> > +  Status = ReadMarvellYoukonPciId (PciIo, JUNO_MARVELL_YUKON_ID);
> > +  if (EFI_ERROR (Status)) {
> > +    return;
> > +  }
> >
> > -  Status = ArmJunoSetNicMacAddress ();
> > +  Status = ArmJunoSetNicMacAddress (PciIo);
> >    if (EFI_ERROR (Status)) {
> >      DEBUG ((DEBUG_ERROR, "ArmJunoDxe: Failed to set Marvell Yukon NIC MAC address\n"));
> >    }
> > +  gBS->CloseEvent (Event);
> >  }
> >
> >  EFI_STATUS
> > @@ -408,7 +311,6 @@ ArmJunoEntryPoint (
> >    CHAR16                *TextDevicePath;
> >    UINTN                 TextDevicePathSize;
> >    UINT32                JunoRevision;
> > -  EFI_EVENT             EndOfDxeEvent;
> >
> >    //
> >    // Register the OHCI and EHCI controllers as non-coherent
> > @@ -497,20 +399,17 @@ ArmJunoEntryPoint (
> >      PcdSetBoolS (PcdPciDisableBusEnumeration, FALSE);
> >
> >      //
> > -    // Create an event belonging to the "gEfiEndOfDxeEventGroupGuid" group.
> > -    // The "OnEndOfDxe()" function is declared as the call back function.
> > -    // It will be called at the end of the DXE phase when an event of the
> > -    // same group is signalled to inform about the end of the DXE phase.
> > -    // Install the INSTALL_FDT_PROTOCOL protocol.
> > +    // Create a protocol notification event handler on the PciIo protocol
> > +    // so we can set the MAC address on the Marvell Yukon as soon as it
> > +    // appears.
> >      //
> > -    Status = gBS->CreateEventEx (
> > -                    EVT_NOTIFY_SIGNAL,
> > -                    TPL_CALLBACK,
> > -                    OnEndOfDxe,
> > -                    NULL,
> > -                    &gEfiEndOfDxeEventGroupGuid,
> > -                    &EndOfDxeEvent
> > -                    );
> > +    EfiCreateProtocolNotifyEvent (
> > +        &gEfiPciIoProtocolGuid,
> > +        TPL_NOTIFY,
>
> (2) I generally prefer the lowest TPL that works, so I'd suggest
> TPL_CALLBACK here.
>

I'd prefer to keep this as TPL_NOTIFY, given that we are attaching to
a PCI device, enabling it, poking some values into a BAR window and
disabling it again.

> With (1) updated, and (2) optionally updated (if you agree):
>
> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
>

Thanks,


> > +        PciIoNotificationEvent,
> > +        NULL,
> > +        &mPciIoNotificationRegistration
> > +        );
> >
> >  #ifndef DYNAMIC_TABLES_FRAMEWORK
> >      // Declare the related ACPI Tables
> > diff --git a/Platform/ARM/JunoPkg/Drivers/ArmJunoDxe/ArmJunoDxe.inf b/Platform/ARM/JunoPkg/Drivers/ArmJunoDxe/ArmJunoDxe.inf
> > index 7c118d9c9c6b..d016967c3c37 100644
> > --- a/Platform/ARM/JunoPkg/Drivers/ArmJunoDxe/ArmJunoDxe.inf
> > +++ b/Platform/ARM/JunoPkg/Drivers/ArmJunoDxe/ArmJunoDxe.inf
> > @@ -44,7 +44,6 @@ [LibraryClasses]
> >    UefiDriverEntryPoint
> >
> >  [Guids]
> > -  gEfiEndOfDxeEventGroupGuid
> >    gEfiFileInfoGuid
> >
> >  [Protocols]
> >
>

Re: [PATCH edk2-platforms 1/2] Platform/ARM/ArmJunoDxe: make mAcpiRegistration STATIC

[Laszlo Ersek]

On 12/06/19 12:02, Ard Biesheuvel wrote:
> Generally, variables should only have external linkage if needed,
> so make mAcpiRegistration STATIC, given that static linkage suffices
> for it.
> 
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
>  Platform/ARM/JunoPkg/Drivers/ArmJunoDxe/ArmJunoDxe.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/Platform/ARM/JunoPkg/Drivers/ArmJunoDxe/ArmJunoDxe.c b/Platform/ARM/JunoPkg/Drivers/ArmJunoDxe/ArmJunoDxe.c
> index ea7591d70443..c0ad7ced2959 100644
> --- a/Platform/ARM/JunoPkg/Drivers/ArmJunoDxe/ArmJunoDxe.c
> +++ b/Platform/ARM/JunoPkg/Drivers/ArmJunoDxe/ArmJunoDxe.c
> @@ -64,7 +64,7 @@ STATIC CONST EFI_PCI_ROOT_BRIDGE_DEVICE_PATH mPciRootComplexDevicePath = {
>      }
>  };
>  
> -EFI_EVENT mAcpiRegistration = NULL;
> +STATIC EFI_EVENT mAcpiRegistration = NULL;
>  
>  /**
>    This function reads PCI ID of the controller.
> 

The patch does what it says on the tin, so:

Reviewed-by: Laszlo Ersek <lersek@redhat.com>

But, this too should have type "VOID *"; so if you have capacity for an
extra patch (or an extra change in this same patch), I'd recommend
fixing up the type.

Related commits (from edk2):

- 10eec5aa9297 ("MdeModulePkg: stop abusing EFI_EVENT for protocol
notify registration", 2019-10-09)

- fcf8bdcd5313 ("SecurityPkg: stop abusing EFI_EVENT for protocol notify
registration", 2019-10-09)

Thanks!
Laszlo

Re: [PATCH edk2-platforms 0/2] Platform/ARM/Juno: remove explicit PCIe enumeration

[Ard Biesheuvel]

On Fri, 6 Dec 2019 at 11:41, Sami Mujawar <Sami.Mujawar@arm.com> wrote:
>
> Tested on Juno R2, the MAC address for Marvel Yukon NIC is programmed correctly.
>
> Tested-by: Sami Mujawar <sami.mujawar@arm.com>
>

Thanks all

Pushed as 8b72f720d53e..1e4952d63b63 (with two instances of EFI_EVENT
converted to VOID*)