On 02/19/16 19:32, Ard Biesheuvel wrote: > This implements a UEFI driver model driver for Virtio devices of type > VIRTIO_SUBSYSTEM_ENTROPY_SOURCE, and exposes them via instances of > the EFI_RNG_PROTOCOL protocol, supporting the EFI_RNG_ALGORITHM_RAW > algorithm only. > > Cc: Jordan Justen <[email protected]> > Cc: Laszlo Ersek <[email protected]> > Contributed-under: TianoCore Contribution Agreement 1.0 > Signed-off-by: Ard Biesheuvel <[email protected]> > --- > OvmfPkg/VirtioRngDxe/VirtioRng.c | 607 ++++++++++++++++++++ > OvmfPkg/VirtioRngDxe/VirtioRng.h | 32 ++ > OvmfPkg/VirtioRngDxe/VirtioRngDxe.inf | 45 ++ > 3 files changed, 684 insertions(+)
The "OvmfPkg/VirtioRngDxe/VirtioRngDxe.inf" file is okay; snipped. Main source file: > diff --git a/OvmfPkg/VirtioRngDxe/VirtioRng.c > b/OvmfPkg/VirtioRngDxe/VirtioRng.c > new file mode 100644 > index 000000000000..5a3605b661dc > --- /dev/null > +++ b/OvmfPkg/VirtioRngDxe/VirtioRng.c > @@ -0,0 +1,607 @@ > +/** @file > + > + This driver produces EFI_RNG_PROTOCOL instances for virtio-rng devices. > + > + The implementation is based on OvmfPkg/VirtioScsiDxe/VirtioScsi.c > + > + Copyright (C) 2012, Red Hat, Inc. > + Copyright (c) 2012 - 2014, Intel Corporation. All rights reserved.<BR> > + > + This driver: > + > + Copyright (C) 2016, Linaro Ltd. > + > + 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 <Library/BaseMemoryLib.h> > +#include <Library/DebugLib.h> > +#include <Library/MemoryAllocationLib.h> > +#include <Library/UefiBootServicesTableLib.h> > +#include <Library/UefiLib.h> > +#include <Library/VirtioLib.h> > + > +#include "VirtioRng.h" > + > +/** > + Returns information about the random number generation implementation. > + > + @param[in] This A pointer to the EFI_RNG_PROTOCOL > instance. > + @param[in,out] RNGAlgorithmListSize On input, the size in bytes of > RNGAlgorithmList. > + On output with a return code of > EFI_SUCCESS, the size > + in bytes of the data returned in > RNGAlgorithmList. On output > + with a return code of > EFI_BUFFER_TOO_SMALL, > + the size of RNGAlgorithmList required > to obtain the list. > + @param[out] RNGAlgorithmList 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. > + > +**/ This comment block is likely copied from EFI_RNG_GET_INFO, in "MdePkg/Include/Protocol/Rng.h". I usually insist on new files under OvmfPkg being no wider than 79 characters, so in all such cases I rewrap these comment blocks. I can do this too should you be annoyed by it. > +STATIC > +EFI_STATUS > +EFIAPI > +VirtioRngGetInfo ( > + IN EFI_RNG_PROTOCOL *This, > + IN OUT UINTN *RNGAlgorithmListSize, > + OUT EFI_RNG_ALGORITHM *RNGAlgorithmList > + ) > +{ > + if (This == NULL || RNGAlgorithmListSize == NULL) { > + return EFI_INVALID_PARAMETER; > + } > + > + if (*RNGAlgorithmListSize < sizeof (EFI_RNG_ALGORITHM)) { > + *RNGAlgorithmListSize = sizeof (EFI_RNG_ALGORITHM); > + return EFI_BUFFER_TOO_SMALL; > + } > + > + if (RNGAlgorithmList != NULL) { > + return EFI_INVALID_PARAMETER; > + } I don't understand this. I think the condition should be negated. > + > + *RNGAlgorithmListSize = sizeof (EFI_RNG_ALGORITHM); > + CopyGuid (RNGAlgorithmList, &gEfiRngAlgorithmRaw); > + > + 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] RNGAlgorithm 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] RNGValueLength The length in bytes of the memory > buffer pointed to by > + RNGValue. The driver shall return > exactly this numbers of bytes. > + @param[out] RNGValue 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. > + > +**/ Same comment about wrapping applies. (So does the same offer :)) > +STATIC > +EFI_STATUS > +EFIAPI > +VirtioRngGetRNG ( > + IN EFI_RNG_PROTOCOL *This, > + IN EFI_RNG_ALGORITHM *RNGAlgorithm, OPTIONAL > + IN UINTN RNGValueLength, > + OUT UINT8 *RNGValue > + ) > +{ > + VIRTIO_RNG_DEV *Dev; > + DESC_INDICES Indices; > + volatile UINT8 *Buffer; > + UINTN Index; > + UINT32 Len; > + > + if (This == NULL || RNGValueLength == 0 || RNGValue == NULL) { > + return EFI_INVALID_PARAMETER; > + } In order to catch pathologic calls, please also check if (RNGValueLength > MAX_UINT32). Namely, in this case, the requested length cannot be represented in the virtio descriptor below -- the BufferSize parameter of VirtioAppendDesc() is UINT32. If the check fails, please return EFI_DEVICE_ERROR. > + > + // > + // We only support the raw algorithm, so reject requests for anything else > + // > + if (RNGAlgorithm && !CompareGuid (RNGAlgorithm, &gEfiRngAlgorithmRaw)) { > + return EFI_UNSUPPORTED; > + } Please rewrite the first subcondition as (RNGAlgorithm != NULL) -- quite verbose, but that's the edk2 coding style (or common practice at least). > + > + Buffer = (volatile UINT8 *)AllocatePool (RNGValueLength); > + if (Buffer == NULL) { > + return EFI_DEVICE_ERROR; > + } (It crossed my mind that this allocation could be large, and maybe we should use a fixed size bounce buffer instead, but then I decided it would be needless complication. This function will mostly be called for seeding other algorithms.) > + > + Dev = VIRTIO_ENTROPY_SOURCE_FROM_RNG (This); > + > + // > + // The Virtio RNG device may return less data than we asked it to. > + // So loop as long as needed to get all the entropy we were asked for > + // > + for (Index = 0; Index < RNGValueLength; Index += Len) { > + Superfluous empty line? > + VirtioPrepare (&Dev->Ring, &Indices); > + VirtioAppendDesc (&Dev->Ring, > + (UINTN) Buffer + Index, Please drop the space from before Buffer, as in: (UINTN)Buffer + Index > + RNGValueLength - Index, > + VRING_DESC_F_WRITE, > + &Indices); > + > + if (VirtioFlush (Dev->VirtIo, 0, &Dev->Ring, &Indices, &Len) != > EFI_SUCCESS) { Line too long. (VirtioRng.h and VirtioRngDxe.inf have good width -- 79 chars --, but this file has several overlong lines.) > + goto FreeBuffer; > + } I propose to add two ASSERT()s here -- the first is guaranteed by the virtio spec -- 5.4.6.2 --, while the second is just sanity: ASSERT (Len > 0); ASSERT (Len <= RNGValueLength - Index); > + } > + > + for (Index = 0; Index < RNGValueLength; Index++) { > + RNGValue[Index] = Buffer[Index]; > + } > + FreePool ((VOID *)Buffer); > + return EFI_SUCCESS; > + > +FreeBuffer: > + FreePool ((VOID *)Buffer); > + return EFI_DEVICE_ERROR; > +} This is correct, but maybe we can improve the style a bit: I propose to introduce an EFI_STATUS variable called Status near the top of the function, and unify the error end success paths here: FreeBuffer: FreePool ((VOID *)Buffer); return Status; Before the goto, set Status to EFI_DEVICE_ERROR; after the copy, set Status to EFI_SUCCESS. If you agree, of course. > + > +STATIC > +EFI_STATUS > +EFIAPI > +VirtioRngInit ( > + IN OUT VIRTIO_RNG_DEV *Dev > + ) > +{ > + UINT8 NextDevStat; > + EFI_STATUS Status; > + UINT16 QueueSize; > + > + // > + // Execute virtio-0.9.5, 2.2.1 Device Initialization Sequence. > + // > + NextDevStat = 0; // step 1 -- reset device > + Status = Dev->VirtIo->SetDeviceStatus (Dev->VirtIo, NextDevStat); > + if (EFI_ERROR (Status)) { > + goto Failed; > + } > + > + NextDevStat |= VSTAT_ACK; // step 2 -- acknowledge device presence > + Status = Dev->VirtIo->SetDeviceStatus (Dev->VirtIo, NextDevStat); > + if (EFI_ERROR (Status)) { > + goto Failed; > + } > + > + NextDevStat |= VSTAT_DRIVER; // step 3 -- we know how to drive it > + Status = Dev->VirtIo->SetDeviceStatus (Dev->VirtIo, NextDevStat); > + if (EFI_ERROR (Status)) { > + goto Failed; > + } > + > + // > + // Set Page Size - MMIO VirtIo Specific > + // > + Status = Dev->VirtIo->SetPageSize (Dev->VirtIo, EFI_PAGE_SIZE); > + if (EFI_ERROR (Status)) { > + goto Failed; > + } Please do not omit step 4a -- retrieve and validate features. I understand that there are no device specific feature bits defined, but the step should not be omitted nevertheless. Just call GetDeviceFeatures() with an otherwise unused Features variable, and check if GetDeviceFeatures() succeeds. (See the virtio-scsi code as an example.) > + > + // > + // allocate request virtqueue, just use #0 > + // please also mark this as step 4b > + Status = Dev->VirtIo->SetQueueSel (Dev->VirtIo, 0); > + if (EFI_ERROR (Status)) { > + goto Failed; > + } > + Status = Dev->VirtIo->GetQueueNumMax (Dev->VirtIo, &QueueSize); > + if (EFI_ERROR (Status)) { > + goto Failed; > + } > + > + // > + // Single queue required > + // The code below is correct, but the comment is wrong. This is about the number of descriptors we want to link together in the longest descriptor chain we ever submit. So the comment should say: // // VirtioRngGetRNG() uses one descriptor // > + if (QueueSize < 1) { > + Status = EFI_UNSUPPORTED; > + goto Failed; > + } > + > + Status = VirtioRingInit (QueueSize, &Dev->Ring); > + if (EFI_ERROR (Status)) { > + goto Failed; > + } > + > + // > + // Additional steps for MMIO: align the queue appropriately, and set the > + // size. If anything fails from here on, we must release the ring > resources. > + // > + Status = Dev->VirtIo->SetQueueNum (Dev->VirtIo, QueueSize); > + if (EFI_ERROR (Status)) { > + goto ReleaseQueue; > + } > + > + Status = Dev->VirtIo->SetQueueAlign (Dev->VirtIo, EFI_PAGE_SIZE); > + if (EFI_ERROR (Status)) { > + goto ReleaseQueue; > + } > + > + // > + // Report GPFN (guest-physical frame number) of queue. > + // Please also mark this as step 4c. > + Status = Dev->VirtIo->SetQueueAddress (Dev->VirtIo, > + (UINT32) ((UINTN) Dev->Ring.Base >> EFI_PAGE_SHIFT)); > + if (EFI_ERROR (Status)) { > + goto ReleaseQueue; > + } At this point, please do not omit step 5, "Report understood features and guest-tuneables". Just restore the removed chunk of code from virtio-scsi, and call SetGuestFeatures() with a Features=0 argument. (Plus check if SetGuestFeatures() succeeds, same as in the virtio-scsi code.) Namely, there are a number of device-independent feature bits, and we certainly want to clear those. (For an example, see the virtio-block driver.) > + > + // > + // initialization complete > + // Please mark this as step 6 as well. The rest of this source file looks okay; snipped. Header file: > diff --git a/OvmfPkg/VirtioRngDxe/VirtioRng.h > b/OvmfPkg/VirtioRngDxe/VirtioRng.h > new file mode 100644 > index 000000000000..38ddd667d11f > --- /dev/null > +++ b/OvmfPkg/VirtioRngDxe/VirtioRng.h > @@ -0,0 +1,32 @@ > +/** @file I'd prefer if you kept a customized copy of the blurb from the corresponding virtio-scsi header file. The main reason for my request is that @file tends to require *something*, according to the coding style. :) > + > + Copyright (C) 2016, Linaro Ltd. > + > + 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 guards would be nice, if for nothing else but consistency with the other drivers. (IIRC it was Jordan's request originally.) > +#include <Protocol/ComponentName.h> > +#include <Protocol/DriverBinding.h> > +#include <Protocol/Rng.h> > + > +#include <IndustryStandard/Virtio.h> > + > +#define VIRTIO_RNG_SIG SIGNATURE_32 ('V', 'R', 'N', 'G') Irrelevant, but I'll note it: all those spaces in the middle are not necessary. Feel free to ignore. > + > +typedef struct { > + UINT32 Signature; > + VIRTIO_DEVICE_PROTOCOL *VirtIo; > + EFI_EVENT ExitBoot; > + VRING Ring; > + EFI_RNG_PROTOCOL Rng; > +} VIRTIO_RNG_DEV; I'd prefer if you could reproduce the init function / init depth annotations here. > + > +#define VIRTIO_ENTROPY_SOURCE_FROM_RNG(RngPointer) \ > + CR (RngPointer, VIRTIO_RNG_DEV, Rng, VIRTIO_RNG_SIG) If you find these requests for the header file annoying, I'd be more than happy to add them myself. Thanks! Laszlo _______________________________________________ edk2-devel mailing list [email protected] https://lists.01.org/mailman/listinfo/edk2-devel
