[PATCH edk2-platforms v2] Silicon/ChaosKeyDxe: don't rely on connect all controllers

[PATCH edk2-platforms v2] Silicon/ChaosKeyDxe: don't rely on connect all controllers

[Ard Biesheuvel]

From: Ard Biesheuvel <ard.biesheuvel@arm.com>

The ChaosKey driver implements the UEFI driver model, and so it is
not guaranteed that any controllers will be attached to this driver
unless it is connected explicitly. On many platforms today, this is
taken care of by the ConnectAll() call that occurs in the BDS, but
this is not something we should rely on.

So add a protocol notification event that will attempt to connect
the ChaosKey on each USB I/O protocol instance that carries the
expected vendor and product IDs. This still relies on the USB host
controllers to be connected by the platform, which is typically the
case (given that a USB keyboard is required to interrupt the boot)

On platforms where USB is not connected at all by default, it is really
not up to a third party driver to meddle with this, and so relying on
the USB host controllers to have been connected non-reursively is the
best we can do. Note that third party drivers registered via Driver####
can set a 'reconnect all' flag if needed to mitigate this.

Signed-off-by: Ard Biesheuvel <ard.biesheuvel@arm.com>
---

Unfortunately, the previous approach of connecting a short-form USB device
path containing the ChaosKey's VID/PID turned out not to be reliable in
practice (it doesn't work at all on ThunderX2 with AMI firmware)

So instead, we have to rely on USB I/O protocols having been instantiated
by the DXE core. This is what typically happens, given that USB keyboards
are also exposed via USB I/O protocol instances. The only difference with
a non-fast [ConnectAll()] boot is that those USB I/O protocol instances
are not connected further automatically, but it is reasonable to expect
that the handles themselves have been instantiated.

Platforms that do not produce those USB I/O handles would not be able to
offer the ability to interrupt the boot or enter the menu using a USB
keyboard, so this is rather rare in practice. Also, if such platforms do
exist, it is not up to this 3rd party driver to override this policy and
enumerate the entire USB hierarchy.

So this means we need to call UsbHwrngDriverBindingSupported() directly
to check whether the USB I/O protocol in question has the right VID/PID.
If that succeeds, there is really no point in using ConnectController(),
so just call UsbHwrngDriverBindingStart() directly to connect the driver
to the controller.

Tested on ThunderX2 using a Driver0000 option.

 Silicon/Openmoko/ChaosKeyDxe/DriverBinding.c | 66 ++++++++++++++++++++
 1 file changed, 66 insertions(+)

diff --git a/Silicon/Openmoko/ChaosKeyDxe/DriverBinding.c b/Silicon/Openmoko/ChaosKeyDxe/DriverBinding.c
index e7d0d3fe563e..611803c6c339 100644
--- a/Silicon/Openmoko/ChaosKeyDxe/DriverBinding.c
+++ b/Silicon/Openmoko/ChaosKeyDxe/DriverBinding.c
@@ -11,6 +11,9 @@
 
 #include "ChaosKeyDriver.h"
 
+STATIC VOID         *mProtocolNotifyRegistration;
+STATIC EFI_EVENT    mProtocolNotifyRegistrationEvent;
+
 /**
   Tests to see if this driver supports a given controller.
 
@@ -157,6 +160,55 @@ EFI_DRIVER_BINDING_PROTOCOL  gUsbDriverBinding = {
 };
 
 
+STATIC
+VOID
+EFIAPI
+UsbHwrngOnProtocolNotify (
+  IN EFI_EVENT            Event,
+  IN VOID                 *Context
+  )
+{
+  EFI_STATUS              Status;
+  EFI_HANDLE              *Handles;
+  UINTN                   HandleCount;
+  UINTN                   Index;
+
+  do {
+    Status = gBS->LocateHandleBuffer (ByRegisterNotify, NULL,
+                    mProtocolNotifyRegistration, &HandleCount, &Handles);
+    if (EFI_ERROR (Status)) {
+      if (Status != EFI_NOT_FOUND) {
+        DEBUG ((DEBUG_WARN, "%a: LocateHandleBuffer() failed - %r\n",
+          __FUNCTION__, Status));
+        }
+      return;
+    }
+
+    if (HandleCount == 0) {
+      return;
+    }
+
+    for (Index = 0; Index < HandleCount; Index++) {
+      Status = UsbHwrngDriverBindingSupported (&gUsbDriverBinding,
+                 Handles[Index], NULL);
+      if (EFI_ERROR (Status)) {
+        continue;
+      }
+
+      //
+      // Attempt to connect the USB device path describing the ChaosKey
+      // hardware via the handle describing a USB host controller.
+      //
+      Status = UsbHwrngDriverBindingStart (&gUsbDriverBinding,
+                 Handles[Index], NULL);
+      DEBUG ((DEBUG_VERBOSE, "%a: UsbHwrngDriverBindingStart () returned %r\n",
+        __FUNCTION__, Status));
+    }
+    gBS->FreePool (Handles);
+  } while (1);
+}
+
+
 /**
   The entry point of ChaosKey UEFI Driver.
 
@@ -185,6 +237,18 @@ EntryPoint (
              NULL, &gChaosKeyDriverComponentName2);
   ASSERT_EFI_ERROR (Status);
 
+  //
+  // This driver produces the EFI Random Number Generator protocol on
+  // compatible USB I/O handles, which is not a protocol that can provide
+  // a boot target. This means that it will not get connected on an ordinary
+  // 'fast' boot (which only connects the console and boot entry device paths)
+  // unless we take extra measures.
+  //
+  mProtocolNotifyRegistrationEvent = EfiCreateProtocolNotifyEvent (
+                                       &gEfiUsbIoProtocolGuid, TPL_CALLBACK,
+                                       UsbHwrngOnProtocolNotify, NULL,
+                                       &mProtocolNotifyRegistration);
+
   DEBUG ((DEBUG_INIT | DEBUG_INFO, "*** Installed ChaosKey driver! ***\n"));
 
   return EFI_SUCCESS;
@@ -211,6 +275,8 @@ UnloadImage (
   UINTN       HandleCount;
   UINTN       Index;
 
+  gBS->CloseEvent (mProtocolNotifyRegistrationEvent);
+
   //
   // Retrieve all USB I/O handles in the handle database
   //
-- 
2.20.1

Re: [PATCH edk2-platforms v2] Silicon/ChaosKeyDxe: don't rely on connect all controllers

[Leif Lindholm]

On Tue, Jun 02, 2020 at 15:38:49 +0200, Ard Biesheuvel wrote:
> From: Ard Biesheuvel <ard.biesheuvel@arm.com>
> 
> The ChaosKey driver implements the UEFI driver model, and so it is
> not guaranteed that any controllers will be attached to this driver
> unless it is connected explicitly. On many platforms today, this is
> taken care of by the ConnectAll() call that occurs in the BDS, but
> this is not something we should rely on.
> 
> So add a protocol notification event that will attempt to connect
> the ChaosKey on each USB I/O protocol instance that carries the
> expected vendor and product IDs. This still relies on the USB host
> controllers to be connected by the platform, which is typically the
> case (given that a USB keyboard is required to interrupt the boot)
> 
> On platforms where USB is not connected at all by default, it is really
> not up to a third party driver to meddle with this, and so relying on
> the USB host controllers to have been connected non-reursively is the
> best we can do. Note that third party drivers registered via Driver####
> can set a 'reconnect all' flag if needed to mitigate this.
> 
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@arm.com>
> ---
> 
> Unfortunately, the previous approach of connecting a short-form USB device
> path containing the ChaosKey's VID/PID turned out not to be reliable in
> practice (it doesn't work at all on ThunderX2 with AMI firmware)
> 
> So instead, we have to rely on USB I/O protocols having been instantiated
> by the DXE core. This is what typically happens, given that USB keyboards
> are also exposed via USB I/O protocol instances. The only difference with
> a non-fast [ConnectAll()] boot is that those USB I/O protocol instances
> are not connected further automatically, but it is reasonable to expect
> that the handles themselves have been instantiated.
> 
> Platforms that do not produce those USB I/O handles would not be able to
> offer the ability to interrupt the boot or enter the menu using a USB
> keyboard, so this is rather rare in practice. Also, if such platforms do
> exist, it is not up to this 3rd party driver to override this policy and
> enumerate the entire USB hierarchy.

I agree in theory.

But what happens when someone explicitly sets up their ChaosKey with a
standalone driver, verifies it working, and at some later date enable
"quick boot" in a BIOS menu?

(I recall some "fun" stories from Peter Jones in this area,
specifically wrt the ability to interrupt boot from a keyboard.)

I'm not saying it should up to a 3rd party driver to override this
policy and enumerate the entire USB hierarchy, but I'm suggesting that
especially for something like ChaosKey, silent failure could be a real
problem.

Does this workaround prevent that?

/
    Leif

> to check whether the USB I/O protocol in question has the right VID/PID.
> If that succeeds, there is really no point in using ConnectController(),
> so just call UsbHwrngDriverBindingStart() directly to connect the driver
> to the controller.
> 
> Tested on ThunderX2 using a Driver0000 option.

>  Silicon/Openmoko/ChaosKeyDxe/DriverBinding.c | 66 ++++++++++++++++++++
>  1 file changed, 66 insertions(+)
> 
> diff --git a/Silicon/Openmoko/ChaosKeyDxe/DriverBinding.c b/Silicon/Openmoko/ChaosKeyDxe/DriverBinding.c
> index e7d0d3fe563e..611803c6c339 100644
> --- a/Silicon/Openmoko/ChaosKeyDxe/DriverBinding.c
> +++ b/Silicon/Openmoko/ChaosKeyDxe/DriverBinding.c
> @@ -11,6 +11,9 @@
>  
>  #include "ChaosKeyDriver.h"
>  
> +STATIC VOID         *mProtocolNotifyRegistration;
> +STATIC EFI_EVENT    mProtocolNotifyRegistrationEvent;
> +
>  /**
>    Tests to see if this driver supports a given controller.
>  
> @@ -157,6 +160,55 @@ EFI_DRIVER_BINDING_PROTOCOL  gUsbDriverBinding = {
>  };
>  
>  
> +STATIC
> +VOID
> +EFIAPI
> +UsbHwrngOnProtocolNotify (
> +  IN EFI_EVENT            Event,
> +  IN VOID                 *Context
> +  )
> +{
> +  EFI_STATUS              Status;
> +  EFI_HANDLE              *Handles;
> +  UINTN                   HandleCount;
> +  UINTN                   Index;
> +
> +  do {
> +    Status = gBS->LocateHandleBuffer (ByRegisterNotify, NULL,
> +                    mProtocolNotifyRegistration, &HandleCount, &Handles);
> +    if (EFI_ERROR (Status)) {
> +      if (Status != EFI_NOT_FOUND) {
> +        DEBUG ((DEBUG_WARN, "%a: LocateHandleBuffer() failed - %r\n",
> +          __FUNCTION__, Status));
> +        }
> +      return;
> +    }
> +
> +    if (HandleCount == 0) {
> +      return;
> +    }
> +
> +    for (Index = 0; Index < HandleCount; Index++) {
> +      Status = UsbHwrngDriverBindingSupported (&gUsbDriverBinding,
> +                 Handles[Index], NULL);
> +      if (EFI_ERROR (Status)) {
> +        continue;
> +      }
> +
> +      //
> +      // Attempt to connect the USB device path describing the ChaosKey
> +      // hardware via the handle describing a USB host controller.
> +      //
> +      Status = UsbHwrngDriverBindingStart (&gUsbDriverBinding,
> +                 Handles[Index], NULL);
> +      DEBUG ((DEBUG_VERBOSE, "%a: UsbHwrngDriverBindingStart () returned %r\n",
> +        __FUNCTION__, Status));
> +    }
> +    gBS->FreePool (Handles);
> +  } while (1);
> +}
> +
> +
>  /**
>    The entry point of ChaosKey UEFI Driver.
>  
> @@ -185,6 +237,18 @@ EntryPoint (
>               NULL, &gChaosKeyDriverComponentName2);
>    ASSERT_EFI_ERROR (Status);
>  
> +  //
> +  // This driver produces the EFI Random Number Generator protocol on
> +  // compatible USB I/O handles, which is not a protocol that can provide
> +  // a boot target. This means that it will not get connected on an ordinary
> +  // 'fast' boot (which only connects the console and boot entry device paths)
> +  // unless we take extra measures.
> +  //
> +  mProtocolNotifyRegistrationEvent = EfiCreateProtocolNotifyEvent (
> +                                       &gEfiUsbIoProtocolGuid, TPL_CALLBACK,
> +                                       UsbHwrngOnProtocolNotify, NULL,
> +                                       &mProtocolNotifyRegistration);
> +
>    DEBUG ((DEBUG_INIT | DEBUG_INFO, "*** Installed ChaosKey driver! ***\n"));
>  
>    return EFI_SUCCESS;
> @@ -211,6 +275,8 @@ UnloadImage (
>    UINTN       HandleCount;
>    UINTN       Index;
>  
> +  gBS->CloseEvent (mProtocolNotifyRegistrationEvent);
> +
>    //
>    // Retrieve all USB I/O handles in the handle database
>    //
> -- 
> 2.20.1
> 

Re: [PATCH edk2-platforms v2] Silicon/ChaosKeyDxe: don't rely on connect all controllers

[Ard Biesheuvel]

On 6/2/20 4:49 PM, Leif Lindholm wrote:
> On Tue, Jun 02, 2020 at 15:38:49 +0200, Ard Biesheuvel wrote:
>> From: Ard Biesheuvel <ard.biesheuvel@arm.com>
>>
>> The ChaosKey driver implements the UEFI driver model, and so it is
>> not guaranteed that any controllers will be attached to this driver
>> unless it is connected explicitly. On many platforms today, this is
>> taken care of by the ConnectAll() call that occurs in the BDS, but
>> this is not something we should rely on.
>>
>> So add a protocol notification event that will attempt to connect
>> the ChaosKey on each USB I/O protocol instance that carries the
>> expected vendor and product IDs. This still relies on the USB host
>> controllers to be connected by the platform, which is typically the
>> case (given that a USB keyboard is required to interrupt the boot)
>>
>> On platforms where USB is not connected at all by default, it is really
>> not up to a third party driver to meddle with this, and so relying on
>> the USB host controllers to have been connected non-reursively is the
>> best we can do. Note that third party drivers registered via Driver####
>> can set a 'reconnect all' flag if needed to mitigate this.
>>
>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@arm.com>
>> ---
>>
>> Unfortunately, the previous approach of connecting a short-form USB device
>> path containing the ChaosKey's VID/PID turned out not to be reliable in
>> practice (it doesn't work at all on ThunderX2 with AMI firmware)
>>
>> So instead, we have to rely on USB I/O protocols having been instantiated
>> by the DXE core. This is what typically happens, given that USB keyboards
>> are also exposed via USB I/O protocol instances. The only difference with
>> a non-fast [ConnectAll()] boot is that those USB I/O protocol instances
>> are not connected further automatically, but it is reasonable to expect
>> that the handles themselves have been instantiated.
>>
>> Platforms that do not produce those USB I/O handles would not be able to
>> offer the ability to interrupt the boot or enter the menu using a USB
>> keyboard, so this is rather rare in practice. Also, if such platforms do
>> exist, it is not up to this 3rd party driver to override this policy and
>> enumerate the entire USB hierarchy.
> 
> I agree in theory.
> 
> But what happens when someone explicitly sets up their ChaosKey with a
> standalone driver, verifies it working, and at some later date enable
> "quick boot" in a BIOS menu?
> 
> (I recall some "fun" stories from Peter Jones in this area,
> specifically wrt the ability to interrupt boot from a keyboard.)
> 
> I'm not saying it should up to a 3rd party driver to override this
> policy and enumerate the entire USB hierarchy, but I'm suggesting that
> especially for something like ChaosKey, silent failure could be a real
> problem.
> 
> Does this workaround prevent that?
> 

No. The driver does not know whether the controller is there or not 
unless the platform enumerates the USB hierarchy, at least 
non-recursively. If the USB I/O handles are not produced, this driver 
will not find the controller.

If on such a platform, you are using Driver#### to load the driver, you 
can set the reconnect-all flag to work around this.

If the platform firmware incorporates this driver, it needs to take this 
limitation into account, and ensure that the USB I/O handles are 
produced in one way or the other (which it will typically already do 
while looking for the USB keyboard)

Adding code to this driver to allow it to infer from its execution 
context which of these situations it finds itself in seems intractible 
to me, especially given my recent experiences with AMI firmware, which 
does not quite behave like EDK2 upstream does in this regard.



>> to check whether the USB I/O protocol in question has the right VID/PID.
>> If that succeeds, there is really no point in using ConnectController(),
>> so just call UsbHwrngDriverBindingStart() directly to connect the driver
>> to the controller.
>>
>> Tested on ThunderX2 using a Driver0000 option.
> 
>>   Silicon/Openmoko/ChaosKeyDxe/DriverBinding.c | 66 ++++++++++++++++++++
>>   1 file changed, 66 insertions(+)
>>
>> diff --git a/Silicon/Openmoko/ChaosKeyDxe/DriverBinding.c b/Silicon/Openmoko/ChaosKeyDxe/DriverBinding.c
>> index e7d0d3fe563e..611803c6c339 100644
>> --- a/Silicon/Openmoko/ChaosKeyDxe/DriverBinding.c
>> +++ b/Silicon/Openmoko/ChaosKeyDxe/DriverBinding.c
>> @@ -11,6 +11,9 @@
>>   
>>   #include "ChaosKeyDriver.h"
>>   
>> +STATIC VOID         *mProtocolNotifyRegistration;
>> +STATIC EFI_EVENT    mProtocolNotifyRegistrationEvent;
>> +
>>   /**
>>     Tests to see if this driver supports a given controller.
>>   
>> @@ -157,6 +160,55 @@ EFI_DRIVER_BINDING_PROTOCOL  gUsbDriverBinding = {
>>   };
>>   
>>   
>> +STATIC
>> +VOID
>> +EFIAPI
>> +UsbHwrngOnProtocolNotify (
>> +  IN EFI_EVENT            Event,
>> +  IN VOID                 *Context
>> +  )
>> +{
>> +  EFI_STATUS              Status;
>> +  EFI_HANDLE              *Handles;
>> +  UINTN                   HandleCount;
>> +  UINTN                   Index;
>> +
>> +  do {
>> +    Status = gBS->LocateHandleBuffer (ByRegisterNotify, NULL,
>> +                    mProtocolNotifyRegistration, &HandleCount, &Handles);
>> +    if (EFI_ERROR (Status)) {
>> +      if (Status != EFI_NOT_FOUND) {
>> +        DEBUG ((DEBUG_WARN, "%a: LocateHandleBuffer() failed - %r\n",
>> +          __FUNCTION__, Status));
>> +        }
>> +      return;
>> +    }
>> +
>> +    if (HandleCount == 0) {
>> +      return;
>> +    }
>> +
>> +    for (Index = 0; Index < HandleCount; Index++) {
>> +      Status = UsbHwrngDriverBindingSupported (&gUsbDriverBinding,
>> +                 Handles[Index], NULL);
>> +      if (EFI_ERROR (Status)) {
>> +        continue;
>> +      }
>> +
>> +      //
>> +      // Attempt to connect the USB device path describing the ChaosKey
>> +      // hardware via the handle describing a USB host controller.
>> +      //
>> +      Status = UsbHwrngDriverBindingStart (&gUsbDriverBinding,
>> +                 Handles[Index], NULL);
>> +      DEBUG ((DEBUG_VERBOSE, "%a: UsbHwrngDriverBindingStart () returned %r\n",
>> +        __FUNCTION__, Status));
>> +    }
>> +    gBS->FreePool (Handles);
>> +  } while (1);
>> +}
>> +
>> +
>>   /**
>>     The entry point of ChaosKey UEFI Driver.
>>   
>> @@ -185,6 +237,18 @@ EntryPoint (
>>                NULL, &gChaosKeyDriverComponentName2);
>>     ASSERT_EFI_ERROR (Status);
>>   
>> +  //
>> +  // This driver produces the EFI Random Number Generator protocol on
>> +  // compatible USB I/O handles, which is not a protocol that can provide
>> +  // a boot target. This means that it will not get connected on an ordinary
>> +  // 'fast' boot (which only connects the console and boot entry device paths)
>> +  // unless we take extra measures.
>> +  //
>> +  mProtocolNotifyRegistrationEvent = EfiCreateProtocolNotifyEvent (
>> +                                       &gEfiUsbIoProtocolGuid, TPL_CALLBACK,
>> +                                       UsbHwrngOnProtocolNotify, NULL,
>> +                                       &mProtocolNotifyRegistration);
>> +
>>     DEBUG ((DEBUG_INIT | DEBUG_INFO, "*** Installed ChaosKey driver! ***\n"));
>>   
>>     return EFI_SUCCESS;
>> @@ -211,6 +275,8 @@ UnloadImage (
>>     UINTN       HandleCount;
>>     UINTN       Index;
>>   
>> +  gBS->CloseEvent (mProtocolNotifyRegistrationEvent);
>> +
>>     //
>>     // Retrieve all USB I/O handles in the handle database
>>     //
>> -- 
>> 2.20.1
>>

Re: [PATCH edk2-platforms v2] Silicon/ChaosKeyDxe: don't rely on connect all controllers

[Leif Lindholm]

On Tue, Jun 02, 2020 at 16:58:54 +0200, Ard Biesheuvel wrote:
> 
> 
> On 6/2/20 4:49 PM, Leif Lindholm wrote:
> > On Tue, Jun 02, 2020 at 15:38:49 +0200, Ard Biesheuvel wrote:
> > > From: Ard Biesheuvel <ard.biesheuvel@arm.com>
> > > 
> > > The ChaosKey driver implements the UEFI driver model, and so it is
> > > not guaranteed that any controllers will be attached to this driver
> > > unless it is connected explicitly. On many platforms today, this is
> > > taken care of by the ConnectAll() call that occurs in the BDS, but
> > > this is not something we should rely on.
> > > 
> > > So add a protocol notification event that will attempt to connect
> > > the ChaosKey on each USB I/O protocol instance that carries the
> > > expected vendor and product IDs. This still relies on the USB host
> > > controllers to be connected by the platform, which is typically the
> > > case (given that a USB keyboard is required to interrupt the boot)
> > > 
> > > On platforms where USB is not connected at all by default, it is really
> > > not up to a third party driver to meddle with this, and so relying on
> > > the USB host controllers to have been connected non-reursively is the
> > > best we can do. Note that third party drivers registered via Driver####
> > > can set a 'reconnect all' flag if needed to mitigate this.
> > > 
> > > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@arm.com>
> > > ---
> > > 
> > > Unfortunately, the previous approach of connecting a short-form USB device
> > > path containing the ChaosKey's VID/PID turned out not to be reliable in
> > > practice (it doesn't work at all on ThunderX2 with AMI firmware)
> > > 
> > > So instead, we have to rely on USB I/O protocols having been instantiated
> > > by the DXE core. This is what typically happens, given that USB keyboards
> > > are also exposed via USB I/O protocol instances. The only difference with
> > > a non-fast [ConnectAll()] boot is that those USB I/O protocol instances
> > > are not connected further automatically, but it is reasonable to expect
> > > that the handles themselves have been instantiated.
> > > 
> > > Platforms that do not produce those USB I/O handles would not be able to
> > > offer the ability to interrupt the boot or enter the menu using a USB
> > > keyboard, so this is rather rare in practice. Also, if such platforms do
> > > exist, it is not up to this 3rd party driver to override this policy and
> > > enumerate the entire USB hierarchy.
> > 
> > I agree in theory.
> > 
> > But what happens when someone explicitly sets up their ChaosKey with a
> > standalone driver, verifies it working, and at some later date enable
> > "quick boot" in a BIOS menu?
> > 
> > (I recall some "fun" stories from Peter Jones in this area,
> > specifically wrt the ability to interrupt boot from a keyboard.)
> > 
> > I'm not saying it should up to a 3rd party driver to override this
> > policy and enumerate the entire USB hierarchy, but I'm suggesting that
> > especially for something like ChaosKey, silent failure could be a real
> > problem.
> > 
> > Does this workaround prevent that?
> > 
> 
> No. The driver does not know whether the controller is there or not unless
> the platform enumerates the USB hierarchy, at least non-recursively. If the
> USB I/O handles are not produced, this driver will not find the controller.
> 
> If on such a platform, you are using Driver#### to load the driver, you can
> set the reconnect-all flag to work around this.

Right. OK, I agree that's fair enough.
It is a shame though - the support for --reconnect was only added to
efibootmgr on 11 October last year, so isn't currently available in
most distros.

> If the platform firmware incorporates this driver, it needs to take this
> limitation into account, and ensure that the USB I/O handles are produced in
> one way or the other (which it will typically already do while looking for
> the USB keyboard)

Agreed.

> Adding code to this driver to allow it to infer from its execution context
> which of these situations it finds itself in seems intractible to me,
> especially given my recent experiences with AMI firmware, which does not
> quite behave like EDK2 upstream does in this regard.

Yeah. OK, taken all of these points into account:
Reviewed-by: Leif Lindholm <leif@nuviainc.com>

/
    Leif

> > > to check whether the USB I/O protocol in question has the right VID/PID.
> > > If that succeeds, there is really no point in using ConnectController(),
> > > so just call UsbHwrngDriverBindingStart() directly to connect the driver
> > > to the controller.
> > > 
> > > Tested on ThunderX2 using a Driver0000 option.
> > 
> > >   Silicon/Openmoko/ChaosKeyDxe/DriverBinding.c | 66 ++++++++++++++++++++
> > >   1 file changed, 66 insertions(+)
> > > 
> > > diff --git a/Silicon/Openmoko/ChaosKeyDxe/DriverBinding.c b/Silicon/Openmoko/ChaosKeyDxe/DriverBinding.c
> > > index e7d0d3fe563e..611803c6c339 100644
> > > --- a/Silicon/Openmoko/ChaosKeyDxe/DriverBinding.c
> > > +++ b/Silicon/Openmoko/ChaosKeyDxe/DriverBinding.c
> > > @@ -11,6 +11,9 @@
> > >   #include "ChaosKeyDriver.h"
> > > +STATIC VOID         *mProtocolNotifyRegistration;
> > > +STATIC EFI_EVENT    mProtocolNotifyRegistrationEvent;
> > > +
> > >   /**
> > >     Tests to see if this driver supports a given controller.
> > > @@ -157,6 +160,55 @@ EFI_DRIVER_BINDING_PROTOCOL  gUsbDriverBinding = {
> > >   };
> > > +STATIC
> > > +VOID
> > > +EFIAPI
> > > +UsbHwrngOnProtocolNotify (
> > > +  IN EFI_EVENT            Event,
> > > +  IN VOID                 *Context
> > > +  )
> > > +{
> > > +  EFI_STATUS              Status;
> > > +  EFI_HANDLE              *Handles;
> > > +  UINTN                   HandleCount;
> > > +  UINTN                   Index;
> > > +
> > > +  do {
> > > +    Status = gBS->LocateHandleBuffer (ByRegisterNotify, NULL,
> > > +                    mProtocolNotifyRegistration, &HandleCount, &Handles);
> > > +    if (EFI_ERROR (Status)) {
> > > +      if (Status != EFI_NOT_FOUND) {
> > > +        DEBUG ((DEBUG_WARN, "%a: LocateHandleBuffer() failed - %r\n",
> > > +          __FUNCTION__, Status));
> > > +        }
> > > +      return;
> > > +    }
> > > +
> > > +    if (HandleCount == 0) {
> > > +      return;
> > > +    }
> > > +
> > > +    for (Index = 0; Index < HandleCount; Index++) {
> > > +      Status = UsbHwrngDriverBindingSupported (&gUsbDriverBinding,
> > > +                 Handles[Index], NULL);
> > > +      if (EFI_ERROR (Status)) {
> > > +        continue;
> > > +      }
> > > +
> > > +      //
> > > +      // Attempt to connect the USB device path describing the ChaosKey
> > > +      // hardware via the handle describing a USB host controller.
> > > +      //
> > > +      Status = UsbHwrngDriverBindingStart (&gUsbDriverBinding,
> > > +                 Handles[Index], NULL);
> > > +      DEBUG ((DEBUG_VERBOSE, "%a: UsbHwrngDriverBindingStart () returned %r\n",
> > > +        __FUNCTION__, Status));
> > > +    }
> > > +    gBS->FreePool (Handles);
> > > +  } while (1);
> > > +}
> > > +
> > > +
> > >   /**
> > >     The entry point of ChaosKey UEFI Driver.
> > > @@ -185,6 +237,18 @@ EntryPoint (
> > >                NULL, &gChaosKeyDriverComponentName2);
> > >     ASSERT_EFI_ERROR (Status);
> > > +  //
> > > +  // This driver produces the EFI Random Number Generator protocol on
> > > +  // compatible USB I/O handles, which is not a protocol that can provide
> > > +  // a boot target. This means that it will not get connected on an ordinary
> > > +  // 'fast' boot (which only connects the console and boot entry device paths)
> > > +  // unless we take extra measures.
> > > +  //
> > > +  mProtocolNotifyRegistrationEvent = EfiCreateProtocolNotifyEvent (
> > > +                                       &gEfiUsbIoProtocolGuid, TPL_CALLBACK,
> > > +                                       UsbHwrngOnProtocolNotify, NULL,
> > > +                                       &mProtocolNotifyRegistration);
> > > +
> > >     DEBUG ((DEBUG_INIT | DEBUG_INFO, "*** Installed ChaosKey driver! ***\n"));
> > >     return EFI_SUCCESS;
> > > @@ -211,6 +275,8 @@ UnloadImage (
> > >     UINTN       HandleCount;
> > >     UINTN       Index;
> > > +  gBS->CloseEvent (mProtocolNotifyRegistrationEvent);
> > > +
> > >     //
> > >     // Retrieve all USB I/O handles in the handle database
> > >     //
> > > -- 
> > > 2.20.1
> > > 

Re: [edk2-devel] [PATCH edk2-platforms v2] Silicon/ChaosKeyDxe: don't rely on connect all controllers

[Andrew Fish]

> On Jun 2, 2020, at 6:38 AM, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
> 
> From: Ard Biesheuvel <ard.biesheuvel@arm.com>
> 
> The ChaosKey driver implements the UEFI driver model, and so it is
> not guaranteed that any controllers will be attached to this driver
> unless it is connected explicitly. On many platforms today, this is
> taken care of by the ConnectAll() call that occurs in the BDS, but
> this is not something we should rely on.
> 
> So add a protocol notification event that will attempt to connect
> the ChaosKey on each USB I/O protocol instance that carries the
> expected vendor and product IDs. This still relies on the USB host
> controllers to be connected by the platform, which is typically the
> case (given that a USB keyboard is required to interrupt the boot)
> 
> On platforms where USB is not connected at all by default, it is really
> not up to a third party driver to meddle with this, and so relying on
> the USB host controllers to have been connected non-reursively is the
> best we can do. Note that third party drivers registered via Driver####
> can set a 'reconnect all' flag if needed to mitigate this.
> 
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@arm.com>
> ---
> 
> Unfortunately, the previous approach of connecting a short-form USB device
> path containing the ChaosKey's VID/PID turned out not to be reliable in
> practice (it doesn't work at all on ThunderX2 with AMI firmware)
> 
> So instead, we have to rely on USB I/O protocols having been instantiated
> by the DXE core. This is what typically happens, given that USB keyboards
> are also exposed via USB I/O protocol instances. The only difference with
> a non-fast [ConnectAll()] boot is that those USB I/O protocol instances
> are not connected further automatically, but it is reasonable to expect
> that the handles themselves have been instantiated.
> 
> Platforms that do not produce those USB I/O handles would not be able to
> offer the ability to interrupt the boot or enter the menu using a USB
> keyboard, so this is rather rare in practice. Also, if such platforms do
> exist, it is not up to this 3rd party driver to override this policy and
> enumerate the entire USB hierarchy.
> 
> So this means we need to call UsbHwrngDriverBindingSupported() directly
> to check whether the USB I/O protocol in question has the right VID/PID.
> If that succeeds, there is really no point in using ConnectController(),
> so just call UsbHwrngDriverBindingStart() directly to connect the driver
> to the controller.
> 

Actually there is a point in using gBS->ConnectController(). 
1) You make it impossible for the platform to override the drivers choice. The philosophy of EFI has alway been that the drivers should not cary policy and the platforms should cary policy. This inverts that. 
2) I’m not sure it is well defined behavior by UEFI to NOT call gBS->ConnectController(). I took a quick look at the DXE Core and the gBS->ConnectController() is not doing any book keeping, other than managing the precedence rules, but it seems like it would be legal to add something in the future. + Mike in case I’m off on this one. 

Also It would like be a good idea to have a PCD to turn on/off this feature. 

Thanks,

Andrew Fish

> Tested on ThunderX2 using a Driver0000 option.
> 
> Silicon/Openmoko/ChaosKeyDxe/DriverBinding.c | 66 ++++++++++++++++++++
> 1 file changed, 66 insertions(+)
> 
> diff --git a/Silicon/Openmoko/ChaosKeyDxe/DriverBinding.c b/Silicon/Openmoko/ChaosKeyDxe/DriverBinding.c
> index e7d0d3fe563e..611803c6c339 100644
> --- a/Silicon/Openmoko/ChaosKeyDxe/DriverBinding.c
> +++ b/Silicon/Openmoko/ChaosKeyDxe/DriverBinding.c
> @@ -11,6 +11,9 @@
> 
> #include "ChaosKeyDriver.h"
> 
> +STATIC VOID         *mProtocolNotifyRegistration;
> +STATIC EFI_EVENT    mProtocolNotifyRegistrationEvent;
> +
> /**
>   Tests to see if this driver supports a given controller.
> 
> @@ -157,6 +160,55 @@ EFI_DRIVER_BINDING_PROTOCOL  gUsbDriverBinding = {
> };
> 
> 
> +STATIC
> +VOID
> +EFIAPI
> +UsbHwrngOnProtocolNotify (
> +  IN EFI_EVENT            Event,
> +  IN VOID                 *Context
> +  )
> +{
> +  EFI_STATUS              Status;
> +  EFI_HANDLE              *Handles;
> +  UINTN                   HandleCount;
> +  UINTN                   Index;
> +
> +  do {
> +    Status = gBS->LocateHandleBuffer (ByRegisterNotify, NULL,
> +                    mProtocolNotifyRegistration, &HandleCount, &Handles);
> +    if (EFI_ERROR (Status)) {
> +      if (Status != EFI_NOT_FOUND) {
> +        DEBUG ((DEBUG_WARN, "%a: LocateHandleBuffer() failed - %r\n",
> +          __FUNCTION__, Status));
> +        }
> +      return;
> +    }
> +
> +    if (HandleCount == 0) {
> +      return;
> +    }
> +
> +    for (Index = 0; Index < HandleCount; Index++) {
> +      Status = UsbHwrngDriverBindingSupported (&gUsbDriverBinding,
> +                 Handles[Index], NULL);
> +      if (EFI_ERROR (Status)) {
> +        continue;
> +      }
> +
> +      //
> +      // Attempt to connect the USB device path describing the ChaosKey
> +      // hardware via the handle describing a USB host controller.
> +      //
> +      Status = UsbHwrngDriverBindingStart (&gUsbDriverBinding,
> +                 Handles[Index], NULL);
> +      DEBUG ((DEBUG_VERBOSE, "%a: UsbHwrngDriverBindingStart () returned %r\n",
> +        __FUNCTION__, Status));
> +    }
> +    gBS->FreePool (Handles);
> +  } while (1);
> +}
> +
> +
> /**
>   The entry point of ChaosKey UEFI Driver.
> 
> @@ -185,6 +237,18 @@ EntryPoint (
>              NULL, &gChaosKeyDriverComponentName2);
>   ASSERT_EFI_ERROR (Status);
> 
> +  //
> +  // This driver produces the EFI Random Number Generator protocol on
> +  // compatible USB I/O handles, which is not a protocol that can provide
> +  // a boot target. This means that it will not get connected on an ordinary
> +  // 'fast' boot (which only connects the console and boot entry device paths)
> +  // unless we take extra measures.
> +  //
> +  mProtocolNotifyRegistrationEvent = EfiCreateProtocolNotifyEvent (
> +                                       &gEfiUsbIoProtocolGuid, TPL_CALLBACK,
> +                                       UsbHwrngOnProtocolNotify, NULL,
> +                                       &mProtocolNotifyRegistration);
> +
>   DEBUG ((DEBUG_INIT | DEBUG_INFO, "*** Installed ChaosKey driver! ***\n"));
> 
>   return EFI_SUCCESS;
> @@ -211,6 +275,8 @@ UnloadImage (
>   UINTN       HandleCount;
>   UINTN       Index;
> 
> +  gBS->CloseEvent (mProtocolNotifyRegistrationEvent);
> +
>   //
>   // Retrieve all USB I/O handles in the handle database
>   //
> -- 
> 2.20.1
> 
> 
> 
> 

Re: [edk2-devel] [PATCH edk2-platforms v2] Silicon/ChaosKeyDxe: don't rely on connect all controllers

[Ard Biesheuvel]

On 6/3/20 12:28 AM, Andrew Fish wrote:
> 
> 
>> On Jun 2, 2020, at 6:38 AM, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
>>
>> From: Ard Biesheuvel <ard.biesheuvel@arm.com>
>>
>> The ChaosKey driver implements the UEFI driver model, and so it is
>> not guaranteed that any controllers will be attached to this driver
>> unless it is connected explicitly. On many platforms today, this is
>> taken care of by the ConnectAll() call that occurs in the BDS, but
>> this is not something we should rely on.
>>
>> So add a protocol notification event that will attempt to connect
>> the ChaosKey on each USB I/O protocol instance that carries the
>> expected vendor and product IDs. This still relies on the USB host
>> controllers to be connected by the platform, which is typically the
>> case (given that a USB keyboard is required to interrupt the boot)
>>
>> On platforms where USB is not connected at all by default, it is really
>> not up to a third party driver to meddle with this, and so relying on
>> the USB host controllers to have been connected non-reursively is the
>> best we can do. Note that third party drivers registered via Driver####
>> can set a 'reconnect all' flag if needed to mitigate this.
>>
>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@arm.com>
>> ---
>>
>> Unfortunately, the previous approach of connecting a short-form USB device
>> path containing the ChaosKey's VID/PID turned out not to be reliable in
>> practice (it doesn't work at all on ThunderX2 with AMI firmware)
>>
>> So instead, we have to rely on USB I/O protocols having been instantiated
>> by the DXE core. This is what typically happens, given that USB keyboards
>> are also exposed via USB I/O protocol instances. The only difference with
>> a non-fast [ConnectAll()] boot is that those USB I/O protocol instances
>> are not connected further automatically, but it is reasonable to expect
>> that the handles themselves have been instantiated.
>>
>> Platforms that do not produce those USB I/O handles would not be able to
>> offer the ability to interrupt the boot or enter the menu using a USB
>> keyboard, so this is rather rare in practice. Also, if such platforms do
>> exist, it is not up to this 3rd party driver to override this policy and
>> enumerate the entire USB hierarchy.
>>
>> So this means we need to call UsbHwrngDriverBindingSupported() directly
>> to check whether the USB I/O protocol in question has the right VID/PID.
>> If that succeeds, there is really no point in using ConnectController(),
>> so just call UsbHwrngDriverBindingStart() directly to connect the driver
>> to the controller.
>>
> 
> Actually there is a point in using gBS->ConnectController().
> 1) You make it impossible for the platform to override the drivers choice. The philosophy of EFI has alway been that the drivers should not cary policy and the platforms should cary policy. This inverts that.
> 2) I’m not sure it is well defined behavior by UEFI to NOT call gBS->ConnectController(). I took a quick look at the DXE Core and the gBS->ConnectController() is not doing any book keeping, other than managing the precedence rules, but it seems like it would be legal to add something in the future. + Mike in case I’m off on this one.
> 

The idea is really that by loading this driver, you are indicating that 
you want it to connect to any supporting controller and produce the RNG 
protocol, despite the platform policy. The UEFI driver model simply 
isn't a great fit here, since the RNG is never a boot target and you 
just want it to connect all the time.

It is a shame my first approach did not work: it attempted to connect 
the short form USB class device path carrying the VID and PID of this 
controller as the remaining device path on each detected USB host 
controller, similar to how USB keyboards are connected in the upstream 
BDS code.

But yes, ConnectController() should also work - it will invoke the 
Supported() method again, and call Start(). I did check that doing this 
directly amounts to the same in terms of bookkeeping, but I agree that 
this could potentially change at some point. However, the question is 
whether connecting the controller to another, higher priority driver is 
the right thing to do in this case. It is really hard to answer that 
question in general terms.

> Also It would like be a good idea to have a PCD to turn on/off this feature.
> 

Fair enough.

> Thanks,
> 
> Andrew Fish
> 
>> Tested on ThunderX2 using a Driver0000 option.
>>
>> Silicon/Openmoko/ChaosKeyDxe/DriverBinding.c | 66 ++++++++++++++++++++
>> 1 file changed, 66 insertions(+)
>>
>> diff --git a/Silicon/Openmoko/ChaosKeyDxe/DriverBinding.c b/Silicon/Openmoko/ChaosKeyDxe/DriverBinding.c
>> index e7d0d3fe563e..611803c6c339 100644
>> --- a/Silicon/Openmoko/ChaosKeyDxe/DriverBinding.c
>> +++ b/Silicon/Openmoko/ChaosKeyDxe/DriverBinding.c
>> @@ -11,6 +11,9 @@
>>
>> #include "ChaosKeyDriver.h"
>>
>> +STATIC VOID         *mProtocolNotifyRegistration;
>> +STATIC EFI_EVENT    mProtocolNotifyRegistrationEvent;
>> +
>> /**
>>    Tests to see if this driver supports a given controller.
>>
>> @@ -157,6 +160,55 @@ EFI_DRIVER_BINDING_PROTOCOL  gUsbDriverBinding = {
>> };
>>
>>
>> +STATIC
>> +VOID
>> +EFIAPI
>> +UsbHwrngOnProtocolNotify (
>> +  IN EFI_EVENT            Event,
>> +  IN VOID                 *Context
>> +  )
>> +{
>> +  EFI_STATUS              Status;
>> +  EFI_HANDLE              *Handles;
>> +  UINTN                   HandleCount;
>> +  UINTN                   Index;
>> +
>> +  do {
>> +    Status = gBS->LocateHandleBuffer (ByRegisterNotify, NULL,
>> +                    mProtocolNotifyRegistration, &HandleCount, &Handles);
>> +    if (EFI_ERROR (Status)) {
>> +      if (Status != EFI_NOT_FOUND) {
>> +        DEBUG ((DEBUG_WARN, "%a: LocateHandleBuffer() failed - %r\n",
>> +          __FUNCTION__, Status));
>> +        }
>> +      return;
>> +    }
>> +
>> +    if (HandleCount == 0) {
>> +      return;
>> +    }
>> +
>> +    for (Index = 0; Index < HandleCount; Index++) {
>> +      Status = UsbHwrngDriverBindingSupported (&gUsbDriverBinding,
>> +                 Handles[Index], NULL);
>> +      if (EFI_ERROR (Status)) {
>> +        continue;
>> +      }
>> +
>> +      //
>> +      // Attempt to connect the USB device path describing the ChaosKey
>> +      // hardware via the handle describing a USB host controller.
>> +      //
>> +      Status = UsbHwrngDriverBindingStart (&gUsbDriverBinding,
>> +                 Handles[Index], NULL);
>> +      DEBUG ((DEBUG_VERBOSE, "%a: UsbHwrngDriverBindingStart () returned %r\n",
>> +        __FUNCTION__, Status));
>> +    }
>> +    gBS->FreePool (Handles);
>> +  } while (1);
>> +}
>> +
>> +
>> /**
>>    The entry point of ChaosKey UEFI Driver.
>>
>> @@ -185,6 +237,18 @@ EntryPoint (
>>               NULL, &gChaosKeyDriverComponentName2);
>>    ASSERT_EFI_ERROR (Status);
>>
>> +  //
>> +  // This driver produces the EFI Random Number Generator protocol on
>> +  // compatible USB I/O handles, which is not a protocol that can provide
>> +  // a boot target. This means that it will not get connected on an ordinary
>> +  // 'fast' boot (which only connects the console and boot entry device paths)
>> +  // unless we take extra measures.
>> +  //
>> +  mProtocolNotifyRegistrationEvent = EfiCreateProtocolNotifyEvent (
>> +                                       &gEfiUsbIoProtocolGuid, TPL_CALLBACK,
>> +                                       UsbHwrngOnProtocolNotify, NULL,
>> +                                       &mProtocolNotifyRegistration);
>> +
>>    DEBUG ((DEBUG_INIT | DEBUG_INFO, "*** Installed ChaosKey driver! ***\n"));
>>
>>    return EFI_SUCCESS;
>> @@ -211,6 +275,8 @@ UnloadImage (
>>    UINTN       HandleCount;
>>    UINTN       Index;
>>
>> +  gBS->CloseEvent (mProtocolNotifyRegistrationEvent);
>> +
>>    //
>>    // Retrieve all USB I/O handles in the handle database
>>    //
>> -- 
>> 2.20.1
>>
>>
>> 
>>
> 

Re: [edk2-devel] [PATCH edk2-platforms v2] Silicon/ChaosKeyDxe: don't rely on connect all controllers

[Andrew Fish]

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



> On Jun 2, 2020, at 11:32 PM, Ard Biesheuvel <ard.biesheuvel@arm.com> wrote:
> 
> On 6/3/20 12:28 AM, Andrew Fish wrote:
>>> On Jun 2, 2020, at 6:38 AM, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
>>> 
>>> From: Ard Biesheuvel <ard.biesheuvel@arm.com>
>>> 
>>> The ChaosKey driver implements the UEFI driver model, and so it is
>>> not guaranteed that any controllers will be attached to this driver
>>> unless it is connected explicitly. On many platforms today, this is
>>> taken care of by the ConnectAll() call that occurs in the BDS, but
>>> this is not something we should rely on.
>>> 
>>> So add a protocol notification event that will attempt to connect
>>> the ChaosKey on each USB I/O protocol instance that carries the
>>> expected vendor and product IDs. This still relies on the USB host
>>> controllers to be connected by the platform, which is typically the
>>> case (given that a USB keyboard is required to interrupt the boot)
>>> 
>>> On platforms where USB is not connected at all by default, it is really
>>> not up to a third party driver to meddle with this, and so relying on
>>> the USB host controllers to have been connected non-reursively is the
>>> best we can do. Note that third party drivers registered via Driver####
>>> can set a 'reconnect all' flag if needed to mitigate this.
>>> 
>>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@arm.com>
>>> ---
>>> 
>>> Unfortunately, the previous approach of connecting a short-form USB device
>>> path containing the ChaosKey's VID/PID turned out not to be reliable in
>>> practice (it doesn't work at all on ThunderX2 with AMI firmware)
>>> 
>>> So instead, we have to rely on USB I/O protocols having been instantiated
>>> by the DXE core. This is what typically happens, given that USB keyboards
>>> are also exposed via USB I/O protocol instances. The only difference with
>>> a non-fast [ConnectAll()] boot is that those USB I/O protocol instances
>>> are not connected further automatically, but it is reasonable to expect
>>> that the handles themselves have been instantiated.
>>> 
>>> Platforms that do not produce those USB I/O handles would not be able to
>>> offer the ability to interrupt the boot or enter the menu using a USB
>>> keyboard, so this is rather rare in practice. Also, if such platforms do
>>> exist, it is not up to this 3rd party driver to override this policy and
>>> enumerate the entire USB hierarchy.
>>> 
>>> So this means we need to call UsbHwrngDriverBindingSupported() directly
>>> to check whether the USB I/O protocol in question has the right VID/PID.
>>> If that succeeds, there is really no point in using ConnectController(),
>>> so just call UsbHwrngDriverBindingStart() directly to connect the driver
>>> to the controller.
>>> 
>> Actually there is a point in using gBS->ConnectController().
>> 1) You make it impossible for the platform to override the drivers choice. The philosophy of EFI has alway been that the drivers should not cary policy and the platforms should cary policy. This inverts that.
>> 2) I’m not sure it is well defined behavior by UEFI to NOT call gBS->ConnectController(). I took a quick look at the DXE Core and the gBS->ConnectController() is not doing any book keeping, other than managing the precedence rules, but it seems like it would be legal to add something in the future. + Mike in case I’m off on this one.
> 
> The idea is really that by loading this driver, you are indicating that you want it to connect to any supporting controller and produce the RNG protocol, despite the platform policy. The UEFI driver model simply isn't a great fit here, since the RNG is never a boot target and you just want it to connect all the time.
> 

I understand the issue. On the fast boot path only things in the NVRAM variables, or hard coded as BDS policy get connected. For example in our custom BDS we have code that connects devices related to the T2 and we have custom code that only connects them at the known location. These devices produce services that other EFI Driver Model drivers use when they connect. 

> It is a shame my first approach did not work: it attempted to connect the short form USB class device path carrying the VID and PID of this controller as the remaining device path on each detected USB host controller, similar to how USB keyboards are connected in the upstream BDS code.
> 

Is there a BZ for that? I’ve gotten feedback over the years that our BDS implementation is hard to understand. Maybe we should invest some time in documentation on how to use it?

Thanks,

Andrew Fish

> But yes, ConnectController() should also work - it will invoke the Supported() method again, and call Start(). I did check that doing this directly amounts to the same in terms of bookkeeping, but I agree that this could potentially change at some point. However, the question is whether connecting the controller to another, higher priority driver is the right thing to do in this case. It is really hard to answer that question in general terms.
> 
>> Also It would like be a good idea to have a PCD to turn on/off this feature.
> 
> Fair enough.
> 
>> Thanks,
>> Andrew Fish
>>> Tested on ThunderX2 using a Driver0000 option.
>>> 
>>> Silicon/Openmoko/ChaosKeyDxe/DriverBinding.c | 66 ++++++++++++++++++++
>>> 1 file changed, 66 insertions(+)
>>> 
>>> diff --git a/Silicon/Openmoko/ChaosKeyDxe/DriverBinding.c b/Silicon/Openmoko/ChaosKeyDxe/DriverBinding.c
>>> index e7d0d3fe563e..611803c6c339 100644
>>> --- a/Silicon/Openmoko/ChaosKeyDxe/DriverBinding.c
>>> +++ b/Silicon/Openmoko/ChaosKeyDxe/DriverBinding.c
>>> @@ -11,6 +11,9 @@
>>> 
>>> #include "ChaosKeyDriver.h"
>>> 
>>> +STATIC VOID         *mProtocolNotifyRegistration;
>>> +STATIC EFI_EVENT    mProtocolNotifyRegistrationEvent;
>>> +
>>> /**
>>>   Tests to see if this driver supports a given controller.
>>> 
>>> @@ -157,6 +160,55 @@ EFI_DRIVER_BINDING_PROTOCOL  gUsbDriverBinding = {
>>> };
>>> 
>>> 
>>> +STATIC
>>> +VOID
>>> +EFIAPI
>>> +UsbHwrngOnProtocolNotify (
>>> +  IN EFI_EVENT            Event,
>>> +  IN VOID                 *Context
>>> +  )
>>> +{
>>> +  EFI_STATUS              Status;
>>> +  EFI_HANDLE              *Handles;
>>> +  UINTN                   HandleCount;
>>> +  UINTN                   Index;
>>> +
>>> +  do {
>>> +    Status = gBS->LocateHandleBuffer (ByRegisterNotify, NULL,
>>> +                    mProtocolNotifyRegistration, &HandleCount, &Handles);
>>> +    if (EFI_ERROR (Status)) {
>>> +      if (Status != EFI_NOT_FOUND) {
>>> +        DEBUG ((DEBUG_WARN, "%a: LocateHandleBuffer() failed - %r\n",
>>> +          __FUNCTION__, Status));
>>> +        }
>>> +      return;
>>> +    }
>>> +
>>> +    if (HandleCount == 0) {
>>> +      return;
>>> +    }
>>> +
>>> +    for (Index = 0; Index < HandleCount; Index++) {
>>> +      Status = UsbHwrngDriverBindingSupported (&gUsbDriverBinding,
>>> +                 Handles[Index], NULL);
>>> +      if (EFI_ERROR (Status)) {
>>> +        continue;
>>> +      }
>>> +
>>> +      //
>>> +      // Attempt to connect the USB device path describing the ChaosKey
>>> +      // hardware via the handle describing a USB host controller.
>>> +      //
>>> +      Status = UsbHwrngDriverBindingStart (&gUsbDriverBinding,
>>> +                 Handles[Index], NULL);
>>> +      DEBUG ((DEBUG_VERBOSE, "%a: UsbHwrngDriverBindingStart () returned %r\n",
>>> +        __FUNCTION__, Status));
>>> +    }
>>> +    gBS->FreePool (Handles);
>>> +  } while (1);
>>> +}
>>> +
>>> +
>>> /**
>>>   The entry point of ChaosKey UEFI Driver.
>>> 
>>> @@ -185,6 +237,18 @@ EntryPoint (
>>>              NULL, &gChaosKeyDriverComponentName2);
>>>   ASSERT_EFI_ERROR (Status);
>>> 
>>> +  //
>>> +  // This driver produces the EFI Random Number Generator protocol on
>>> +  // compatible USB I/O handles, which is not a protocol that can provide
>>> +  // a boot target. This means that it will not get connected on an ordinary
>>> +  // 'fast' boot (which only connects the console and boot entry device paths)
>>> +  // unless we take extra measures.
>>> +  //
>>> +  mProtocolNotifyRegistrationEvent = EfiCreateProtocolNotifyEvent (
>>> +                                       &gEfiUsbIoProtocolGuid, TPL_CALLBACK,
>>> +                                       UsbHwrngOnProtocolNotify, NULL,
>>> +                                       &mProtocolNotifyRegistration);
>>> +
>>>   DEBUG ((DEBUG_INIT | DEBUG_INFO, "*** Installed ChaosKey driver! ***\n"));
>>> 
>>>   return EFI_SUCCESS;
>>> @@ -211,6 +275,8 @@ UnloadImage (
>>>   UINTN       HandleCount;
>>>   UINTN       Index;
>>> 
>>> +  gBS->CloseEvent (mProtocolNotifyRegistrationEvent);
>>> +
>>>   //
>>>   // Retrieve all USB I/O handles in the handle database
>>>   //
>>> -- 
>>> 2.20.1
>>> 
>>> 
>>> 
> 
> 
> 


[-- Attachment #2: Type: text/html, Size: 21246 bytes --]

Re: [edk2-devel] [PATCH edk2-platforms v2] Silicon/ChaosKeyDxe: don't rely on connect all controllers

[Ard Biesheuvel]

On 6/3/20 5:47 PM, Andrew Fish wrote:
> 
> 
>> On Jun 2, 2020, at 11:32 PM, Ard Biesheuvel <ard.biesheuvel@arm.com 
>> <mailto:ard.biesheuvel@arm.com>> wrote:
>>
>> On 6/3/20 12:28 AM, Andrew Fish wrote:
>>>> On Jun 2, 2020, at 6:38 AM, Ard Biesheuvel 
>>>> <ard.biesheuvel@linaro.org <mailto:ard.biesheuvel@linaro.org>> wrote:
>>>>
>>>> From: Ard Biesheuvel <ard.biesheuvel@arm.com 
>>>> <mailto:ard.biesheuvel@arm.com>>
>>>>
>>>> The ChaosKey driver implements the UEFI driver model, and so it is
>>>> not guaranteed that any controllers will be attached to this driver
>>>> unless it is connected explicitly. On many platforms today, this is
>>>> taken care of by the ConnectAll() call that occurs in the BDS, but
>>>> this is not something we should rely on.
>>>>
>>>> So add a protocol notification event that will attempt to connect
>>>> the ChaosKey on each USB I/O protocol instance that carries the
>>>> expected vendor and product IDs. This still relies on the USB host
>>>> controllers to be connected by the platform, which is typically the
>>>> case (given that a USB keyboard is required to interrupt the boot)
>>>>
>>>> On platforms where USB is not connected at all by default, it is really
>>>> not up to a third party driver to meddle with this, and so relying on
>>>> the USB host controllers to have been connected non-reursively is the
>>>> best we can do. Note that third party drivers registered via Driver####
>>>> can set a 'reconnect all' flag if needed to mitigate this.
>>>>
>>>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@arm.com 
>>>> <mailto:ard.biesheuvel@arm.com>>
>>>> ---
>>>>
>>>> Unfortunately, the previous approach of connecting a short-form USB 
>>>> device
>>>> path containing the ChaosKey's VID/PID turned out not to be reliable in
>>>> practice (it doesn't work at all on ThunderX2 with AMI firmware)
>>>>
>>>> So instead, we have to rely on USB I/O protocols having been 
>>>> instantiated
>>>> by the DXE core. This is what typically happens, given that USB 
>>>> keyboards
>>>> are also exposed via USB I/O protocol instances. The only difference 
>>>> with
>>>> a non-fast [ConnectAll()] boot is that those USB I/O protocol instances
>>>> are not connected further automatically, but it is reasonable to expect
>>>> that the handles themselves have been instantiated.
>>>>
>>>> Platforms that do not produce those USB I/O handles would not be able to
>>>> offer the ability to interrupt the boot or enter the menu using a USB
>>>> keyboard, so this is rather rare in practice. Also, if such platforms do
>>>> exist, it is not up to this 3rd party driver to override this policy and
>>>> enumerate the entire USB hierarchy.
>>>>
>>>> So this means we need to call UsbHwrngDriverBindingSupported() directly
>>>> to check whether the USB I/O protocol in question has the right VID/PID.
>>>> If that succeeds, there is really no point in using ConnectController(),
>>>> so just call UsbHwrngDriverBindingStart() directly to connect the driver
>>>> to the controller.
>>>>
>>> Actually there is a point in using gBS->ConnectController().
>>> 1) You make it impossible for the platform to override the drivers 
>>> choice. The philosophy of EFI has alway been that the drivers should 
>>> not cary policy and the platforms should cary policy. This inverts that.
>>> 2) I’m not sure it is well defined behavior by UEFI to NOT call 
>>> gBS->ConnectController(). I took a quick look at the DXE Core and the 
>>> gBS->ConnectController() is not doing any book keeping, other than 
>>> managing the precedence rules, but it seems like it would be legal to 
>>> add something in the future. + Mike in case I’m off on this one.
>>
>> The idea is really that by loading this driver, you are indicating 
>> that you want it to connect to any supporting controller and produce 
>> the RNG protocol, despite the platform policy. The UEFI driver model 
>> simply isn't a great fit here, since the RNG is never a boot target 
>> and you just want it to connect all the time.
>>
> 
> I understand the issue. On the fast boot path only things in the NVRAM 
> variables, or hard coded as BDS policy get connected. For example in our 
> custom BDS we have code that connects devices related to the T2 and we 
> have custom code that only connects them at the known location. These 
> devices produce services that other EFI Driver Model drivers use when 
> they connect.
> 

Sure. If the controller is known to live at a particular location on a 
particular platform, you can just connect it. The idea of this driver is 
that you can install it on any platform, plug in the USB stick and off 
you go, which is substantially more complicated.


>> It is a shame my first approach did not work: it attempted to connect 
>> the short form USB class device path carrying the VID and PID of this 
>> controller as the remaining device path on each detected USB host 
>> controller, similar to how USB keyboards are connected in the upstream 
>> BDS code.
>>
> 
> Is there a BZ for that? I’ve gotten feedback over the years that our BDS 
> implementation is hard to understand. Maybe we should invest some time 
> in documentation on how to use it?
> 

I'm not sure that would help. I was hoping we could rely on spec'ed 
behavior here, rather than how some BDSes might behave.

Note that the AMI BDS behaved differently here than the EDK2 one.