[PATCH 0/2] ArmVirtPkg, OvmfPkg: connect Virtio RNG devices again

[PATCH 0/2] ArmVirtPkg, OvmfPkg: connect Virtio RNG devices again

[Laszlo Ersek]

Repo:   https://github.com/lersek/edk2.git
Branch: connect_vrng_bz1579518

This is a fix for another regression from the March 2018 series

  [edk2] [PATCH 0/6] OvmfPkg, ArmVirtQemu: leaner platform BDS policy
                     for connecting devices

Turns out EfiBootManagerConnectAll() used to connect devices one doesn't
really think about every day. I stumbled upon this when an aarch64 guest
kernel printed

> EFI stub: EFI_RNG_PROTOCOL unavailable, no randomness supplied

whereas I had just looked at the domain XML and seen the virtio-rng
device model in it.

Tested extensively as described in
<https://bugzilla.redhat.com/show_bug.cgi?id=1579518#c0> and
<https://bugzilla.redhat.com/show_bug.cgi?id=1579518#c2>.

Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Jordan Justen <jordan.l.justen@intel.com>

Thanks,
Laszlo

Laszlo Ersek (2):
  ArmVirtPkg/PlatformBootManagerLib: connect Virtio RNG devices again
  OvmfPkg/PlatformBootManagerLib: connect Virtio RNG devices again

 ArmVirtPkg/Library/PlatformBootManagerLib/PlatformBm.c               | 129 ++++++++++++++++++++
 ArmVirtPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf |   1 +
 OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.c                 | 105 ++++++++++++++++
 OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.h                 |   1 +
 4 files changed, 236 insertions(+)

-- 
2.14.1.3.gb7cf6e02401b

[PATCH 1/2] ArmVirtPkg/PlatformBootManagerLib: connect Virtio RNG devices again

[Laszlo Ersek]

Virtio RNG devices are never boot devices, so in commit ff1d0fbfbaec we
stopped connecting them. This is a problem because an OS boot loader may
depend on EFI_RNG_PROTOCOL to seed the OS's RNG.

Connect Virtio RNG devices again. And, while commit ff1d0fbfbaec removed
that from PlatformBootManagerAfterConsole(), reintroduce it now to
PlatformBootManagerBeforeConsole() -- this way Driver#### options launched
between both functions may access EFI_RNG_PROTOCOL too.

Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Fixes: ff1d0fbfbaec55038ccf888759588fa4e21516f4
Ref: https://bugzilla.redhat.com/show_bug.cgi?id=1579518
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
 ArmVirtPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf |   1 +
 ArmVirtPkg/Library/PlatformBootManagerLib/PlatformBm.c               | 129 ++++++++++++++++++++
 2 files changed, 130 insertions(+)

diff --git a/ArmVirtPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf b/ArmVirtPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf
index 1e22f8bb38ef..d6c1ef95dc44 100644
--- a/ArmVirtPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf
+++ b/ArmVirtPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf
@@ -83,3 +83,4 @@ [Protocols]
   gEfiLoadedImageProtocolGuid
   gEfiPciRootBridgeIoProtocolGuid
   gEfiSimpleFileSystemProtocolGuid
+  gVirtioDeviceProtocolGuid
diff --git a/ArmVirtPkg/Library/PlatformBootManagerLib/PlatformBm.c b/ArmVirtPkg/Library/PlatformBootManagerLib/PlatformBm.c
index 5d5e51d8c870..62cce6a01e1a 100644
--- a/ArmVirtPkg/Library/PlatformBootManagerLib/PlatformBm.c
+++ b/ArmVirtPkg/Library/PlatformBootManagerLib/PlatformBm.c
@@ -16,6 +16,7 @@
 **/
 
 #include <IndustryStandard/Pci22.h>
+#include <IndustryStandard/Virtio095.h>
 #include <Library/BootLogoLib.h>
 #include <Library/DevicePathLib.h>
 #include <Library/PcdLib.h>
