[PATCH] EmbeddedPkg/DtPlatformDxe: remove /chosen/stdout-path on GOP registration

[PATCH] EmbeddedPkg/DtPlatformDxe: remove /chosen/stdout-path on GOP registration

[Ard Biesheuvel]

The presence of a /chosen/stdout-path property will force Linux to use
the serial port as the primary console, even if a graphical console is
available as well. But the presence of the Graphics Output Protocol (GOP)
is a strong indication that the user may prefer to use his keyboard and
mouse rather than his terminal emulator to interact with the system, so
let's remove /chosen/stdout-path as soon as a GOP instance is registered.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 EmbeddedPkg/Drivers/DtPlatformDxe/DtPlatformDxe.c   | 41 ++++++++++++++++++++
 EmbeddedPkg/Drivers/DtPlatformDxe/DtPlatformDxe.inf |  4 ++
 2 files changed, 45 insertions(+)

diff --git a/EmbeddedPkg/Drivers/DtPlatformDxe/DtPlatformDxe.c b/EmbeddedPkg/Drivers/DtPlatformDxe/DtPlatformDxe.c
index 1014be2281d4..4f9ac8090fac 100644
--- a/EmbeddedPkg/Drivers/DtPlatformDxe/DtPlatformDxe.c
+++ b/EmbeddedPkg/Drivers/DtPlatformDxe/DtPlatformDxe.c
@@ -12,6 +12,7 @@
 *
 **/
 
+#include <libfdt.h>
 #include <Library/BaseLib.h>
 #include <Library/DebugLib.h>
 #include <Library/DevicePathLib.h>
@@ -54,6 +55,9 @@ STATIC HII_VENDOR_DEVICE_PATH     mDtPlatformDxeVendorDevicePath = {
   }
 };
 
