Re: [PATCH RFC 02/18] OvmfPkg: Add basic skeleton for the Xenbus driver.

[Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>]

On Wed, Jul 16, 2014 at 04:15:31PM +0100, Anthony PERARD wrote:
> This includes Component Name and Driver Binding.

The title says driver, but the cover letter said 'bus driver'.
Should it have bus driver in it?

.. snip..
> +/**
> +  Retrieves a Unicode string that is the user readable name of the controller
> +  that is being managed by an EFI Driver.
> +
> +  @param  This             A pointer to the EFI_COMPONENT_NAME_PROTOCOL instance.
> +  @param  ControllerHandle The handle of a controller that the driver specified by
> +                           This is managing.  This handle specifies the controller
> +                           whose name is to be returned.
> +  @param  ChildHandle      The handle of the child controller to retrieve the name
> +                           of.  This is an optional parameter that may be NULL.  It
> +                           will be NULL for device drivers.  It will also be NULL
> +                           for a bus drivers that wish to retrieve the name of the
> +                           bus controller.  It will not be NULL for a bus driver
> +                           that wishes to retrieve the name of a child controller.
.. snip..
> +EFI_STATUS
> +EFIAPI
> +XenbusDxeComponentNameGetControllerName (
> +  IN  EFI_COMPONENT_NAME2_PROTOCOL  *This,
> +  IN  EFI_HANDLE                    ControllerHandle,
> +  IN  EFI_HANDLE                    ChildHandle        OPTIONAL,
> +  IN  CHAR8                         *Language,
> +  OUT CHAR16                        **ControllerName
> +  )
> +{
> +  EFI_STATUS  Status;
> +
> +  //
> +  // ChildHandle must be NULL for a Device Driver

OK, but this is a bus driver in which case ChildHandle can be NULL or not (at
least that is what the comment says) ?
> +  //
> +  if (ChildHandle != NULL) {
> +    return EFI_UNSUPPORTED;
> +  }
> +
> +  //
> +  // Lookup name of controller specified by ControllerHandle
> +  //
> +  Status = EFI_UNSUPPORTED;
> +
> +  return Status;
> +}
.. snip..

> diff --git a/OvmfPkg/XenbusDxe/XenbusDxe.c b/OvmfPkg/XenbusDxe/XenbusDxe.c
> new file mode 100644
> index 0000000..14113ad
> --- /dev/null
> +++ b/OvmfPkg/XenbusDxe/XenbusDxe.c
> @@ -0,0 +1,314 @@
> +
> +/** @file
> +  TODO: Brief Description of UEFI Driver XenbusDxe
> +
> +  TODO: Detailed Description of UEFI Driver XenbusDxe
> +
> +  TODO: Copyright for UEFI Driver XenbusDxe
> +
> +  TODO: License for UEFI Driver XenbusDxe
> +
> +**/
> +
> +#include <IndustryStandard/Pci.h>
> +#include <IndustryStandard/Acpi.h>
> +#include <Library/DebugLib.h>
> +
> +#include "XenbusDxe.h"
> +
> +
> +///
> +/// Driver Binding Protocol instance
> +///
> +EFI_DRIVER_BINDING_PROTOCOL gXenbusDxeDriverBinding = {
> +  XenbusDxeDriverBindingSupported,
> +  XenbusDxeDriverBindingStart,
> +  XenbusDxeDriverBindingStop,
> +  XENBUS_DXE_VERSION,
> +  NULL,
> +  NULL
> +};
> +
> +
> +/**
> +  Unloads an image.
> +
> +  @param  ImageHandle           Handle that identifies the image to be unloaded.
> +
> +  @retval EFI_SUCCESS           The image has been unloaded.
> +  @retval EFI_INVALID_PARAMETER ImageHandle is not a valid image handle.
> +
> +**/
> +EFI_STATUS
> +EFIAPI
> +XenbusDxeUnload (
> +  IN EFI_HANDLE  ImageHandle
> +  )
> +{
> +  EFI_STATUS  Status;
> +
> +  EFI_HANDLE  *HandleBuffer;
> +  UINTN       HandleCount;
> +  UINTN       Index;
> +
> +
> +  Status = EFI_SUCCESS;

Is that neccessary considering the next line you overwrite it?
> +
> +  //
> +  // Retrieve array of all handles in the handle database
> +  //
> +  Status = gBS->LocateHandleBuffer (
> +                  AllHandles,
> +                  NULL,
> +                  NULL,
> +                  &HandleCount,
> +                  &HandleBuffer
> +                  );
> +  if (EFI_ERROR (Status)) {
> +    return Status;
> +  }
> +
> +  //
> +  // Disconnect the current driver from handles in the handle database
> +  //
> +  for (Index = 0; Index < HandleCount; Index++) {
> +    Status = gBS->DisconnectController (HandleBuffer[Index], gImageHandle, NULL);
> +  }

Should you check the Status?
I guess it doesn't matter at all. In which case why don't you just
(void)gBS->DisconnectController ..
> +
> +  //
> +  // Free the array of handles
> +  //
> +  FreePool (HandleBuffer);
> +
> +
> +  //
> +  // Uninstall protocols installed in the driver entry point
> +  //
> +  Status = gBS->UninstallMultipleProtocolInterfaces (
> +                  ImageHandle,
> +                  &gEfiDriverBindingProtocolGuid, &gXenbusDxeDriverBinding,
> +                  &gEfiComponentNameProtocolGuid,  &gXenbusDxeComponentName,
> +                  &gEfiComponentName2ProtocolGuid, &gXenbusDxeComponentName2,
> +                  NULL
> +                  );
> +  if (EFI_ERROR (Status)) {
> +    return Status;
> +  }
> +
> +
> +  //
> +  // Do any additional cleanup that is required for this driver
> +  //
> +
> +  return EFI_SUCCESS;
> +}
> +
> +/**
> +  This is the declaration of an EFI image entry point. This entry point is
> +  the same for UEFI Applications, UEFI OS Loaders, and UEFI Drivers including
> +  both device drivers and bus drivers.
> +
> +  @param  ImageHandle           The firmware allocated handle for the UEFI image.
> +  @param  SystemTable           A pointer to the EFI System Table.
> +
> +  @retval EFI_SUCCESS           The operation completed successfully.
> +  @retval Others                An unexpected error occurred.
> +**/
> +EFI_STATUS
> +EFIAPI
> +XenbusDxeDriverEntryPoint (
> +  IN EFI_HANDLE        ImageHandle,
> +  IN EFI_SYSTEM_TABLE  *SystemTable
> +  )
> +{
> +  EFI_STATUS  Status;
> +
> +
> +  Status = EFI_SUCCESS;

Not needed.

> +
> +
> +  //
> +  // Install UEFI Driver Model protocol(s).
> +  //
> +  Status = EfiLibInstallDriverBindingComponentName2 (
> +             ImageHandle,
> +             SystemTable,
> +             &gXenbusDxeDriverBinding,
> +             ImageHandle,
> +             &gXenbusDxeComponentName,
> +             &gXenbusDxeComponentName2
> +             );
> +  ASSERT_EFI_ERROR (Status);
> +
> +
> +  return Status;
> +}