Maurice,
But LocatePciExpressCapabilityRegBlock() checks the PciIoDevice->IsPciExp in
the very beginning. So if the code runs to the while-loop, the device should be
a PCIe device. Then I do not understand why you said Pci.Read() failed due to
the access to a PCIe extended configuration register. Can you help to trace the
failure and tell me which line of code fails in pci.Read()?
Does the patch I sent to you earlier to change the
DuetPkg/PciRootBridgeNoEnumeration matter to your issue? If it matters, I will
have a strong reason to send the patch for formal review and check in it. If it
doesn't, I will not check in it.
Thanks,
Ray
-----Original Message-----
From: Ma, Maurice
Sent: Saturday, May 16, 2015 12:49 AM
To: Justen, Jordan L; Ni, Ruiyu; Agyeman, Prince
Cc: reza.jel...@tuhh.de; Zeng, Star; ler...@redhat.com;
edk2-devel@lists.sourceforge.net
Subject: RE: [edk2] [PATCH] CorebootPayloadPkg: Replace PciHostBridge driver
with PciRootBridgeNoEnumeration
Hi, Ray and Jordan,
Thank you for looking into this.
For the PciBus driver with PcdPciDisableBusEnumeration, the issue was caused
by the deadloop in PciBus driver LocatePciExpressCapabilityRegBlock() function.
PciBus driver should check the return status for
PciIoDevice->PciIo.Pci.Read() in LocatePciExpressCapabilityRegBlock()
function. If it fails, it should provide an exit condition gracefully.
In my case PciIo.Pci.Read() failed due to the access to a PCIe extended
configuration register. As a result , the CapabilityEntry will not be updated
and still keep the old value.
If the uninitialized local value CapabilityEntry has a big enough initial
value, the while loop will never end.
BTW, LocatePciExpressCapabilityRegBlock() is only used to support SRIO
virtualization in the standard PciBus driver. And these features are ON by
default.
For the PciBusNoEnumeration driver in DuetPkg , it does not support SRIO
virtualization and it won't encounter the same issue.
It explains why PciBusNoEnumeration worked, but PciBus didn't.
I tried to disable PCD PcdSrIovSupport/PcdAriSupport in DSC, then the standard
PciBus driver starts to work.
Given this fact I think it is possible to use the standard PciBus driver for
CorebootPayloadPkg.
Please refer to the code below:
EFI_STATUS
LocatePciExpressCapabilityRegBlock (
IN PCI_IO_DEVICE *PciIoDevice,
IN UINT16 CapId,
IN OUT UINT32 *Offset,
OUT UINT32 *NextRegBlock OPTIONAL
)
{
UINT32 CapabilityPtr;
UINT32 CapabilityEntry;
UINT16 CapabilityID;
//
// To check the capability of this device supports
//
if (!PciIoDevice->IsPciExp) {
return EFI_UNSUPPORTED;
}
if (*Offset != 0) {
CapabilityPtr = *Offset;
} else {
CapabilityPtr = EFI_PCIE_CAPABILITY_BASE_OFFSET;
}
while (CapabilityPtr != 0) {
//
// Mask it to DWORD alignment per PCI spec
//
CapabilityPtr &= 0xFFC;
PciIoDevice->PciIo.Pci.Read (
&PciIoDevice->PciIo,
EfiPciIoWidthUint32,
CapabilityPtr,
1,
&CapabilityEntry
);
CapabilityID = (UINT16) CapabilityEntry;
if (CapabilityID == CapId) {
*Offset = CapabilityPtr;
if (NextRegBlock != NULL) {
*NextRegBlock = (CapabilityEntry >> 20) & 0xFFF;
}
return EFI_SUCCESS;
}
CapabilityPtr = (CapabilityEntry >> 20) & 0xFFF;
}
return EFI_NOT_FOUND;
}
For the PciHostBridgeDxe, we still have some gaps here for CorebootPayloadPkg.
1. The current PcAtChipsetPkg/PciHostBridgeDxe/PciHostBridgeDxe does not
support PCIe extended configuration register access through the RootBridgeIo
protocol.
It sets the register offset limit to 0xFF to prevent any extended space
access. And since it uses PciWrite8/16/32, it also depends on project
specific PciLib library class mapping.
2. The current PcAtChipsetPkg/PciHostBridgeDxe/PciHostBridgeDxe does not
provide valid the PCI resource configuration for the root bridges. It is
because the PciBus driver
will not call many host bridge interfaces with PcdPciDisableBusEnumeration
set to TRUE. As a result the configurations are all in their initial default
values.
The driver PciRootBridgeNoEnumerationDxe in DuetPkg actually does a
scanning for all PCI resources during the initialization and then updates the
resource configuration
accordingly using the scanning results. In this case the PCI bus ranges,
memory IO resources match the actual hardware configuration.
It explains why Shell can list PCI devices on bus 1 and above with
PciRootBridgeNoEnumerationDxe driver, but not with PciHostBridgeDxe driver.
I believe the same limitations exist for OvmfPkg if it uses the
PciHostBridgeDxe driver.
Thanks
Maurice
-----Original Message-----
From: Justen, Jordan L
Sent: Thursday, May 14, 2015 8:42 PM
To: Ni, Ruiyu; Ma, Maurice; Agyeman, Prince
Cc: reza.jel...@tuhh.de; Zeng, Star; ler...@redhat.com;
edk2-devel@lists.sourceforge.net
Subject: RE: [edk2] [PATCH] CorebootPayloadPkg: Replace PciHostBridge driver
with PciRootBridgeNoEnumeration
On 2015-05-14 19:17:26, Ni, Ruiyu wrote:
> Maurice,
> When PcdPciDisableBusEnumeration is TRUE, PciBus driver assumes the
> bus number and IO/MMIO BAR are all initialized before it gets started,
> so it doesn't call SetBusNumbers(). It's also true for
> PciBusNoEnumeration driver in DuetPkg.
> I compared the PciBus and PciBusNoEnumeration before, the major gap is
> PciBus won't dispatch the option roms collected by
> PciRootBridgeNoEnumeration driver.
Can modify MdeModulePkg/Bus/Pci/PciBusDxe and PcAtChipsetPkg/PciHostBridgeDxe
so they can work for DuetPkg, CorebootPayloadPkg and OvmfPkg? (Note that
OvmfPkg already uses them.)
I think it is better to get all these platforms using common code, I it doesn't
seem like the gap is that big.
Maybe MdeModulePkg/Bus/Pci/PciBusDxe or
PcAtChipsetPkg/PciHostBridgeDxe could use additional PCDs to close the gap on
the differences preventing DuetPkg and CorebootPayloadPkg from using these two
modules?
> Another gap is the PciRootBridgeNoEnumeration needs a slightly change
> in order to work with PciBus driver. Can you try this patch?
You mean this should allow CorebootPayloadPkg to use
MdeModulePkg/Bus/Pci/PciBusDxe? What about DuetPkg as well?
Also, what changes would PcAtChipsetPkg/PciHostBridgeDxe need to be able to
replace DuetPkg/PciRootBridgeNoEnumerationDxe for CorebootPayloadPkg and
DuetPkg?
Thanks,
-Jordan
> -----Original Message-----
> From: Ma, Maurice
> Sent: Saturday, May 9, 2015 6:33 AM
> To: Justen, Jordan L; Agyeman, Prince
> Cc: reza.jel...@tuhh.de; Zeng, Star; Ni, Ruiyu; ler...@redhat.com;
> edk2-devel@lists.sourceforge.net
> Subject: RE: [edk2] [PATCH] CorebootPayloadPkg: Replace PciHostBridge
> driver with PciRootBridgeNoEnumeration
>
> Hi, Jordan,
>
> Thank you for your feedback.
>
> Ideally I don't want CorebootPayloadPkg to depend on DuetPkg. But on the
> other side I hesitate to duplicate drivers into the CorebootPayloadPkg from
> other packages.
> If I have to make a choice, I'd like the former.
>
> I just tried to use the standard PciBus driver with
> PcdPciDisableBusEnumeration set to TRUE. However It caused boot failure on
> my Baytrail platform. I think it might needs some slightly changes to make
> it behavior the same way as PciBusNoEnumeration in DuetPkg.
>
> However, even with the standard PciBus driver, I still need a
> PciRootBridgeNoEnumeration equivalent driver. By looking into the PciBus
> driver code, I found PciBus driver will not call PCI host bridge resource
> allocation interface SetBusNumbers() when PcdPciDisableBusEnumeration is set
> to TRUE. As a result the PCI root bridge bus range is always 0 because
> nobody will updated it. In this case Shell PCI command will think only bus
> 0 exists and no devices on other buses will be listed.
>
> For SataControllerDxe driver, I don't want to duplicate it
> CorebootPayloadPkg again if it has been done in OvmfPkg.
>
> Unless we have a more functional common drivers to replace the current ones
> used in CorebootPayloadPkg, I would prefer the current way.
>
> Please let me know if you have any other ideas.
>
> Thanks
> Maurice
>
> -----Original Message-----
> From: Justen, Jordan L
> Sent: Friday, May 08, 2015 1:03 PM
> To: Ma, Maurice; edk2-devel@lists.sourceforge.net; Agyeman, Prince
> Cc: reza.jel...@tuhh.de; Zeng, Star; Ni, Ruiyu; ler...@redhat.com
> Subject: Re: [edk2] [PATCH] CorebootPayloadPkg: Replace PciHostBridge
> driver with PciRootBridgeNoEnumeration
>
> Can you use git send-email to send your patches? (Note the email
> double-space strangeness below)
>
> I don't think CorebootPayloadPkg should depend on DuetPkg.
>
> Regarding the PciEnumeration issue, in OVMF we set
> gEfiMdeModulePkgTokenSpaceGuid.PcdPciDisableBusEnumeration to TRUE at runtime
> for Xen to disable enumeration. Maybe CorebootPayloadPkg could just set it as
> a fixed PCD to TRUE?
>
> Regarding using DuetPkg/SataControllerDxe in CorebootPayloadPkg, well, we
> tried to create a common module under PcAtChipsetPkg to share with OVMF, but
> it was rejected.
>
> The recommendation was to duplicate the code in OVMF:
> http://permalink.gmane.org/gmane.comp.bios.tianocore.devel/9036
>
> Shouldn't CorebootPayloadPkg then do something similar?
>
> One unfortunate outcome of the SataControllerDxe discussion for OVMF is that
> we still haven't managed to enable SATA in OVMF... But, I think this is maybe
> because Reza got tired of trying to work with us.
> :)
>
> -Jordan
>
> On 2015-05-08 09:02:11, Ma, Maurice wrote:
> > Current CorebootPayloadPkg uses PciHostBridge and
> > PciBusNoEnumeration
> >
> > driver. It will cause the PCI bus resource incorrectly set in
> > root
> >
> > bridge instance. As a result all PCI devices behind a PCI bridge
> > will
> >
> > not show up in Shell 'PCI' command.
> >
> >
> >
> > To resolve it use PciRootBridgeNoEnumeration driver instead.
> >
> >
> >
> > Contributed-under: TianoCore Contribution Agreement 1.0
> >
> > Signed-off-by: Maurice Ma <maurice...@intel.com>
> >
> > ---
> >
> > CorebootPayloadPkg/CorebootPayloadPkg.fdf | 2 +-
> >
> > CorebootPayloadPkg/CorebootPayloadPkgIa32.dsc | 2 +-
> >
> > CorebootPayloadPkg/CorebootPayloadPkgIa32X64.dsc | 4 ++--
> >
> > 3 files changed, 4 insertions(+), 4 deletions(-)
> >
> >
> >
> > diff --git a/CorebootPayloadPkg/CorebootPayloadPkg.fdf
> > b/CorebootPayloadPkg/CorebootPayloadPkg.fdf
> >
> > index a1e72e7..810dcb1 100644
> >
> > --- a/CorebootPayloadPkg/CorebootPayloadPkg.fdf
> >
> > +++ b/CorebootPayloadPkg/CorebootPayloadPkg.fdf
> >
> > @@ -110,7 +110,7 @@ INF
> > MdeModulePkg/Universal/SmbiosDxe/SmbiosDxe.inf
> >
> > #
> >
> > # PCI Support
> >
> > #
> >
> > -INF PcAtChipsetPkg/PciHostBridgeDxe/PciHostBridgeDxe.inf
> >
> > +INF
> > DuetPkg/PciRootBridgeNoEnumerationDxe/PciRootBridgeNoEnumeration.inf
> >
> > INF DuetPkg/PciBusNoEnumerationDxe/PciBusNoEnumeration.inf
> >
> >
> >
> > #
> >
> > diff --git a/CorebootPayloadPkg/CorebootPayloadPkgIa32.dsc
> > b/CorebootPayloadPkg/CorebootPayloadPkgIa32.dsc
> >
> > index feae027..a661f5d 100644
> >
> > --- a/CorebootPayloadPkg/CorebootPayloadPkgIa32.dsc
> >
> > +++ b/CorebootPayloadPkg/CorebootPayloadPkgIa32.dsc
> >
> > @@ -312,7 +312,7 @@
> >
> > #
> >
> > # PCI Support
> >
> > #
> >
> > - PcAtChipsetPkg/PciHostBridgeDxe/PciHostBridgeDxe.inf
> >
> > +
> > DuetPkg/PciRootBridgeNoEnumerationDxe/PciRootBridgeNoEnumeration.inf
> >
> > DuetPkg/PciBusNoEnumerationDxe/PciBusNoEnumeration.inf
> >
> >
> >
> > #
> >
> > diff --git a/CorebootPayloadPkg/CorebootPayloadPkgIa32X64.dsc
> > b/CorebootPayloadPkg/CorebootPayloadPkgIa32X64.dsc
> >
> > index d954666..ecd12fb 100644
> >
> > --- a/CorebootPayloadPkg/CorebootPayloadPkgIa32X64.dsc
> >
> > +++ b/CorebootPayloadPkg/CorebootPayloadPkgIa32X64.dsc
> >
> > @@ -313,8 +313,8 @@
> >
> >
> >
> > #
> >
> > # PCI Support
> >
> > - #
> >
> > - PcAtChipsetPkg/PciHostBridgeDxe/PciHostBridgeDxe.inf
> >
> > + #
> >
> > +
> > DuetPkg/PciRootBridgeNoEnumerationDxe/PciRootBridgeNoEnumeration.inf
> >
> > DuetPkg/PciBusNoEnumerationDxe/PciBusNoEnumeration.inf
> >
> >
> >
> > #
> >
> > --
> >
> > 1.8.3.1
> >
> >
> >
> > Thanks
> >
> > Maurice
------------------------------------------------------------------------------
One dashboard for servers and applications across Physical-Virtual-Cloud
Widest out-of-the-box monitoring support with 50+ applications
Performance metrics, stats and reports that give you Actionable Insights
Deep dive visibility with transaction tracing using APM Insight.
http://ad.doubleclick.net/ddm/clk/290420510;117567292;y
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/edk2-devel
