[PATCH V4] MdeModulePkg/DxeCore: Fixed Interface returned by CoreOpenProtocol

[PATCH V4] MdeModulePkg/DxeCore: Fixed Interface returned by CoreOpenProtocol

[Amit Kumar]

Change since v3:
1) Fixed issue when Attributes = EFI_OPEN_PROTOCOL_TEST_PROTOCOL
and Inteface = NULL case. [Reported by:star.zeng at intel.com]

Change Since v2:
1) Modified to use EFI_ERROR to get status code

Change since v1:
1) Fixed typo protocal to protocol
2) Fixed coding style

Modified source code to update Interface as per spec.
1) In case of Protocol is un-supported, interface should be returned NULL.
2) In case of any error, interface should not be modified.
3) In case of Test Protocol, interface is optional.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Amit Kumar <amit.ak@samsung.com>
---
 MdeModulePkg/Core/Dxe/Hand/Handle.c | 26 ++++++++++++++------------
 1 file changed, 14 insertions(+), 12 deletions(-)

diff --git a/MdeModulePkg/Core/Dxe/Hand/Handle.c b/MdeModulePkg/Core/Dxe/Hand/Handle.c
index 59b8914..fe58b6c 100644
--- a/MdeModulePkg/Core/Dxe/Hand/Handle.c
+++ b/MdeModulePkg/Core/Dxe/Hand/Handle.c
@@ -1006,12 +1006,8 @@ CoreOpenProtocol (
   //
   // Check for invalid Interface
   //
-  if (Attributes != EFI_OPEN_PROTOCOL_TEST_PROTOCOL) {
-    if (Interface == NULL) {
-      return EFI_INVALID_PARAMETER;
-    } else {
-      *Interface = NULL;
-    }
+  if ((Attributes != EFI_OPEN_PROTOCOL_TEST_PROTOCOL) && (Interface == NULL)) {
+    return EFI_INVALID_PARAMETER;
   }
 
   //
@@ -1075,15 +1071,13 @@ CoreOpenProtocol (
   Prot = CoreGetProtocolInterface (UserHandle, Protocol);
   if (Prot == NULL) {
     Status = EFI_UNSUPPORTED;
+    if (Attributes != EFI_OPEN_PROTOCOL_TEST_PROTOCOL){
+      //Return NULL Interface if Unsupported Protocol
+      *Interface = NULL;
+    }
     goto Done;
   }
 
-  //
-  // This is the protocol interface entry for this protocol
-  //
-  if (Attributes != EFI_OPEN_PROTOCOL_TEST_PROTOCOL) {
-    *Interface = Prot->Interface;
-  }
   Status = EFI_SUCCESS;
 
   ByDriver        = FALSE;
@@ -1177,6 +1171,14 @@ CoreOpenProtocol (
   }
 
 Done:
+
+  //
+  // This is the protocol interface entry for this protocol.
+  // In case of any Error, Interface should not be updated as per spec.
+  //
+  if (!EFI_ERROR (Status) && (Attributes != EFI_OPEN_PROTOCOL_TEST_PROTOCOL)) {
+    *Interface = Prot->Interface;
+  }
   //
   // Done. Release the database lock are return
   //
-- 
1.9.1

Re: [PATCH V4] MdeModulePkg/DxeCore: Fixed Interface returned by CoreOpenProtocol

[Zeng, Star]

Reviewed-by: Star Zeng <star.zeng@intel.com>

-----Original Message-----
From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Amit Kumar
Sent: Friday, June 23, 2017 6:10 PM
To: edk2-devel@lists.01.org
Cc: Tian, Feng <feng.tian@intel.com>; Gao, Liming <liming.gao@intel.com>; Kinney, Michael D <michael.d.kinney@intel.com>; Zeng, Star <star.zeng@intel.com>
Subject: [edk2] [PATCH V4] MdeModulePkg/DxeCore: Fixed Interface returned by CoreOpenProtocol

Change since v3:
1) Fixed issue when Attributes = EFI_OPEN_PROTOCOL_TEST_PROTOCOL and Inteface = NULL case. [Reported by:star.zeng at intel.com]

Change Since v2:
1) Modified to use EFI_ERROR to get status code

Change since v1:
1) Fixed typo protocal to protocol
2) Fixed coding style

Modified source code to update Interface as per spec.
1) In case of Protocol is un-supported, interface should be returned NULL.
2) In case of any error, interface should not be modified.
3) In case of Test Protocol, interface is optional.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Amit Kumar <amit.ak@samsung.com>
---
 MdeModulePkg/Core/Dxe/Hand/Handle.c | 26 ++++++++++++++------------
 1 file changed, 14 insertions(+), 12 deletions(-)

diff --git a/MdeModulePkg/Core/Dxe/Hand/Handle.c b/MdeModulePkg/Core/Dxe/Hand/Handle.c
index 59b8914..fe58b6c 100644
--- a/MdeModulePkg/Core/Dxe/Hand/Handle.c
+++ b/MdeModulePkg/Core/Dxe/Hand/Handle.c
@@ -1006,12 +1006,8 @@ CoreOpenProtocol (
   //
   // Check for invalid Interface
   //
-  if (Attributes != EFI_OPEN_PROTOCOL_TEST_PROTOCOL) {
-    if (Interface == NULL) {
-      return EFI_INVALID_PARAMETER;
-    } else {
-      *Interface = NULL;
-    }
+  if ((Attributes != EFI_OPEN_PROTOCOL_TEST_PROTOCOL) && (Interface == NULL)) {
+    return EFI_INVALID_PARAMETER;
   }
 
   //
@@ -1075,15 +1071,13 @@ CoreOpenProtocol (
   Prot = CoreGetProtocolInterface (UserHandle, Protocol);
   if (Prot == NULL) {
     Status = EFI_UNSUPPORTED;
+    if (Attributes != EFI_OPEN_PROTOCOL_TEST_PROTOCOL){
+      //Return NULL Interface if Unsupported Protocol
+      *Interface = NULL;
+    }
     goto Done;
   }
 
-  //
-  // This is the protocol interface entry for this protocol
-  //
-  if (Attributes != EFI_OPEN_PROTOCOL_TEST_PROTOCOL) {
-    *Interface = Prot->Interface;
-  }
   Status = EFI_SUCCESS;
 
   ByDriver        = FALSE;
@@ -1177,6 +1171,14 @@ CoreOpenProtocol (
   }
 
 Done:
+
+  //
+  // This is the protocol interface entry for this protocol.
+  // In case of any Error, Interface should not be updated as per spec.
+  //
+  if (!EFI_ERROR (Status) && (Attributes != EFI_OPEN_PROTOCOL_TEST_PROTOCOL)) {
+    *Interface = Prot->Interface;
+  }
   //
   // Done. Release the database lock are return
   //
--
1.9.1

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [PATCH V4] MdeModulePkg/DxeCore: Fixed Interface returned by CoreOpenProtocol

[Zeng, Star]

Patch has been pushed at 45cfcd8dccf84b8abbc1d6f587fedb5d2037ec79. :)

Thanks,
Star
-----Original Message-----
From: Zeng, Star 
Sent: Monday, June 26, 2017 9:20 AM
To: Amit Kumar <amit.ak@samsung.com>; edk2-devel@lists.01.org
Cc: Tian, Feng <feng.tian@intel.com>; Gao, Liming <liming.gao@intel.com>; Kinney, Michael D <michael.d.kinney@intel.com>; Zeng, Star <star.zeng@intel.com>
Subject: RE: [edk2] [PATCH V4] MdeModulePkg/DxeCore: Fixed Interface returned by CoreOpenProtocol

Reviewed-by: Star Zeng <star.zeng@intel.com>

-----Original Message-----
From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Amit Kumar
Sent: Friday, June 23, 2017 6:10 PM
To: edk2-devel@lists.01.org
Cc: Tian, Feng <feng.tian@intel.com>; Gao, Liming <liming.gao@intel.com>; Kinney, Michael D <michael.d.kinney@intel.com>; Zeng, Star <star.zeng@intel.com>
Subject: [edk2] [PATCH V4] MdeModulePkg/DxeCore: Fixed Interface returned by CoreOpenProtocol

Change since v3:
1) Fixed issue when Attributes = EFI_OPEN_PROTOCOL_TEST_PROTOCOL and Inteface = NULL case. [Reported by:star.zeng at intel.com]

Change Since v2:
1) Modified to use EFI_ERROR to get status code

Change since v1:
1) Fixed typo protocal to protocol
2) Fixed coding style

Modified source code to update Interface as per spec.
1) In case of Protocol is un-supported, interface should be returned NULL.
2) In case of any error, interface should not be modified.
3) In case of Test Protocol, interface is optional.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Amit Kumar <amit.ak@samsung.com>
---
 MdeModulePkg/Core/Dxe/Hand/Handle.c | 26 ++++++++++++++------------
 1 file changed, 14 insertions(+), 12 deletions(-)