+STATIC EFI_EVENT                  mRegisterGopEvent;
+STATIC VOID                       *mGopRegistration;
+
 STATIC
 EFI_STATUS
 InstallHiiPages (
@@ -89,6 +93,32 @@ InstallHiiPages (
   return EFI_SUCCESS;
 }
 
+STATIC
+VOID
+OnRegisterGop (
+  IN EFI_EVENT  Event,
+  IN VOID       *Dtb
+  )
+{
+  INT32     Node;
+  INT32     Error;
+
+  DEBUG ((DEBUG_INFO,
+    "%a: a GOP has been registered, removing /chosen/stdout-path from DT\n",
+    __FUNCTION__));
+
+  Node = fdt_path_offset (Dtb, "/chosen");
+  if (Node < 0) {
+    return;
+  }
+
+  Error = fdt_delprop (Dtb, Node, "stdout-path");
+  if (Error) {
+    DEBUG ((DEBUG_INFO, "%a: Failed to delete 'stdout-path' property: %a\n",
+      __FUNCTION__, fdt_strerror (Error)));
+  }
+}
+
 /**
   The entry point for DtPlatformDxe driver.
 
@@ -181,6 +211,17 @@ DtPlatformDxeEntryPoint (
         __FUNCTION__));
       goto FreeDtb;
     }
+
+    Status = gBS->CreateEvent (EVT_NOTIFY_SIGNAL, TPL_CALLBACK,
+                    OnRegisterGop, Dtb, &mRegisterGopEvent);
+    if (EFI_ERROR (Status)) {
+      DEBUG ((DEBUG_ERROR, "%a: failed to create event - %r\n",
+        __FUNCTION__, Status));
+    }
+
+    Status = gBS->RegisterProtocolNotify (&gEfiGraphicsOutputProtocolGuid,
+                    mRegisterGopEvent, &mGopRegistration);
+    ASSERT_EFI_ERROR (Status);
   } else {
     ASSERT (FALSE);
   }
diff --git a/EmbeddedPkg/Drivers/DtPlatformDxe/DtPlatformDxe.inf b/EmbeddedPkg/Drivers/DtPlatformDxe/DtPlatformDxe.inf
index 45dfd9088bf0..6b331294df8a 100644
--- a/EmbeddedPkg/Drivers/DtPlatformDxe/DtPlatformDxe.inf
+++ b/EmbeddedPkg/Drivers/DtPlatformDxe/DtPlatformDxe.inf
@@ -41,6 +41,7 @@ [LibraryClasses]
   BaseLib
   DebugLib
   DtPlatformDtbLoaderLib
+  FdtLib
   HiiLib
   MemoryAllocationLib
   UefiBootServicesTableLib
@@ -53,6 +54,9 @@ [Guids]
   gEdkiiPlatformHasAcpiGuid
   gFdtTableGuid
 
+[Protocols]
+  gEfiGraphicsOutputProtocolGuid
+
 [Depex]
   gEfiVariableArchProtocolGuid        AND
   gEfiVariableWriteArchProtocolGuid
-- 
2.11.0

Re: [PATCH] EmbeddedPkg/DtPlatformDxe: remove /chosen/stdout-path on GOP registration

[Leif Lindholm]

On Thu, Oct 19, 2017 at 08:21:41PM +0100, Ard Biesheuvel wrote:
> The presence of a /chosen/stdout-path property will force Linux to use
> the serial port as the primary console, even if a graphical console is
> available as well. But the presence of the Graphics Output Protocol (GOP)
> is a strong indication that the user may prefer to use his keyboard and
> mouse rather than his terminal emulator to interact with the system, so
> let's remove /chosen/stdout-path as soon as a GOP instance is registered.

This may be the sensible thing to do, but something just doesn't feel
right.

Also, whatever we do here, we should try to mirror in ACPI with SPCR.

Are we guaranteed to always want to disable serial console if there is
a graphics adapter?

I still dream of a world in which I can run consplitter up to the
point where an OS takes over, and an OS installer will listen to input
from both sources and register its selection from there.

But if we can't have that, can we have a menu setting to select preference?
The default can be a platform-specific Pcd.

/
    Leif

> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
>  EmbeddedPkg/Drivers/DtPlatformDxe/DtPlatformDxe.c   | 41 ++++++++++++++++++++
>  EmbeddedPkg/Drivers/DtPlatformDxe/DtPlatformDxe.inf |  4 ++
>  2 files changed, 45 insertions(+)
> 
> diff --git a/EmbeddedPkg/Drivers/DtPlatformDxe/DtPlatformDxe.c b/EmbeddedPkg/Drivers/DtPlatformDxe/DtPlatformDxe.c
> index 1014be2281d4..4f9ac8090fac 100644
> --- a/EmbeddedPkg/Drivers/DtPlatformDxe/DtPlatformDxe.c
> +++ b/EmbeddedPkg/Drivers/DtPlatformDxe/DtPlatformDxe.c
> @@ -12,6 +12,7 @@
>  *
>  **/
>  
> +#include <libfdt.h>
>  #include <Library/BaseLib.h>
>  #include <Library/DebugLib.h>
>  #include <Library/DevicePathLib.h>
> @@ -54,6 +55,9 @@ STATIC HII_VENDOR_DEVICE_PATH     mDtPlatformDxeVendorDevicePath = {
>    }
>  };
>  
> +STATIC EFI_EVENT                  mRegisterGopEvent;
> +STATIC VOID                       *mGopRegistration;
> +
>  STATIC
>  EFI_STATUS
>  InstallHiiPages (
> @@ -89,6 +93,32 @@ InstallHiiPages (
>    return EFI_SUCCESS;
>  }
>  
> +STATIC
> +VOID
> +OnRegisterGop (
> +  IN EFI_EVENT  Event,
> +  IN VOID       *Dtb
> +  )
> +{
> +  INT32     Node;
> +  INT32     Error;
> +
> +  DEBUG ((DEBUG_INFO,
> +    "%a: a GOP has been registered, removing /chosen/stdout-path from DT\n",
> +    __FUNCTION__));
> +
> +  Node = fdt_path_offset (Dtb, "/chosen");
> +  if (Node < 0) {
> +    return;
> +  }
> +
> +  Error = fdt_delprop (Dtb, Node, "stdout-path");
> +  if (Error) {
> +    DEBUG ((DEBUG_INFO, "%a: Failed to delete 'stdout-path' property: %a\n",
> +      __FUNCTION__, fdt_strerror (Error)));
> +  }
> +}
> +
>  /**
>    The entry point for DtPlatformDxe driver.
>  
> @@ -181,6 +211,17 @@ DtPlatformDxeEntryPoint (
>          __FUNCTION__));
>        goto FreeDtb;
>      }
> +
> +    Status = gBS->CreateEvent (EVT_NOTIFY_SIGNAL, TPL_CALLBACK,
> +                    OnRegisterGop, Dtb, &mRegisterGopEvent);
> +    if (EFI_ERROR (Status)) {
> +      DEBUG ((DEBUG_ERROR, "%a: failed to create event - %r\n",
> +        __FUNCTION__, Status));
> +    }
> +
> +    Status = gBS->RegisterProtocolNotify (&gEfiGraphicsOutputProtocolGuid,
> +                    mRegisterGopEvent, &mGopRegistration);
> +    ASSERT_EFI_ERROR (Status);
>    } else {
>      ASSERT (FALSE);
>    }
> diff --git a/EmbeddedPkg/Drivers/DtPlatformDxe/DtPlatformDxe.inf b/EmbeddedPkg/Drivers/DtPlatformDxe/DtPlatformDxe.inf
> index 45dfd9088bf0..6b331294df8a 100644
> --- a/EmbeddedPkg/Drivers/DtPlatformDxe/DtPlatformDxe.inf
> +++ b/EmbeddedPkg/Drivers/DtPlatformDxe/DtPlatformDxe.inf
> @@ -41,6 +41,7 @@ [LibraryClasses]
>    BaseLib
>    DebugLib
>    DtPlatformDtbLoaderLib
> +  FdtLib
>    HiiLib
>    MemoryAllocationLib
>    UefiBootServicesTableLib
> @@ -53,6 +54,9 @@ [Guids]
>    gEdkiiPlatformHasAcpiGuid
>    gFdtTableGuid
>  
> +[Protocols]
> +  gEfiGraphicsOutputProtocolGuid
> +
>  [Depex]
>    gEfiVariableArchProtocolGuid        AND
>    gEfiVariableWriteArchProtocolGuid
> -- 
> 2.11.0
> 

Re: [PATCH] EmbeddedPkg/DtPlatformDxe: remove /chosen/stdout-path on GOP registration

[Ard Biesheuvel]

On 19 October 2017 at 21:55, Leif Lindholm <leif.lindholm@linaro.org> wrote:
> On Thu, Oct 19, 2017 at 08:21:41PM +0100, Ard Biesheuvel wrote:
>> The presence of a /chosen/stdout-path property will force Linux to use
>> the serial port as the primary console, even if a graphical console is
>> available as well. But the presence of the Graphics Output Protocol (GOP)
>> is a strong indication that the user may prefer to use his keyboard and
>> mouse rather than his terminal emulator to interact with the system, so
>> let's remove /chosen/stdout-path as soon as a GOP instance is registered.
>
> This may be the sensible thing to do, but something just doesn't feel
> right.
>
> Also, whatever we do here, we should try to mirror in ACPI with SPCR.
>

Yes, but it does not belong in this driver.

> Are we guaranteed to always want to disable serial console if there is
> a graphics adapter?
>

This does not disable the serial console, it just makes it more
difficult to access :-) You will have to add console=ttyAMA0 if you
have a screen connected but want your console to appear on the serial
port. (Just like on a normal computer)

> I still dream of a world in which I can run consplitter up to the
> point where an OS takes over, and an OS installer will listen to input
> from both sources and register its selection from there.
>

Yeah. I need this change to get the Debian installer to run on the FB
console without having to add 'console=tty0' to the kernel command
line.

> But if we can't have that, can we have a menu setting to select preference?
> The default can be a platform-specific Pcd.
>

That seems reasonable.

Re: [PATCH] EmbeddedPkg/DtPlatformDxe: remove /chosen/stdout-path on GOP registration

[Leif Lindholm]

On Thu, Oct 19, 2017 at 10:19:44PM +0100, Ard Biesheuvel wrote:
> On 19 October 2017 at 21:55, Leif Lindholm <leif.lindholm@linaro.org> wrote:
> > On Thu, Oct 19, 2017 at 08:21:41PM +0100, Ard Biesheuvel wrote:
> >> The presence of a /chosen/stdout-path property will force Linux to use
> >> the serial port as the primary console, even if a graphical console is
> >> available as well. But the presence of the Graphics Output Protocol (GOP)
> >> is a strong indication that the user may prefer to use his keyboard and
> >> mouse rather than his terminal emulator to interact with the system, so
> >> let's remove /chosen/stdout-path as soon as a GOP instance is registered.
> >
> > This may be the sensible thing to do, but something just doesn't feel
> > right.
> >
> > Also, whatever we do here, we should try to mirror in ACPI with SPCR.
> 
> Yes, but it does not belong in this driver.

Absolutely not - but it ties in with the Pcd suggestion below.

> > Are we guaranteed to always want to disable serial console if there is
> > a graphics adapter?
> 
> This does not disable the serial console, it just makes it more
> difficult to access :-) You will have to add console=ttyAMA0 if you
> have a screen connected but want your console to appear on the serial
> port. (Just like on a normal computer)

Oh, I know - seeing those 8 characters just makes me twitch.

> > I still dream of a world in which I can run consplitter up to the
> > point where an OS takes over, and an OS installer will listen to input
> > from both sources and register its selection from there.
> 
> Yeah. I need this change to get the Debian installer to run on the FB
> console without having to add 'console=tty0' to the kernel command
> line.

Yes, I really want that too. So I'm good with the halfway house of a
dynamic preference Pcd.

> > But if we can't have that, can we have a menu setting to select preference?
> > The default can be a platform-specific Pcd.
> 
> That seems reasonable.

Thanks!

/
    Leif

Re: [PATCH] EmbeddedPkg/DtPlatformDxe: remove /chosen/stdout-path on GOP registration

[Laszlo Ersek]

On 10/19/17 21:21, Ard Biesheuvel wrote:
> The presence of a /chosen/stdout-path property will force Linux to use
> the serial port as the primary console, even if a graphical console is
> available as well. But the presence of the Graphics Output Protocol (GOP)
> is a strong indication that the user may prefer to use his keyboard and
> mouse rather than his terminal emulator to interact with the system, so
> let's remove /chosen/stdout-path as soon as a GOP instance is registered.
> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
>  EmbeddedPkg/Drivers/DtPlatformDxe/DtPlatformDxe.c   | 41 ++++++++++++++++++++
>  EmbeddedPkg/Drivers/DtPlatformDxe/DtPlatformDxe.inf |  4 ++
>  2 files changed, 45 insertions(+)

Some general notes (I understand you might not proceed with this
direction after all):

> 
> diff --git a/EmbeddedPkg/Drivers/DtPlatformDxe/DtPlatformDxe.c b/EmbeddedPkg/Drivers/DtPlatformDxe/DtPlatformDxe.c
> index 1014be2281d4..4f9ac8090fac 100644
> --- a/EmbeddedPkg/Drivers/DtPlatformDxe/DtPlatformDxe.c
> +++ b/EmbeddedPkg/Drivers/DtPlatformDxe/DtPlatformDxe.c
> @@ -12,6 +12,7 @@
>  *
>  **/
>  
> +#include <libfdt.h>
>  #include <Library/BaseLib.h>
>  #include <Library/DebugLib.h>
>  #include <Library/DevicePathLib.h>
> @@ -54,6 +55,9 @@ STATIC HII_VENDOR_DEVICE_PATH     mDtPlatformDxeVendorDevicePath = {
>    }
>  };
>  
> +STATIC EFI_EVENT                  mRegisterGopEvent;
> +STATIC VOID                       *mGopRegistration;
> +
>  STATIC
>  EFI_STATUS
>  InstallHiiPages (
> @@ -89,6 +93,32 @@ InstallHiiPages (
>    return EFI_SUCCESS;
>  }
>  
> +STATIC
> +VOID
> +OnRegisterGop (
> +  IN EFI_EVENT  Event,
> +  IN VOID       *Dtb
> +  )
> +{
> +  INT32     Node;
> +  INT32     Error;
> +
> +  DEBUG ((DEBUG_INFO,
> +    "%a: a GOP has been registered, removing /chosen/stdout-path from DT\n",
> +    __FUNCTION__));
> +
> +  Node = fdt_path_offset (Dtb, "/chosen");
> +  if (Node < 0) {
> +    return;
> +  }
> +
> +  Error = fdt_delprop (Dtb, Node, "stdout-path");
> +  if (Error) {
> +    DEBUG ((DEBUG_INFO, "%a: Failed to delete 'stdout-path' property: %a\n",
> +      __FUNCTION__, fdt_strerror (Error)));
> +  }
> +}

In theory multiple GOPs can be installed. The last DEBUG message seems
to imply that the node removal is expected to succeed on the first call,
and then shouldn't be attempted. For this (i.e., for "one-shot" protocol
notifies), I suggest closing the event in the callback. That covers the
de-registration automatically.

