From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wr0-x230.google.com (mail-wr0-x230.google.com [IPv6:2a00:1450:400c:c0c::230]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 446EB21AEB0CE for ; Thu, 24 Aug 2017 04:27:57 -0700 (PDT) Received: by mail-wr0-x230.google.com with SMTP id w9so1360670wre.1 for ; Thu, 24 Aug 2017 04:30:32 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to:user-agent; bh=qSPP3mQn45HQG6QfGYmn5HFpoXzdtVrpO5IQmtN77xg=; b=BBJfwUjl3RcvxoYjSIZfuFYMSvHh1+RCXkIyDbZZwe5bhB29Z134R5AUrg7hqNUpHb OXyNtYS7ovZVsO9GV5I4Y0egr7g3cUFxm8+bOhEARs0i3tCSgxALNPaFjHwPyU5gQWT/ VWCB6O/qQEl8cTyUgJL4OxDCuUxtm4jq8Uj0c= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to:user-agent; bh=qSPP3mQn45HQG6QfGYmn5HFpoXzdtVrpO5IQmtN77xg=; b=m74oS/1AGEtKE4ZC2jRbA+Nd4a0B4O0mhyXEYQNqkRLbg4689cJIaFbw3WUjV8h0QG /s8zlArlATAgoD51htoN+gxEQXnLmvrWOWzC0ssBL+IcPfLfceNir/6hm11XD2pCBBFR CEOc6ULrS/Ufpn7lpo2zbvroBsZOuFEolQ5Pg3Om7k/2i/IhqG9SGP7SSH5RupAKlt2V gEHBuQdbTCRApQ8YopasSVctwT0eK9I0gpFFYE2M281eAo/n8vMloGcD/yg5Q1sXeOun e5kE6yRm+3Vw+N7BuM/5eRMPplZbw+mAgoVT72EQiMGVH3CSYSId5navQCvDP8nuy2ny iq/A== X-Gm-Message-State: AHYfb5i/CGyG2o4lQz900nhqmb9i8unQXTEESDi3Fj7/+qbmlW29nrYG TWK8TxrMgvFuydEK X-Received: by 10.223.139.134 with SMTP id o6mr3581995wra.178.1503574230373; Thu, 24 Aug 2017 04:30:30 -0700 (PDT) Received: from bivouac.eciton.net (bivouac.eciton.net. [2a00:1098:0:86:1000:23:0:2]) by smtp.gmail.com with ESMTPSA id w16sm7014097wrc.84.2017.08.24.04.30.29 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Thu, 24 Aug 2017 04:30:29 -0700 (PDT) Date: Thu, 24 Aug 2017 12:30:27 +0100 From: Leif Lindholm To: Ard Biesheuvel Cc: "edk2-devel@lists.01.org" , "Kinney, Michael D" Message-ID: <20170824113027.25phx7ckxva6oyin@bivouac.eciton.net> References: <20170823131206.25008-1-ard.biesheuvel@linaro.org> <20170823234802.yk5u4ilnci34uy3p@bivouac.eciton.net> MIME-Version: 1.0 In-Reply-To: User-Agent: NeoMutt/20170113 (1.7.2) Subject: Re: [PATCH edk2-platforms] Silicon/Openmoko: add driver for ChaosKey RNG USB device X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.22 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 24 Aug 2017 11:27:57 -0000 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline On Thu, Aug 24, 2017 at 10:44:46AM +0100, Ard Biesheuvel wrote: > >> Silicon/Openmoko/ChaosKeyDxe/ChaosKeyDriver.c | 346 ++++++++++++++++++++ > >> Silicon/Openmoko/ChaosKeyDxe/ChaosKeyDriver.h | 61 ++++ > >> Silicon/Openmoko/ChaosKeyDxe/ChaosKeyDxe.inf | 48 +++ > >> Silicon/Openmoko/ChaosKeyDxe/ComponentName.c | 205 ++++++++++++ > >> Silicon/Openmoko/ChaosKeyDxe/DriverBinding.c | 256 +++++++++++++++ > >> 5 files changed, 916 insertions(+) > > > > This is all a bit new territory, so I don't know what the best way of > > dealing with this is, but I think we could do with a .dsc somewhere to > > enable building this driver standalone. My gut feel is that an > > Openmoko/Openmoko.dsc would be a reasonable candidate. > > Yeah, I can add one. Also useful for building this driver as an EBC binary. Yes, indeed. Thanks. > >> diff --git a/Silicon/Openmoko/ChaosKeyDxe/ChaosKeyDriver.c b/Silicon/Openmoko/ChaosKeyDxe/ChaosKeyDriver.c > >> new file mode 100644 > >> index 000000000000..1870080d2c70 > >> --- /dev/null > >> +++ b/Silicon/Openmoko/ChaosKeyDxe/ChaosKeyDriver.c > >> @@ -0,0 +1,346 @@ > >> +/** @file > >> + Device driver for the ChaosKey hardware random number generator. > >> + > >> + Copyright (c) 2016 - 2017, Linaro Ltd. All rights reserved.
> >> + > >> + This program and the accompanying materials > >> + are licensed and made available under the terms and conditions of the BSD > >> + License which accompanies this distribution. The full text of the license may > >> + be found at http://opensource.org/licenses/bsd-license.php. > >> + > >> + THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS, > >> + WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED. > >> + > >> +**/ > >> + > >> +#include "ChaosKeyDriver.h" > >> + > >> +#include > >> +#include > >> +#include > >> + > >> +STATIC > >> +BOOLEAN > >> +IsBulkInEndpoint ( > >> + IN EFI_USB_ENDPOINT_DESCRIPTOR *Endpoint > >> + ) > >> +{ > >> + if ((Endpoint->Attributes & USB_ENDPOINT_TYPE_MASK) == USB_ENDPOINT_BULK) { > >> + if (Endpoint->EndpointAddress & USB_ENDPOINT_DIR_IN) { > >> + return TRUE; > >> + } > >> + } > >> + return FALSE; > >> +} > >> + > >> + > >> +STATIC > >> +EFI_STATUS > >> +FindEndpoint ( > >> + IN CHAOSKEY_DEV *ChaosKey > >> + ) > >> +{ > >> + EFI_USB_IO_PROTOCOL *UsbIo; > >> + EFI_STATUS Status; > >> + UINTN Index; > >> + EFI_USB_INTERFACE_DESCRIPTOR InterfaceDescriptor; > >> + > >> + UsbIo = ChaosKey->UsbIo; > >> + > >> + // > >> + // Get interface & endpoint descriptor > >> + // > >> + Status = UsbIo->UsbGetInterfaceDescriptor (UsbIo, &InterfaceDescriptor); > >> + if (EFI_ERROR (Status)) { > >> + return Status; > >> + } > >> + > >> + // > >> + // The ChaosKey provides two endpoints: > >> + // - The first one is the 'cooked' one, to be used as random data input > >> + // - The second one is the raw bitstream from the generator, higher > >> + // throughput, but lower randomness. > >> + // So locate the first bulk IN endpoint and save it for later use. > >> + // > >> + for (Index = 0; Index < InterfaceDescriptor.NumEndpoints; Index++) { > >> + EFI_USB_ENDPOINT_DESCRIPTOR Endpoint; > >> + > >> + Status = UsbIo->UsbGetEndpointDescriptor (UsbIo, Index, &Endpoint); > >> + if (EFI_ERROR (Status)) { > >> + DEBUG ((DEBUG_ERROR, "UsbGetEndPointDescriptor(%d) failed!\n", Index)); > >> + return Status; > >> + } > >> + > >> + if (IsBulkInEndpoint(&Endpoint)) { > >> + ChaosKey->EndpointAddress = Endpoint.EndpointAddress; > >> + ChaosKey->EndpointSize = Endpoint.MaxPacketSize; > >> + return EFI_SUCCESS; > >> + } > >> + } > >> + > >> + DEBUG ((DEBUG_ERROR, "Failed to locate suitable BULK IN USB endpoint!\n")); > >> + return EFI_DEVICE_ERROR; > >> +} > >> + > >> + > >> +/** > >> + Returns information about the random number generation implementation. > >> + > >> + @param[in] This A pointer to the EFI_RNG_PROTOCOL instance. > >> + @param[in,out] AlgorithmListSize On input, the size in bytes of AlgorithmList > >> + On output with a return code of EFI_SUCCESS, > >> + the size in bytes of the data returned in > >> + AlgorithmList. On output with a return > >> + code of EFI_BUFFER_TOO_SMALL, the size of > >> + AlgorithmList required to obtain the list. > >> + @param[out] AlgorithmList A caller-allocated memory buffer filled by > >> + the driver with one EFI_RNG_ALGORITHM > >> + element for each supported RNG algorithm. > >> + The list must not change across multiple > >> + calls to the same driver. The first > >> + algorithm in the list is the default > >> + algorithm for the driver. > >> + > >> + @retval EFI_SUCCESS The RNG algorithm list was returned > >> + successfully. > >> + @retval EFI_UNSUPPORTED The services is not supported by this driver > >> + @retval EFI_DEVICE_ERROR The list of algorithms could not be > >> + retrieved due to a hardware or firmware > >> + error. > >> + @retval EFI_INVALID_PARAMETER One or more of the parameters are incorrect. > >> + @retval EFI_BUFFER_TOO_SMALL The buffer RNGAlgorithmList is too small to > >> + hold the result. > >> + > >> +**/ > >> +STATIC > >> +EFI_STATUS > >> +EFIAPI > >> +GetInfo ( > >> + IN EFI_RNG_PROTOCOL *This, > >> + IN OUT UINTN *AlgorithmListSize, > >> + OUT EFI_RNG_ALGORITHM *AlgorithmList > >> +) > >> +{ > >> + UINTN Size; > >> + > >> + // > >> + // We only implement the raw algorithm > >> + // > >> + Size = sizeof gEfiRngAlgorithmRaw; > >> + > >> + if (*AlgorithmListSize < Size) { > >> + *AlgorithmListSize = Size; > >> + return EFI_BUFFER_TOO_SMALL; > >> + } > >> + > >> + gBS->CopyMem (AlgorithmList, &gEfiRngAlgorithmRaw, Size); > >> + *AlgorithmListSize = Size; > >> + > >> + return EFI_SUCCESS; > >> +} > >> + > >> + > >> +/** > >> + Produces and returns an RNG value using either the default or specified RNG > >> + algorithm. > >> + > >> + @param[in] This A pointer to the EFI_RNG_PROTOCOL instance. > >> + @param[in] Algorithm A pointer to the EFI_RNG_ALGORITHM that > >> + identifies the RNG algorithm to use. May be > >> + NULL in which case the function will use its > >> + default RNG algorithm. > >> + @param[in] ValueLength The length in bytes of the memory buffer > >> + pointed to by RNGValue. The driver shall > >> + return exactly this numbers of bytes. > >> + @param[out] Value A caller-allocated memory buffer filled by the > >> + driver with the resulting RNG value. > >> + > >> + @retval EFI_SUCCESS The RNG value was returned successfully. > >> + @retval EFI_UNSUPPORTED The algorithm specified by RNGAlgorithm is not > >> + supported by this driver. > >> + @retval EFI_DEVICE_ERROR An RNG value could not be retrieved due to a > >> + hardware or firmware error. > >> + @retval EFI_NOT_READY There is not enough random data available to > >> + satisfy the length requested by > >> + RNGValueLength. > >> + @retval EFI_INVALID_PARAMETER RNGValue is NULL or RNGValueLength is zero. > >> + > >> +**/ > >> +STATIC > >> +EFI_STATUS > >> +EFIAPI > >> +GetRNG ( > >> + IN EFI_RNG_PROTOCOL *This, > >> + IN EFI_RNG_ALGORITHM *Algorithm OPTIONAL, > >> + IN UINTN ValueLength, > >> + OUT UINT8 *Value > >> +) > >> +{ > >> + EFI_STATUS Status; > >> + CHAOSKEY_DEV *ChaosKey; > >> + EFI_TPL Tpl; > >> + UINT8 Buffer[CHAOSKEY_MAX_EP_SIZE]; > >> + UINT8 *OutPointer; > >> + UINTN OutSize; > >> + UINT32 Result; > >> + > >> + if (Algorithm != NULL && !CompareGuid (Algorithm, &gEfiRngAlgorithmRaw)) { > >> + return EFI_UNSUPPORTED; > >> + } > >> + > >> + ChaosKey = CHAOSKEY_DEV_FROM_THIS (This); > >> + > >> + while (ValueLength > 0) { > >> + // > >> + // If more data is requested than the endpoint can deliver in a single > >> + // transfer, put it straight into the caller's buffer. > >> + // > >> + if (ValueLength >= ChaosKey->EndpointSize) { > >> + OutPointer = Value; > >> + } else { > >> + OutPointer = Buffer; > >> + } > >> + OutSize = ChaosKey->EndpointSize; > >> + > >> + Tpl = gBS->RaiseTPL (TPL_NOTIFY); > > > > You mentioned to me on IRC that this should not be necessary, and that > > UsbIo should adjust tpl as necessary. > > > > It is quite likely I misinterpreted behaviour I encountered when using > > USB passthrough in QEMU for testing (and managed to break the host > > system's driver). > > I will remove it. We are dealing with an abstract protocol here, not > hardware, and if completing a single bulk transfer does not tolerate > interruptions, it should be up to the USB I/O protocol implementation > to deal with that. Yes, sounds good. > >> diff --git a/Silicon/Openmoko/ChaosKeyDxe/DriverBinding.c b/Silicon/Openmoko/ChaosKeyDxe/DriverBinding.c > >> new file mode 100644 > >> index 000000000000..a1a5a796d38d > >> --- /dev/null > >> +++ b/Silicon/Openmoko/ChaosKeyDxe/DriverBinding.c > >> @@ -0,0 +1,256 @@ > >> +/** @file > >> + Device driver for the ChaosKey hardware random number generator. > >> + > >> + Copyright (c) 2016 - 2017, Linaro Ltd. All rights reserved.
> >> + > >> + This program and the accompanying materials > >> + are licensed and made available under the terms and conditions of the BSD > >> + License which accompanies this distribution. The full text of the license may > >> + be found at http://opensource.org/licenses/bsd-license.php. > >> + > >> + THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS, > >> + WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED. > >> + > >> +**/ > >> + > >> +#include > >> + > >> +#include "ChaosKeyDriver.h" > >> + > >> +/** > >> + Tests to see if this driver supports a given controller. > >> + > >> + @param This[in] A pointer to the EFI_DRIVER_BINDING_PROTOCOL > >> + instance. > >> + @param ControllerHandle[in] The handle of the controller to test. > >> + @param RemainingDevicePath[in] The remaining device path. > >> + (Ignored - this is not a bus driver.) > >> + > >> + @retval EFI_SUCCESS The driver supports this controller. > >> + @retval EFI_ALREADY_STARTED The device specified by ControllerHandle is > >> + already being managed by the driver specified > >> + by This. > >> + @retval EFI_UNSUPPORTED The device specified by ControllerHandle is > >> + not supported by the driver specified by This. > >> + > >> +**/ > >> +EFI_STATUS > >> +EFIAPI > >> +UsbHwrngDriverBindingSupported ( > >> + IN EFI_DRIVER_BINDING_PROTOCOL *This, > >> + IN EFI_HANDLE ControllerHandle, > >> + IN EFI_DEVICE_PATH_PROTOCOL *RemainingDevicePath > >> + ) > >> +{ > >> + EFI_USB_DEVICE_DESCRIPTOR Device; > >> + EFI_USB_IO_PROTOCOL *UsbIo; > >> + EFI_STATUS Status; > >> + > >> + // > >> + // Connect to the USB stack > >> + // > >> + Status = gBS->OpenProtocol (ControllerHandle, > >> + &gEfiUsbIoProtocolGuid, > >> + (VOID **) &UsbIo, > >> + This->DriverBindingHandle, > >> + ControllerHandle, > >> + EFI_OPEN_PROTOCOL_BY_DRIVER); > >> + if (EFI_ERROR (Status)) { > >> + return Status; > >> + } > >> + > >> + // > >> + // Get the interface descriptor to check the USB class and find a transport > >> + // protocol handler. > >> + // > >> + Status = UsbIo->UsbGetDeviceDescriptor (UsbIo, &Device); > >> + if (!EFI_ERROR (Status)) { > >> + // > >> + // Validate the adapter > >> + // > >> + if ((Device.IdVendor != CHAOSKEY_VENDOR_ID) || > >> + (Device.IdProduct != CHAOSKEY_PRODUCT_ID)) { > >> + Status = EFI_UNSUPPORTED; > >> + } else { > >> + DEBUG ((DEBUG_INIT | DEBUG_INFO, > >> + "ChaosKey (0x%04x:0x%04x) is my homeboy!\n", > >> + Device.IdVendor, Device.IdProduct)); > > > > That was intended as a cute remark in my original blog post. Perhaps a > > more descriptive message would be better. > > "Detected ChaosKey RNG device." or such. > > > > OK, will change that. Thanks. > >> + Status = EFI_SUCCESS; > >> + } > >> + } > >> + > >> + // > >> + // Clean up. > >> + // > >> + gBS->CloseProtocol (ControllerHandle, > >> + &gEfiUsbIoProtocolGuid, > >> + This->DriverBindingHandle, > >> + ControllerHandle); > >> + > >> + return Status; > >> +} > >> + > >> + > >> +/** > >> + Starts a device controller or a bus controller. > >> + > >> + @param[in] This A pointer to the EFI_DRIVER_BINDING_PROTOCOL > >> + instance. > >> + @param[in] ControllerHandle The handle of the device to start. This > >> + handle must support a protocol interface that > >> + supplies an I/O abstraction to the driver. > >> + @param[in] RemainingDevicePath The remaining portion of the device path. > >> + (Ignored - this is not a bus driver.) > >> + > >> + @retval EFI_SUCCESS The device was started. > >> + @retval EFI_DEVICE_ERROR The device could not be started due to a > >> + device error. > >> + @retval EFI_OUT_OF_RESOURCES The request could not be completed due to a > >> + lack of resources. > >> + > >> +**/ > >> +EFI_STATUS > >> +EFIAPI > >> +UsbHwrngDriverBindingStart ( > >> + IN EFI_DRIVER_BINDING_PROTOCOL *This, > >> + IN EFI_HANDLE ControllerHandle, > >> + IN EFI_DEVICE_PATH_PROTOCOL *RemainingDevicePath OPTIONAL > >> + ) > >> +{ > >> + return ChaosKeyInit (This->DriverBindingHandle, ControllerHandle); > >> +} > >> + > >> + > >> +/** > >> + Stops a device controller or a bus controller. > >> + > >> + @param[in] This A pointer to the EFI_DRIVER_BINDING_PROTOCOL > >> + instance. > >> + @param[in] ControllerHandle A handle to the device being stopped. The handle > >> + must support a bus specific I/O protocol for the > >> + driver to use to stop the device. > >> + @param[in] NumberOfChildren The number of child device handles in > >> + ChildHandleBuffer. > >> + @param[in] ChildHandleBuffer An array of child handles to be freed. May be > >> + NULL if NumberOfChildren is 0. > >> + > >> + @retval EFI_SUCCESS The device was stopped. > >> + @retval EFI_DEVICE_ERROR The device could not be stopped due to a device > >> + error. > >> + > >> +**/ > >> +EFI_STATUS > >> +EFIAPI > >> +UsbHwrngDriverBindingStop ( > >> + IN EFI_DRIVER_BINDING_PROTOCOL *This, > >> + IN EFI_HANDLE ControllerHandle, > >> + IN UINTN NumberOfChildren, > >> + IN EFI_HANDLE *ChildHandleBuffer OPTIONAL > >> + ) > >> +{ > >> + return ChaosKeyRelease (This->DriverBindingHandle, ControllerHandle); > >> +} > >> + > >> + > >> +STATIC > >> +EFI_DRIVER_BINDING_PROTOCOL gUsbDriverBinding = { > >> + UsbHwrngDriverBindingSupported, > >> + UsbHwrngDriverBindingStart, > >> + UsbHwrngDriverBindingStop, > >> + 0xa, > >> + NULL, > >> + NULL > >> +}; > >> + > >> + > >> +/** > >> + The entry point of ChaosKey UEFI Driver. > >> + > >> + @param ImageHandle The image handle of the UEFI Driver. > >> + @param SystemTable A pointer to the EFI System Table. > >> + > >> + @retval EFI_SUCCESS The Driver or UEFI Driver exited normally. > >> + @retval EFI_INCOMPATIBLE_VERSION _gUefiDriverRevision is greater than > >> + SystemTable->Hdr.Revision. > >> + > >> +**/ > >> +EFI_STATUS > >> +EFIAPI > >> +EntryPoint ( > >> + IN EFI_HANDLE ImageHandle, > >> + IN EFI_SYSTEM_TABLE *SystemTable > >> + ) > >> +{ > >> + EFI_STATUS Status; > >> + > >> + // > >> + // Add the driver to the list of drivers > >> + // > >> + Status = EfiLibInstallDriverBindingComponentName2 ( > >> + ImageHandle, SystemTable, &gUsbDriverBinding, ImageHandle, > >> + &gChaosKeyDriverComponentName, &gChaosKeyDriverComponentName2); > > > > Random question ... do we _want_ to provide > > EFI_COMPONENT_NAME_PROTOCOL, in addition to > > EFI_COMPONENT_NAME2_PROTOCOL, for new drivers? > > > > If this is supposed to be a UEFI Driver Model driver, I'd suggest we > keep it for compatibility with older firmwares. I am not strongly opposed to it, but I think the following statement from UEFI 2.7 suggests that it may be worth starting to consider leaving it out of new drivers: "To provide backwards compatibility with preexisting EFI 1.10 drivers, a UEFI platforms may support deprecated protocols which represent languages in the ISO 639-2 format. This includes the following protocols: UNICODE_COLLATION_INTERFACE, EFI_DRIVER_CONFIGURATION_PROTOCOL, EFI_DRIVER_DIAGNOSTICS_PROTOCOL, and EFI_COMPONENT_NAME_PROTOCOL." The protocol is mentioned in only one other location in the specification. > > I should also point out that I don't actually see a DRIVER NAME when > > executing the "drivers" command from UEFI Shell. > > That worked fine for me, both for 'drivers' and 'devices'. Which > platform? Does it display other driver names correctly? *cough* No. Platform/ARM/VExpressPkg/ArmVExpress.dsc.inc: gEfiMdePkgTokenSpaceGuid.PcdComponentNameDisable|TRUE Platform/ARM/VExpressPkg/ArmVExpress.dsc.inc: gEfiMdePkgTokenSpaceGuid.PcdComponentName2Disable|TRUE And this has been cargo-culted across to other platforms. Or, well, the FAT driver does show up (how!?). Ah, FatBinPkg. Ok, the world makes sense again, I will delete these illogical overrides on all ARM platforms in edk2-platforms. And probably from BeagleBoardPkh as well - I guess this is where this originated, together with Ebl. In an effort to reduce image size. Since BeagleBoardPkg now includes UefiShell, I don't think we need to worry about that as much. It is time for me to start deduplicating the common bits of platform configuration, originally prototyped by Ryan a couple of years back, in order to avoid this kind of spread in future. / Leif