diff --git a/MdeModulePkg/Core/Dxe/Hand/Handle.c b/MdeModulePkg/Core/Dxe/Hand/Handle.c
index 59b8914..fe58b6c 100644
--- a/MdeModulePkg/Core/Dxe/Hand/Handle.c
+++ b/MdeModulePkg/Core/Dxe/Hand/Handle.c
@@ -1006,12 +1006,8 @@ CoreOpenProtocol (
   //
   // Check for invalid Interface
   //
-  if (Attributes != EFI_OPEN_PROTOCOL_TEST_PROTOCOL) {
-    if (Interface == NULL) {
-      return EFI_INVALID_PARAMETER;
-    } else {
-      *Interface = NULL;
-    }
+  if ((Attributes != EFI_OPEN_PROTOCOL_TEST_PROTOCOL) && (Interface == NULL)) {
+    return EFI_INVALID_PARAMETER;
   }
 
   //
@@ -1075,15 +1071,13 @@ CoreOpenProtocol (
   Prot = CoreGetProtocolInterface (UserHandle, Protocol);
   if (Prot == NULL) {
     Status = EFI_UNSUPPORTED;
+    if (Attributes != EFI_OPEN_PROTOCOL_TEST_PROTOCOL){
+      //Return NULL Interface if Unsupported Protocol
+      *Interface = NULL;
+    }
     goto Done;
   }
 
-  //
-  // This is the protocol interface entry for this protocol
-  //
-  if (Attributes != EFI_OPEN_PROTOCOL_TEST_PROTOCOL) {
-    *Interface = Prot->Interface;
-  }
   Status = EFI_SUCCESS;
 
   ByDriver        = FALSE;
@@ -1177,6 +1171,14 @@ CoreOpenProtocol (
   }
 
 Done:
+
+  //
+  // This is the protocol interface entry for this protocol.
+  // In case of any Error, Interface should not be updated as per spec.
+  //
+  if (!EFI_ERROR (Status) && (Attributes != EFI_OPEN_PROTOCOL_TEST_PROTOCOL)) {
+    *Interface = Prot->Interface;
+  }
   //
   // Done. Release the database lock are return
   //
--
1.9.1

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [PATCH V4] MdeModulePkg/DxeCore: Fixed Interface returned by CoreOpenProtocol

[Laszlo Ersek]

[-- Attachment #1: Type: text/plain, Size: 2547 bytes --]

On 06/23/17 12:09, Amit Kumar wrote:
> Change since v3:
> 1) Fixed issue when Attributes = EFI_OPEN_PROTOCOL_TEST_PROTOCOL
> and Inteface = NULL case. [Reported by:star.zeng at intel.com]
> 
> Change Since v2:
> 1) Modified to use EFI_ERROR to get status code
> 
> Change since v1:
> 1) Fixed typo protocal to protocol
> 2) Fixed coding style
> 
> Modified source code to update Interface as per spec.
> 1) In case of Protocol is un-supported, interface should be returned NULL.
> 2) In case of any error, interface should not be modified.
> 3) In case of Test Protocol, interface is optional.
> 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Amit Kumar <amit.ak@samsung.com>
> ---
>  MdeModulePkg/Core/Dxe/Hand/Handle.c | 26 ++++++++++++++------------
>  1 file changed, 14 insertions(+), 12 deletions(-)

This patch breaks *at least* the following drivers:
- IntelFrameworkModulePkg/.../IsaBusDxe
- IntelFrameworkModulePkg/.../IsaSerialDxe
- MdeModulePkg/.../TerminalDxe
- MdeModulePkg/.../ScsiBusDxe

I have patches for the first three. I'm too tired to complete the patch
for the fourth module (and any modules that might still be affected, but
I'd only see those in the OVMF boot process if I got past the ScsiBusDxe
problem.)

The issue was reported by Gabriel here:

https://lists.01.org/pipermail/edk2-devel/2017-June/011886.html

although he didn't get past of the first driver, of course.

Now, what this patch does is valid, as far as the UEFI spec is
concerned. And the above (now broken) drivers are all incorrect to
assume that EFI_ALREADY_STARTED guarantees that OpenProtocol() has
filled in Interface on output.

However, such an intrusive fix must be accompanied by extensive testing;
preferably using one of the in-tree emulation platforms, such as OvmfPkg
running on QEMU. And, when testing uncovers the failures in those
drivers, patches should be submitted to fix those drivers *first*, and
then the patch for OpenProtocol() should be presented *last*.

I'm attaching the first three patches I have now. As I said I'm too
tired to work on ScsiBusDxe (the SCSIBusDriverBindingStart() function
has the same bug around EFI_ALREADY_STARTED). Which is why I'm not
posting the first three patches normally, with git-send-email; the work
is incomplete.

Honestly, I'm thinking that commit 45cfcd8dccf8 should be reverted now,
then all the drivers used by OvmfPkg should be fixed up (not by me,
obviously!), and once OVMF boots again, we can re-commit (= cherry-pick)
commit 45cfcd8dccf8.

Thanks
Laszlo


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-IntelFrameworkModulePkg-IsaBusDxe-fix-OpenProtocol-usage.patch --]
[-- Type: text/x-patch; name="0001-IntelFrameworkModulePkg-IsaBusDxe-fix-OpenProtocol-usage.patch", Size: 5181 bytes --]

From 0726d69fa17396cfdf77db262dcfcac27aa8c848 Mon Sep 17 00:00:00 2001
From: Laszlo Ersek <lersek@redhat.com>
Date: Tue, 27 Jun 2017 01:34:25 +0200
Subject: [PATCH 1/3] IntelFrameworkModulePkg/IsaBusDxe: fix OpenProtocol()
 usage

The IsaBusControllerDriverStart() function exploits the previous,
non-standard behavior of edk2 that OpenProtocol() outputs the valid
protocol Interface even when OpenProtocol() returns EFI_ALREADY_STARTED.

This behavior has been deleted with commit 45cfcd8dccf8
("MdeModulePkg/DxeCore: Fixed Interface returned by CoreOpenProtocol",
2017-06-23); Interface is no longer set on error (which conforms to the
UEFI spec). As a result, when the OpenProtocol() calls in
IsaBusControllerDriverStart() fail with EFI_ALREADY_STARTED,
ParentDevicePath and IsaAcpi have uninitialized (indeterminate) values,
leading to crashes.

Restore the previous behavior of IsaBusControllerDriverStart() by
explicitly getting the protocol interfaces with
EFI_OPEN_PROTOCOL_GET_PROTOCOL.

Cc: "Gabriel L. Somlo" <gsomlo@gmail.com>
Cc: Amit Kumar <amit.ak@samsung.com>
Cc: Jeff Fan <jeff.fan@intel.com>
Cc: Liming Gao <liming.gao@intel.com>
Cc: Star Zeng <star.zeng@intel.com>
Reported-by: "Gabriel L. Somlo" <gsomlo@gmail.com>
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
 IntelFrameworkModulePkg/Bus/Isa/IsaBusDxe/IsaBus.c | 51 +++++++++++++++++-----
 1 file changed, 40 insertions(+), 11 deletions(-)

diff --git a/IntelFrameworkModulePkg/Bus/Isa/IsaBusDxe/IsaBus.c b/IntelFrameworkModulePkg/Bus/Isa/IsaBusDxe/IsaBus.c
index 1312f260f9ae..7d996cf86eb1 100644
--- a/IntelFrameworkModulePkg/Bus/Isa/IsaBusDxe/IsaBus.c
+++ b/IntelFrameworkModulePkg/Bus/Isa/IsaBusDxe/IsaBus.c
@@ -252,62 +252,91 @@ IsaBusControllerDriverStart (
                   Controller,
                   EFI_OPEN_PROTOCOL_GET_PROTOCOL
                   );
   if (EFI_ERROR (Status)) {
     return Status;
   }
 
   //
   // Open Device Path Protocol
   //
   Status = gBS->OpenProtocol (
                   Controller,
                   &gEfiDevicePathProtocolGuid,
                   (VOID **) &ParentDevicePath,
                   This->DriverBindingHandle,
                   Controller,
                   EFI_OPEN_PROTOCOL_BY_DRIVER
                   );
-  if (EFI_ERROR (Status) && Status != EFI_ALREADY_STARTED) {
-    return Status;
+  if (EFI_ERROR (Status)) {
+    if  (Status != EFI_ALREADY_STARTED) {
+      return Status;
+    }
+    //
+    // Repeat the above OpenProtocol() call with GET_PROTOCOL.
+    //
+    Status = gBS->OpenProtocol (
+                    Controller,
+                    &gEfiDevicePathProtocolGuid,
+                    (VOID **) &ParentDevicePath,
+                    This->DriverBindingHandle,
+                    Controller,
+                    EFI_OPEN_PROTOCOL_GET_PROTOCOL
+                    );
+    ASSERT_EFI_ERROR (Status);
   }
 
   //
   // Open ISA Acpi Protocol
   //
   Status = gBS->OpenProtocol (
                   Controller,
                   &gEfiIsaAcpiProtocolGuid,
                   (VOID **) &IsaAcpi,
                   This->DriverBindingHandle,
                   Controller,
                   EFI_OPEN_PROTOCOL_BY_DRIVER
                   );
-  if (EFI_ERROR (Status) && Status != EFI_ALREADY_STARTED) {
+  if (EFI_ERROR (Status)) {
+    if (Status != EFI_ALREADY_STARTED) {
+      //
+      // Close opened protocol. (This is required after BY_DRIVER, and not
+      // required but permitted after GET_PROTOCOL.)
+      //
+      gBS->CloseProtocol (
+             Controller,
+             &gEfiDevicePathProtocolGuid,
+             This->DriverBindingHandle,
+             Controller
+             );
+      return Status;
+    }
     //
-    // Close opened protocol
+    // Repeat the above OpenProtocol() call with GET_PROTOCOL.
     //
-    gBS->CloseProtocol (
-           Controller,
-           &gEfiDevicePathProtocolGuid,
-           This->DriverBindingHandle,
-           Controller
-           );
-    return Status;
+    Status = gBS->OpenProtocol (
+                    Controller,
+                    &gEfiIsaAcpiProtocolGuid,
+                    (VOID **) &IsaAcpi,
+                    This->DriverBindingHandle,
+                    Controller,
+                    EFI_OPEN_PROTOCOL_GET_PROTOCOL
+                    );
+    ASSERT_EFI_ERROR (Status);
   }
   //
   // The IsaBus driver will use memory below 16M, which is not tested yet,
   // so call CompatibleRangeTest to test them. Since memory below 1M should
   // be reserved to CSM, and 15M~16M might be reserved for Isa hole, test 1M
   // ~15M here
   //
   Status = gBS->LocateProtocol (
                   &gEfiGenericMemTestProtocolGuid,
                   NULL,
                   (VOID **) &GenMemoryTest
                   );
 
   if (!EFI_ERROR (Status)) {
     Status = GenMemoryTest->CompatibleRangeTest (
                               GenMemoryTest,
                               0x100000,
                               0xE00000
-- 
2.13.1.3.g8be5a757fa67


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #3: 0002-IntelFrameworkModulePkg-IsaSerialDxe-fix-OpenProtocol-usage.patch --]
[-- Type: text/x-patch; name="0002-IntelFrameworkModulePkg-IsaSerialDxe-fix-OpenProtocol-usage.patch", Size: 6480 bytes --]

From cf82bd00e6253ebe61b846e263fdc22b11d12dc3 Mon Sep 17 00:00:00 2001
From: Laszlo Ersek <lersek@redhat.com>
Date: Tue, 27 Jun 2017 01:34:25 +0200
Subject: [PATCH 2/3] IntelFrameworkModulePkg/IsaSerialDxe: fix OpenProtocol()
 usage

The SerialControllerDriverStart() function exploits the previous,
non-standard behavior of edk2 that OpenProtocol() outputs the valid
protocol Interface even when OpenProtocol() returns EFI_ALREADY_STARTED.

This behavior has been deleted with commit 45cfcd8dccf8
("MdeModulePkg/DxeCore: Fixed Interface returned by CoreOpenProtocol",
2017-06-23); Interface is no longer set on error (which conforms to the
UEFI spec). As a result, when the OpenProtocol() calls in
SerialControllerDriverStart() fail with EFI_ALREADY_STARTED,
ParentDevicePath and IsaIo have uninitialized (indeterminate) values,
leading to a failed assertion later.

Restore the previous behavior of SerialControllerDriverStart() by
explicitly getting the protocol interfaces with
EFI_OPEN_PROTOCOL_GET_PROTOCOL.

Cc: "Gabriel L. Somlo" <gsomlo@gmail.com>
Cc: Amit Kumar <amit.ak@samsung.com>
Cc: Jeff Fan <jeff.fan@intel.com>
Cc: Liming Gao <liming.gao@intel.com>
Cc: Star Zeng <star.zeng@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
 .../Bus/Isa/IsaSerialDxe/Serial.c                  | 36 ++++++++++++++++++++--
 1 file changed, 34 insertions(+), 2 deletions(-)

diff --git a/IntelFrameworkModulePkg/Bus/Isa/IsaSerialDxe/Serial.c b/IntelFrameworkModulePkg/Bus/Isa/IsaSerialDxe/Serial.c
index 57ee669d1406..48836f6fee92 100644
--- a/IntelFrameworkModulePkg/Bus/Isa/IsaSerialDxe/Serial.c
+++ b/IntelFrameworkModulePkg/Bus/Isa/IsaSerialDxe/Serial.c
@@ -409,73 +409,100 @@ SerialControllerDriverStart (
   UART_DEVICE_PATH                    *Uart;
   UINT32                              FlowControlMap;
   UART_FLOW_CONTROL_DEVICE_PATH       *FlowControl;
   EFI_DEVICE_PATH_PROTOCOL            *TempDevicePath;
   UINT32                              Control;
 
   SerialDevice = NULL;
   //
   // Get the Parent Device Path
   //
   Status = gBS->OpenProtocol (
                   Controller,
                   &gEfiDevicePathProtocolGuid,
                   (VOID **) &ParentDevicePath,
                   This->DriverBindingHandle,
                   Controller,
                   EFI_OPEN_PROTOCOL_BY_DRIVER
                   );
-  if (EFI_ERROR (Status) && Status != EFI_ALREADY_STARTED) {
-    return Status;
+  if (EFI_ERROR (Status)) {
+    if (Status != EFI_ALREADY_STARTED) {
+      return Status;
+    }
+    //
+    // Repeat the above OpenProtocol() call with GET_PROTOCOL.
+    //
+    Status = gBS->OpenProtocol (
+                    Controller,
+                    &gEfiDevicePathProtocolGuid,
+                    (VOID **) &ParentDevicePath,
+                    This->DriverBindingHandle,
+                    Controller,
+                    EFI_OPEN_PROTOCOL_GET_PROTOCOL
+                    );
+    ASSERT_EFI_ERROR (Status);
   }
   //
   // Report status code enable the serial
   //
   REPORT_STATUS_CODE_WITH_DEVICE_PATH (
     EFI_PROGRESS_CODE,
     EFI_P_PC_ENABLE | EFI_PERIPHERAL_SERIAL_PORT,
     ParentDevicePath
     );
 
   //
   // Grab the IO abstraction we need to get any work done
   //
   Status = gBS->OpenProtocol (
                   Controller,
                   &gEfiIsaIoProtocolGuid,
                   (VOID **) &IsaIo,
                   This->DriverBindingHandle,
                   Controller,
                   EFI_OPEN_PROTOCOL_BY_DRIVER
                   );
   if (EFI_ERROR (Status) && Status != EFI_ALREADY_STARTED) {
     goto Error;
   }
 
   if (Status == EFI_ALREADY_STARTED) {
 
     if (RemainingDevicePath == NULL || IsDevicePathEnd (RemainingDevicePath)) {
       //
       // If RemainingDevicePath is NULL or is the End of Device Path Node
       //
       return EFI_SUCCESS;
     }
 
     //
+    // Repeat the above OpenProtocol() call with GET_PROTOCOL.
+    //
+    Status = gBS->OpenProtocol (
+                    Controller,
+                    &gEfiIsaIoProtocolGuid,
+                    (VOID **) &IsaIo,
+                    This->DriverBindingHandle,
+                    Controller,
+                    EFI_OPEN_PROTOCOL_GET_PROTOCOL
+                    );
+    ASSERT_EFI_ERROR (Status);
+
+    //
     // Make sure a child handle does not already exist.  This driver can only
     // produce one child per serial port.
     //
     Status = gBS->OpenProtocolInformation (
                     Controller,
                     &gEfiIsaIoProtocolGuid,
                     &OpenInfoBuffer,
                     &EntryCount
                     );
     if (EFI_ERROR (Status)) {
       return Status;
     }
 
     Status = EFI_ALREADY_STARTED;
     for (Index = 0; Index < EntryCount; Index++) {
       if ((OpenInfoBuffer[Index].Attributes & EFI_OPEN_PROTOCOL_BY_CHILD_CONTROLLER) != 0) {
         Status = gBS->OpenProtocol (
                         OpenInfoBuffer[Index].ControllerHandle,
@@ -659,36 +686,41 @@ SerialControllerDriverStart (
                   );
   if (EFI_ERROR (Status)) {
     goto Error;
   }
   //
   // Open For Child Device
   //
   Status = gBS->OpenProtocol (
                   Controller,
                   &gEfiIsaIoProtocolGuid,
                   (VOID **) &IsaIo,
                   This->DriverBindingHandle,
                   SerialDevice->Handle,
                   EFI_OPEN_PROTOCOL_BY_CHILD_CONTROLLER
                   );
 
 Error:
   if (EFI_ERROR (Status)) {
+    //
+    // Closing the protocol interfaces opened with BY_DRIVER is required.
+    // Closing the protocol interfaces opened with GET_PROTOCOL is not required
+    // but permitted.
+    //
     gBS->CloseProtocol (
            Controller,
            &gEfiDevicePathProtocolGuid,
            This->DriverBindingHandle,
            Controller
            );
     gBS->CloseProtocol (
            Controller,
            &gEfiIsaIoProtocolGuid,
            This->DriverBindingHandle,
            Controller
            );
     if (SerialDevice != NULL) {
       if (SerialDevice->DevicePath != NULL) {
         gBS->FreePool (SerialDevice->DevicePath);
       }
 
       FreeUnicodeStringTable (SerialDevice->ControllerNameTable);
-- 
2.13.1.3.g8be5a757fa67


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #4: 0003-MdeModulePkg-TerminalDxe-fix-OpenProtocol-usage.patch --]
[-- Type: text/x-patch; name="0003-MdeModulePkg-TerminalDxe-fix-OpenProtocol-usage.patch", Size: 6776 bytes --]

From 899c1020de7d1239891d61e15a034aa1b13a8381 Mon Sep 17 00:00:00 2001
From: Laszlo Ersek <lersek@redhat.com>
Date: Tue, 27 Jun 2017 01:34:25 +0200
Subject: [PATCH 3/3] MdeModulePkg/TerminalDxe: fix OpenProtocol() usage

The TerminalDriverBindingStart() function exploits the previous,
non-standard behavior of edk2 that OpenProtocol() outputs the valid
protocol Interface even when OpenProtocol() returns EFI_ALREADY_STARTED.

This behavior has been deleted with commit 45cfcd8dccf8
("MdeModulePkg/DxeCore: Fixed Interface returned by CoreOpenProtocol",
2017-06-23); Interface is no longer set on error (which conforms to the
UEFI spec). As a result, when the OpenProtocol() calls in
TerminalDriverBindingStart() fail with EFI_ALREADY_STARTED,
ParentDevicePath and SerialIo have uninitialized (indeterminate) values,
leading to a failed assertion later.

Restore the previous behavior of TerminalDriverBindingStart() by
explicitly getting the protocol interfaces with
EFI_OPEN_PROTOCOL_GET_PROTOCOL.

Cc: "Gabriel L. Somlo" <gsomlo@gmail.com>
Cc: Amit Kumar <amit.ak@samsung.com>
Cc: Eric Dong <eric.dong@intel.com>
Cc: Liming Gao <liming.gao@intel.com>
Cc: Star Zeng <star.zeng@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
 .../Universal/Console/TerminalDxe/Terminal.c       | 32 ++++++++++++++++++++++
 1 file changed, 32 insertions(+)

diff --git a/MdeModulePkg/Universal/Console/TerminalDxe/Terminal.c b/MdeModulePkg/Universal/Console/TerminalDxe/Terminal.c
index c90879b0dd87..16fe6de21f1e 100644
--- a/MdeModulePkg/Universal/Console/TerminalDxe/Terminal.c
+++ b/MdeModulePkg/Universal/Console/TerminalDxe/Terminal.c
@@ -508,36 +508,50 @@ TerminalDriverBindingStart (
   UINTN                               EntryCount;
   UINTN                               Index;
   EFI_SIMPLE_TEXT_OUTPUT_PROTOCOL     *SimpleTextOutput;
   EFI_SIMPLE_TEXT_INPUT_PROTOCOL      *SimpleTextInput;
   EFI_UNICODE_STRING_TABLE            *ControllerNameTable;
 
   //
   // Get the Device Path Protocol to build the device path of the child device
   //
   Status = gBS->OpenProtocol (
                   Controller,
                   &gEfiDevicePathProtocolGuid,
                   (VOID **) &ParentDevicePath,
                   This->DriverBindingHandle,
                   Controller,
                   EFI_OPEN_PROTOCOL_BY_DRIVER
                   );
   ASSERT ((Status == EFI_SUCCESS) || (Status == EFI_ALREADY_STARTED));
+  if (Status == EFI_ALREADY_STARTED) {
+    //
+    // Repeat the above OpenProtocol() call with GET_PROTOCOL.
+    //
+    Status = gBS->OpenProtocol (
+                    Controller,
+                    &gEfiDevicePathProtocolGuid,
+                    (VOID **) &ParentDevicePath,
+                    This->DriverBindingHandle,
+                    Controller,
+                    EFI_OPEN_PROTOCOL_GET_PROTOCOL
+                    );
+    ASSERT_EFI_ERROR (Status);
+  }
 
   //
   // Open the Serial I/O Protocol BY_DRIVER.  It might already be started.
   //
   Status = gBS->OpenProtocol (
                   Controller,
                   &gEfiSerialIoProtocolGuid,
                   (VOID **) &SerialIo,
                   This->DriverBindingHandle,
                   Controller,
                   EFI_OPEN_PROTOCOL_BY_DRIVER
                   );
   ASSERT ((Status == EFI_SUCCESS) || (Status == EFI_ALREADY_STARTED));
 
   if (!IsHotPlugDevice (ParentDevicePath)) {
     //
     // if the serial device is a hot plug device, do not update the
     // ConInDev, ConOutDev, and StdErrDev variables.
@@ -549,36 +563,49 @@ TerminalDriverBindingStart (
 
   //
   // Do not create any child for END remaining device path.
   //
   if ((RemainingDevicePath != NULL) && IsDevicePathEnd (RemainingDevicePath)) {
     return EFI_SUCCESS;
   }
 
   if (Status == EFI_ALREADY_STARTED) {
 
     if (RemainingDevicePath == NULL) {
       //
       // If RemainingDevicePath is NULL or is the End of Device Path Node
       //
       return EFI_SUCCESS;
     }
 
     //
+    // Repeat the above OpenProtocol() call with GET_PROTOCOL.
+    //
+    Status = gBS->OpenProtocol (
+                    Controller,
+                    &gEfiSerialIoProtocolGuid,
+                    (VOID **) &SerialIo,
+                    This->DriverBindingHandle,
+                    Controller,
+                    EFI_OPEN_PROTOCOL_GET_PROTOCOL
+                    );
+    ASSERT_EFI_ERROR (Status);
+
+    //
     // This driver can only produce one child per serial port.
     // Change its terminal type as remaining device path requests.
     //
     Status = gBS->OpenProtocolInformation (
                     Controller,
                     &gEfiSerialIoProtocolGuid,
                     &OpenInfoBuffer,
                     &EntryCount
                     );
     if (!EFI_ERROR (Status)) {
       Status = EFI_NOT_FOUND;
       for (Index = 0; Index < EntryCount; Index++) {
         if ((OpenInfoBuffer[Index].Attributes & EFI_OPEN_PROTOCOL_BY_CHILD_CONTROLLER) != 0) {
           Status = gBS->OpenProtocol (
                           OpenInfoBuffer[Index].ControllerHandle,
                           &gEfiSimpleTextInProtocolGuid,
                           (VOID **) &SimpleTextInput,
                           This->DriverBindingHandle,
@@ -850,36 +877,41 @@ FreeResources:
   }
 
   if (TerminalDevice->TerminalConsoleModeData != NULL) {
     FreePool (TerminalDevice->TerminalConsoleModeData);
   }
 
   FreePool (TerminalDevice);
 
 CloseProtocols:
 
   //
   // Remove Parent Device Path from
   // the Console Device Environment Variables
   //
   TerminalRemoveConsoleDevVariable (EFI_CON_IN_DEV_VARIABLE_NAME, ParentDevicePath);
   TerminalRemoveConsoleDevVariable (EFI_CON_OUT_DEV_VARIABLE_NAME, ParentDevicePath);
   TerminalRemoveConsoleDevVariable (EFI_ERR_OUT_DEV_VARIABLE_NAME, ParentDevicePath);
 
+  //
+  // Closing the protocol interfaces opened with BY_DRIVER is required. Closing
+  // the protocol interfaces opened with GET_PROTOCOL is not required but
+  // permitted.
+  //
   Status = gBS->CloseProtocol (
                   Controller,
                   &gEfiSerialIoProtocolGuid,
                   This->DriverBindingHandle,
                   Controller
                   );
   ASSERT_EFI_ERROR (Status);
 
   Status = gBS->CloseProtocol (
                   Controller,
                   &gEfiDevicePathProtocolGuid,
                   This->DriverBindingHandle,
                   Controller
                   );
   ASSERT_EFI_ERROR (Status);
   return Status;
 }
 
-- 
2.13.1.3.g8be5a757fa67

Re: [PATCH V4] MdeModulePkg/DxeCore: Fixed Interface returned by CoreOpenProtocol

[Zeng, Star]

Laszlo,

I agree to revert this patch first since it breaks something.
Could you help do that with detailed revert log?

Anyway, have you found any bug in this patch itself?


Thanks,
Star
-----Original Message-----
From: Laszlo Ersek [mailto:lersek@redhat.com] 
Sent: Tuesday, June 27, 2017 8:47 AM
To: Amit Kumar <amit.ak@samsung.com>; edk2-devel@lists.01.org
Cc: Tian, Feng <feng.tian@intel.com>; Gao, Liming <liming.gao@intel.com>; Kinney, Michael D <michael.d.kinney@intel.com>; Zeng, Star <star.zeng@intel.com>; Gabriel L. Somlo (GMail) <gsomlo@gmail.com>; Fan, Jeff <jeff.fan@intel.com>
Subject: Re: [edk2] [PATCH V4] MdeModulePkg/DxeCore: Fixed Interface returned by CoreOpenProtocol

On 06/23/17 12:09, Amit Kumar wrote:
> Change since v3:
> 1) Fixed issue when Attributes = EFI_OPEN_PROTOCOL_TEST_PROTOCOL and 
> Inteface = NULL case. [Reported by:star.zeng at intel.com]
> 
> Change Since v2:
> 1) Modified to use EFI_ERROR to get status code
> 
> Change since v1:
> 1) Fixed typo protocal to protocol
> 2) Fixed coding style
> 
> Modified source code to update Interface as per spec.
> 1) In case of Protocol is un-supported, interface should be returned NULL.
> 2) In case of any error, interface should not be modified.
> 3) In case of Test Protocol, interface is optional.
> 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Amit Kumar <amit.ak@samsung.com>
> ---
>  MdeModulePkg/Core/Dxe/Hand/Handle.c | 26 ++++++++++++++------------
>  1 file changed, 14 insertions(+), 12 deletions(-)

This patch breaks *at least* the following drivers:
- IntelFrameworkModulePkg/.../IsaBusDxe
- IntelFrameworkModulePkg/.../IsaSerialDxe
- MdeModulePkg/.../TerminalDxe
- MdeModulePkg/.../ScsiBusDxe

I have patches for the first three. I'm too tired to complete the patch for the fourth module (and any modules that might still be affected, but I'd only see those in the OVMF boot process if I got past the ScsiBusDxe
problem.)

The issue was reported by Gabriel here:

https://lists.01.org/pipermail/edk2-devel/2017-June/011886.html

although he didn't get past of the first driver, of course.

Now, what this patch does is valid, as far as the UEFI spec is concerned. And the above (now broken) drivers are all incorrect to assume that EFI_ALREADY_STARTED guarantees that OpenProtocol() has filled in Interface on output.

However, such an intrusive fix must be accompanied by extensive testing; preferably using one of the in-tree emulation platforms, such as OvmfPkg running on QEMU. And, when testing uncovers the failures in those drivers, patches should be submitted to fix those drivers *first*, and then the patch for OpenProtocol() should be presented *last*.

I'm attaching the first three patches I have now. As I said I'm too tired to work on ScsiBusDxe (the SCSIBusDriverBindingStart() function has the same bug around EFI_ALREADY_STARTED). Which is why I'm not posting the first three patches normally, with git-send-email; the work is incomplete.

Honestly, I'm thinking that commit 45cfcd8dccf8 should be reverted now, then all the drivers used by OvmfPkg should be fixed up (not by me, obviously!), and once OVMF boots again, we can re-commit (= cherry-pick) commit 45cfcd8dccf8.

Thanks
Laszlo

Re: [PATCH V4] MdeModulePkg/DxeCore: Fixed Interface returned by CoreOpenProtocol

[Zeng, Star]

I have reverted this patch at fd220166c435479a81dfe8519c92abec9acf7d82 first.


Thanks,
Star
-----Original Message-----
From: Zeng, Star 
Sent: Tuesday, June 27, 2017 8:53 AM
To: Laszlo Ersek <lersek@redhat.com>; Amit Kumar <amit.ak@samsung.com>; edk2-devel@lists.01.org
Cc: Tian, Feng <feng.tian@intel.com>; Gao, Liming <liming.gao@intel.com>; Kinney, Michael D <michael.d.kinney@intel.com>; Gabriel L. Somlo (GMail) <gsomlo@gmail.com>; Fan, Jeff <jeff.fan@intel.com>; Zeng, Star <star.zeng@intel.com>
Subject: RE: [edk2] [PATCH V4] MdeModulePkg/DxeCore: Fixed Interface returned by CoreOpenProtocol

Laszlo,

I agree to revert this patch first since it breaks something.
Could you help do that with detailed revert log?

Anyway, have you found any bug in this patch itself?


Thanks,
Star
-----Original Message-----
From: Laszlo Ersek [mailto:lersek@redhat.com]
Sent: Tuesday, June 27, 2017 8:47 AM
To: Amit Kumar <amit.ak@samsung.com>; edk2-devel@lists.01.org
Cc: Tian, Feng <feng.tian@intel.com>; Gao, Liming <liming.gao@intel.com>; Kinney, Michael D <michael.d.kinney@intel.com>; Zeng, Star <star.zeng@intel.com>; Gabriel L. Somlo (GMail) <gsomlo@gmail.com>; Fan, Jeff <jeff.fan@intel.com>
Subject: Re: [edk2] [PATCH V4] MdeModulePkg/DxeCore: Fixed Interface returned by CoreOpenProtocol

On 06/23/17 12:09, Amit Kumar wrote:
> Change since v3:
> 1) Fixed issue when Attributes = EFI_OPEN_PROTOCOL_TEST_PROTOCOL and 
> Inteface = NULL case. [Reported by:star.zeng at intel.com]
> 
> Change Since v2:
> 1) Modified to use EFI_ERROR to get status code
> 
> Change since v1:
> 1) Fixed typo protocal to protocol
> 2) Fixed coding style
> 
> Modified source code to update Interface as per spec.
> 1) In case of Protocol is un-supported, interface should be returned NULL.
> 2) In case of any error, interface should not be modified.
> 3) In case of Test Protocol, interface is optional.
> 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Amit Kumar <amit.ak@samsung.com>
> ---
>  MdeModulePkg/Core/Dxe/Hand/Handle.c | 26 ++++++++++++++------------
>  1 file changed, 14 insertions(+), 12 deletions(-)

This patch breaks *at least* the following drivers:
- IntelFrameworkModulePkg/.../IsaBusDxe
- IntelFrameworkModulePkg/.../IsaSerialDxe
- MdeModulePkg/.../TerminalDxe
- MdeModulePkg/.../ScsiBusDxe

I have patches for the first three. I'm too tired to complete the patch for the fourth module (and any modules that might still be affected, but I'd only see those in the OVMF boot process if I got past the ScsiBusDxe
problem.)

The issue was reported by Gabriel here:

https://lists.01.org/pipermail/edk2-devel/2017-June/011886.html

although he didn't get past of the first driver, of course.

Now, what this patch does is valid, as far as the UEFI spec is concerned. And the above (now broken) drivers are all incorrect to assume that EFI_ALREADY_STARTED guarantees that OpenProtocol() has filled in Interface on output.

However, such an intrusive fix must be accompanied by extensive testing; preferably using one of the in-tree emulation platforms, such as OvmfPkg running on QEMU. And, when testing uncovers the failures in those drivers, patches should be submitted to fix those drivers *first*, and then the patch for OpenProtocol() should be presented *last*.

I'm attaching the first three patches I have now. As I said I'm too tired to work on ScsiBusDxe (the SCSIBusDriverBindingStart() function has the same bug around EFI_ALREADY_STARTED). Which is why I'm not posting the first three patches normally, with git-send-email; the work is incomplete.

Honestly, I'm thinking that commit 45cfcd8dccf8 should be reverted now, then all the drivers used by OvmfPkg should be fixed up (not by me, obviously!), and once OVMF boots again, we can re-commit (= cherry-pick) commit 45cfcd8dccf8.

Thanks
Laszlo

Re: [PATCH V4] MdeModulePkg/DxeCore: Fixed Interface returned by CoreOpenProtocol

[Laszlo Ersek]

Hi Star,

On 06/27/17 03:37, Zeng, Star wrote:
> I have reverted this patch at fd220166c435479a81dfe8519c92abec9acf7d82 first.

Thanks for that. More comments below:

> 
> 
> Thanks,
> Star
> -----Original Message-----
> From: Zeng, Star 
> Sent: Tuesday, June 27, 2017 8:53 AM
> To: Laszlo Ersek <lersek@redhat.com>; Amit Kumar <amit.ak@samsung.com>; edk2-devel@lists.01.org
> Cc: Tian, Feng <feng.tian@intel.com>; Gao, Liming <liming.gao@intel.com>; Kinney, Michael D <michael.d.kinney@intel.com>; Gabriel L. Somlo (GMail) <gsomlo@gmail.com>; Fan, Jeff <jeff.fan@intel.com>; Zeng, Star <star.zeng@intel.com>
> Subject: RE: [edk2] [PATCH V4] MdeModulePkg/DxeCore: Fixed Interface returned by CoreOpenProtocol
> 
> Laszlo,
> 
> I agree to revert this patch first since it breaks something.
> Could you help do that with detailed revert log?
> 
> Anyway, have you found any bug in this patch itself?

No, as I said, I think the patch has the right goal (it improves
compliance with the UEFI specification).

And, while I didn't personally review the patch, I trust your and
Liming's review on it.

The problem is not with the patch per se. The problem is that the patch
triggers an existing bug in many other drivers.

So the actual bugs are in those drivers that incorrectly expect
OpenProtocol() to set Interface even when OpenProtocol() returns
EFI_ALREADY_STARTED.

The end result is that all these drivers should be fixed first (with
separate patches), before fixing OpenProtocol() itself.

I'll try to work a bit more on those drivers and discover how many there
are of them. Obviously I can't audit *all* drivers in the edk2 tree,
just those that break the OVMF boot process.

In fact, the newest idea I have is the following: I think I'll introduce
an OpenProtocol() wrapper function to UefiLib. This function should work
identically to OpenProtocol(), except when OpenProtocol() returns
EFI_ALREADY_STARTED. In that case, the wrapper function will repeat the
exact same OpenProtocol() call just with GET_PROTOCOL attribute, and set
Interface on output. This will restore and legitimize the OpenProtocol()
behavior expected by all those problematic drivers, without having to do
intrusive error handling in them.

Thanks,
Laszlo

Re: [PATCH V4] MdeModulePkg/DxeCore: Fixed Interface returned by CoreOpenProtocol

[Zeng, Star]

Laszlo,

I got the point and I like the idea of wrapper function personally.
Although we can clean up the UEFI drivers in EDK2 tree, but it is really hard to evaluate the UEFI drivers in OPROM on add-on card.
The patch did not just break OVMF, but also broke all real platforms we have in hand (it's my fault that I did not catch the failure when reviewing patch), the patch is so risky.


Thanks,
Star
-----Original Message-----
From: Laszlo Ersek [mailto:lersek@redhat.com] 
Sent: Tuesday, June 27, 2017 5:45 PM
To: Zeng, Star <star.zeng@intel.com>; Amit Kumar <amit.ak@samsung.com>; edk2-devel@lists.01.org
Cc: Tian, Feng <feng.tian@intel.com>; Gao, Liming <liming.gao@intel.com>; Kinney, Michael D <michael.d.kinney@intel.com>; Gabriel L. Somlo (GMail) <gsomlo@gmail.com>; Fan, Jeff <jeff.fan@intel.com>
Subject: Re: [edk2] [PATCH V4] MdeModulePkg/DxeCore: Fixed Interface returned by CoreOpenProtocol

Hi Star,

On 06/27/17 03:37, Zeng, Star wrote:
> I have reverted this patch at fd220166c435479a81dfe8519c92abec9acf7d82 first.

Thanks for that. More comments below:

> 
> 
> Thanks,
> Star
> -----Original Message-----
> From: Zeng, Star
> Sent: Tuesday, June 27, 2017 8:53 AM
> To: Laszlo Ersek <lersek@redhat.com>; Amit Kumar 
> <amit.ak@samsung.com>; edk2-devel@lists.01.org
> Cc: Tian, Feng <feng.tian@intel.com>; Gao, Liming 
> <liming.gao@intel.com>; Kinney, Michael D 
> <michael.d.kinney@intel.com>; Gabriel L. Somlo (GMail) 
> <gsomlo@gmail.com>; Fan, Jeff <jeff.fan@intel.com>; Zeng, Star 
> <star.zeng@intel.com>
> Subject: RE: [edk2] [PATCH V4] MdeModulePkg/DxeCore: Fixed Interface 
> returned by CoreOpenProtocol
> 
> Laszlo,
> 
> I agree to revert this patch first since it breaks something.
> Could you help do that with detailed revert log?
> 
> Anyway, have you found any bug in this patch itself?

No, as I said, I think the patch has the right goal (it improves compliance with the UEFI specification).

And, while I didn't personally review the patch, I trust your and Liming's review on it.

The problem is not with the patch per se. The problem is that the patch triggers an existing bug in many other drivers.

So the actual bugs are in those drivers that incorrectly expect
OpenProtocol() to set Interface even when OpenProtocol() returns EFI_ALREADY_STARTED.

The end result is that all these drivers should be fixed first (with separate patches), before fixing OpenProtocol() itself.

I'll try to work a bit more on those drivers and discover how many there are of them. Obviously I can't audit *all* drivers in the edk2 tree, just those that break the OVMF boot process.

In fact, the newest idea I have is the following: I think I'll introduce an OpenProtocol() wrapper function to UefiLib. This function should work identically to OpenProtocol(), except when OpenProtocol() returns EFI_ALREADY_STARTED. In that case, the wrapper function will repeat the exact same OpenProtocol() call just with GET_PROTOCOL attribute, and set Interface on output. This will restore and legitimize the OpenProtocol() behavior expected by all those problematic drivers, without having to do intrusive error handling in them.

Thanks,
Laszlo

Re: [PATCH V4] MdeModulePkg/DxeCore: Fixed Interface returned by CoreOpenProtocol

[Laszlo Ersek]

On 06/27/17 11:59, Zeng, Star wrote:
> Laszlo,
> 
> I got the point and I like the idea of wrapper function personally.
> Although we can clean up the UEFI drivers in EDK2 tree, but it is really hard to evaluate the UEFI drivers in OPROM on add-on card.
> The patch did not just break OVMF, but also broke all real platforms we have in hand (it's my fault that I did not catch the failure when reviewing patch), the patch is so risky.

Can we perhaps add a FeaturePCD (default value: FALSE) and rework Amit's
patch to consider the PCD? I really like the idea of being true to the
UEFI spec.

In the edk2 tree we could rebase the affected drivers to the wrapper
function. And in OVMF (and in other in-tree emulation platforms), we
could set the FeaturePCD to TRUE.

It is possible to use OVMF with assigned physical PCI devices, but
people can easily rebuild OVMF themselves, with the FeaturePCD re-set to
FALSE. Users can also quickly report the exact cards / oproms that break
with the FeaturePCD set to TRUE.


If you think it's simply not worth the effort, I'm OK with that, but
then we should specify this behavior in the UEFI spec -- we'll need a
Mantis bug for that. IMO it's not great that so many UEFI_DRIVERs in the
wild depend on behavior that is expressly forbidden by the UEFI spec at
the moment. If we can't modify all those drivers, then we should adapt
the spec, so that it reflects reality.

I'm not going to suggest specific language for the UEFI spec right now.
But the wording could be based on the documentation of the wrapper
function that I'm working on now. I think I might post the patches just
for illustration.

Thanks,
Laszlo

> 
> 
> Thanks,
> Star
> -----Original Message-----
> From: Laszlo Ersek [mailto:lersek@redhat.com] 
> Sent: Tuesday, June 27, 2017 5:45 PM
> To: Zeng, Star <star.zeng@intel.com>; Amit Kumar <amit.ak@samsung.com>; edk2-devel@lists.01.org
> Cc: Tian, Feng <feng.tian@intel.com>; Gao, Liming <liming.gao@intel.com>; Kinney, Michael D <michael.d.kinney@intel.com>; Gabriel L. Somlo (GMail) <gsomlo@gmail.com>; Fan, Jeff <jeff.fan@intel.com>
> Subject: Re: [edk2] [PATCH V4] MdeModulePkg/DxeCore: Fixed Interface returned by CoreOpenProtocol
> 
> Hi Star,
> 
> On 06/27/17 03:37, Zeng, Star wrote:
>> I have reverted this patch at fd220166c435479a81dfe8519c92abec9acf7d82 first.
> 
> Thanks for that. More comments below:
> 
>>
>>
>> Thanks,
>> Star
>> -----Original Message-----
>> From: Zeng, Star
>> Sent: Tuesday, June 27, 2017 8:53 AM
>> To: Laszlo Ersek <lersek@redhat.com>; Amit Kumar 
>> <amit.ak@samsung.com>; edk2-devel@lists.01.org
>> Cc: Tian, Feng <feng.tian@intel.com>; Gao, Liming 
>> <liming.gao@intel.com>; Kinney, Michael D 
>> <michael.d.kinney@intel.com>; Gabriel L. Somlo (GMail) 
>> <gsomlo@gmail.com>; Fan, Jeff <jeff.fan@intel.com>; Zeng, Star 
>> <star.zeng@intel.com>
>> Subject: RE: [edk2] [PATCH V4] MdeModulePkg/DxeCore: Fixed Interface 
>> returned by CoreOpenProtocol
>>
>> Laszlo,
>>
>> I agree to revert this patch first since it breaks something.
>> Could you help do that with detailed revert log?
>>
>> Anyway, have you found any bug in this patch itself?
> 
> No, as I said, I think the patch has the right goal (it improves compliance with the UEFI specification).
> 
> And, while I didn't personally review the patch, I trust your and Liming's review on it.
> 
> The problem is not with the patch per se. The problem is that the patch triggers an existing bug in many other drivers.
> 
> So the actual bugs are in those drivers that incorrectly expect
> OpenProtocol() to set Interface even when OpenProtocol() returns EFI_ALREADY_STARTED.
> 
> The end result is that all these drivers should be fixed first (with separate patches), before fixing OpenProtocol() itself.
> 
> I'll try to work a bit more on those drivers and discover how many there are of them. Obviously I can't audit *all* drivers in the edk2 tree, just those that break the OVMF boot process.
> 
> In fact, the newest idea I have is the following: I think I'll introduce an OpenProtocol() wrapper function to UefiLib. This function should work identically to OpenProtocol(), except when OpenProtocol() returns EFI_ALREADY_STARTED. In that case, the wrapper function will repeat the exact same OpenProtocol() call just with GET_PROTOCOL attribute, and set Interface on output. This will restore and legitimize the OpenProtocol() behavior expected by all those problematic drivers, without having to do intrusive error handling in them.
> 
> Thanks,
> Laszlo
> 

Re: [PATCH V4] MdeModulePkg/DxeCore: Fixed Interface returned by CoreOpenProtocol

[Laszlo Ersek]

On 06/27/17 13:15, Laszlo Ersek wrote:
> On 06/27/17 11:59, Zeng, Star wrote:
>> Laszlo,
>>
>> I got the point and I like the idea of wrapper function personally.
>> Although we can clean up the UEFI drivers in EDK2 tree, but it is really hard to evaluate the UEFI drivers in OPROM on add-on card.
>> The patch did not just break OVMF, but also broke all real platforms we have in hand (it's my fault that I did not catch the failure when reviewing patch), the patch is so risky.
> 
> Can we perhaps add a FeaturePCD (default value: FALSE) and rework Amit's
> patch to consider the PCD? I really like the idea of being true to the
> UEFI spec.
> 
> In the edk2 tree we could rebase the affected drivers to the wrapper
> function. And in OVMF (and in other in-tree emulation platforms), we
> could set the FeaturePCD to TRUE.
> 
> It is possible to use OVMF with assigned physical PCI devices, but
> people can easily rebuild OVMF themselves, with the FeaturePCD re-set to
> FALSE. Users can also quickly report the exact cards / oproms that break
> with the FeaturePCD set to TRUE.
> 
> 
> If you think it's simply not worth the effort, I'm OK with that, but
> then we should specify this behavior in the UEFI spec -- we'll need a
> Mantis bug for that. IMO it's not great that so many UEFI_DRIVERs in the
> wild depend on behavior that is expressly forbidden by the UEFI spec at
> the moment. If we can't modify all those drivers, then we should adapt
> the spec, so that it reflects reality.
> 
> I'm not going to suggest specific language for the UEFI spec right now.
> But the wording could be based on the documentation of the wrapper
> function that I'm working on now. I think I might post the patches just
> for illustration.

OK, I'm giving up. It is too hard to track down every single crash --
the register dump written by UefiCpuPkg's exception handler to the
serial port is not much help, even though it names a module to look at.

So I guess we have to accept that the dependency on EFI_ALREADY_STARTED
setting Interface is just too wide-spread. :/ As I wrote above, I think
this should be documented in the UEFI spec.

I filed the following mantis ticket:

  Permit OpenProtocol() to output the supported protocol interface even
  on error
  https://mantis.uefi.org/mantis/view.php?id=1815

Thanks
Laszlo

Re: [PATCH V4] MdeModulePkg/DxeCore: Fixed Interface returned by CoreOpenProtocol

[Zeng, Star]

I also prefer to document it in UEFI spec personally.
And we are also having more discussion about it internally, nice to share more after that. :)


Thanks,
Star
-----Original Message-----
From: Laszlo Ersek [mailto:lersek@redhat.com] 
Sent: Tuesday, June 27, 2017 9:23 PM
To: Zeng, Star <star.zeng@intel.com>; Amit Kumar <amit.ak@samsung.com>; edk2-devel@lists.01.org
Cc: Kinney, Michael D <michael.d.kinney@intel.com>; Tian, Feng <feng.tian@intel.com>; Gabriel L. Somlo (GMail) <gsomlo@gmail.com>; Gao, Liming <liming.gao@intel.com>; Fan, Jeff <jeff.fan@intel.com>
Subject: Re: [edk2] [PATCH V4] MdeModulePkg/DxeCore: Fixed Interface returned by CoreOpenProtocol

On 06/27/17 13:15, Laszlo Ersek wrote:
> On 06/27/17 11:59, Zeng, Star wrote:
>> Laszlo,
>>
>> I got the point and I like the idea of wrapper function personally.
>> Although we can clean up the UEFI drivers in EDK2 tree, but it is really hard to evaluate the UEFI drivers in OPROM on add-on card.
>> The patch did not just break OVMF, but also broke all real platforms we have in hand (it's my fault that I did not catch the failure when reviewing patch), the patch is so risky.
> 
> Can we perhaps add a FeaturePCD (default value: FALSE) and rework 
> Amit's patch to consider the PCD? I really like the idea of being true 
> to the UEFI spec.
> 
> In the edk2 tree we could rebase the affected drivers to the wrapper 
> function. And in OVMF (and in other in-tree emulation platforms), we 
> could set the FeaturePCD to TRUE.
> 
> It is possible to use OVMF with assigned physical PCI devices, but 
> people can easily rebuild OVMF themselves, with the FeaturePCD re-set 
> to FALSE. Users can also quickly report the exact cards / oproms that 
> break with the FeaturePCD set to TRUE.
> 
> 
> If you think it's simply not worth the effort, I'm OK with that, but 
> then we should specify this behavior in the UEFI spec -- we'll need a 
> Mantis bug for that. IMO it's not great that so many UEFI_DRIVERs in 
> the wild depend on behavior that is expressly forbidden by the UEFI 
> spec at the moment. If we can't modify all those drivers, then we 
> should adapt the spec, so that it reflects reality.
> 
> I'm not going to suggest specific language for the UEFI spec right now.
> But the wording could be based on the documentation of the wrapper 
> function that I'm working on now. I think I might post the patches 
> just for illustration.

OK, I'm giving up. It is too hard to track down every single crash -- the register dump written by UefiCpuPkg's exception handler to the serial port is not much help, even though it names a module to look at.

So I guess we have to accept that the dependency on EFI_ALREADY_STARTED setting Interface is just too wide-spread. :/ As I wrote above, I think this should be documented in the UEFI spec.

I filed the following mantis ticket:

  Permit OpenProtocol() to output the supported protocol interface even
  on error
  https://mantis.uefi.org/mantis/view.php?id=1815

Thanks
Laszlo

Re: [PATCH V4] MdeModulePkg/DxeCore: Fixed Interface returned by CoreOpenProtocol

[Zeng, Star]

Laszlo,

Summary the return status codes for OpenProtocol() as below.
EFI_SUCCESS
  Return the protocol interface in *Interface if Attributes is not EFI_OPEN_PROTOCOL_TEST_PROTOCOL.
EFI_INVALID_PARAMETER
  Do not update *Interface because one of the parameters has a value that does not allow this API to function correctly.
EFI_UNSUPPORTED
  Return NULL in *Interface because UserHandle does not support Protocol if Attributes is not EFI_OPEN_PROTOCOL_TEST_PROTOCOL.
EFI_ACCESS_DENIED
  Do not update *Interface because the protocol is already being used exclusively be someone else.  Returning the *Interface just makes it easy for the caller to use an interface they are not allowed to use.
EFI_ALREADY_STARTED
  Return the protocol interface in *Interface.  This allows bus drivers to use the interface they are already managing.

How about we clarify UEFI spec to only return the corresponding protocol interface in *Interface if the return status is EFI_SUCCESS or EFI_ALREADY_STARTED.
Then it preserves the exiting behavior for EFI_ALREADY_STARTED, but also updates OpenProtocol() to not modify Interface output parameter for all other error conditions except EFI_UNSUPPORTED (NULL will be returned in *Interface if Attributes is not EFI_OPEN_PROTOCOL_TEST_PROTOCOL).

UEFI Spec (Current language)
==========================
There are a number of reasons that this function call can return an error. If an error is returned, then AgentHandle, ControllerHandle, and Attributes are not added to the list of agents consuming the protocol interface specified by Handle and Protocol, and Interface is returned unmodified. The following is the list of conditions that must be checked before this function can return EFI_SUCCESS.

UEFI Spec (Proposed new language)
==========================
There are a number of reasons that this function call can return an error. If an error is returned, then AgentHandle, ControllerHandle, and Attributes are not added to the list of agents consuming the protocol interface specified by Handle and Protocol. Interface is returned unmodified for all error conditions except EFI_UNSUPPORTED and EFI_ALREADY_STARTED, NULL will be returned in *Interface when EFI_UNSUPPORTED and Attributes is not EFI_OPEN_PROTOCOL_TEST_PROTOCOL, the protocol interface will be returned in *Interface when EFI_ALREADY_STARTED. The following is the list of conditions that must be checked before this function can return EFI_SUCCESS.


Of course, the code still needs to be updated and the revised patch based on this V4 patch is attached.


Thanks,
Star

-----Original Message-----
From: Zeng, Star 
Sent: Tuesday, June 27, 2017 9:31 PM
To: Laszlo Ersek <lersek@redhat.com>; Amit Kumar <amit.ak@samsung.com>; edk2-devel@lists.01.org
Cc: Kinney, Michael D <michael.d.kinney@intel.com>; Tian, Feng <feng.tian@intel.com>; Gabriel L. Somlo (GMail) <gsomlo@gmail.com>; Gao, Liming <liming.gao@intel.com>; Fan, Jeff <jeff.fan@intel.com>; Zeng, Star <star.zeng@intel.com>
Subject: RE: [edk2] [PATCH V4] MdeModulePkg/DxeCore: Fixed Interface returned by CoreOpenProtocol

I also prefer to document it in UEFI spec personally.
And we are also having more discussion about it internally, nice to share more after that. :)


Thanks,
Star
-----Original Message-----
From: Laszlo Ersek [mailto:lersek@redhat.com] 
Sent: Tuesday, June 27, 2017 9:23 PM
To: Zeng, Star <star.zeng@intel.com>; Amit Kumar <amit.ak@samsung.com>; edk2-devel@lists.01.org
Cc: Kinney, Michael D <michael.d.kinney@intel.com>; Tian, Feng <feng.tian@intel.com>; Gabriel L. Somlo (GMail) <gsomlo@gmail.com>; Gao, Liming <liming.gao@intel.com>; Fan, Jeff <jeff.fan@intel.com>
Subject: Re: [edk2] [PATCH V4] MdeModulePkg/DxeCore: Fixed Interface returned by CoreOpenProtocol

On 06/27/17 13:15, Laszlo Ersek wrote:
> On 06/27/17 11:59, Zeng, Star wrote:
>> Laszlo,
>>
>> I got the point and I like the idea of wrapper function personally.
>> Although we can clean up the UEFI drivers in EDK2 tree, but it is really hard to evaluate the UEFI drivers in OPROM on add-on card.
>> The patch did not just break OVMF, but also broke all real platforms we have in hand (it's my fault that I did not catch the failure when reviewing patch), the patch is so risky.
> 
> Can we perhaps add a FeaturePCD (default value: FALSE) and rework 
> Amit's patch to consider the PCD? I really like the idea of being true 
> to the UEFI spec.
> 
> In the edk2 tree we could rebase the affected drivers to the wrapper 
> function. And in OVMF (and in other in-tree emulation platforms), we 
> could set the FeaturePCD to TRUE.
> 
> It is possible to use OVMF with assigned physical PCI devices, but 
> people can easily rebuild OVMF themselves, with the FeaturePCD re-set 
> to FALSE. Users can also quickly report the exact cards / oproms that 
> break with the FeaturePCD set to TRUE.
> 
> 
> If you think it's simply not worth the effort, I'm OK with that, but 
> then we should specify this behavior in the UEFI spec -- we'll need a 
> Mantis bug for that. IMO it's not great that so many UEFI_DRIVERs in 
> the wild depend on behavior that is expressly forbidden by the UEFI 
> spec at the moment. If we can't modify all those drivers, then we 
> should adapt the spec, so that it reflects reality.
> 
> I'm not going to suggest specific language for the UEFI spec right now.
> But the wording could be based on the documentation of the wrapper 
> function that I'm working on now. I think I might post the patches 
> just for illustration.

OK, I'm giving up. It is too hard to track down every single crash -- the register dump written by UefiCpuPkg's exception handler to the serial port is not much help, even though it names a module to look at.

So I guess we have to accept that the dependency on EFI_ALREADY_STARTED setting Interface is just too wide-spread. :/ As I wrote above, I think this should be documented in the UEFI spec.

I filed the following mantis ticket:

  Permit OpenProtocol() to output the supported protocol interface even
  on error
  https://mantis.uefi.org/mantis/view.php?id=1815

Thanks
Laszlo

Re: [PATCH V4] MdeModulePkg/DxeCore: Fixed Interface returned by CoreOpenProtocol

[Laszlo Ersek]

On 06/28/17 11:34, Zeng, Star wrote:
> Laszlo,
> 
> Summary the return status codes for OpenProtocol() as below.
> EFI_SUCCESS
>   Return the protocol interface in *Interface if Attributes is not EFI_OPEN_PROTOCOL_TEST_PROTOCOL.
> EFI_INVALID_PARAMETER
>   Do not update *Interface because one of the parameters has a value that does not allow this API to function correctly.
> EFI_UNSUPPORTED
>   Return NULL in *Interface because UserHandle does not support Protocol if Attributes is not EFI_OPEN_PROTOCOL_TEST_PROTOCOL.
> EFI_ACCESS_DENIED
>   Do not update *Interface because the protocol is already being used exclusively be someone else.  Returning the *Interface just makes it easy for the caller to use an interface they are not allowed to use.
> EFI_ALREADY_STARTED
>   Return the protocol interface in *Interface.  This allows bus drivers to use the interface they are already managing.
> 
> How about we clarify UEFI spec to only return the corresponding protocol interface in *Interface if the return status is EFI_SUCCESS or EFI_ALREADY_STARTED.
> Then it preserves the exiting behavior for EFI_ALREADY_STARTED, but also updates OpenProtocol() to not modify Interface output parameter for all other error conditions except EFI_UNSUPPORTED (NULL will be returned in *Interface if Attributes is not EFI_OPEN_PROTOCOL_TEST_PROTOCOL).
> 
> UEFI Spec (Current language)
> ==========================
> There are a number of reasons that this function call can return an error. If an error is returned, then AgentHandle, ControllerHandle, and Attributes are not added to the list of agents consuming the protocol interface specified by Handle and Protocol, and Interface is returned unmodified. The following is the list of conditions that must be checked before this function can return EFI_SUCCESS.
> 
> UEFI Spec (Proposed new language)
> ==========================
> There are a number of reasons that this function call can return an error. If an error is returned, then AgentHandle, ControllerHandle, and Attributes are not added to the list of agents consuming the protocol interface specified by Handle and Protocol. Interface is returned unmodified for all error conditions except EFI_UNSUPPORTED and EFI_ALREADY_STARTED, NULL will be returned in *Interface when EFI_UNSUPPORTED and Attributes is not EFI_OPEN_PROTOCOL_TEST_PROTOCOL, the protocol interface will be returned in *Interface when EFI_ALREADY_STARTED. The following is the list of conditions that must be checked before this function can return EFI_SUCCESS.

This looks great to me!

(EFI_ALREADY_STARTED is only possible for BY_DRIVER and
BY_DRIVER|EXCLUSIVE, so TEST_PROTOCOL need not be considered for
EFI_ALREADY_STARTED. Good!)

Can you please add this language to
<https://mantis.uefi.org/mantis/view.php?id=1815>, in a new note?

> Of course, the code still needs to be updated and the revised patch based on this V4 patch is attached.

Please post the patch to the list and let the respective owners /
maintainers review it :) If it matches the above specification, then it
should be good.

Thanks!
Laszlo

Re: [PATCH V4] MdeModulePkg/DxeCore: Fixed Interface returned by CoreOpenProtocol

[Zeng, Star]

Just added note in https://mantis.uefi.org/mantis/view.php?id=1815, and I will post the patch soon.

Thanks,
Star
-----Original Message-----
From: Laszlo Ersek [mailto:lersek@redhat.com] 
Sent: Wednesday, June 28, 2017 8:39 PM
To: Zeng, Star <star.zeng@intel.com>; Amit Kumar <amit.ak@samsung.com>; edk2-devel@lists.01.org
Cc: Kinney, Michael D <michael.d.kinney@intel.com>; Tian, Feng <feng.tian@intel.com>; Gabriel L. Somlo (GMail) <gsomlo@gmail.com>; Gao, Liming <liming.gao@intel.com>; Fan, Jeff <jeff.fan@intel.com>
Subject: Re: [edk2] [PATCH V4] MdeModulePkg/DxeCore: Fixed Interface returned by CoreOpenProtocol

On 06/28/17 11:34, Zeng, Star wrote:
> Laszlo,
> 
> Summary the return status codes for OpenProtocol() as below.
> EFI_SUCCESS
>   Return the protocol interface in *Interface if Attributes is not EFI_OPEN_PROTOCOL_TEST_PROTOCOL.
> EFI_INVALID_PARAMETER
>   Do not update *Interface because one of the parameters has a value that does not allow this API to function correctly.
> EFI_UNSUPPORTED
>   Return NULL in *Interface because UserHandle does not support Protocol if Attributes is not EFI_OPEN_PROTOCOL_TEST_PROTOCOL.
> EFI_ACCESS_DENIED
>   Do not update *Interface because the protocol is already being used exclusively be someone else.  Returning the *Interface just makes it easy for the caller to use an interface they are not allowed to use.
> EFI_ALREADY_STARTED
>   Return the protocol interface in *Interface.  This allows bus drivers to use the interface they are already managing.
> 
> How about we clarify UEFI spec to only return the corresponding protocol interface in *Interface if the return status is EFI_SUCCESS or EFI_ALREADY_STARTED.
> Then it preserves the exiting behavior for EFI_ALREADY_STARTED, but also updates OpenProtocol() to not modify Interface output parameter for all other error conditions except EFI_UNSUPPORTED (NULL will be returned in *Interface if Attributes is not EFI_OPEN_PROTOCOL_TEST_PROTOCOL).
> 
> UEFI Spec (Current language)
> ==========================
> There are a number of reasons that this function call can return an error. If an error is returned, then AgentHandle, ControllerHandle, and Attributes are not added to the list of agents consuming the protocol interface specified by Handle and Protocol, and Interface is returned unmodified. The following is the list of conditions that must be checked before this function can return EFI_SUCCESS.
> 
> UEFI Spec (Proposed new language)
> ==========================
> There are a number of reasons that this function call can return an error. If an error is returned, then AgentHandle, ControllerHandle, and Attributes are not added to the list of agents consuming the protocol interface specified by Handle and Protocol. Interface is returned unmodified for all error conditions except EFI_UNSUPPORTED and EFI_ALREADY_STARTED, NULL will be returned in *Interface when EFI_UNSUPPORTED and Attributes is not EFI_OPEN_PROTOCOL_TEST_PROTOCOL, the protocol interface will be returned in *Interface when EFI_ALREADY_STARTED. The following is the list of conditions that must be checked before this function can return EFI_SUCCESS.

This looks great to me!

(EFI_ALREADY_STARTED is only possible for BY_DRIVER and BY_DRIVER|EXCLUSIVE, so TEST_PROTOCOL need not be considered for EFI_ALREADY_STARTED. Good!)

Can you please add this language to
<https://mantis.uefi.org/mantis/view.php?id=1815>, in a new note?

> Of course, the code still needs to be updated and the revised patch based on this V4 patch is attached.

Please post the patch to the list and let the respective owners / maintainers review it :) If it matches the above specification, then it should be good.

Thanks!
Laszlo