@@ -27,6 +28,7 @@
 #include <Protocol/LoadedImage.h>
 #include <Protocol/PciIo.h>
 #include <Protocol/PciRootBridgeIo.h>
+#include <Protocol/VirtioDevice.h>
 #include <Guid/EventGroup.h>
 #include <Guid/RootBridgesConnectedEventGroup.h>
 
@@ -260,6 +262,121 @@ IsPciDisplay (
 }
 
 
+/**
+  This FILTER_FUNCTION checks if a handle corresponds to a Virtio RNG device at
+  the VIRTIO_DEVICE_PROTOCOL level.
+**/
+STATIC
+BOOLEAN
+EFIAPI
+IsVirtioRng (
+  IN EFI_HANDLE   Handle,
+  IN CONST CHAR16 *ReportText
+  )
+{
+  EFI_STATUS             Status;
+  VIRTIO_DEVICE_PROTOCOL *VirtIo;
+
+  Status = gBS->HandleProtocol (Handle, &gVirtioDeviceProtocolGuid,
+                  (VOID**)&VirtIo);
+  if (EFI_ERROR (Status)) {
+    return FALSE;
+  }
+  return (BOOLEAN)(VirtIo->SubSystemDeviceId ==
+                   VIRTIO_SUBSYSTEM_ENTROPY_SOURCE);
+}
+
+
+/**
+  This FILTER_FUNCTION checks if a handle corresponds to a Virtio RNG device at
+  the EFI_PCI_IO_PROTOCOL level.
+**/
+STATIC
+BOOLEAN
+EFIAPI
+IsVirtioPciRng (
+  IN EFI_HANDLE   Handle,
+  IN CONST CHAR16 *ReportText
+  )
+{
+  EFI_STATUS          Status;
+  EFI_PCI_IO_PROTOCOL *PciIo;
+  UINT16              VendorId;
+  UINT16              DeviceId;
+  UINT8               RevisionId;
+  BOOLEAN             Virtio10;
+  UINT16              SubsystemId;
+
+  Status = gBS->HandleProtocol (Handle, &gEfiPciIoProtocolGuid,
+                  (VOID**)&PciIo);
+  if (EFI_ERROR (Status)) {
+    return FALSE;
+  }
+
+  //
+  // Read and check VendorId.
+  //
+  Status = PciIo->Pci.Read (PciIo, EfiPciIoWidthUint16, PCI_VENDOR_ID_OFFSET,
+                        1, &VendorId);
+  if (EFI_ERROR (Status)) {
+    goto PciError;
+  }
+  if (VendorId != VIRTIO_VENDOR_ID) {
+    return FALSE;
+  }
+
+  //
+  // Read DeviceId and RevisionId.
+  //
+  Status = PciIo->Pci.Read (PciIo, EfiPciIoWidthUint16, PCI_DEVICE_ID_OFFSET,
+                        1, &DeviceId);
+  if (EFI_ERROR (Status)) {
+    goto PciError;
+  }
+  Status = PciIo->Pci.Read (PciIo, EfiPciIoWidthUint8, PCI_REVISION_ID_OFFSET,
+                        1, &RevisionId);
+  if (EFI_ERROR (Status)) {
+    goto PciError;
+  }
+
+  //
+  // From DeviceId and RevisionId, determine whether the device is a
+  // modern-only Virtio 1.0 device. In case of Virtio 1.0, DeviceId can
+  // immediately be restricted to VIRTIO_SUBSYSTEM_ENTROPY_SOURCE, and
+  // SubsystemId will only play a sanity-check role. Otherwise, DeviceId can
+  // only be sanity-checked, and SubsystemId will decide.
+  //
+  if (DeviceId == 0x1040 + VIRTIO_SUBSYSTEM_ENTROPY_SOURCE &&
+      RevisionId >= 0x01) {
+    Virtio10 = TRUE;
+  } else if (DeviceId >= 0x1000 && DeviceId <= 0x103F && RevisionId == 0x00) {
+    Virtio10 = FALSE;
+  } else {
+    return FALSE;
+  }
+
+  //
+  // Read and check SubsystemId as dictated by Virtio10.
+  //
+  Status = PciIo->Pci.Read (PciIo, EfiPciIoWidthUint16,
+                        PCI_SUBSYSTEM_ID_OFFSET, 1, &SubsystemId);
+  if (EFI_ERROR (Status)) {
+    goto PciError;
+  }
+  if (Virtio10 && SubsystemId >= 0x40) {
+    return TRUE;
+  }
+  if (!Virtio10 && SubsystemId == VIRTIO_SUBSYSTEM_ENTROPY_SOURCE) {
+    return TRUE;
+  }
+  return FALSE;
+
+PciError:
+  DEBUG ((DEBUG_ERROR, "%a: %s: %r\n", __FUNCTION__, ReportText, Status));
+  return FALSE;
+}
+
+
 /**
   This CALLBACK_FUNCTION attempts to connect a handle non-recursively, asking
   the matching driver to produce all first-level child handles.
@@ -644,6 +761,18 @@ PlatformBootManagerBeforeConsole (
   // Register platform-specific boot options and keyboard shortcuts.
   //
   PlatformRegisterOptionsAndKeys ();
+
+  //
+  // At this point, VIRTIO_DEVICE_PROTOCOL instances exist only for Virtio MMIO
+  // transports. Install EFI_RNG_PROTOCOL instances on Virtio MMIO RNG devices.
+  //
+  FilterAndProcess (&gVirtioDeviceProtocolGuid, IsVirtioRng, Connect);
+
+  //
+  // Install both VIRTIO_DEVICE_PROTOCOL and (dependent) EFI_RNG_PROTOCOL
+  // instances on Virtio PCI RNG devices.
+  //
+  FilterAndProcess (&gEfiPciIoProtocolGuid, IsVirtioPciRng, Connect);
 }
 
 /**
-- 
2.14.1.3.gb7cf6e02401b

[PATCH 2/2] OvmfPkg/PlatformBootManagerLib: connect Virtio RNG devices again

[Laszlo Ersek]

Virtio RNG devices are never boot devices, so in commit 245c643cc8b7 we
stopped connecting them. This is a problem because an OS boot loader may
depend on EFI_RNG_PROTOCOL to seed the OS's RNG.

Connect Virtio RNG devices again. And, while commit 245c643cc8b7 removed
that from PlatformBootManagerAfterConsole(), reintroduce it now to
PlatformBootManagerBeforeConsole() -- this way Driver#### options launched
between both functions may access EFI_RNG_PROTOCOL too.

Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Fixes: 245c643cc8b73240c3b88cb55b2911b285a8c10d
Ref: https://bugzilla.redhat.com/show_bug.cgi?id=1579518
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
 OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.h |   1 +
 OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.c | 105 ++++++++++++++++++++
 2 files changed, 106 insertions(+)

diff --git a/OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.h b/OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.h
index 97ffbb514825..4948ca6518ec 100644
--- a/OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.h
+++ b/OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.h
@@ -30,6 +30,7 @@ Abstract:
 #include <IndustryStandard/Acpi.h>
 #include <IndustryStandard/SmBios.h>
 #include <IndustryStandard/PeImage.h>
+#include <IndustryStandard/Virtio095.h>
 
 #include <Library/DebugLib.h>
 #include <Library/BaseMemoryLib.h>
diff --git a/OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.c b/OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.c
index 004b753f4d26..5d4d323d7306 100644
--- a/OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.c
+++ b/OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.c
@@ -318,6 +318,15 @@ ConnectRootBridge (
   IN VOID        *Context
   );
 
+STATIC
+EFI_STATUS
+EFIAPI
+ConnectVirtioPciRng (
+  IN EFI_HANDLE Handle,
+  IN VOID       *Instance,
+  IN VOID       *Context
+  );
+
 STATIC
 VOID
 SaveS3BootScript (
@@ -399,6 +408,13 @@ PlatformBootManagerBeforeConsole (
   ASSERT_RETURN_ERROR (PcdStatus);
 
   PlatformRegisterOptionsAndKeys ();
+
+  //
+  // Install both VIRTIO_DEVICE_PROTOCOL and (dependent) EFI_RNG_PROTOCOL
+  // instances on Virtio PCI RNG devices.
+  //
+  VisitAllInstancesOfProtocol (&gEfiPciIoProtocolGuid, ConnectVirtioPciRng,
+    NULL);
 }
 
 
@@ -427,6 +443,95 @@ ConnectRootBridge (
 }
 
 
+STATIC
+EFI_STATUS
+EFIAPI
+ConnectVirtioPciRng (
+  IN EFI_HANDLE Handle,
+  IN VOID       *Instance,
+  IN VOID       *Context
+  )
+{
+  EFI_PCI_IO_PROTOCOL *PciIo;
+  EFI_STATUS          Status;
+  UINT16              VendorId;
+  UINT16              DeviceId;
+  UINT8               RevisionId;
+  BOOLEAN             Virtio10;
+  UINT16              SubsystemId;
+
+  PciIo = Instance;
+
+  //
+  // Read and check VendorId.
+  //
+  Status = PciIo->Pci.Read (PciIo, EfiPciIoWidthUint16, PCI_VENDOR_ID_OFFSET,
+                        1, &VendorId);
+  if (EFI_ERROR (Status)) {
+    goto Error;
+  }
+  if (VendorId != VIRTIO_VENDOR_ID) {
+    return EFI_SUCCESS;
+  }
+
+  //
+  // Read DeviceId and RevisionId.
+  //
+  Status = PciIo->Pci.Read (PciIo, EfiPciIoWidthUint16, PCI_DEVICE_ID_OFFSET,
+                        1, &DeviceId);
+  if (EFI_ERROR (Status)) {
+    goto Error;
+  }
+  Status = PciIo->Pci.Read (PciIo, EfiPciIoWidthUint8, PCI_REVISION_ID_OFFSET,
+                        1, &RevisionId);
+  if (EFI_ERROR (Status)) {
+    goto Error;
+  }
+
+  //
+  // From DeviceId and RevisionId, determine whether the device is a
+  // modern-only Virtio 1.0 device. In case of Virtio 1.0, DeviceId can
+  // immediately be restricted to VIRTIO_SUBSYSTEM_ENTROPY_SOURCE, and
+  // SubsystemId will only play a sanity-check role. Otherwise, DeviceId can
+  // only be sanity-checked, and SubsystemId will decide.
+  //
+  if (DeviceId == 0x1040 + VIRTIO_SUBSYSTEM_ENTROPY_SOURCE &&
+      RevisionId >= 0x01) {
+    Virtio10 = TRUE;
+  } else if (DeviceId >= 0x1000 && DeviceId <= 0x103F && RevisionId == 0x00) {
+    Virtio10 = FALSE;
+  } else {
+    return EFI_SUCCESS;
+  }
+
+  //
+  // Read and check SubsystemId as dictated by Virtio10.
+  //
+  Status = PciIo->Pci.Read (PciIo, EfiPciIoWidthUint16,
+                        PCI_SUBSYSTEM_ID_OFFSET, 1, &SubsystemId);
+  if (EFI_ERROR (Status)) {
+    goto Error;
+  }
+  if ((Virtio10 && SubsystemId >= 0x40) ||
+      (!Virtio10 && SubsystemId == VIRTIO_SUBSYSTEM_ENTROPY_SOURCE)) {
+    Status = gBS->ConnectController (
+                    Handle, // ControllerHandle
+                    NULL,   // DriverImageHandle -- connect all drivers
+                    NULL,   // RemainingDevicePath -- produce all child handles
+                    FALSE   // Recursive -- don't follow child handles
+                    );
+    if (EFI_ERROR (Status)) {
+      goto Error;
+    }
+  }
+  return EFI_SUCCESS;
+
+Error:
+  DEBUG ((DEBUG_ERROR, "%a: %r\n", __FUNCTION__, Status));
+  return Status;
+}
+
+
 /**
   Add IsaKeyboard to ConIn; add IsaSerial to ConOut, ConIn, ErrOut.
 
-- 
2.14.1.3.gb7cf6e02401b

Re: [PATCH 0/2] ArmVirtPkg, OvmfPkg: connect Virtio RNG devices again

[Ard Biesheuvel]

On 18 May 2018 at 08:20, Laszlo Ersek <lersek@redhat.com> wrote:
> Repo:   https://github.com/lersek/edk2.git
> Branch: connect_vrng_bz1579518
>
> This is a fix for another regression from the March 2018 series
>
>   [edk2] [PATCH 0/6] OvmfPkg, ArmVirtQemu: leaner platform BDS policy
>                      for connecting devices
>
> Turns out EfiBootManagerConnectAll() used to connect devices one doesn't
> really think about every day. I stumbled upon this when an aarch64 guest
> kernel printed
>
>> EFI stub: EFI_RNG_PROTOCOL unavailable, no randomness supplied
>
> whereas I had just looked at the domain XML and seen the virtio-rng
> device model in it.
>
> Tested extensively as described in
> <https://bugzilla.redhat.com/show_bug.cgi?id=1579518#c0> and
> <https://bugzilla.redhat.com/show_bug.cgi?id=1579518#c2>.
>
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Cc: Jordan Justen <jordan.l.justen@intel.com>
>
> Thanks,
> Laszlo
>
> Laszlo Ersek (2):
>   ArmVirtPkg/PlatformBootManagerLib: connect Virtio RNG devices again
>   OvmfPkg/PlatformBootManagerLib: connect Virtio RNG devices again
>

Thanks for spotting that

Reviewed-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

Re: [PATCH 0/2] ArmVirtPkg, OvmfPkg: connect Virtio RNG devices again

[Laszlo Ersek]

On 05/18/18 11:09, Ard Biesheuvel wrote:
> On 18 May 2018 at 08:20, Laszlo Ersek <lersek@redhat.com> wrote:
>> Repo:   https://github.com/lersek/edk2.git
>> Branch: connect_vrng_bz1579518
>>
>> This is a fix for another regression from the March 2018 series
>>
>>   [edk2] [PATCH 0/6] OvmfPkg, ArmVirtQemu: leaner platform BDS policy
>>                      for connecting devices
>>
>> Turns out EfiBootManagerConnectAll() used to connect devices one doesn't
>> really think about every day. I stumbled upon this when an aarch64 guest
>> kernel printed
>>
>>> EFI stub: EFI_RNG_PROTOCOL unavailable, no randomness supplied
>>
>> whereas I had just looked at the domain XML and seen the virtio-rng
>> device model in it.
>>
>> Tested extensively as described in
>> <https://bugzilla.redhat.com/show_bug.cgi?id=1579518#c0> and
>> <https://bugzilla.redhat.com/show_bug.cgi?id=1579518#c2>.
>>
>> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> Cc: Jordan Justen <jordan.l.justen@intel.com>
>>
>> Thanks,
>> Laszlo
>>
>> Laszlo Ersek (2):
>>   ArmVirtPkg/PlatformBootManagerLib: connect Virtio RNG devices again
>>   OvmfPkg/PlatformBootManagerLib: connect Virtio RNG devices again
>>
> 
> Thanks for spotting that
> 
> Reviewed-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

Thank you! Commit range 333f32ec23dd..7ebad830d6ab.

Cheers
Laszlo