Re: [Xen-devel] [edk2] [PATCH] MdeModulePkg: mark completion of PCI enumeration in PciEnumeratorLight

["Ni, Ruiyu" <ruiyu.ni@xxxxxxxxx>]

Jordan,
You are correct that subsequent installations of this protocol will fail.

But I don't think locating this protocol before installing it is a good 
implementation choice. This would make the code confusing.

Thanks,
Ray

-----Original Message-----
From: Justen, Jordan L 
Sent: Friday, February 13, 2015 5:03 AM
To: edk2-devel@xxxxxxxxxxxxxxxxxxxxx; Ni, Ruiyu; wei.liu2@xxxxxxxxxx
Cc: xen-devel@xxxxxxxxxxxxx
Subject: Re: [edk2] [PATCH] MdeModulePkg: mark completion of PCI enumeration in 
PciEnumeratorLight

On 2015-02-11 17:23:26, Ni, Ruiyu wrote:
> Wei,
> No you cannot install gEfiPciEnumerationCompleteProtocolGuid in
> PciEnumeratorLight().
> For a real platform, PCI BUS is fully enumerated in PciEnumerator()
> and later if reconnect happens, it's light enumerated in
> PciEnumeratorLight(). The protocol should only be installed once in
> PeiEnumerator(). Your fix will cause this protocol installed every
> time a reconnect happens.

I don't think it will, since the protocol is already installed on the
Host Bridge Handle, I think EFI_INVALID_PARAMETER will be returned on
subsequent calls to install the protocol.

But, getting that error back will lead to PciEnumeratorLight returning
an error, and this could cause other issues.

> The protocol 's meaning is that the PCI BUS is fully enumerated. If
> the PCI BUS is fully enumerated before starting PciBus driver, light
> PCI enumeration is used.
> For your OVMF/QEMU case, an alternative fix is to install this
> protocol in a platform driver when it detects that the PCI BUS is
> fully enumerated.

I think the PciBusDxe driver is still the right place to install the
protocol, but I agree we need to be careful to prevent trying to
install the protocol multiple times.

I guess we could try to locate the protocol on the Host Bridge Handle
before installing it to prevent multiple installation attempts.

-Jordan

> -----Original Message-----
> From: Wei Liu [mailto:wei.liu2@xxxxxxxxxx] 
> Sent: Thursday, February 12, 2015 4:24 AM
> To: edk2-devel@xxxxxxxxxxxxxxxxxxxxx
> Cc: xen-devel@xxxxxxxxxxxxx; Laszlo
> Subject: [edk2] [PATCH] MdeModulePkg: mark completion of PCI enumeration in 
> PciEnumeratorLight
> 
> I had an issue when trying to boot Xen HVM guest with latest OVMF
> master. Guest crashed with memory violation, and the bisection pointed
> to 66b280df2 ("OvmfPkg: AcpiPlatformDxe: make dependency on PCI
> enumeration explicit"). That commit made AcpiPlatformDxe depend on PCI
> enumeration using gEfiPciEnumerationCompleteProtocolGuid, which is a
> very reasonable change.
> 
> The real culprit is that Xen HVM is using PciEnumeratorLight which
> doesn't install gEfiPciEnumerationCompleteProtocolGuid. This, in
> combination with 66b280df2, makes AcpiPlatformDxe not able to be loaded,
> resulting in guest crash.
> 
> The fix is to install gEfiPciEnumerationCompleteProtocolGuid in
> PciEnumeratorLight.
> 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Wei Liu <wei.liu2@xxxxxxxxxx>
> Cc: Feng Tian <feng.tian@xxxxxxxxx>
> Cc: Anthony Perard <anthony.perard@xxxxxxxxxx>
> Cc: Laszlo Ersek <lersek@xxxxxxxxxx>
> Cc: Jordan Justen <jordan.l.justen@xxxxxxxxx>
> ---
>  MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumeratorSupport.c | 15 ++++++++++++++-
>  1 file changed, 14 insertions(+), 1 deletion(-)
> 
> diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumeratorSupport.c 
> b/MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumeratorSupport.c
> index 9e7ac74..7659585 100644
> --- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumeratorSupport.c
> +++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumeratorSupport.c
> @@ -2256,6 +2256,7 @@ PciEnumeratorLight (
>  {
>  
>    EFI_STATUS                        Status;
> +  EFI_HANDLE                        HostBridgeHandle;
>    EFI_PCI_ROOT_BRIDGE_IO_PROTOCOL   *PciRootBridgeIo;
>    PCI_IO_DEVICE                     *RootBridgeDev;
>    UINT16                            MinBus;
> @@ -2288,6 +2289,11 @@ PciEnumeratorLight (
>      return Status;
>    }
>  
> +  //
> +  // Get the host bridge handle
> +  //
> +  HostBridgeHandle = PciRootBridgeIo->ParentHandle;
> +
>    Status = PciRootBridgeIo->Configuration (PciRootBridgeIo, (VOID **) 
> &Descriptors);
>  
>    if (EFI_ERROR (Status)) {
> @@ -2348,7 +2354,14 @@ PciEnumeratorLight (
>      Descriptors++;
>    }
>  
> -  return EFI_SUCCESS;
> +  Status = gBS->InstallProtocolInterface (
> +                  &HostBridgeHandle,
> +                  &gEfiPciEnumerationCompleteProtocolGuid,
> +                  EFI_NATIVE_INTERFACE,
> +                  NULL
> +                  );
> +
> +  return Status;
>  }
>  
>  /**
> -- 
> 1.9.1
> 
> 
> ------------------------------------------------------------------------------
> Dive into the World of Parallel Programming. The Go Parallel Website,
> sponsored by Intel and developed in partnership with Slashdot Media, is your
> hub for all things parallel software development, from weekly thought
> leadership blogs to news, videos, case studies, tutorials and more. Take a
> look and join the conversation now. http://goparallel.sourceforge.net/
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@xxxxxxxxxxxxxxxxxxxxx
> https://lists.sourceforge.net/lists/listinfo/edk2-devel
> 
> ------------------------------------------------------------------------------
> Dive into the World of Parallel Programming. The Go Parallel Website,
> sponsored by Intel and developed in partnership with Slashdot Media, is your
> hub for all things parallel software development, from weekly thought
> leadership blogs to news, videos, case studies, tutorials and more. Take a
> look and join the conversation now. http://goparallel.sourceforge.net/
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@xxxxxxxxxxxxxxxxxxxxx
> https://lists.sourceforge.net/lists/listinfo/edk2-devel
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel