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

[Anthony PERARD <anthony.perard@xxxxxxxxxx>]

On Wed, Jul 16, 2014 at 01:25:03PM -0400, Konrad Rzeszutek Wilk wrote:
> 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?

Do you mean having "Xenbus bus driver" in the title? It feels a bit
redondant, but might be better.

> .. 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) ?

Yes. Most (all?) of the code in this files (and this patch) comes from a
template. I haven't been through the comment yet. But yes this bus
driver can have childs.

> > +  //
> > +  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?

No, it is not.

> > +
> > +  //
> > +  // 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 ..

Will do.

> > +
> > +  //
> > +  // 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;
> > +}

-- 
Anthony PERARD

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel