Re: [edk2-devel] [PATCH for-next] MdeModulePkg/PciBusDxe: catch unimplemented extended config space reads
On 06/04/19 23:44, Laszlo Ersek wrote:
> When assigning a physical PCIe device to a QEMU/KVM guest, PciBusDxe may
> find that the extended config space is not (fully) implemented. In
> LocatePciExpressCapabilityRegBlock(), "CapabilityEntry" may be read as
> 0x_ at a given config space offset, after which the loop gets
> stuck spinning on offset 0xFFC (the read at offset 0xFFC returns
> 0x_ most likely as well).
>
> Another scenario (not related to virtualization) for triggering the above
> is when a Conventional PCI bus -- exposed by a PCIe-to-PCI bridge in the
> topology -- intervenes between a PCI Express Root Port and a PCI Express
> Endpoint. The Conventional PCI bus limits the accessible config space of
> the PCI Express Endpoint, even though the endpoint advertizes the PCI
> Express capability. Here's a diagram, courtesy of Alex Williamson:
>
> [PCIe Root Port]--[PCIe-to-PCI]--[PCI-to-PCIe]--[PCIe EP]
> ->| |<- Conventional PCI bus
>
> Catch reads of 0x_ in LocatePciExpressCapabilityRegBlock(), and
> break out of the scan with a warning message. The function will return
> EFI_NOT_FOUND.
>
> Cc: Alex Williamson
> Cc: Hao A Wu
> Cc: Jian J Wang
> Cc: Ray Ni
> Cc: Star Zeng
> Signed-off-by: Laszlo Ersek
> ---
>
> Notes:
> Repo: https://github.com/lersek/edk2.git
> Branch: pcibus_no_ext_conf
>
> MdeModulePkg/Bus/Pci/PciBusDxe/PciCommand.c | 13 +
> 1 file changed, 13 insertions(+)
>
> diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciCommand.c
> b/MdeModulePkg/Bus/Pci/PciBusDxe/PciCommand.c
> index 214aeecdd40a..6283d602207c 100644
> --- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciCommand.c
> +++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciCommand.c
> @@ -236,6 +236,19 @@ LocatePciExpressCapabilityRegBlock (
>break;
> }
>
> +if (CapabilityEntry == MAX_UINT32) {
> + DEBUG ((
> +DEBUG_WARN,
> +"%a: [%02x|%02x|%02x] failed to access config space at offset
> 0x%x\n",
> +__FUNCTION__,
> +PciIoDevice->BusNumber,
> +PciIoDevice->DeviceNumber,
> +PciIoDevice->FunctionNumber,
> +CapabilityPtr
> +));
> + break;
> +}
> +
> CapabilityID = (UINT16) CapabilityEntry;
>
> if (CapabilityID == CapId) {
>
Thank you everyone, patch pushed as commit e5b4d825afc4.
Laszlo
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#42219): https://edk2.groups.io/g/devel/message/42219
Mute This Topic: https://groups.io/mt/31931246/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH for-next] MdeModulePkg/PciBusDxe: catch unimplemented extended config space reads
Reviewed-by: Ray Ni
> -Original Message-
> From: Wu, Hao A
> Sent: Monday, June 10, 2019 3:04 PM
> To: devel@edk2.groups.io; ler...@redhat.com; Ni, Ray
> Cc: Alex Williamson ; Wang, Jian J
> ; Ard Biesheuvel ; Zeng,
> Star
> Subject: RE: [edk2-devel] [PATCH for-next] MdeModulePkg/PciBusDxe:
> catch unimplemented extended config space reads
>
> Hello Ray,
>
> Do you have comments on this patch?
>
> > -Original Message-
> > From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of
> > Laszlo Ersek
> > Sent: Wednesday, June 05, 2019 6:15 PM
> > To: Ard Biesheuvel; edk2-devel-groups-io
> > Cc: Alex Williamson; Wu, Hao A; Wang, Jian J; Ni, Ray; Zeng, Star
> > Subject: Re: [edk2-devel] [PATCH for-next] MdeModulePkg/PciBusDxe:
> > catch unimplemented extended config space reads
> >
> > On 06/05/19 11:25, Ard Biesheuvel wrote:
> > > On Tue, 4 Jun 2019 at 23:44, Laszlo Ersek wrote:
> > >>
> > >> When assigning a physical PCIe device to a QEMU/KVM guest,
> > >> PciBusDxe
> > may
> > >> find that the extended config space is not (fully) implemented. In
> > >> LocatePciExpressCapabilityRegBlock(), "CapabilityEntry" may be read
> > >> as 0x_ at a given config space offset, after which the loop
> > >> gets stuck spinning on offset 0xFFC (the read at offset 0xFFC
> > >> returns 0x_ most likely as well).
> > >>
> > >> Another scenario (not related to virtualization) for triggering the
> > >> above is when a Conventional PCI bus -- exposed by a PCIe-to-PCI
> > >> bridge in the topology -- intervenes between a PCI Express Root
> > >> Port and a PCI Express Endpoint. The Conventional PCI bus limits
> > >> the accessible config space of the PCI Express Endpoint, even
> > >> though the endpoint advertizes the PCI Express capability. Here's a
> diagram, courtesy of Alex Williamson:
> > >>
> > >> [PCIe Root Port]--[PCIe-to-PCI]--[PCI-to-PCIe]--[PCIe EP]
> > >> ->| |<- Conventional PCI bus
> > >>
> > >> Catch reads of 0x_ in LocatePciExpressCapabilityRegBlock(),
> > >> and break out of the scan with a warning message. The function will
> > >> return EFI_NOT_FOUND.
> > >>
> > >> Cc: Alex Williamson
> > >> Cc: Hao A Wu
> > >> Cc: Jian J Wang
> > >> Cc: Ray Ni
> > >> Cc: Star Zeng
> > >> Signed-off-by: Laszlo Ersek
> > >> ---
> > >>
> > >> Notes:
> > >> Repo: https://github.com/lersek/edk2.git
> > >> Branch: pcibus_no_ext_conf
> > >>
> > >> MdeModulePkg/Bus/Pci/PciBusDxe/PciCommand.c | 13
> > +
> > >> 1 file changed, 13 insertions(+)
> > >>
> > >> diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciCommand.c
> > b/MdeModulePkg/Bus/Pci/PciBusDxe/PciCommand.c
> > >> index 214aeecdd40a..6283d602207c 100644
> > >> --- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciCommand.c
> > >> +++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciCommand.c
> > >> @@ -236,6 +236,19 @@ LocatePciExpressCapabilityRegBlock (
> > >>break;
> > >> }
> > >>
> > >> +if (CapabilityEntry == MAX_UINT32) {
> > >
> > > Should we check here that the offset > 0x100 ? Otherwise, this
> > > affects more than just the extended config space.
> >
> > A separate function exists for locating caps in the conventional
> > config space (LocateCapabilityRegBlock()).
> >
> > Whereas the function being patched --
> > LocatePciExpressCapabilityRegBlock() -- is supposed to start with a
> > capability offset into the extended config space, passed in by the
> > caller via *Offset, or else at 0x100 if *Offset is 0.
> >
> > And, my understanding is that an extended cap shall never chain to a
> > conventional cap. The spec says,
> >
> > Next Capability Offset - This field contains the offset to the next
> > PCI Express Capability structure or 000h if no other items exist in
> > the linked list of Capabilities.
> >
> > For Extended Capabilities implemented in Configuration Space, this
> > offset is relative to the beginning of PCI compatible Configuration
> > Space and thus must always be either 000h (for terminating list of
> > Capabilities) or greater than 0FFh.
> >
> > The bott
Re: [edk2-devel] [PATCH for-next] MdeModulePkg/PciBusDxe: catch unimplemented extended config space reads
Hello Ray,
Do you have comments on this patch?
> -Original Message-
> From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of
> Laszlo Ersek
> Sent: Wednesday, June 05, 2019 6:15 PM
> To: Ard Biesheuvel; edk2-devel-groups-io
> Cc: Alex Williamson; Wu, Hao A; Wang, Jian J; Ni, Ray; Zeng, Star
> Subject: Re: [edk2-devel] [PATCH for-next] MdeModulePkg/PciBusDxe:
> catch unimplemented extended config space reads
>
> On 06/05/19 11:25, Ard Biesheuvel wrote:
> > On Tue, 4 Jun 2019 at 23:44, Laszlo Ersek wrote:
> >>
> >> When assigning a physical PCIe device to a QEMU/KVM guest, PciBusDxe
> may
> >> find that the extended config space is not (fully) implemented. In
> >> LocatePciExpressCapabilityRegBlock(), "CapabilityEntry" may be read as
> >> 0x_ at a given config space offset, after which the loop gets
> >> stuck spinning on offset 0xFFC (the read at offset 0xFFC returns
> >> 0x_ most likely as well).
> >>
> >> Another scenario (not related to virtualization) for triggering the above
> >> is when a Conventional PCI bus -- exposed by a PCIe-to-PCI bridge in the
> >> topology -- intervenes between a PCI Express Root Port and a PCI Express
> >> Endpoint. The Conventional PCI bus limits the accessible config space of
> >> the PCI Express Endpoint, even though the endpoint advertizes the PCI
> >> Express capability. Here's a diagram, courtesy of Alex Williamson:
> >>
> >> [PCIe Root Port]--[PCIe-to-PCI]--[PCI-to-PCIe]--[PCIe EP]
> >> ->| |<- Conventional PCI bus
> >>
> >> Catch reads of 0x_ in LocatePciExpressCapabilityRegBlock(), and
> >> break out of the scan with a warning message. The function will return
> >> EFI_NOT_FOUND.
> >>
> >> Cc: Alex Williamson
> >> Cc: Hao A Wu
> >> Cc: Jian J Wang
> >> Cc: Ray Ni
> >> Cc: Star Zeng
> >> Signed-off-by: Laszlo Ersek
> >> ---
> >>
> >> Notes:
> >> Repo: https://github.com/lersek/edk2.git
> >> Branch: pcibus_no_ext_conf
> >>
> >> MdeModulePkg/Bus/Pci/PciBusDxe/PciCommand.c | 13
> +
> >> 1 file changed, 13 insertions(+)
> >>
> >> diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciCommand.c
> b/MdeModulePkg/Bus/Pci/PciBusDxe/PciCommand.c
> >> index 214aeecdd40a..6283d602207c 100644
> >> --- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciCommand.c
> >> +++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciCommand.c
> >> @@ -236,6 +236,19 @@ LocatePciExpressCapabilityRegBlock (
> >>break;
> >> }
> >>
> >> +if (CapabilityEntry == MAX_UINT32) {
> >
> > Should we check here that the offset > 0x100 ? Otherwise, this affects
> > more than just the extended config space.
>
> A separate function exists for locating caps in the conventional config
> space (LocateCapabilityRegBlock()).
>
> Whereas the function being patched --
> LocatePciExpressCapabilityRegBlock() -- is supposed to start with a
> capability offset into the extended config space, passed in by the
> caller via *Offset, or else at 0x100 if *Offset is 0.
>
> And, my understanding is that an extended cap shall never chain to a
> conventional cap. The spec says,
>
> Next Capability Offset – This field contains the offset to the next
> PCI Express Capability structure or 000h if no other items exist in
> the linked list of Capabilities.
>
> For Extended Capabilities implemented in Configuration Space, this
> offset is relative to the beginning of PCI compatible Configuration
> Space and thus must always be either 000h (for terminating list of
> Capabilities) or greater than 0FFh.
>
> The bottom 2 bits of this offset are Reserved and must be
> implemented as 00b although software must mask them to allow for
> future uses of these bits.
>
> Additionally, the capability header is different for conventional
> capabilities: EFI_PCI_CAPABILITY_HDR -- 2 bytes -- vs.
> PCI_EXPRESS_EXTENDED_CAPABILITIES_HEADER -- 4 bytes. So if this loop
> ever crossed over into normal config space, it would break horribly,
> regardless of this patch.
>
> A more general question would be how much we should armor such
> functions
> -- i.e., capability list scanning -- with sanity checks.
>
> My answer to that was authoring PciCapLib, which detects loops in cap
> lists, oversized capability reads/writes, an absent extended config
> space in spite of a present express capability; maybe m
Re: [edk2-devel] [PATCH for-next] MdeModulePkg/PciBusDxe: catch unimplemented extended config space reads
On Wed, 5 Jun 2019 at 12:15, Laszlo Ersek wrote:
>
> On 06/05/19 11:25, Ard Biesheuvel wrote:
> > On Tue, 4 Jun 2019 at 23:44, Laszlo Ersek wrote:
> >>
> >> When assigning a physical PCIe device to a QEMU/KVM guest, PciBusDxe may
> >> find that the extended config space is not (fully) implemented. In
> >> LocatePciExpressCapabilityRegBlock(), "CapabilityEntry" may be read as
> >> 0x_ at a given config space offset, after which the loop gets
> >> stuck spinning on offset 0xFFC (the read at offset 0xFFC returns
> >> 0x_ most likely as well).
> >>
> >> Another scenario (not related to virtualization) for triggering the above
> >> is when a Conventional PCI bus -- exposed by a PCIe-to-PCI bridge in the
> >> topology -- intervenes between a PCI Express Root Port and a PCI Express
> >> Endpoint. The Conventional PCI bus limits the accessible config space of
> >> the PCI Express Endpoint, even though the endpoint advertizes the PCI
> >> Express capability. Here's a diagram, courtesy of Alex Williamson:
> >>
> >> [PCIe Root Port]--[PCIe-to-PCI]--[PCI-to-PCIe]--[PCIe EP]
> >> ->| |<- Conventional PCI bus
> >>
> >> Catch reads of 0x_ in LocatePciExpressCapabilityRegBlock(), and
> >> break out of the scan with a warning message. The function will return
> >> EFI_NOT_FOUND.
> >>
> >> Cc: Alex Williamson
> >> Cc: Hao A Wu
> >> Cc: Jian J Wang
> >> Cc: Ray Ni
> >> Cc: Star Zeng
> >> Signed-off-by: Laszlo Ersek
> >> ---
> >>
> >> Notes:
> >> Repo: https://github.com/lersek/edk2.git
> >> Branch: pcibus_no_ext_conf
> >>
> >> MdeModulePkg/Bus/Pci/PciBusDxe/PciCommand.c | 13 +
> >> 1 file changed, 13 insertions(+)
> >>
> >> diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciCommand.c
> >> b/MdeModulePkg/Bus/Pci/PciBusDxe/PciCommand.c
> >> index 214aeecdd40a..6283d602207c 100644
> >> --- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciCommand.c
> >> +++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciCommand.c
> >> @@ -236,6 +236,19 @@ LocatePciExpressCapabilityRegBlock (
> >>break;
> >> }
> >>
> >> +if (CapabilityEntry == MAX_UINT32) {
> >
> > Should we check here that the offset > 0x100 ? Otherwise, this affects
> > more than just the extended config space.
>
> A separate function exists for locating caps in the conventional config
> space (LocateCapabilityRegBlock()).
>
> Whereas the function being patched --
> LocatePciExpressCapabilityRegBlock() -- is supposed to start with a
> capability offset into the extended config space, passed in by the
> caller via *Offset, or else at 0x100 if *Offset is 0.
>
> And, my understanding is that an extended cap shall never chain to a
> conventional cap. The spec says,
>
> Next Capability Offset – This field contains the offset to the next
> PCI Express Capability structure or 000h if no other items exist in
> the linked list of Capabilities.
>
> For Extended Capabilities implemented in Configuration Space, this
> offset is relative to the beginning of PCI compatible Configuration
> Space and thus must always be either 000h (for terminating list of
> Capabilities) or greater than 0FFh.
>
> The bottom 2 bits of this offset are Reserved and must be
> implemented as 00b although software must mask them to allow for
> future uses of these bits.
>
> Additionally, the capability header is different for conventional
> capabilities: EFI_PCI_CAPABILITY_HDR -- 2 bytes -- vs.
> PCI_EXPRESS_EXTENDED_CAPABILITIES_HEADER -- 4 bytes. So if this loop
> ever crossed over into normal config space, it would break horribly,
> regardless of this patch.
>
> A more general question would be how much we should armor such functions
> -- i.e., capability list scanning -- with sanity checks.
>
> My answer to that was authoring PciCapLib, which detects loops in cap
> lists, oversized capability reads/writes, an absent extended config
> space in spite of a present express capability; maybe more. Basically
> everything I could think of and/or had encountered by then.
>
> You probably remember that I originally attempted to get PciCapLib and
> its accessories into MdePkg, with an intent to rebase core PCI drivers
> to them -- including PciBusDxe. (The original "sales pitch" can be found
> at
> .)
> I hadn't received either positive or negative feedback regarding that
> idea for a month or so, after which we merged the library into OvmfPkg,
> in the end. (And it is now used by ArmVirtQemu* and OVMF only, as part
> of OvmfPkg/PciHotPlugInitDxe and OvmfPkg/Virtio10Dxe).
>
> I did file a longer-term reminder BZ at
> . But, I gave up on
> that as well in about 4 months.
>
> The upshot is that now I can only contribute piecemeal fixes for
> PciBusDxe, whenever I come across something. This particular issue has
> bitten us at RH twice by now -- unfortunately, both RHBZs are private,
> h
Re: [edk2-devel] [PATCH for-next] MdeModulePkg/PciBusDxe: catch unimplemented extended config space reads
On 06/05/19 11:25, Ard Biesheuvel wrote:
> On Tue, 4 Jun 2019 at 23:44, Laszlo Ersek wrote:
>>
>> When assigning a physical PCIe device to a QEMU/KVM guest, PciBusDxe may
>> find that the extended config space is not (fully) implemented. In
>> LocatePciExpressCapabilityRegBlock(), "CapabilityEntry" may be read as
>> 0x_ at a given config space offset, after which the loop gets
>> stuck spinning on offset 0xFFC (the read at offset 0xFFC returns
>> 0x_ most likely as well).
>>
>> Another scenario (not related to virtualization) for triggering the above
>> is when a Conventional PCI bus -- exposed by a PCIe-to-PCI bridge in the
>> topology -- intervenes between a PCI Express Root Port and a PCI Express
>> Endpoint. The Conventional PCI bus limits the accessible config space of
>> the PCI Express Endpoint, even though the endpoint advertizes the PCI
>> Express capability. Here's a diagram, courtesy of Alex Williamson:
>>
>> [PCIe Root Port]--[PCIe-to-PCI]--[PCI-to-PCIe]--[PCIe EP]
>> ->| |<- Conventional PCI bus
>>
>> Catch reads of 0x_ in LocatePciExpressCapabilityRegBlock(), and
>> break out of the scan with a warning message. The function will return
>> EFI_NOT_FOUND.
>>
>> Cc: Alex Williamson
>> Cc: Hao A Wu
>> Cc: Jian J Wang
>> Cc: Ray Ni
>> Cc: Star Zeng
>> Signed-off-by: Laszlo Ersek
>> ---
>>
>> Notes:
>> Repo: https://github.com/lersek/edk2.git
>> Branch: pcibus_no_ext_conf
>>
>> MdeModulePkg/Bus/Pci/PciBusDxe/PciCommand.c | 13 +
>> 1 file changed, 13 insertions(+)
>>
>> diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciCommand.c
>> b/MdeModulePkg/Bus/Pci/PciBusDxe/PciCommand.c
>> index 214aeecdd40a..6283d602207c 100644
>> --- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciCommand.c
>> +++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciCommand.c
>> @@ -236,6 +236,19 @@ LocatePciExpressCapabilityRegBlock (
>>break;
>> }
>>
>> +if (CapabilityEntry == MAX_UINT32) {
>
> Should we check here that the offset > 0x100 ? Otherwise, this affects
> more than just the extended config space.
A separate function exists for locating caps in the conventional config
space (LocateCapabilityRegBlock()).
Whereas the function being patched --
LocatePciExpressCapabilityRegBlock() -- is supposed to start with a
capability offset into the extended config space, passed in by the
caller via *Offset, or else at 0x100 if *Offset is 0.
And, my understanding is that an extended cap shall never chain to a
conventional cap. The spec says,
Next Capability Offset – This field contains the offset to the next
PCI Express Capability structure or 000h if no other items exist in
the linked list of Capabilities.
For Extended Capabilities implemented in Configuration Space, this
offset is relative to the beginning of PCI compatible Configuration
Space and thus must always be either 000h (for terminating list of
Capabilities) or greater than 0FFh.
The bottom 2 bits of this offset are Reserved and must be
implemented as 00b although software must mask them to allow for
future uses of these bits.
Additionally, the capability header is different for conventional
capabilities: EFI_PCI_CAPABILITY_HDR -- 2 bytes -- vs.
PCI_EXPRESS_EXTENDED_CAPABILITIES_HEADER -- 4 bytes. So if this loop
ever crossed over into normal config space, it would break horribly,
regardless of this patch.
A more general question would be how much we should armor such functions
-- i.e., capability list scanning -- with sanity checks.
My answer to that was authoring PciCapLib, which detects loops in cap
lists, oversized capability reads/writes, an absent extended config
space in spite of a present express capability; maybe more. Basically
everything I could think of and/or had encountered by then.
You probably remember that I originally attempted to get PciCapLib and
its accessories into MdePkg, with an intent to rebase core PCI drivers
to them -- including PciBusDxe. (The original "sales pitch" can be found
at
.)
I hadn't received either positive or negative feedback regarding that
idea for a month or so, after which we merged the library into OvmfPkg,
in the end. (And it is now used by ArmVirtQemu* and OVMF only, as part
of OvmfPkg/PciHotPlugInitDxe and OvmfPkg/Virtio10Dxe).
I did file a longer-term reminder BZ at
. But, I gave up on
that as well in about 4 months.
The upshot is that now I can only contribute piecemeal fixes for
PciBusDxe, whenever I come across something. This particular issue has
bitten us at RH twice by now -- unfortunately, both RHBZs are private,
hence I didn't reference them in the commit message. (It's super
annoying if you click a BZ link, just to be rejected access.)
In summary, adding a standalone check for "next" cap offsets that fall
into the forbidden range [1, 255] (inclusive) would be wort
Re: [edk2-devel] [PATCH for-next] MdeModulePkg/PciBusDxe: catch unimplemented extended config space reads
On Tue, 4 Jun 2019 at 23:44, Laszlo Ersek wrote:
>
> When assigning a physical PCIe device to a QEMU/KVM guest, PciBusDxe may
> find that the extended config space is not (fully) implemented. In
> LocatePciExpressCapabilityRegBlock(), "CapabilityEntry" may be read as
> 0x_ at a given config space offset, after which the loop gets
> stuck spinning on offset 0xFFC (the read at offset 0xFFC returns
> 0x_ most likely as well).
>
> Another scenario (not related to virtualization) for triggering the above
> is when a Conventional PCI bus -- exposed by a PCIe-to-PCI bridge in the
> topology -- intervenes between a PCI Express Root Port and a PCI Express
> Endpoint. The Conventional PCI bus limits the accessible config space of
> the PCI Express Endpoint, even though the endpoint advertizes the PCI
> Express capability. Here's a diagram, courtesy of Alex Williamson:
>
> [PCIe Root Port]--[PCIe-to-PCI]--[PCI-to-PCIe]--[PCIe EP]
> ->| |<- Conventional PCI bus
>
> Catch reads of 0x_ in LocatePciExpressCapabilityRegBlock(), and
> break out of the scan with a warning message. The function will return
> EFI_NOT_FOUND.
>
> Cc: Alex Williamson
> Cc: Hao A Wu
> Cc: Jian J Wang
> Cc: Ray Ni
> Cc: Star Zeng
> Signed-off-by: Laszlo Ersek
> ---
>
> Notes:
> Repo: https://github.com/lersek/edk2.git
> Branch: pcibus_no_ext_conf
>
> MdeModulePkg/Bus/Pci/PciBusDxe/PciCommand.c | 13 +
> 1 file changed, 13 insertions(+)
>
> diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciCommand.c
> b/MdeModulePkg/Bus/Pci/PciBusDxe/PciCommand.c
> index 214aeecdd40a..6283d602207c 100644
> --- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciCommand.c
> +++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciCommand.c
> @@ -236,6 +236,19 @@ LocatePciExpressCapabilityRegBlock (
>break;
> }
>
> +if (CapabilityEntry == MAX_UINT32) {
Should we check here that the offset > 0x100 ? Otherwise, this affects
more than just the extended config space.
> + DEBUG ((
> +DEBUG_WARN,
> +"%a: [%02x|%02x|%02x] failed to access config space at offset
> 0x%x\n",
> +__FUNCTION__,
> +PciIoDevice->BusNumber,
> +PciIoDevice->DeviceNumber,
> +PciIoDevice->FunctionNumber,
> +CapabilityPtr
> +));
> + break;
> +}
> +
> CapabilityID = (UINT16) CapabilityEntry;
>
> if (CapabilityID == CapId) {
> --
> 2.19.1.3.g30247aa5d201
>
>
>
> Groups.io Links: You receive all messages sent to this group.
>
> View/Reply Online (#41893): https://edk2.groups.io/g/devel/message/41893
> Mute This Topic: https://groups.io/mt/31931246/1761188
> Group Owner: devel+ow...@edk2.groups.io
> Unsubscribe: https://edk2.groups.io/g/devel/unsub [ard.biesheu...@linaro.org]
>
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#41933): https://edk2.groups.io/g/devel/message/41933
Mute This Topic: https://groups.io/mt/31931246/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH for-next] MdeModulePkg/PciBusDxe: catch unimplemented extended config space reads
On 6/4/19 11:44 PM, Laszlo Ersek wrote:
> When assigning a physical PCIe device to a QEMU/KVM guest, PciBusDxe may
> find that the extended config space is not (fully) implemented. In
> LocatePciExpressCapabilityRegBlock(), "CapabilityEntry" may be read as
> 0x_ at a given config space offset, after which the loop gets
> stuck spinning on offset 0xFFC (the read at offset 0xFFC returns
> 0x_ most likely as well).
>
> Another scenario (not related to virtualization) for triggering the above
> is when a Conventional PCI bus -- exposed by a PCIe-to-PCI bridge in the
> topology -- intervenes between a PCI Express Root Port and a PCI Express
> Endpoint. The Conventional PCI bus limits the accessible config space of
> the PCI Express Endpoint, even though the endpoint advertizes the PCI
> Express capability. Here's a diagram, courtesy of Alex Williamson:
>
> [PCIe Root Port]--[PCIe-to-PCI]--[PCI-to-PCIe]--[PCIe EP]
> ->| |<- Conventional PCI bus
>
> Catch reads of 0x_ in LocatePciExpressCapabilityRegBlock(), and
> break out of the scan with a warning message. The function will return
> EFI_NOT_FOUND.
>
> Cc: Alex Williamson
> Cc: Hao A Wu
> Cc: Jian J Wang
> Cc: Ray Ni
> Cc: Star Zeng
> Signed-off-by: Laszlo Ersek
> ---
>
> Notes:
> Repo: https://github.com/lersek/edk2.git
> Branch: pcibus_no_ext_conf
>
> MdeModulePkg/Bus/Pci/PciBusDxe/PciCommand.c | 13 +
> 1 file changed, 13 insertions(+)
>
> diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciCommand.c
> b/MdeModulePkg/Bus/Pci/PciBusDxe/PciCommand.c
> index 214aeecdd40a..6283d602207c 100644
> --- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciCommand.c
> +++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciCommand.c
> @@ -236,6 +236,19 @@ LocatePciExpressCapabilityRegBlock (
>break;
> }
>
> +if (CapabilityEntry == MAX_UINT32) {
> + DEBUG ((
> +DEBUG_WARN,
> +"%a: [%02x|%02x|%02x] failed to access config space at offset
> 0x%x\n",
> +__FUNCTION__,
> +PciIoDevice->BusNumber,
> +PciIoDevice->DeviceNumber,
> +PciIoDevice->FunctionNumber,
> +CapabilityPtr
> +));
> + break;
> +}
> +
> CapabilityID = (UINT16) CapabilityEntry;
>
> if (CapabilityID == CapId) {
>
Reviewed-by: Philippe Mathieu-Daude
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#41932): https://edk2.groups.io/g/devel/message/41932
Mute This Topic: https://groups.io/mt/31931246/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
