From mboxrd@z Thu Jan 1 00:00:00 1970 From: Konrad Rzeszutek Wilk Subject: Re: [PATCH RFC 02/18] OvmfPkg: Add basic skeleton for the Xenbus driver. Date: Wed, 16 Jul 2014 13:25:03 -0400 Message-ID: <20140716172503.GC30483@laptop.dumpdata.com> References: <1405523747-5024-1-git-send-email-anthony.perard@citrix.com> <1405523747-5024-3-git-send-email-anthony.perard@citrix.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Content-Disposition: inline In-Reply-To: <1405523747-5024-3-git-send-email-anthony.perard@citrix.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Anthony PERARD Cc: EDK2 devel , Xen Devel List-Id: xen-devel@lists.xenproject.org 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 > +#include > +#include > + > +#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; > +}