> +
>  /**
>    The entry point for DtPlatformDxe driver.
>  
> @@ -181,6 +211,17 @@ DtPlatformDxeEntryPoint (
>          __FUNCTION__));
>        goto FreeDtb;
>      }
> +
> +    Status = gBS->CreateEvent (EVT_NOTIFY_SIGNAL, TPL_CALLBACK,
> +                    OnRegisterGop, Dtb, &mRegisterGopEvent);
> +    if (EFI_ERROR (Status)) {
> +      DEBUG ((DEBUG_ERROR, "%a: failed to create event - %r\n",
> +        __FUNCTION__, Status));
> +    }
> +
> +    Status = gBS->RegisterProtocolNotify (&gEfiGraphicsOutputProtocolGuid,
> +                    mRegisterGopEvent, &mGopRegistration);

If CreateEvent() fails, RegisterProtocolNotify() should not be called on
mRegisterGopEvent.

> +    ASSERT_EFI_ERROR (Status);
>    } else {
>      ASSERT (FALSE);
>    }

This function seems intent on releasing resources on error exit (see the
"FreeDtb" label and the many jumps to it). In that spirit, I suggest
replacing the ASSERT_EFI_ERROR() with a jump to a new error label, for
closing mRegisterGopEvent.

(Again, I know this could be total moot after Leif's feedback; I was a
bit curious nonetheless.)

Thanks
Laszlo

> diff --git a/EmbeddedPkg/Drivers/DtPlatformDxe/DtPlatformDxe.inf b/EmbeddedPkg/Drivers/DtPlatformDxe/DtPlatformDxe.inf
> index 45dfd9088bf0..6b331294df8a 100644
> --- a/EmbeddedPkg/Drivers/DtPlatformDxe/DtPlatformDxe.inf
> +++ b/EmbeddedPkg/Drivers/DtPlatformDxe/DtPlatformDxe.inf
> @@ -41,6 +41,7 @@ [LibraryClasses]
>    BaseLib
>    DebugLib
>    DtPlatformDtbLoaderLib
> +  FdtLib
>    HiiLib
>    MemoryAllocationLib
>    UefiBootServicesTableLib
> @@ -53,6 +54,9 @@ [Guids]
>    gEdkiiPlatformHasAcpiGuid
>    gFdtTableGuid
>  
> +[Protocols]
> +  gEfiGraphicsOutputProtocolGuid
> +
>  [Depex]
>    gEfiVariableArchProtocolGuid        AND
>    gEfiVariableWriteArchProtocolGuid
> 

Re: [PATCH] EmbeddedPkg/DtPlatformDxe: remove /chosen/stdout-path on GOP registration

[Ard Biesheuvel]

On 20 October 2017 at 11:44, Laszlo Ersek <lersek@redhat.com> wrote:
> On 10/19/17 21:21, Ard Biesheuvel wrote:
>> The presence of a /chosen/stdout-path property will force Linux to use
>> the serial port as the primary console, even if a graphical console is
>> available as well. But the presence of the Graphics Output Protocol (GOP)
>> is a strong indication that the user may prefer to use his keyboard and
>> mouse rather than his terminal emulator to interact with the system, so
>> let's remove /chosen/stdout-path as soon as a GOP instance is registered.
>>
>> Contributed-under: TianoCore Contribution Agreement 1.1
>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> ---
>>  EmbeddedPkg/Drivers/DtPlatformDxe/DtPlatformDxe.c   | 41 ++++++++++++++++++++
>>  EmbeddedPkg/Drivers/DtPlatformDxe/DtPlatformDxe.inf |  4 ++
>>  2 files changed, 45 insertions(+)
>
> Some general notes (I understand you might not proceed with this
> direction after all):
>
>>
>> diff --git a/EmbeddedPkg/Drivers/DtPlatformDxe/DtPlatformDxe.c b/EmbeddedPkg/Drivers/DtPlatformDxe/DtPlatformDxe.c
>> index 1014be2281d4..4f9ac8090fac 100644
>> --- a/EmbeddedPkg/Drivers/DtPlatformDxe/DtPlatformDxe.c
>> +++ b/EmbeddedPkg/Drivers/DtPlatformDxe/DtPlatformDxe.c
>> @@ -12,6 +12,7 @@
>>  *
>>  **/
>>
>> +#include <libfdt.h>
>>  #include <Library/BaseLib.h>
>>  #include <Library/DebugLib.h>
>>  #include <Library/DevicePathLib.h>
>> @@ -54,6 +55,9 @@ STATIC HII_VENDOR_DEVICE_PATH     mDtPlatformDxeVendorDevicePath = {
>>    }
>>  };
>>
>> +STATIC EFI_EVENT                  mRegisterGopEvent;
>> +STATIC VOID                       *mGopRegistration;
>> +
>>  STATIC
>>  EFI_STATUS
>>  InstallHiiPages (
>> @@ -89,6 +93,32 @@ InstallHiiPages (
>>    return EFI_SUCCESS;
>>  }
>>
>> +STATIC
>> +VOID
>> +OnRegisterGop (
>> +  IN EFI_EVENT  Event,
>> +  IN VOID       *Dtb
>> +  )
>> +{
>> +  INT32     Node;
>> +  INT32     Error;
>> +
>> +  DEBUG ((DEBUG_INFO,
>> +    "%a: a GOP has been registered, removing /chosen/stdout-path from DT\n",
>> +    __FUNCTION__));
>> +
>> +  Node = fdt_path_offset (Dtb, "/chosen");
>> +  if (Node < 0) {
>> +    return;
>> +  }
>> +
>> +  Error = fdt_delprop (Dtb, Node, "stdout-path");
>> +  if (Error) {
>> +    DEBUG ((DEBUG_INFO, "%a: Failed to delete 'stdout-path' property: %a\n",
>> +      __FUNCTION__, fdt_strerror (Error)));
>> +  }
>> +}
>
> In theory multiple GOPs can be installed. The last DEBUG message seems
> to imply that the node removal is expected to succeed on the first call,
> and then shouldn't be attempted. For this (i.e., for "one-shot" protocol
> notifies), I suggest closing the event in the callback. That covers the
> de-registration automatically.
>

Good point, thanks. I realised failure was possible here if the
property does not exist to begin with (put perhaps the notify callback
should not be registered in the first place) hence the _INFO

>> +
>>  /**
>>    The entry point for DtPlatformDxe driver.
>>
>> @@ -181,6 +211,17 @@ DtPlatformDxeEntryPoint (
>>          __FUNCTION__));
>>        goto FreeDtb;
>>      }
>> +
>> +    Status = gBS->CreateEvent (EVT_NOTIFY_SIGNAL, TPL_CALLBACK,
>> +                    OnRegisterGop, Dtb, &mRegisterGopEvent);
>> +    if (EFI_ERROR (Status)) {
>> +      DEBUG ((DEBUG_ERROR, "%a: failed to create event - %r\n",
>> +        __FUNCTION__, Status));
>> +    }
>> +
>> +    Status = gBS->RegisterProtocolNotify (&gEfiGraphicsOutputProtocolGuid,
>> +                    mRegisterGopEvent, &mGopRegistration);
>
> If CreateEvent() fails, RegisterProtocolNotify() should not be called on
> mRegisterGopEvent.
>
>> +    ASSERT_EFI_ERROR (Status);
>>    } else {
>>      ASSERT (FALSE);
>>    }
>
> This function seems intent on releasing resources on error exit (see the
> "FreeDtb" label and the many jumps to it). In that spirit, I suggest
> replacing the ASSERT_EFI_ERROR() with a jump to a new error label, for
> closing mRegisterGopEvent.
>

Another good point.

> (Again, I know this could be total moot after Leif's feedback; I was a
> bit curious nonetheless.)
>

I think the best way forward is to implement a separate driver with
its own HII form, that removes /chosen/stdout-path and uninstalls the
SPCR table if either or both are present. I couldn't figure out how to
uninstall a ACPI table though, so I may need to move the SPCR
installation into that driver as well.

Re: [PATCH] EmbeddedPkg/DtPlatformDxe: remove /chosen/stdout-path on GOP registration

[Laszlo Ersek]

On 10/20/17 13:40, Ard Biesheuvel wrote:

> I think the best way forward is to implement a separate driver with
> its own HII form, that removes /chosen/stdout-path and uninstalls the
> SPCR table if either or both are present. I couldn't figure out how to
> uninstall a ACPI table though, so I may need to move the SPCR
> installation into that driver as well.

In general it's best to consider an ACPI table "owned" by the driver
that installs it in the first place. The

  EFI_ACPI_TABLE_PROTOCOL.InstallAcpiTable()

function (from the UEFI spec) outputs a TableKey parameter, and only
that parameter can be used as input to the

  EFI_ACPI_TABLE_PROTOCOL.UninstallAcpiTable()

function (also from the UEFI spec).


The PI spec defines

  EFI_ACPI_SDT_PROTOCOL

which has a member function called

  EFI_ACPI_SDT_PROTOCOL.GetAcpiTable()

Using this function, you can iterate over all the tables, investigate
each, and even get the TableKey for uninstallation. (See the function's
specification itself.)


So, if you are willing to "go PI", then independent uninstallation is
possible. But, IMO, the unclear ownership of any table can lead to a
mess down the road.

In edk2, EFI_ACPI_SDT_PROTOCOL is available from

  MdeModulePkg/Universal/Acpi/AcpiTableDxe

if

  gEfiMdeModulePkgTokenSpaceGuid.PcdInstallAcpiSdtProtocol

is TRUE.

For one example, OVMF set the PCD in commit 259d87146b07 ("OvmfPkg:
Modify FDF/DSC files for RamDiskDxe's adding NFIT report feature",
2016-04-19).

Thanks!
Laszlo