Re: [PATCH v2 14/18] OvmfPkg/XenBusDxe: Indroduce XenBus support itself.

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

On Thu, Sep 04, 2014 at 05:51:09PM +0100, Anthony PERARD wrote:
> This is a bus-like on top of XenStore. It will look for advertised
> ParaVirtualized devices and initialize them by producing XenBus
> protocol.
> 
> Origin: FreeBSD 10.0
> 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
> 
> ---
> Change in V2:
> - comment, file header
> - Fix comment style
> - Error handling in the main init function
> - coding style
> - Fix error path in add device.
> ---
>  OvmfPkg/Include/Protocol/XenBus.h |  11 +-
>  OvmfPkg/XenBusDxe/XenBus.c        | 368 ++++++++++++++++++++++++++++++++++++++
>  OvmfPkg/XenBusDxe/XenBus.h        |  67 +++++++
>  OvmfPkg/XenBusDxe/XenBusDxe.c     |  71 ++++++++
>  OvmfPkg/XenBusDxe/XenBusDxe.h     |  19 ++
>  OvmfPkg/XenBusDxe/XenBusDxe.inf   |   3 +
>  6 files changed, 538 insertions(+), 1 deletion(-)
>  create mode 100644 OvmfPkg/XenBusDxe/XenBus.c
>  create mode 100644 OvmfPkg/XenBusDxe/XenBus.h
> 
> diff --git a/OvmfPkg/Include/Protocol/XenBus.h b/OvmfPkg/Include/Protocol/XenBus.h
> index 99e174b..f223be7 100644
> --- a/OvmfPkg/Include/Protocol/XenBus.h
> +++ b/OvmfPkg/Include/Protocol/XenBus.h
> @@ -45,7 +45,7 @@
>  ///
>  typedef struct _XENBUS_PROTOCOL XENBUS_PROTOCOL;
>  
> -typedef enum xenbus_state XenbusState;
> +typedef enum xenbus_state XenBusState;
>  
>  typedef struct
>  {
> @@ -138,6 +138,14 @@ XENSTORE_STATUS
>    );
>  
>  typedef
> +XENSTORE_STATUS
> +(EFIAPI *XENBUS_SET_STATE)(
> +  IN XENBUS_PROTOCOL        *This,
> +  IN XENSTORE_TRANSACTION   Transaction,
> +  IN XenBusState            State
> +  );
> +
> +typedef
>  EFI_STATUS
>  (EFIAPI *XENBUS_GRANT_ACCESS)(
>    IN  XENBUS_PROTOCOL *This,
> @@ -196,6 +204,7 @@ struct _XENBUS_PROTOCOL {
>    XENBUS_XS_REMOVE              XsRemove;
>    XENBUS_XS_TRANSACTION_START   XsTransactionStart;
>    XENBUS_XS_TRANSACTION_END     XsTransactionEnd;
> +  XENBUS_SET_STATE              SetState;
>  
>    XENBUS_GRANT_ACCESS           GrantAccess;
>    XENBUS_GRANT_END_ACCESS       GrantEndAccess;
> diff --git a/OvmfPkg/XenBusDxe/XenBus.c b/OvmfPkg/XenBusDxe/XenBus.c
> new file mode 100644
> index 0000000..2e1f939
> --- /dev/null
> +++ b/OvmfPkg/XenBusDxe/XenBus.c
> @@ -0,0 +1,368 @@
> +/** @file
> +  XenBus Bus driver implemtation.
> +
> +  This file implement the necessary to discover and enumerate Xen PV devices
> +  through XenStore.
> +
> +  Copyright (C) 2010 Spectra Logic Corporation
> +  Copyright (C) 2008 Doug Rabson
> +  Copyright (C) 2005 Rusty Russell, IBM Corporation
> +  Copyright (C) 2005 Mike Wray, Hewlett-Packard
> +  Copyright (C) 2005 XenSource Ltd
> +  Copyright (C) 2014, Citrix Ltd.
> +
> +  This file may be distributed separately from the Linux kernel, or
> +  incorporated into other software packages, subject to the following license:
> +
> +  Permission is hereby granted, free of charge, to any person obtaining a copy
> +  of this source file (the "Software"), to deal in the Software without
> +  restriction, including without limitation the rights to use, copy, modify,
> +  merge, publish, distribute, sublicense, and/or sell copies of the Software,
> +  and to permit persons to whom the Software is furnished to do so, subject to
> +  the following conditions:
> +
> +  The above copyright notice and this permission notice shall be included in
> +  all copies or substantial portions of the Software.
> +
> +  THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> +  IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> +  FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
> +  AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> +  LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
> +  FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
> +  IN THE SOFTWARE.
> +**/
> +
> +#include <Library/PrintLib.h>
> +
> +#include "XenBus.h"
> +#include "GrantTable.h"
> +#include "XenStore.h"
> +#include "EventChannel.h"
> +
> +#include <IndustryStandard/Xen/io/xenbus.h>
> +
> +STATIC XENBUS_PRIVATE_DATA gXenBusPrivateData;
> +
> +STATIC XENBUS_DEVICE_PATH gXenBusDevicePathTemplate = {
> +  .Vendor.Header.Type = HARDWARE_DEVICE_PATH,
> +  .Vendor.Header.SubType = HW_VENDOR_DP,
> +  .Vendor.Header.Length[0] = (UINT8) sizeof (XENBUS_DEVICE_PATH),
> +  .Vendor.Header.Length[1] = (UINT8) (sizeof (XENBUS_DEVICE_PATH) >> 8),
> +  .Vendor.Guid = XENBUS_PROTOCOL_GUID,
> +  .Type = 0,
> +  .DeviceId = 0
> +};
> +
> +
> +/**
> +  Search our internal record of configured devices (not the XenStore) to
> +  determine if the XenBus device indicated by Node is known to the system.
> +
> +  @param Dev   The XENBUS_DEVICE instance to search for device children.
> +  @param Node  The XenStore node path for the device to find.
> +
> +  @return  The XENBUS_PRIVATE_DATA of the found device if any, or NULL.
> + */
> +STATIC
> +XENBUS_PRIVATE_DATA *
> +XenBusDeviceInitialized (
> +  IN XENBUS_DEVICE *Dev,
> +  IN CONST CHAR8 *Node
> +  )
> +{
> +  LIST_ENTRY *Entry;
> +  XENBUS_PRIVATE_DATA *Child;
> +  XENBUS_PRIVATE_DATA *Result;
> +
> +  if (IsListEmpty (&Dev->ChildList)) {
> +    return NULL;
> +  }
> +
> +  Result = NULL;
> +  for (Entry = GetFirstNode (&Dev->ChildList);
> +       !IsNodeAtEnd (&Dev->ChildList, Entry);
> +       Entry = GetNextNode (&Dev->ChildList, Entry)) {
> +    Child = XENBUS_PRIVATE_DATA_FROM_LINK (Entry);
> +    if (!AsciiStrCmp (Child->XenBusIo.Node, Node)) {
> +      Result = Child;
> +      break;
> +    }
> +  }
> +
> +  return (Result);
> +}
> +
> +STATIC
> +XenbusState
> +XenBusReadDriverState (
> +  IN CONST CHAR8 *Path
> +  )
> +{
> +  XenbusState State;
> +  CHAR8 *Ptr = NULL;
> +  XENSTORE_STATUS Status;
> +
> +  Status = XenStoreRead (XST_NIL, Path, "state", NULL, (VOID **)&Ptr);
> +  if (Status != XENSTORE_STATUS_SUCCESS) {
> +    State = XenbusStateClosed;
> +  } else {
> +    State = AsciiStrDecimalToUintn (Ptr);
> +  }
> +
> +  if (Ptr != NULL) {
> +    FreePool (Ptr);
> +  }
> +
> +  return State;
> +}
> +

Could you add a comment here saying that the callers should 
ensure that they are the only ones calling this function - as this
function does not take a lock when adding in the Dev->ChildList

> +STATIC
> +EFI_STATUS
> +XenBusAddDevice (
> +  XENBUS_DEVICE *Dev,
> +  CONST CHAR8 *Type,
> +  CONST CHAR8 *Id)
> +{
> +  CHAR8 DevicePath[XENSTORE_ABS_PATH_MAX];
> +  XENSTORE_STATUS StatusXenStore;
> +  XENBUS_PRIVATE_DATA *Private;
> +  EFI_STATUS Status;
> +  XENBUS_DEVICE_PATH *TempXenBusPath;
> +  VOID *ChildPciIo;
> +
> +  AsciiSPrint (DevicePath, sizeof (DevicePath),
> +               XENBUS_XENSTORE_NODE "/%a/%a", Type, Id);
> +
> +  if (XenStorePathExists (XST_NIL, DevicePath, "")) {
> +    XENBUS_PRIVATE_DATA *Child;
> +    enum xenbus_state State;
> +    CHAR8 *BackendPath;
> +
> +    Child = XenBusDeviceInitialized (Dev, DevicePath);
> +    if (Child != NULL) {
> +      /*
> +       * We are already tracking this node
> +       */
> +      Status = EFI_SUCCESS;
> +      goto out;
> +    }
> +
> +    State = XenBusReadDriverState (DevicePath);
> +    if (State != XenbusStateInitialising) {
> +      /*
> +       * Device is not new, so ignore it. This can
> +       * happen if a device is going away after
> +       * switching to Closed.
> +       */
> +      DEBUG ((EFI_D_INFO, "XenBus: Device %a ignored. "
> +              "State %d\n", DevicePath, State));
> +      Status = EFI_SUCCESS;
> +      goto out;
> +    }
> +
> +    StatusXenStore = XenStoreRead (XST_NIL, DevicePath, "backend",
> +                                   NULL, (VOID **) &BackendPath);
> +    if (StatusXenStore != XENSTORE_STATUS_SUCCESS) {
> +      DEBUG ((EFI_D_ERROR, "xenbus: %a no backend path.\n", DevicePath));
> +      Status = EFI_NOT_FOUND;
> +      goto out;
> +    }
> +
> +    Private = AllocateCopyPool (sizeof *Private, &gXenBusPrivateData);
> +    InsertTailList (&Dev->ChildList, &Private->Link);
> +
> +    Private->XenBusIo.Type = AsciiStrDup (Type);
> +    Private->XenBusIo.Node = AsciiStrDup (DevicePath);
> +    Private->XenBusIo.Backend = BackendPath;
> +    Private->XenBusIo.DeviceId = AsciiStrDecimalToUintn (Id);
> +    Private->Dev = Dev;
> +
> +    TempXenBusPath = AllocateCopyPool (sizeof (XENBUS_DEVICE_PATH),
> +                                       &gXenBusDevicePathTemplate);
> +    if (!AsciiStrCmp (Private->XenBusIo.Type, "vbd")) {
> +      TempXenBusPath->Type = XENBUS_DEVICE_PATH_TYPE_VBD;
> +    }
> +    TempXenBusPath->DeviceId = Private->XenBusIo.DeviceId;
> +    Private->DevicePath = (XENBUS_DEVICE_PATH *)AppendDevicePathNode (
> +                            Dev->DevicePath,
> +                            &TempXenBusPath->Vendor.Header);

Could you move the 'InsertTailList' to be done after the 'Private'
has been fully populated? I know that the current caller of this function is
only called during initialization - but in the future this could be
extended to called multiple times - and if there are two threads - one
adding and another reading from Dev->ChildList - the second thread can
get incomplete data.

> +    FreePool (TempXenBusPath);
> +
> +    Status = gBS->InstallMultipleProtocolInterfaces (
> +               &Private->Handle,
> +               &gEfiDevicePathProtocolGuid, Private->DevicePath,
> +               &gXenBusProtocolGuid, &Private->XenBusIo,
> +               NULL);
> +    if (EFI_ERROR (Status)) {
> +      goto ErrorInstallProtocol;
> +    }
> +
> +    Status = gBS->OpenProtocol (Dev->ControllerHandle,
> +               &gEfiPciIoProtocolGuid,
> +               &ChildPciIo, Dev->This->DriverBindingHandle,
> +               Private->Handle,
> +               EFI_OPEN_PROTOCOL_BY_CHILD_CONTROLLER);
> +    if (EFI_ERROR (Status)) {
> +      DEBUG ((EFI_D_ERROR, "open by child controller fail (%r)\n",
> +              Status));
> +      goto ErrorOpenProtocolByChild;
> +    }
> +  } else {
> +    DEBUG ((EFI_D_ERROR, "XenBus: does not exist: %a\n", DevicePath));
> +    Status = EFI_NOT_FOUND;
> +  }
> +
> +out:
> +  return Status;
> +ErrorOpenProtocolByChild:
> +  gBS->UninstallMultipleProtocolInterfaces (
> +    &Private->Handle,
> +    &gEfiDevicePathProtocolGuid, Private->DevicePath,
> +    &gXenBusProtocolGuid, &Private->XenBusIo,
> +    NULL);
> +ErrorInstallProtocol:
> +  FreePool (Private->DevicePath);
> +  FreePool ((VOID *) Private->XenBusIo.Backend);
> +  FreePool ((VOID *) Private->XenBusIo.Node);
> +  FreePool ((VOID *) Private->XenBusIo.Type);
> +  RemoveEntryList (&Private->Link);
> +  FreePool (Private);
> +  return Status;
> +}
> +
> +/**
> +  Enumerate all devices of the given type on this bus.
> +
> +  @param Dev   A XENBUS_DEVICE instance.
> +  @param Type  String indicating the device sub-tree (e.g. "vfb", "vif")
> +               to enumerate.
> +
> +  Devices that are found are been initialize via XenBusAddDevice ().
> +  XenBusAddDevice () ignores duplicate detects and ignores duplicate devices,
> +  so it can be called unconditionally for any device found in the XenStore.


.. And the caller needs to ensure it is the only one calling it - or
it holds a lock.

> + */
> +STATIC
> +VOID
> +XenBusEnumerateDeviceType (
> +  XENBUS_DEVICE *Dev,
> +  CONST CHAR8 *Type
> +  )
> +{
> +  CONST CHAR8 **Directory;
> +  UINTN Index;
> +  UINT32 Count;
> +  XENSTORE_STATUS Status;
> +
> +  Status = XenStoreListDirectory (XST_NIL,
> +                                  XENBUS_XENSTORE_NODE, Type,
> +                                  &Count, &Directory);
> +  if (Status != XENSTORE_STATUS_SUCCESS) {
> +    return;
> +  }
> +  for (Index = 0; Index < Count; Index++) {
> +    XenBusAddDevice (Dev, Type, Directory[Index]);
> +  }
> +
> +  FreePool (Directory);
> +}
> +
> +
> +/**
> +  Enumerate the devices on a XenBus bus and install a XenBus Protocol instance.
> +
> +  @param Dev   A XENBUS_DEVICE instance.
> +
> +  @return  On success, XENSTORE_STATUS_SUCCESS. Otherwise an errno value
> +           indicating the type of failure.

Since this is public, I think it should also perculate the need to hold
a lock and not call this concurrently. Or that this function ought to not
be called at the same as XenBusDeviceInitialized

> + */
> +XENSTORE_STATUS
> +XenBusEnumerateBus (
> +  XENBUS_DEVICE *Dev
> +  )
> +{
> +  CONST CHAR8 **Types;
> +  UINTN Index;
> +  UINT32 Count;
> +  XENSTORE_STATUS Status;
> +
> +  Status = XenStoreListDirectory (XST_NIL,
> +                                  XENBUS_XENSTORE_NODE, "",
> +                                  &Count, &Types);
> +  if (Status != XENSTORE_STATUS_SUCCESS) {
> +    return Status;
> +  }
> +
> +  for (Index = 0; Index < Count; Index++) {
> +    XenBusEnumerateDeviceType (Dev, Types[Index]);
> +  }
> +
> +  FreePool (Types);
> +
> +  return XENSTORE_STATUS_SUCCESS;
> +}
> +
> +STATIC
> +XENSTORE_STATUS
> +EFIAPI
> +XenBusSetState (
> +  IN XENBUS_PROTOCOL      *This,
> +  IN XENSTORE_TRANSACTION Transaction,
> +  IN enum xenbus_state    NewState
> +  )
> +{
> +  XENBUS_PRIVATE_DATA *Private;
> +  enum xenbus_state CurrentState;
> +  XENSTORE_STATUS Status;
> +  CHAR8 *Temp;
> +
> +  DEBUG ((EFI_D_INFO, "XenBus: Set state to %d\n", NewState));
> +
> +  Private = XENBUS_PRIVATE_DATA_FROM_THIS (This);
> +
> +  Status = XenStoreRead (Transaction, This->Node, "state", NULL, (VOID **)&Temp);
> +  if (Status != XENSTORE_STATUS_SUCCESS) {
> +    goto Out;
> +  }
> +  CurrentState = AsciiStrDecimalToUintn (Temp);
> +  FreePool (Temp);
> +  if (CurrentState == NewState) {
> +    goto Out;
> +  }
> +
> +  do {
> +    Status = XenStoreSPrint (Transaction, This->Node, "state", "%d", NewState);
> +  } while (Status == XENSTORE_STATUS_EAGAIN);
> +  if (Status != XENSTORE_STATUS_SUCCESS) {
> +    DEBUG ((EFI_D_ERROR, "XenBus: failed to write new state\n"));
> +    goto Out;
> +  }
> +  DEBUG ((EFI_D_INFO, "XenBus: Set state to %d, done\n", NewState));
> +
> +Out:
> +  return Status;
> +}
> +
> +STATIC XENBUS_PRIVATE_DATA gXenBusPrivateData = {
> +  .Signature = XENBUS_PRIVATE_DATA_SIGNATURE,
> +
> +  .XenBusIo.XsRead = XenBusXenStoreRead,
> +  .XenBusIo.XsBackendRead = XenBusXenStoreBackendRead,
> +  .XenBusIo.XsPrintf = XenBusXenStoreSPrint,
> +  .XenBusIo.XsRemove = XenBusXenStoreRemove,
> +  .XenBusIo.XsTransactionStart = XenBusXenStoreTransactionStart,
> +  .XenBusIo.XsTransactionEnd = XenBusXenStoreTransactionEnd,
> +  .XenBusIo.SetState = XenBusSetState,
> +  .XenBusIo.GrantAccess = XenBusGrantAccess,
> +  .XenBusIo.GrantEndAccess = XenBusGrantEndAccess,
> +  .XenBusIo.RegisterWatch = XenBusRegisterWatch,
> +  .XenBusIo.RegisterWatchBackend = XenBusRegisterWatchBackend,
> +  .XenBusIo.UnregisterWatch = XenBusUnregisterWatch,
> +  .XenBusIo.WaitForWatch = XenBusWaitForWatch,
> +
> +  .XenBusIo.Type = NULL,
> +  .XenBusIo.Node = NULL,
> +  .XenBusIo.Backend = NULL,
> +
> +  .Dev = NULL
> +};
> diff --git a/OvmfPkg/XenBusDxe/XenBus.h b/OvmfPkg/XenBusDxe/XenBus.h
> new file mode 100644
> index 0000000..48e0389
> --- /dev/null
> +++ b/OvmfPkg/XenBusDxe/XenBus.h
> @@ -0,0 +1,67 @@
> +/** @file
> +  Core definitions and data structures shareable across OS platforms.
> +
> +  Copyright (c) 2010 Spectra Logic Corporation
> +  Copyright (C) 2008 Doug Rabson
> +  All rights reserved.
> +  Copyright (C) 2014, Citrix Ltd.
> +
> +  Redistribution and use in source and binary forms, with or without
> +  modification, are permitted provided that the following conditions
> +  are met:
> +  1. Redistributions of source code must retain the above copyright
> +     notice, this list of conditions, and the following disclaimer,
> +     without modification.
> +  2. Redistributions in binary form must reproduce at minimum a disclaimer
> +     substantially similar to the "NO WARRANTY" disclaimer below
> +     ("Disclaimer") and any redistribution must be conditioned upon
> +     including a substantially similar Disclaimer requirement for further
> +     binary redistribution.
> +
> +  NO WARRANTY
> +  THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
> +  "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
> +  LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTIBILITY AND FITNESS FOR
> +  A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
> +  HOLDERS OR CONTRIBUTORS BE LIABLE FOR SPECIAL, EXEMPLARY, OR CONSEQUENTIAL
> +  DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS
> +  OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION)
> +  HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT,
> +  STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING
> +  IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE
> +  POSSIBILITY OF SUCH DAMAGES.
> +
> +**/
> +#ifndef _XEN_XENBUS_XENBUSB_H
> +#define _XEN_XENBUS_XENBUSB_H
> +
> +#include "XenBusDxe.h"
> +
> +#define XENBUS_DEVICE_PATH_TYPE_VBD 0x1
> +struct _XENBUS_DEVICE_PATH {
> +  VENDOR_DEVICE_PATH  Vendor;
> +  UINT8               Type;
> +  UINT16              DeviceId;
> +};
> +
> +/**
> +  The VM relative path to the XenStore subtree this
> +  bus attachment manages.

Huh?
> +**/
> +#define XENBUS_XENSTORE_NODE "device"
> +
> +
> +/**
> +  Perform XenBus bus enumeration and install protocol for childs.

childs? children?
> +
> +  @param Dev   The NewBus device representing this XenBus bus.
> +
> +  @return      On success, XENSTORE_STATUS_SUCCESS. Otherwise an errno value
> +               indicating the type of failure.
> +**/
> +XENSTORE_STATUS
> +XenBusEnumerateBus (
> +  XENBUS_DEVICE *Dev
> +  );
> +
> +#endif /* _XEN_XENBUS_XENBUSB_H */
> diff --git a/OvmfPkg/XenBusDxe/XenBusDxe.c b/OvmfPkg/XenBusDxe/XenBusDxe.c
> index 76ea67c..50090ba 100644
> --- a/OvmfPkg/XenBusDxe/XenBusDxe.c
> +++ b/OvmfPkg/XenBusDxe/XenBusDxe.c
> @@ -48,6 +48,7 @@
>  #include "XenHypercall.h"
>  #include "GrantTable.h"
>  #include "XenStore.h"
> +#include "XenBus.h"
>  
>  
>  ///
> @@ -301,6 +302,7 @@ XenBusDxeDriverBindingStart (
>    EFI_PCI_IO_PROTOCOL *PciIo;
>    EFI_ACPI_ADDRESS_SPACE_DESCRIPTOR *BarDesc;
>    UINT64 MmioAddr;
> +  EFI_DEVICE_PATH_PROTOCOL *DevicePath;
>  
>    Status = gBS->OpenProtocol (
>                       ControllerHandle,
> @@ -314,12 +316,32 @@ XenBusDxeDriverBindingStart (
>      return Status;
>    }
>  
> +  Status = gBS->OpenProtocol (
> +                  ControllerHandle,
> +                  &gEfiDevicePathProtocolGuid,
> +                  (VOID **) &DevicePath,
> +                  This->DriverBindingHandle,
> +                  ControllerHandle,
> +                  EFI_OPEN_PROTOCOL_BY_DRIVER
> +                  );
> +
> +  if (EFI_ERROR (Status)) {
> +    goto ErrorOpenningProtocol;
> +  }
> +
>    Dev = AllocateZeroPool (sizeof (*Dev));
>    Dev->Signature = XENBUS_DEVICE_SIGNATURE;
>    Dev->This = This;
>    Dev->ControllerHandle = ControllerHandle;
>    Dev->PciIo = PciIo;
> +  Dev->DevicePath = DevicePath;
> +  InitializeListHead (&Dev->ChildList);
>  
> +  //
> +  // The BAR1 of this PCI device is used for shared memory and is supposed to
> +  // look like MMIO. The address space of the BAR1 will be used to map the
> +  // Grant Table.
> +  //
>    Status = PciIo->GetBarAttributes (PciIo, PCI_BAR_IDX1, NULL, (VOID**) &BarDesc);
>    ASSERT_EFI_ERROR (Status);
>    ASSERT (BarDesc->ResType == ACPI_ADDRESS_SPACE_TYPE_MEM);
> @@ -348,6 +370,8 @@ XenBusDxeDriverBindingStart (
>    Status = XenStoreInit (Dev);
>    ASSERT_EFI_ERROR (Status);
>  
> +  XenBusEnumerateBus (Dev);
> +
>    Status = gBS->CreateEvent (EVT_SIGNAL_EXIT_BOOT_SERVICES, TPL_CALLBACK,
>                               NotifyExitBoot,
>                               (VOID*) Dev,
> @@ -359,6 +383,9 @@ XenBusDxeDriverBindingStart (
>  
>  ErrorNoHyperpage:
>    FreePool (Dev);
> +  gBS->CloseProtocol (ControllerHandle, &gEfiDevicePathProtocolGuid,
> +                      This->DriverBindingHandle, ControllerHandle);
> +ErrorOpenningProtocol:
>    gBS->CloseProtocol (ControllerHandle, &gEfiPciIoProtocolGuid,
>                        This->DriverBindingHandle, ControllerHandle);
>    return Status;
> @@ -399,12 +426,56 @@ XenBusDxeDriverBindingStop (
>    IN EFI_HANDLE                   *ChildHandleBuffer OPTIONAL
>    )
>  {
> +  UINTN Index;
> +  XENBUS_PROTOCOL *XenBusIo;
> +  XENBUS_PRIVATE_DATA *ChildData;
> +  EFI_STATUS Status;
>    XENBUS_DEVICE *Dev = mMyDevice;
>  
> +  for (Index = 0; Index < NumberOfChildren; Index++) {
> +    Status = gBS->OpenProtocol (
> +               ChildHandleBuffer[Index],
> +               &gXenBusProtocolGuid,
> +               (VOID **) &XenBusIo,
> +               This->DriverBindingHandle,
> +               ControllerHandle,
> +               EFI_OPEN_PROTOCOL_GET_PROTOCOL);
> +    if (EFI_ERROR (Status)) {
> +      DEBUG ((EFI_D_ERROR, "XenBusDxe: get children protocol failed: %r\n", Status));
> +      continue;
> +    }
> +    ChildData = XENBUS_PRIVATE_DATA_FROM_THIS (XenBusIo);
> +    Status = gBS->DisconnectController (ChildData->Handle, NULL, NULL);
> +    if (EFI_ERROR (Status)) {
> +      DEBUG ((EFI_D_ERROR, "XenBusDxe: error disconnecting child: %r\n",
> +              Status));
> +      continue;
> +    }
> +
> +    Status = gBS->UninstallMultipleProtocolInterfaces (
> +               ChildData->Handle,
> +               &gEfiDevicePathProtocolGuid, ChildData->DevicePath,
> +               &gXenBusProtocolGuid, &ChildData->XenBusIo,
> +               NULL);
> +    ASSERT_EFI_ERROR (Status);
> +
> +    FreePool ((VOID*)ChildData->XenBusIo.Type);
> +    FreePool ((VOID*)ChildData->XenBusIo.Node);
> +    FreePool ((VOID*)ChildData->XenBusIo.Backend);
> +    FreePool (ChildData->DevicePath);
> +    RemoveEntryList (&ChildData->Link);
> +    FreePool (ChildData);
> +  }
> +  if (NumberOfChildren > 0) {
> +    return EFI_SUCCESS;
> +  }
> +
>    gBS->CloseEvent (Dev->ExitBootEvent);
>    XenStoreDeinit (Dev);
>    XenGrantTableDeinit (Dev);
>  
> +  gBS->CloseProtocol (ControllerHandle, &gEfiDevicePathProtocolGuid,
> +         This->DriverBindingHandle, ControllerHandle);
>    gBS->CloseProtocol (ControllerHandle, &gEfiPciIoProtocolGuid,
>           This->DriverBindingHandle, ControllerHandle);
>    return EFI_SUCCESS;
> diff --git a/OvmfPkg/XenBusDxe/XenBusDxe.h b/OvmfPkg/XenBusDxe/XenBusDxe.h
> index dc48a46..7213184 100644
> --- a/OvmfPkg/XenBusDxe/XenBusDxe.h
> +++ b/OvmfPkg/XenBusDxe/XenBusDxe.h
> @@ -98,6 +98,7 @@ extern EFI_COMPONENT_NAME_PROTOCOL  gXenBusDxeComponentName;
>  #define PCI_DEVICE_ID_XEN_PLATFORM       0x0001
>  
>  
> +typedef struct _XENBUS_DEVICE_PATH XENBUS_DEVICE_PATH;
>  typedef struct _XENBUS_DEVICE XENBUS_DEVICE;
>  
>  // Have the state of the driver.
> @@ -108,11 +109,29 @@ struct _XENBUS_DEVICE {
>    EFI_HANDLE                    ControllerHandle;
>    EFI_PCI_IO_PROTOCOL           *PciIo;
>    EFI_EVENT                     ExitBootEvent;
> +  EFI_DEVICE_PATH_PROTOCOL      *DevicePath;
> +  LIST_ENTRY                    ChildList;
>  
>    VOID                          *Hyperpage;
>    shared_info_t                 *SharedInfo;
>  };
>  
> +// There is one of this struct allocated for every child.
> +#define XENBUS_PRIVATE_DATA_SIGNATURE SIGNATURE_32 ('X', 'B', 'p', 'd')
> +typedef struct {
> +    UINTN Signature;
> +    LIST_ENTRY Link;
> +    EFI_HANDLE Handle;
> +    XENBUS_PROTOCOL XenBusIo;
> +    XENBUS_DEVICE *Dev;
> +    XENBUS_DEVICE_PATH *DevicePath;
> +} XENBUS_PRIVATE_DATA;
> +
> +#define XENBUS_PRIVATE_DATA_FROM_THIS(a) \
> +  CR (a, XENBUS_PRIVATE_DATA, XenBusIo, XENBUS_PRIVATE_DATA_SIGNATURE)
> +#define XENBUS_PRIVATE_DATA_FROM_LINK(a) \
> +  CR (a, XENBUS_PRIVATE_DATA, Link, XENBUS_PRIVATE_DATA_SIGNATURE)
> +
>  /*
>   * Helpers
>   */
> diff --git a/OvmfPkg/XenBusDxe/XenBusDxe.inf b/OvmfPkg/XenBusDxe/XenBusDxe.inf
> index 9052967..c326bcf 100644
> --- a/OvmfPkg/XenBusDxe/XenBusDxe.inf
> +++ b/OvmfPkg/XenBusDxe/XenBusDxe.inf
> @@ -40,6 +40,9 @@
>    EventChannel.h
>    XenStore.c
>    XenStore.h
> +  XenBus.c
> +  XenBus.h
> +  Helpers.c
>  
>  [Sources.X64]
>    X64/hypercall.S
> -- 
> Anthony PERARD
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel