comments related to the UEFI driver model at the bottom
On 11/04/13 07:21, Chris Ruffin wrote:
>>From c799ca97b4dfb8e8c39ee93e72be92a8d1b693ff Mon Sep 17 00:00:00 2001
> From: Chris Ruffin <[email protected]>
> Date: Sun, 3 Nov 2013 21:40:34 -0500
> Subject: [PATCH] OvmfPkg/QemuVideoDxe: use child handle to open parent
> protocols
>
> Reorder when the child handle is created so that a parent / child
> relationship is established when PciIo protocol is opened.
> This resolves some issues with code which depends on the parent/child
> relationship to locate the GOP protocol.
>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Chris Ruffin <[email protected])
> ---
> OvmfPkg/QemuVideoDxe/Driver.c | 112
> +++++++++++++++++++++---------------------
> 1 file changed, 56 insertions(+), 56 deletions(-)
>
> diff --git a/OvmfPkg/QemuVideoDxe/Driver.c b/OvmfPkg/QemuVideoDxe/Driver.c
> index 1dd8899..d8b9498 100644
> --- a/OvmfPkg/QemuVideoDxe/Driver.c
> +++ b/OvmfPkg/QemuVideoDxe/Driver.c
> @@ -2,7 +2,7 @@
> This driver is a sample implementation of the Graphics Output Protocol for
> the QEMU (Cirrus Logic 5446) video controller.
>
> - Copyright (c) 2006 - 2010, Intel Corporation. All rights reserved.<BR>
> + Copyright (c) 2006 - 2013, Intel Corporation. All rights reserved.<BR>
>
> This program and the accompanying materials
> are licensed and made available under the terms and conditions of
> the BSD License
> @@ -227,6 +227,59 @@ QemuVideoControllerDriverStart (
> Private->Handle = NULL;
>
> //
> + // Get ParentDevicePath
> + //
> + Status = gBS->HandleProtocol (
> + Controller,
> + &gEfiDevicePathProtocolGuid,
> + (VOID **) &ParentDevicePath
> + );
> + if (EFI_ERROR (Status)) {
> + goto Error;
> + }
> +
> + //
> + // Set Gop Device Path
> + //
> + if (RemainingDevicePath == NULL) {
> + ZeroMem (&AcpiDeviceNode, sizeof (ACPI_ADR_DEVICE_PATH));
> + AcpiDeviceNode.Header.Type = ACPI_DEVICE_PATH;
> + AcpiDeviceNode.Header.SubType = ACPI_ADR_DP;
> + AcpiDeviceNode.ADR = ACPI_DISPLAY_ADR (1, 0, 0, 1, 0,
> ACPI_ADR_DISPLAY_TYPE_VGA, 0, 0);
> + SetDevicePathNodeLength (&AcpiDeviceNode.Header, sizeof
> (ACPI_ADR_DEVICE_PATH));
> +
> + Private->GopDevicePath = AppendDevicePathNode (
> + ParentDevicePath,
> + (EFI_DEVICE_PATH_PROTOCOL *)
> &AcpiDeviceNode
> + );
> + } else if (!IsDevicePathEnd (RemainingDevicePath)) {
> + //
> + // If RemainingDevicePath isn't the End of Device Path Node,
> + // only scan the specified device by RemainingDevicePath
> + //
> + Private->GopDevicePath = AppendDevicePathNode (ParentDevicePath,
> RemainingDevicePath);
> + } else {
> + //
> + // If RemainingDevicePath is the End of Device Path Node,
> + // don't create child device and return EFI_SUCCESS
> + //
> + Private->GopDevicePath = NULL;
> + }
> +
> + if (Private->GopDevicePath != NULL) {
> + //
> + // Creat child handle and device path protocol firstly
> + //
> + Private->Handle = NULL;
> + Status = gBS->InstallMultipleProtocolInterfaces (
> + &Private->Handle,
> + &gEfiDevicePathProtocolGuid,
> + Private->GopDevicePath,
> + NULL
> + );
> + }
> +
> + //
> // Open PCI I/O Protocol
> //
> Status = gBS->OpenProtocol (
> @@ -234,8 +287,8 @@ QemuVideoControllerDriverStart (
> &gEfiPciIoProtocolGuid,
> (VOID **) &Private->PciIo,
> This->DriverBindingHandle,
> - Controller,
> - EFI_OPEN_PROTOCOL_BY_DRIVER
> + Private->Handle,
> + EFI_OPEN_PROTOCOL_BY_CHILD_CONTROLLER
> );
> if (EFI_ERROR (Status)) {
> goto Error;
> @@ -322,59 +375,6 @@ QemuVideoControllerDriverStart (
> }
>
> //
> - // Get ParentDevicePath
> - //
> - Status = gBS->HandleProtocol (
> - Controller,
> - &gEfiDevicePathProtocolGuid,
> - (VOID **) &ParentDevicePath
> - );
> - if (EFI_ERROR (Status)) {
> - goto Error;
> - }
> -
> - //
> - // Set Gop Device Path
> - //
> - if (RemainingDevicePath == NULL) {
> - ZeroMem (&AcpiDeviceNode, sizeof (ACPI_ADR_DEVICE_PATH));
> - AcpiDeviceNode.Header.Type = ACPI_DEVICE_PATH;
> - AcpiDeviceNode.Header.SubType = ACPI_ADR_DP;
> - AcpiDeviceNode.ADR = ACPI_DISPLAY_ADR (1, 0, 0, 1, 0,
> ACPI_ADR_DISPLAY_TYPE_VGA, 0, 0);
> - SetDevicePathNodeLength (&AcpiDeviceNode.Header, sizeof
> (ACPI_ADR_DEVICE_PATH));
> -
> - Private->GopDevicePath = AppendDevicePathNode (
> - ParentDevicePath,
> - (EFI_DEVICE_PATH_PROTOCOL *)
> &AcpiDeviceNode
> - );
> - } else if (!IsDevicePathEnd (RemainingDevicePath)) {
> - //
> - // If RemainingDevicePath isn't the End of Device Path Node,
> - // only scan the specified device by RemainingDevicePath
> - //
> - Private->GopDevicePath = AppendDevicePathNode (ParentDevicePath,
> RemainingDevicePath);
> - } else {
> - //
> - // If RemainingDevicePath is the End of Device Path Node,
> - // don't create child device and return EFI_SUCCESS
> - //
> - Private->GopDevicePath = NULL;
> - }
> -
> - if (Private->GopDevicePath != NULL) {
> - //
> - // Creat child handle and device path protocol firstly
> - //
> - Private->Handle = NULL;
> - Status = gBS->InstallMultipleProtocolInterfaces (
> - &Private->Handle,
> - &gEfiDevicePathProtocolGuid,
> - Private->GopDevicePath,
> - NULL
> - );
> - }
> -
> - //
> // Construct video mode buffer
> //
> switch (Private->Variant) {
OK, so the current OVMF GOP driver seems to follow the general GOP
driver requirements set forth in:
- 23.2 Graphics Output Protocol Implementation (DWG)
- 11.10 Rules for PCI/AGP Devices (spec) -- these are BTW
mind-bogglingly complex and I only skimmed them
In QemuVideoControllerDriverStart() we
(1) open (ie. persistently reference) the parent controller (which
is the graphics card, ie. a PCI device/function pair), with
- PciIo,
- Attributes == EFI_OPEN_PROTOCOL_BY_DRIVER,
- ControllerHandle == the parent controller (the graphics card),
(2) create a child handle,
(3) install a new Device Path Protocol instance on it,
(4) install a Graphics Output Protocol instance on it.
As far as I understand, the patch effects the following (for me the
patch doesn't apply from the email so I can't really look at the
resultant code):
(1') create a child handle,
(2') install a new Device Path Protocol instance on it,
(3') open (ie. persistently reference) the parent controller (the
graphics card) with:
- PciIo,
- Attributes == EFI_OPEN_PROTOCOL_BY_CHILD_CONTROLLER,
- ControllerHandle == the child controller from step (1')
(4') install a Graphics Output Protocol instance on the child handle.
I think the change goes in the right direction, but I think it's
incomplete. I'm basing this on:
(a) the EFI_DRIVER_BINDING_PROTOCOL.Start() specification in 10.1, under
Pseudo Code
Bus Driver that is able to create all or one of its child
handles on each call to Start()
step 1
step 4, RemainingDevicePath != NULL, paragraph f
step 4, RemainingDevicePath == NULL, end of paragraph c
(b) earlier emails from Michael and Andrew (which I can't of course
locate now!)
In essence, step (3') should not *replace* original step (1), step (3')
should happen *in addition*. The patch as-is will remove the BY_DRIVER
reference to the parent controller's PciIo, and I think that's not good.
I think the patch should keep the original sequence (1) to (4), and add
the following step:
(5) same as (3')
I believe this is what I did in the virtio-net driver
(VirtioNetDriverBindingStart() in
"OvmfPkg/VirtioNetDxe/DriverBinding.c"), based on the emails I seem to
recall from Michael and Andrew. That use case seems quite symmetric,
with the MAC child node doubling for the GOP child node.
Anyway I'm not certain, it's just a thought.
Regarding your other patch ("[edk2] [PATCH] OvmfPkg/QemuVideoDxe: close
the appropriate protocol"), I agree that the current CloseProtocol() on
the error path in QemuVideoControllerDriverStart() is incorrect.
There should be two CloseProtocol() calls in total, mirroring the
BY_DRIVER reference in (1), and the (proposed) BY_CHILD_CONTROLLER
reference in (5).
Thanks,
Laszlo
------------------------------------------------------------------------------
Android is increasing in popularity, but the open development platform that
developers love is also attractive to malware creators. Download this white
paper to learn more about secure code signing practices that can help keep
Android apps secure.
http://pubads.g.doubleclick.net/gampad/clk?id=65839951&iu=/4140/ostg.clktrk
_______________________________________________
edk2-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/edk2-devel
