|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v2 27/29] ArmVirtualizationPkg: add XenIoMmioLib
comments below
On 01/26/15 20:03, Ard Biesheuvel wrote:
> This adds a XenIoMmioLib declaration and implementation that can
> be invoked to install the XENIO_PROTOCOL and a corresponding
> grant table address on a EFI handle.
>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@xxxxxxxxxx>
> ---
> ArmPlatformPkg/ArmVirtualizationPkg/ArmVirtualizationPkg.dec |
> 6 +++++
> ArmPlatformPkg/ArmVirtualizationPkg/Include/Library/XenIoMmioLib.h |
> 20 +++++++++++++++
> ArmPlatformPkg/ArmVirtualizationPkg/Library/XenIoMmioLib/XenIoMmioLib.c |
> 91 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> ArmPlatformPkg/ArmVirtualizationPkg/Library/XenIoMmioLib/XenIoMmioLib.inf |
> 38 +++++++++++++++++++++++++++++
> 4 files changed, 155 insertions(+)
(1) In order to stay "symmetric" with VirtioMmioDeviceLib, I propose to
add this library (both class & the first instance) to OvmfPkg.
> diff --git a/ArmPlatformPkg/ArmVirtualizationPkg/ArmVirtualizationPkg.dec
> b/ArmPlatformPkg/ArmVirtualizationPkg/ArmVirtualizationPkg.dec
> index 868488906643..c690f1481093 100644
> --- a/ArmPlatformPkg/ArmVirtualizationPkg/ArmVirtualizationPkg.dec
> +++ b/ArmPlatformPkg/ArmVirtualizationPkg/ArmVirtualizationPkg.dec
> @@ -30,6 +30,12 @@
> [Includes.common]
> Include # Root include for the package
>
> +[LibraryClasses]
> + #
> + # library to create handles containing the XENIO_PROTOCOL I/O protocol
> + #
> + XenIoMmioLib|Include/Library/XenIoMmioLib.h
> +
> [Guids.common]
> gArmVirtualizationTokenSpaceGuid = { 0x0B6F5CA7, 0x4F53, 0x445A, { 0xB7,
> 0x6E, 0x2E, 0x36, 0x5B, 0x80, 0x63, 0x66 } }
> gEarlyPL011BaseAddressGuid = { 0xB199DEA9, 0xFD5C, 0x4A84, { 0x80,
> 0x82, 0x2F, 0x41, 0x70, 0x78, 0x03, 0x05 } }
> diff --git
> a/ArmPlatformPkg/ArmVirtualizationPkg/Include/Library/XenIoMmioLib.h
> b/ArmPlatformPkg/ArmVirtualizationPkg/Include/Library/XenIoMmioLib.h
> new file mode 100644
> index 000000000000..faeabe5affe0
> --- /dev/null
> +++ b/ArmPlatformPkg/ArmVirtualizationPkg/Include/Library/XenIoMmioLib.h
> @@ -0,0 +1,20 @@
> +/** @file
> +* Library to install the XENIO_PROTOCOL on a handle
> +*
> +* Copyright (c) 2015, Linaro Ltd. All rights reserved.<BR>
> +*
> +* 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.
> +*
> +**/
> +
> +EFI_STATUS
> +XenIoMmioInstall (
> + IN EFI_HANDLE *Handle,
> + IN UINT64 GrantTableAddress
> + );
(2) Please add an uninstall function.
> diff --git
> a/ArmPlatformPkg/ArmVirtualizationPkg/Library/XenIoMmioLib/XenIoMmioLib.c
> b/ArmPlatformPkg/ArmVirtualizationPkg/Library/XenIoMmioLib/XenIoMmioLib.c
> new file mode 100644
> index 000000000000..2d8413638680
> --- /dev/null
> +++ b/ArmPlatformPkg/ArmVirtualizationPkg/Library/XenIoMmioLib/XenIoMmioLib.c
> @@ -0,0 +1,91 @@
> +/** @file
> +* Library to install the XENIO_PROTOCOL on a handle
(3) The XenIoMmioInstall() function does more than that, it also
installs the device path protocol. That's okay per se, but you should be
fully clear about it in the leading comment here, or in the function's
comment.
If you update this comment here, please update all of its occurrences.
> +*
> +* Copyright (c) 2015, Linaro Ltd. All rights reserved.<BR>
> +*
> +* 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/BaseLib.h>
> +#include <Library/UefiBootServicesTableLib.h>
> +#include <Library/DebugLib.h>
> +#include <Library/UefiLib.h>
> +#include <Library/BaseMemoryLib.h>
> +#include <Library/MemoryAllocationLib.h>
> +#include <Library/DevicePathLib.h>
> +#include <Library/XenIoMmioLib.h>
> +
> +#include <Protocol/XenIo.h>
> +#include <Guid/XenBusRootDevice.h>
> +
> +#pragma pack (1)
> +typedef struct {
> + VENDOR_DEVICE_PATH Vendor;
> + EFI_DEVICE_PATH_PROTOCOL End;
> +} XENBUS_ROOT_DEVICE_PATH;
> +#pragma pack ()
> +
> +EFI_STATUS
> +XenIoMmioInstall (
> + IN EFI_HANDLE *Handle,
> + IN UINT64 GrantTableAddress
> + )
> +{
> + EFI_STATUS Status;
> + XENIO_PROTOCOL *XenIo;
> + XENBUS_ROOT_DEVICE_PATH *XenBusDevicePath;
> +
> + ASSERT (Handle != NULL);
(4) This is wrong. (I'm not sure how you are using the library in the
following patches, but this in itself does look wrong.)
A handle is created in UEFI when you install the first protocol on an
initially NULL handle. Please see the documentation of
InstallProtocolInterface(). Similarly, if UninstallProtocolInterface()
removes the last protocol interface from a handle, the handle is released.
(Same applies to InstallMultipleProtocolInterfaces() and
UninstallMultipleProtocolInterfaces().)
Meaning, the prototype of your function is *almost* correct. It takes
EFI_HANDLE*, which is fine, but you should qualify it as IN OUT.
If the caller passes in a non-NULL handle, you'll just add two more
protocol interfaces, fine.
If the caller passes in a NULL handle, you'll *allocate* tha handle for
the caller, transparently in the first protocol interface installation.
Similarly, if something goes wrong in the function, then on the error
path (== destruction path) you'll uninstall the protocols you have
installed thus far. If you created the handle in the first place, then
that error handling will release it for you at once.
(Note that the uninstall functions can't null the Handle parameter in
that case -- which means that you should save a copy of the incoming
handle, and restore it at the end when something fails. If it was
non-NULL initially, nothing will change, but if it was NULL initially,
you have to reset it to NULL.)
... If you find this too complex, then you can demand a non-NULL handle
on input, but:
- not with an ASSERT() please (return EFI_INVALID_PARAMETER instead),
- remember that the caller can also only create a new handle by
installing *some* protocol interface on an initially NULL handle. Since
you're installing the device path and the xenio protocols here, I'm not
sure what's left for the caller to install *before* it calls this
function. (But, given your ASSERT(), this is a problem you must have
solved anyway.)
> +
> + XenIo = AllocateZeroPool (sizeof *XenIo);
> + ASSERT (XenIo != NULL);
(5) Please return EFI_OUT_OF_RESOURCES instead.
> + XenIo->GrantTableAddress = GrantTableAddress;
> +
> + XenBusDevicePath = (XENBUS_ROOT_DEVICE_PATH *)CreateDeviceNode (
> + HARDWARE_DEVICE_PATH,
> + HW_VENDOR_DP,
> + sizeof (XENBUS_ROOT_DEVICE_PATH));
(6) I dislike this. The indentation is incorrect, but that's a minor nit.
The problem is a semantic one. CreateDeviceNode() is supposed to create
a *single* device path node. It allows you to set the size because you
might want to pack untold horrors later into that *one* node.
(Especially a vendor hw node.)
I can see that later on you fix up the size of the first node, but it's
misleading and ugly.
I recommend to allocate the full device path structure with
AllocatePool, and fill it in field-wise.
Another frequent approach is to create an initialized template (of
static storage duration) of the XENBUS_ROOT_DEVICE_PATH structure, and
use AllocateCopyPool() here.
... Anyway I realize this follows existing code in
"ArmPlatformPkg/ArmVirtualizationPkg/VirtFdtDxe/VirtFdtDxe.c". If I
didn't complain then, I shouldn't now. :) I'll leave it to you, it's not
a bug after all.
> + if (XenBusDevicePath == NULL) {
> + DEBUG ((EFI_D_ERROR, "%a: Out of memory\n", __FUNCTION__));
> + return EFI_OUT_OF_RESOURCES;
> + }
(7) This will leak XenIo.
Please employ the cascade style error handling, with the ladder of goto
labels. There's really no way around it.
Not when I'm your reviewer, anyway. ;)
> +
> + CopyMem (&XenBusDevicePath->Vendor.Guid, &gXenBusRootDeviceGuid,
> + sizeof (EFI_GUID));
Side remark: CopyGuid() is preferred. (Yes, yes,
"ArmPlatformPkg/ArmVirtualizationPkg/VirtFdtDxe/VirtFdtDxe.c" says
CopyMem too. :))
> + SetDevicePathNodeLength (&XenBusDevicePath->Vendor,
> + sizeof (*XenBusDevicePath) - sizeof (XenBusDevicePath->End));
Brrrrr. :) See (6). But, again, not strictly a bug.
> + SetDevicePathEndNode (&XenBusDevicePath->End);
> +
> + Status = gBS->InstallProtocolInterface (Handle,
> + &gEfiDevicePathProtocolGuid, EFI_NATIVE_INTERFACE,
> + XenBusDevicePath);
(8) Not a bug in itself, just a tip: since you are installing two
protocol interfaces, please call InstallMultipleProtocolInterfaces()
instead. That function is preferable because:
- It saves you some effort with error handling. Either both protocol
interfaces will be installed, or none.
- This function internally enforces the unicity of device paths in the
system (so that every handle remains uniquely addressable). Now, because
you use an invariable device path, the second call to your function will
be guaranteed to fail.
> + if (EFI_ERROR (Status)) {
> + DEBUG ((EFI_D_ERROR, "%a: Failed to install the EFI_DEVICE_PATH "
> + "protocol on handle 0x%p (Status == %r)\n",
I prefer not to use 0x before %p, but it's your call.
> + __FUNCTION__, *Handle, Status));
> + FreePool (XenBusDevicePath);
> + return Status;
> + }
Leaks XenIo. (I'm not adding a new numbered point for this, see (7) --
please just cover everything with the cascading error handling.)
> +
> + Status = gBS->InstallProtocolInterface (Handle,
> + &gXenIoProtocolGuid, EFI_NATIVE_INTERFACE,
> + XenIo);
> + if (EFI_ERROR (Status)) {
> + DEBUG ((EFI_D_ERROR, "%a: Failed to install XENIO_PROTOCOL on handle
> 0x%p "
> + "(Status == %r)\n", __FUNCTION__, *Handle, Status));
(0x%p)
> +
> + Status = gBS->UninstallProtocolInterface (*Handle,
> + &gEfiDevicePathProtocolGuid, XenBusDevicePath);
> + ASSERT_EFI_ERROR (Status);
> + FreePool (XenBusDevicePath);
> + FreePool (XenIo);
> + }
Right, this is why this style of free-on-error is not recommended. When
you remember to free everything (as you do here), it doesn't scale (ever
growing error handling blocks). And, it's easy to forget stuff to free.
> + return Status;
> +}
> diff --git
> a/ArmPlatformPkg/ArmVirtualizationPkg/Library/XenIoMmioLib/XenIoMmioLib.inf
> b/ArmPlatformPkg/ArmVirtualizationPkg/Library/XenIoMmioLib/XenIoMmioLib.inf
> new file mode 100644
> index 000000000000..14f24ff7fd2c
> --- /dev/null
> +++
> b/ArmPlatformPkg/ArmVirtualizationPkg/Library/XenIoMmioLib/XenIoMmioLib.inf
> @@ -0,0 +1,38 @@
> +## @file
> +# Library to install the XENIO_PROTOCOL on a handle
> +#
> +# Copyright (c) 2015, Linaro Ltd. All rights reserved.<BR>
Indentation is off by one, but it's not a deal breaker. :)
> +#
> +# 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.
> +#
> +#
> +##
> +
> +[Defines]
> + INF_VERSION = 0x00010005
> + BASE_NAME = XenIoMmioLib
> + FILE_GUID = 3CD90EEC-EBF3-425D-AAE8-B16215AC4F50
(9) I've been on the lookout throughout your patch series, and finally
here it is. A FILE_GUID that you forgot to set from a fresh invocation
of "uuidgen" :)
This FILE_GUID is already used in
"ArmPlatformPkg/ArmVirtualizationPkg/Library/ArmVirtualizationDxeHobLib/ArmVirtualizationDxeHobLib.inf".
(What, you don't write your INF files from zero? Unforgivable! ;))
> + MODULE_TYPE = DXE_DRIVER
> + VERSION_STRING = 1.0
> + LIBRARY_CLASS = XenIoMmioLib
> +
> +[Sources]
> + XenIoMmioLib.c
> +
> +[Packages]
> + MdePkg/MdePkg.dec
> + OvmfPkg/OvmfPkg.dec
> +
> +[LibraryClasses]
> + BaseMemoryLib
> +
> +[Guids]
> + gXenBusRootDeviceGuid
> +
> +[Protocols]
> + gEfiDevicePathProtocolGuid
>
Thanks
Laszlo
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |