[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Xen-devel] [PATCH v2 21/29] Ovmf/Xen: move XenBusDxe to abstract XENIO_PROTOCOL



comments below

On 01/26/15 20:03, Ard Biesheuvel wrote:
> While Xen on Intel uses a virtual PCI device to communicate the
> base address of the grant table, the ARM implementation uses a DT
> node, which is fundamentally incompatible with the way XenBusDxe is
> implemented, i.e., as a UEFI Driver Model implementation for a PCI
> device.
> 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@xxxxxxxxxx>
> ---
>  OvmfPkg/OvmfPkgIa32.dsc           |  1 +
>  OvmfPkg/OvmfPkgIa32.fdf           |  1 +
>  OvmfPkg/OvmfPkgIa32X64.dsc        |  1 +
>  OvmfPkg/OvmfPkgIa32X64.fdf        |  1 +
>  OvmfPkg/OvmfPkgX64.dsc            |  1 +
>  OvmfPkg/OvmfPkgX64.fdf            |  1 +
>  OvmfPkg/XenBusDxe/ComponentName.c |  2 +-
>  OvmfPkg/XenBusDxe/GrantTable.c    |  5 ++---
>  OvmfPkg/XenBusDxe/GrantTable.h    |  3 +--
>  OvmfPkg/XenBusDxe/XenBus.c        |  6 +++---
>  OvmfPkg/XenBusDxe/XenBusDxe.c     | 57 
> ++++++++++++++-------------------------------------------
>  OvmfPkg/XenBusDxe/XenBusDxe.h     |  8 ++------
>  OvmfPkg/XenBusDxe/XenBusDxe.inf   |  2 +-
>  13 files changed, 30 insertions(+), 59 deletions(-)
> 
> diff --git a/OvmfPkg/OvmfPkgIa32.dsc b/OvmfPkg/OvmfPkgIa32.dsc
> index 90540272745c..8c880613851d 100644
> --- a/OvmfPkg/OvmfPkgIa32.dsc
> +++ b/OvmfPkg/OvmfPkgIa32.dsc
> @@ -444,6 +444,7 @@
>    OvmfPkg/VirtioBlkDxe/VirtioBlk.inf
>    OvmfPkg/VirtioScsiDxe/VirtioScsi.inf
>    OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FvbServicesRuntimeDxe.inf
> +  OvmfPkg/XenIoPciDxe/XenIoPciDxe.inf
>    OvmfPkg/XenBusDxe/XenBusDxe.inf
>    OvmfPkg/XenPvBlkDxe/XenPvBlkDxe.inf
>    OvmfPkg/EmuVariableFvbRuntimeDxe/Fvb.inf {
> diff --git a/OvmfPkg/OvmfPkgIa32.fdf b/OvmfPkg/OvmfPkgIa32.fdf
> index f47e7ddc7834..ecef963d1e85 100644
> --- a/OvmfPkg/OvmfPkgIa32.fdf
> +++ b/OvmfPkg/OvmfPkgIa32.fdf
> @@ -227,6 +227,7 @@ INF  OvmfPkg/VirtioScsiDxe/VirtioScsi.inf
>  INF  OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FvbServicesRuntimeDxe.inf
>  INF  OvmfPkg/EmuVariableFvbRuntimeDxe/Fvb.inf
>  INF  MdeModulePkg/Universal/FaultTolerantWriteDxe/FaultTolerantWriteDxe.inf
> +INF  OvmfPkg/XenIoPciDxe/XenIoPciDxe.inf
>  INF  OvmfPkg/XenBusDxe/XenBusDxe.inf
>  INF  OvmfPkg/XenPvBlkDxe/XenPvBlkDxe.inf
>  
> diff --git a/OvmfPkg/OvmfPkgIa32X64.dsc b/OvmfPkg/OvmfPkgIa32X64.dsc
> index 0a331eda8be0..ff32ecefd07b 100644
> --- a/OvmfPkg/OvmfPkgIa32X64.dsc
> +++ b/OvmfPkg/OvmfPkgIa32X64.dsc
> @@ -451,6 +451,7 @@
>    OvmfPkg/VirtioBlkDxe/VirtioBlk.inf
>    OvmfPkg/VirtioScsiDxe/VirtioScsi.inf
>    OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FvbServicesRuntimeDxe.inf
> +  OvmfPkg/XenIoPciDxe/XenIoPciDxe.inf
>    OvmfPkg/XenBusDxe/XenBusDxe.inf
>    OvmfPkg/XenPvBlkDxe/XenPvBlkDxe.inf
>    OvmfPkg/EmuVariableFvbRuntimeDxe/Fvb.inf {
> diff --git a/OvmfPkg/OvmfPkgIa32X64.fdf b/OvmfPkg/OvmfPkgIa32X64.fdf
> index 4c034460d5d2..29414ff04082 100644
> --- a/OvmfPkg/OvmfPkgIa32X64.fdf
> +++ b/OvmfPkg/OvmfPkgIa32X64.fdf
> @@ -227,6 +227,7 @@ INF  OvmfPkg/VirtioScsiDxe/VirtioScsi.inf
>  INF  OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FvbServicesRuntimeDxe.inf
>  INF  OvmfPkg/EmuVariableFvbRuntimeDxe/Fvb.inf
>  INF  MdeModulePkg/Universal/FaultTolerantWriteDxe/FaultTolerantWriteDxe.inf
> +INF  OvmfPkg/XenIoPciDxe/XenIoPciDxe.inf
>  INF  OvmfPkg/XenBusDxe/XenBusDxe.inf
>  INF  OvmfPkg/XenPvBlkDxe/XenPvBlkDxe.inf
>  
> diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc
> index e2b37c271681..8bac6dc313f0 100644
> --- a/OvmfPkg/OvmfPkgX64.dsc
> +++ b/OvmfPkg/OvmfPkgX64.dsc
> @@ -449,6 +449,7 @@
>    OvmfPkg/VirtioBlkDxe/VirtioBlk.inf
>    OvmfPkg/VirtioScsiDxe/VirtioScsi.inf
>    OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FvbServicesRuntimeDxe.inf
> +  OvmfPkg/XenIoPciDxe/XenIoPciDxe.inf
>    OvmfPkg/XenBusDxe/XenBusDxe.inf
>    OvmfPkg/XenPvBlkDxe/XenPvBlkDxe.inf
>    OvmfPkg/EmuVariableFvbRuntimeDxe/Fvb.inf {
> diff --git a/OvmfPkg/OvmfPkgX64.fdf b/OvmfPkg/OvmfPkgX64.fdf
> index 2323eb2edf33..f1feb698ba66 100644
> --- a/OvmfPkg/OvmfPkgX64.fdf
> +++ b/OvmfPkg/OvmfPkgX64.fdf
> @@ -227,6 +227,7 @@ INF  OvmfPkg/VirtioScsiDxe/VirtioScsi.inf
>  INF  OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FvbServicesRuntimeDxe.inf
>  INF  OvmfPkg/EmuVariableFvbRuntimeDxe/Fvb.inf
>  INF  MdeModulePkg/Universal/FaultTolerantWriteDxe/FaultTolerantWriteDxe.inf
> +INF  OvmfPkg/XenIoPciDxe/XenIoPciDxe.inf
>  INF  OvmfPkg/XenBusDxe/XenBusDxe.inf
>  INF  OvmfPkg/XenPvBlkDxe/XenPvBlkDxe.inf
>  
> diff --git a/OvmfPkg/XenBusDxe/ComponentName.c 
> b/OvmfPkg/XenBusDxe/ComponentName.c
> index 4530509e65dc..3f2dd406c77d 100644
> --- a/OvmfPkg/XenBusDxe/ComponentName.c
> +++ b/OvmfPkg/XenBusDxe/ComponentName.c
> @@ -155,7 +155,7 @@ XenBusDxeComponentNameGetControllerName (
>    Status = EfiTestManagedDevice (
>               ControllerHandle,
>               gXenBusDxeDriverBinding.DriverBindingHandle,
> -             &gEfiPciIoProtocolGuid
> +             &gXenIoProtocolGuid
>               );
>    if (EFI_ERROR (Status)) {
>      return Status;
> diff --git a/OvmfPkg/XenBusDxe/GrantTable.c b/OvmfPkg/XenBusDxe/GrantTable.c
> index a80d5eff39cd..19117fbe0373 100644
> --- a/OvmfPkg/XenBusDxe/GrantTable.c
> +++ b/OvmfPkg/XenBusDxe/GrantTable.c
> @@ -139,8 +139,7 @@ XenGrantTableEndAccess (
>  
>  VOID
>  XenGrantTableInit (
> -  IN XENBUS_DEVICE  *Dev,
> -  IN UINT64         MmioAddr
> +  IN XENBUS_DEVICE  *Dev
>    )
>  {
>    xen_add_to_physmap_t Parameters;
> @@ -155,7 +154,7 @@ XenGrantTableInit (
>      XenGrantTablePutFreeEntry ((grant_ref_t)Index);
>    }
>  
> -  GrantTable = (VOID*)(UINTN) MmioAddr;
> +  GrantTable = (VOID*)(UINTN) Dev->XenIo->GrantTableAddress;
>    for (Index = 0; Index < NR_GRANT_FRAMES; Index++) {
>      Parameters.domid = DOMID_SELF;
>      Parameters.idx = Index;
> diff --git a/OvmfPkg/XenBusDxe/GrantTable.h b/OvmfPkg/XenBusDxe/GrantTable.h
> index 5772c56662df..194275ba7ed5 100644
> --- a/OvmfPkg/XenBusDxe/GrantTable.h
> +++ b/OvmfPkg/XenBusDxe/GrantTable.h
> @@ -29,8 +29,7 @@
>  **/
>  VOID
>  XenGrantTableInit (
> -  IN XENBUS_DEVICE  *Dev,
> -  IN UINT64         MmioAddr
> +  IN XENBUS_DEVICE  *Dev
>    );
>  
>  /**
> diff --git a/OvmfPkg/XenBusDxe/XenBus.c b/OvmfPkg/XenBusDxe/XenBus.c
> index f69c27dd184a..ee9526c33252 100644
> --- a/OvmfPkg/XenBusDxe/XenBus.c
> +++ b/OvmfPkg/XenBusDxe/XenBus.c
> @@ -138,7 +138,7 @@ XenBusAddDevice (
>    XENBUS_PRIVATE_DATA *Private;
>    EFI_STATUS Status;
>    XENBUS_DEVICE_PATH *TempXenBusPath;
> -  VOID *ChildPciIo;
> +  VOID *ChildXenIo;
>  
>    AsciiSPrint (DevicePath, sizeof (DevicePath),
>                 "device/%a/%a", Type, Id);
> @@ -208,8 +208,8 @@ XenBusAddDevice (
>      }
>  
>      Status = gBS->OpenProtocol (Dev->ControllerHandle,
> -               &gEfiPciIoProtocolGuid,
> -               &ChildPciIo, Dev->This->DriverBindingHandle,
> +               &gXenIoProtocolGuid,
> +               &ChildXenIo, Dev->This->DriverBindingHandle,
>                 Private->Handle,
>                 EFI_OPEN_PROTOCOL_BY_CHILD_CONTROLLER);
>      if (EFI_ERROR (Status)) {
> diff --git a/OvmfPkg/XenBusDxe/XenBusDxe.c b/OvmfPkg/XenBusDxe/XenBusDxe.c
> index cc334c086c1f..6474428b79e5 100644
> --- a/OvmfPkg/XenBusDxe/XenBusDxe.c
> +++ b/OvmfPkg/XenBusDxe/XenBusDxe.c
> @@ -23,8 +23,6 @@
>  
>  **/
>  
> -#include <IndustryStandard/Pci.h>
> -#include <IndustryStandard/Acpi.h>
>  #include <Library/DebugLib.h>
>  #include <Library/XenHypercallLib.h>
>  
> @@ -233,35 +231,23 @@ XenBusDxeDriverBindingSupported (
>    )
>  {
>    EFI_STATUS          Status;
> -  EFI_PCI_IO_PROTOCOL *PciIo;
> -  PCI_TYPE00          Pci;
> +  XENIO_PROTOCOL      *XenIo;
>  
>    Status = gBS->OpenProtocol (
>                       ControllerHandle,
> -                     &gEfiPciIoProtocolGuid,
> -                     (VOID **)&PciIo,
> +                     &gXenIoProtocolGuid,
> +                     (VOID **)&XenIo,
>                       This->DriverBindingHandle,
>                       ControllerHandle,
>                       EFI_OPEN_PROTOCOL_BY_DRIVER
>                       );
> +
>    if (EFI_ERROR (Status)) {
>      return Status;
>    }
>  
> -  Status = PciIo->Pci.Read (PciIo, EfiPciIoWidthUint32, 0,
> -                            sizeof Pci / sizeof (UINT32), &Pci);
> -
> -  if (Status == EFI_SUCCESS) {
> -    if (Pci.Hdr.VendorId == PCI_VENDOR_ID_XEN &&
> -        Pci.Hdr.DeviceId == PCI_DEVICE_ID_XEN_PLATFORM) {
> -      Status = EFI_SUCCESS;
> -    } else {
> -      Status = EFI_UNSUPPORTED;
> -    }
> -  }
> -
> -  gBS->CloseProtocol (ControllerHandle, &gEfiPciIoProtocolGuid,
> -         This->DriverBindingHandle, ControllerHandle);
> +  gBS->CloseProtocol (ControllerHandle, &gXenIoProtocolGuid,
> +    This->DriverBindingHandle, ControllerHandle);

Whitespace damage. The parameter should be indented one or two spaces
relative to the called member.

>  
>    return Status;
>  }
> @@ -326,19 +312,18 @@ XenBusDxeDriverBindingStart (
>  {
>    EFI_STATUS Status;
>    XENBUS_DEVICE *Dev;
> -  EFI_PCI_IO_PROTOCOL *PciIo;
> -  EFI_ACPI_ADDRESS_SPACE_DESCRIPTOR *BarDesc;
> -  UINT64 MmioAddr;
> +  XENIO_PROTOCOL *XenIo;
>    EFI_DEVICE_PATH_PROTOCOL *DevicePath;
>  
>    Status = gBS->OpenProtocol (
>                       ControllerHandle,
> -                     &gEfiPciIoProtocolGuid,
> -                     (VOID **) &PciIo,
> +                     &gXenIoProtocolGuid,
> +                     (VOID**)&XenIo,
>                       This->DriverBindingHandle,
>                       ControllerHandle,
>                       EFI_OPEN_PROTOCOL_BY_DRIVER
>                       );
> +
>    if (EFI_ERROR (Status)) {
>      return Status;
>    }
> @@ -360,7 +345,7 @@ XenBusDxeDriverBindingStart (
>    Dev->Signature = XENBUS_DEVICE_SIGNATURE;
>    Dev->This = This;
>    Dev->ControllerHandle = ControllerHandle;
> -  Dev->PciIo = PciIo;
> +  Dev->XenIo = XenIo;
>    Dev->DevicePath = DevicePath;
>    InitializeListHead (&Dev->ChildList);
>  
> @@ -376,20 +361,6 @@ XenBusDxeDriverBindingStart (
>    mMyDevice = Dev;
>    EfiReleaseLock (&mMyDeviceLock);
>  
> -  //
> -  // 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);
> -
> -  /* Get a Memory address for mapping the Grant Table. */
> -  DEBUG ((EFI_D_INFO, "XenBus: BAR at %LX\n", BarDesc->AddrRangeMin));
> -  MmioAddr = BarDesc->AddrRangeMin;
> -  FreePool (BarDesc);
> -
>    Status = XenGetSharedInfoPage (Dev);
>    if (EFI_ERROR (Status)) {
>      DEBUG ((EFI_D_ERROR, "XenBus: Unable to get the shared info page.\n"));
> @@ -397,7 +368,7 @@ XenBusDxeDriverBindingStart (
>      goto ErrorAllocated;
>    }
>  
> -  XenGrantTableInit (Dev, MmioAddr);
> +  XenGrantTableInit (Dev);
>  
>    Status = XenStoreInit (Dev);
>    ASSERT_EFI_ERROR (Status);
> @@ -417,7 +388,7 @@ ErrorAllocated:
>    gBS->CloseProtocol (ControllerHandle, &gEfiDevicePathProtocolGuid,
>                        This->DriverBindingHandle, ControllerHandle);
>  ErrorOpenningProtocol:
> -  gBS->CloseProtocol (ControllerHandle, &gEfiPciIoProtocolGuid,
> +  gBS->CloseProtocol (ControllerHandle, &gXenIoProtocolGuid,
>                        This->DriverBindingHandle, ControllerHandle);
>    return Status;
>  }
> @@ -507,7 +478,7 @@ XenBusDxeDriverBindingStop (
>  
>    gBS->CloseProtocol (ControllerHandle, &gEfiDevicePathProtocolGuid,
>           This->DriverBindingHandle, ControllerHandle);
> -  gBS->CloseProtocol (ControllerHandle, &gEfiPciIoProtocolGuid,
> +  gBS->CloseProtocol (ControllerHandle, &gXenIoProtocolGuid,
>           This->DriverBindingHandle, ControllerHandle);
>  
>    mMyDevice = NULL;
> diff --git a/OvmfPkg/XenBusDxe/XenBusDxe.h b/OvmfPkg/XenBusDxe/XenBusDxe.h
> index 9b7219906a69..6c306e017b07 100644
> --- a/OvmfPkg/XenBusDxe/XenBusDxe.h
> +++ b/OvmfPkg/XenBusDxe/XenBusDxe.h
> @@ -39,7 +39,7 @@
>  //
>  // Consumed Protocols
>  //
> -#include <Protocol/PciIo.h>
> +#include <Protocol/XenIo.h>
>  
>  
>  //
> @@ -73,10 +73,6 @@ extern EFI_COMPONENT_NAME_PROTOCOL  
> gXenBusDxeComponentName;
>  //
>  #include <IndustryStandard/Xen/xen.h>
>  
> -#define PCI_VENDOR_ID_XEN                0x5853
> -#define PCI_DEVICE_ID_XEN_PLATFORM       0x0001
> -
> -
>  typedef struct _XENBUS_DEVICE_PATH XENBUS_DEVICE_PATH;
>  typedef struct _XENBUS_DEVICE XENBUS_DEVICE;
>  
> @@ -86,7 +82,7 @@ struct _XENBUS_DEVICE {
>    UINT32                        Signature;
>    EFI_DRIVER_BINDING_PROTOCOL   *This;
>    EFI_HANDLE                    ControllerHandle;
> -  EFI_PCI_IO_PROTOCOL           *PciIo;
> +  XENIO_PROTOCOL                *XenIo;
>    EFI_EVENT                     ExitBootEvent;
>    EFI_DEVICE_PATH_PROTOCOL      *DevicePath;
>    LIST_ENTRY                    ChildList;
> diff --git a/OvmfPkg/XenBusDxe/XenBusDxe.inf b/OvmfPkg/XenBusDxe/XenBusDxe.inf
> index 714607dbd6f8..31553ac5a64a 100644
> --- a/OvmfPkg/XenBusDxe/XenBusDxe.inf
> +++ b/OvmfPkg/XenBusDxe/XenBusDxe.inf
> @@ -67,8 +67,8 @@
>  
>  [Protocols]
>    gEfiDriverBindingProtocolGuid
> -  gEfiPciIoProtocolGuid
>    gEfiComponentName2ProtocolGuid
>    gEfiComponentNameProtocolGuid
>    gXenBusProtocolGuid
> +  gXenIoProtocolGuid
>  
> 

I won't "audit" XenBusDxe, obviously, but if you covered all PciIo
occurrences with the above, then it should not regress anything.

Please restore the indentation.

Acked-by: Laszlo Ersek <lersek@xxxxxxxxxx>

Thanks
Laszlo

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


 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.