diff mbox

[v2] pci: fix pci_requester_id()

Message ID 1463382800-25863-1-git-send-email-peterx@redhat.com
State New
Headers show

Commit Message

Peter Xu May 16, 2016, 7:13 a.m. UTC
The original pci_requester_id() is not really getting requester ID, but
only BDF. It will not work if we enable IOMMU IR with SID verifications
when with PCI bridges.  Renaming it to pci_get_bdf().

Meanwhile, we provide the correct implementation to get requester
ID. VT-d spec 5.1.1 is a good reference to go, though it talks only
about interrupt delivery, the rule works exactly the same for
non-interrupt cases.

Currently, there are three use cases for pci_requester_id():

- PCIX status bits: here we need BDF only, not requester ID. Replacing
  with pci_get_bdf().
- PCIe Error injection and MSI delivery: for both these cases, we are
  looking for requester IDs. Kept untouched to leverage the new impl.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 hw/i386/kvm/pci-assign.c |  2 +-
 hw/pci/pci.c             | 29 +++++++++++++++++++++++++++++
 include/hw/pci/pci.h     |  7 +++++--
 3 files changed, 35 insertions(+), 3 deletions(-)

Comments

Michael S. Tsirkin May 16, 2016, 7:54 a.m. UTC | #1
On Mon, May 16, 2016 at 03:13:20PM +0800, Peter Xu wrote:
> The original pci_requester_id() is not really getting requester ID, but
> only BDF. It will not work if we enable IOMMU IR with SID verifications
> when with PCI bridges.  Renaming it to pci_get_bdf().
> 
> Meanwhile, we provide the correct implementation to get requester
> ID. VT-d spec 5.1.1 is a good reference to go, though it talks only
> about interrupt delivery, the rule works exactly the same for
> non-interrupt cases.
> 
> Currently, there are three use cases for pci_requester_id():
> 
> - PCIX status bits: here we need BDF only, not requester ID. Replacing
>   with pci_get_bdf().
> - PCIe Error injection and MSI delivery: for both these cases, we are
>   looking for requester IDs. Kept untouched to leverage the new impl.
> 
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  hw/i386/kvm/pci-assign.c |  2 +-
>  hw/pci/pci.c             | 29 +++++++++++++++++++++++++++++
>  include/hw/pci/pci.h     |  7 +++++--
>  3 files changed, 35 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/i386/kvm/pci-assign.c b/hw/i386/kvm/pci-assign.c
> index bf425a2..c40ab36 100644
> --- a/hw/i386/kvm/pci-assign.c
> +++ b/hw/i386/kvm/pci-assign.c
> @@ -1481,7 +1481,7 @@ static int assigned_device_pci_cap_init(PCIDevice *pci_dev, Error **errp)
>           * error bits, leave the rest. */
>          status = pci_get_long(pci_dev->config + pos + PCI_X_STATUS);
>          status &= ~(PCI_X_STATUS_BUS | PCI_X_STATUS_DEVFN);
> -        status |= pci_requester_id(pci_dev);
> +        status |= pci_get_bdf(pci_dev);
>          status &= ~(PCI_X_STATUS_SPL_DISC | PCI_X_STATUS_UNX_SPL |
>                      PCI_X_STATUS_SPL_ERR);
>          pci_set_long(pci_dev->config + pos + PCI_X_STATUS, status);
> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> index bb605ef..7430715 100644
> --- a/hw/pci/pci.c
> +++ b/hw/pci/pci.c
> @@ -2498,6 +2498,35 @@ PCIDevice *pci_get_function_0(PCIDevice *pci_dev)
>      }
>  }
>  
> +/* Parse bridges up to the root complex and get final Requester ID
> + * for this device.  For PCIe-all topology, this works exactly as
> + * pci_get_bdf() does. However, several tricks are required for
> + * legacy PCI topology and PCIe-to-PCI bridges, to be better aligned
> + * with spec. */
> +uint16_t pci_requester_id(PCIDevice *dev)
> +{
> +    uint8_t bus_n;
> +    uint16_t result = pci_get_bdf(dev);

add empty line here pls.

> +    while ((bus_n = pci_bus_num(dev->bus))) {
> +        /* We are under PCI/PCIe bridges */
> +        dev = dev->bus->parent_dev;
> +        if (pci_is_express(dev)) {
> +            if (pcie_cap_get_type(dev) == PCI_EXP_TYPE_PCI_BRIDGE) {
> +                /* When we pass through PCIe-to-PCI/PCIX bridges, we
> +                 * override the requester ID using secondary bus
> +                 * number with zeroed devfn (pcie-to-pci bridge spec
> +                 * chap 2.3). */
> +                result = PCI_BUILD_BDF(bus_n, 0);
> +            }
> +        } else {


> +            /* Legacy PCI bus, override requester ID with the
> +             * bridge's BDF upstream. */

Don't document what is done, pls document why.
Please add an explanation why this is the right thing to do.

> +            result = pci_get_bdf(dev);

Won't dev be NULL for a root bus?

> +        }
> +    }
> +    return result;
> +}
> +
>  static const TypeInfo pci_device_type_info = {
>      .name = TYPE_PCI_DEVICE,
>      .parent = TYPE_DEVICE,

OK but this is used on data path by msi_send_message and
doing scans there isn't nice.
Can we store a pointer to device who's requester id is used instead?


> diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
> index ef6ba51..351266c 100644
> --- a/include/hw/pci/pci.h
> +++ b/include/hw/pci/pci.h
> @@ -15,6 +15,7 @@
>  #define PCI_DEVFN(slot, func)   ((((slot) & 0x1f) << 3) | ((func) & 0x07))
>  #define PCI_SLOT(devfn)         (((devfn) >> 3) & 0x1f)
>  #define PCI_FUNC(devfn)         ((devfn) & 0x07)
> +#define PCI_BUILD_BDF(bus, devfn)     ((bus << 8) | (devfn))
>  #define PCI_SLOT_MAX            32
>  #define PCI_FUNC_MAX            8
>  
> @@ -685,11 +686,13 @@ static inline uint32_t pci_config_size(const PCIDevice *d)
>      return pci_is_express(d) ? PCIE_CONFIG_SPACE_SIZE : PCI_CONFIG_SPACE_SIZE;
>  }
>  
> -static inline uint16_t pci_requester_id(PCIDevice *dev)
> +static inline uint16_t pci_get_bdf(PCIDevice *dev)
>  {
> -    return (pci_bus_num(dev->bus) << 8) | dev->devfn;
> +    return PCI_BUILD_BDF(pci_bus_num(dev->bus), dev->devfn);
>  }
>  
> +uint16_t pci_requester_id(PCIDevice *dev);
> +
>  /* DMA access functions */
>  static inline AddressSpace *pci_get_address_space(PCIDevice *dev)
>  {
> -- 
> 2.4.11
Peter Xu May 16, 2016, 9 a.m. UTC | #2
On Mon, May 16, 2016 at 10:54:13AM +0300, Michael S. Tsirkin wrote:
[...]
> >  
> > +/* Parse bridges up to the root complex and get final Requester ID
> > + * for this device.  For PCIe-all topology, this works exactly as
> > + * pci_get_bdf() does. However, several tricks are required for
> > + * legacy PCI topology and PCIe-to-PCI bridges, to be better aligned
> > + * with spec. */
> > +uint16_t pci_requester_id(PCIDevice *dev)
> > +{
> > +    uint8_t bus_n;
> > +    uint16_t result = pci_get_bdf(dev);
> 
> add empty line here pls.

Will do. Do I need to leave a blank line after declaration of local
variables every time? Just to confirm this for next time.

> 
> > +    while ((bus_n = pci_bus_num(dev->bus))) {
> > +        /* We are under PCI/PCIe bridges */
> > +        dev = dev->bus->parent_dev;
> > +        if (pci_is_express(dev)) {
> > +            if (pcie_cap_get_type(dev) == PCI_EXP_TYPE_PCI_BRIDGE) {
> > +                /* When we pass through PCIe-to-PCI/PCIX bridges, we
> > +                 * override the requester ID using secondary bus
> > +                 * number with zeroed devfn (pcie-to-pci bridge spec
> > +                 * chap 2.3). */
> > +                result = PCI_BUILD_BDF(bus_n, 0);
> > +            }
> > +        } else {
> 
> 
> > +            /* Legacy PCI bus, override requester ID with the
> > +             * bridge's BDF upstream. */
> 
> Don't document what is done, pls document why.
> Please add an explanation why this is the right thing to do.

I see the above partly the "why" for it.. Then, how about this one:

"Legacy PCI bus, override requester ID with the bridge's BDF
upstream.  The root complex of legacy PCI system can only get
requester ID from directly attached devices (including bridges). If
devices are attached under specific bridge (no matter there are one
or more bridges), only the requester ID of the bridge that directly
attached to the root complex can be recognized."

> 
> > +            result = pci_get_bdf(dev);
> 
> Won't dev be NULL for a root bus?

Should not. The above while() is checking whether dev's parent bus
number (N) is zero, and reach here only if it's non-zero. Here, dev
is already re-used to store the PCIDevice struct for the bus device,
whose secondary bus number is N (as checked in the while
condition). So it should never be the root pci bus (which has
so-called secondary bus number 0).

> 
> > +        }
> > +    }
> > +    return result;
> > +}
> > +
> >  static const TypeInfo pci_device_type_info = {
> >      .name = TYPE_PCI_DEVICE,
> >      .parent = TYPE_DEVICE,
> 
> OK but this is used on data path by msi_send_message and
> doing scans there isn't nice.
> Can we store a pointer to device who's requester id is used instead?

Sure. I can try that in v3.

Thanks,

-- peterx
Michael S. Tsirkin May 16, 2016, 9:21 a.m. UTC | #3
On Mon, May 16, 2016 at 05:00:05PM +0800, Peter Xu wrote:
> On Mon, May 16, 2016 at 10:54:13AM +0300, Michael S. Tsirkin wrote:
> [...]
> > >  
> > > +/* Parse bridges up to the root complex and get final Requester ID
> > > + * for this device.  For PCIe-all topology, this works exactly as
> > > + * pci_get_bdf() does. However, several tricks are required for
> > > + * legacy PCI topology and PCIe-to-PCI bridges, to be better aligned
> > > + * with spec. */
> > > +uint16_t pci_requester_id(PCIDevice *dev)
> > > +{
> > > +    uint8_t bus_n;
> > > +    uint16_t result = pci_get_bdf(dev);
> > 
> > add empty line here pls.
> 
> Will do. Do I need to leave a blank line after declaration of local
> variables every time? Just to confirm this for next time.

Yes, that's preferable.

> > 
> > > +    while ((bus_n = pci_bus_num(dev->bus))) {
> > > +        /* We are under PCI/PCIe bridges */
> > > +        dev = dev->bus->parent_dev;
> > > +        if (pci_is_express(dev)) {
> > > +            if (pcie_cap_get_type(dev) == PCI_EXP_TYPE_PCI_BRIDGE) {
> > > +                /* When we pass through PCIe-to-PCI/PCIX bridges, we
> > > +                 * override the requester ID using secondary bus
> > > +                 * number with zeroed devfn (pcie-to-pci bridge spec
> > > +                 * chap 2.3). */
> > > +                result = PCI_BUILD_BDF(bus_n, 0);
> > > +            }
> > > +        } else {
> > 
> > 
> > > +            /* Legacy PCI bus, override requester ID with the
> > > +             * bridge's BDF upstream. */
> > 
> > Don't document what is done, pls document why.
> > Please add an explanation why this is the right thing to do.
> 
> I see the above partly the "why" for it.. Then, how about this one:
> 
> "Legacy PCI bus, override requester ID with the bridge's BDF
> upstream.  The root complex of legacy PCI system can only get
> requester ID from directly attached devices (including bridges).

When do legacy pci systems use requester id at all?
PCI spec does not mention this concept.

> If
> devices are attached under specific bridge (no matter

should be "no matter if"

> there are one
> or more bridges), only the requester ID of the bridge that directly

should be "that is directly"

> attached to the root complex can be recognized."
> 
> > 
> > > +            result = pci_get_bdf(dev);
> > 
> > Won't dev be NULL for a root bus?
> 
> Should not. The above while() is checking whether dev's parent bus
> number (N) is zero,

OK but from pci perspective it's not a given that it's zero.
I think it isn't for pci expander.
BTW did you try this with expander bridges?

> and reach here only if it's non-zero. Here, dev
> is already re-used to store the PCIDevice struct for the bus device,
> whose secondary bus number is N (as checked in the while
> condition). So it should never be the root pci bus (which has
> so-called secondary bus number 0).

Pls don't make this assumption. If you want to know
whether it's a root, call pci_bus_is_root.

> > 
> > > +        }
> > > +    }
> > > +    return result;
> > > +}
> > > +
> > >  static const TypeInfo pci_device_type_info = {
> > >      .name = TYPE_PCI_DEVICE,
> > >      .parent = TYPE_DEVICE,
> > 
> > OK but this is used on data path by msi_send_message and
> > doing scans there isn't nice.
> > Can we store a pointer to device who's requester id is used instead?
> 
> Sure. I can try that in v3.
> 
> Thanks,
> 
> -- peterx
Peter Xu May 16, 2016, 9:58 a.m. UTC | #4
On Mon, May 16, 2016 at 12:21:54PM +0300, Michael S. Tsirkin wrote:
[...]
> > "Legacy PCI bus, override requester ID with the bridge's BDF
> > upstream.  The root complex of legacy PCI system can only get
> > requester ID from directly attached devices (including bridges).
> 
> When do legacy pci systems use requester id at all?
> PCI spec does not mention this concept.

I see some descriptions about this in vt-d spec, e.g., chap
3.9.2. Maybe somewhere else too, but I cannot remember. In the spec,
it is told something like:

"...the source-id in the DMA requests is the requester-id of the
bridge device..."

Similar thing on interrupt remapping desc in 5.1.1.

Actually I am curious about how generic PCI system delivers
requester ID (if there is)... For PCIe, we have encoded TLP header,
and requester ID is filled in the specific field of the header.
However for legacy PCI system, all the data is transmitted via the
parallel interface (no matter 32/64 bits) and I found no place that
the requester ID can be included. I was assuming there is some way
for the root complex to know it (when request comes, the root
complex should be able to know where the request come from, or say,
its connected BDF). Never digger into the details, or am I wrong?

> 
> > If
> > devices are attached under specific bridge (no matter
> 
> should be "no matter if"
> 
> > there are one
> > or more bridges), only the requester ID of the bridge that directly
> 
> should be "that is directly"

Will fix above two.

> 
> > attached to the root complex can be recognized."
> > 
> > > 
> > > > +            result = pci_get_bdf(dev);
> > > 
> > > Won't dev be NULL for a root bus?
> > 
> > Should not. The above while() is checking whether dev's parent bus
> > number (N) is zero,
> 
> OK but from pci perspective it's not a given that it's zero.
> I think it isn't for pci expander.
> BTW did you try this with expander bridges?

Nop... I used the same test in as in v1 (Radim's one, with IR
patchset applied, since until now IR seems the only one that uses
this field), since I found it hard to cover all the combinations
(include different PCI/PCIX/PCIe buses, PCI/PCIe devices, and all
kinds of topologies, etc.).  Do you think I should do thorough tests
for this change? If so, do you have suggestion on which test cases I
should (at least) cover?

> 
> > and reach here only if it's non-zero. Here, dev
> > is already re-used to store the PCIDevice struct for the bus device,
> > whose secondary bus number is N (as checked in the while
> > condition). So it should never be the root pci bus (which has
> > so-called secondary bus number 0).
> 
> Pls don't make this assumption. If you want to know
> whether it's a root, call pci_bus_is_root.

Ah, yes, I should use that.

Thanks,

-- peterx
Alex Williamson May 16, 2016, 3:44 p.m. UTC | #5
On Mon, 16 May 2016 17:58:18 +0800
Peter Xu <peterx@redhat.com> wrote:

> On Mon, May 16, 2016 at 12:21:54PM +0300, Michael S. Tsirkin wrote:
> [...]
> > > "Legacy PCI bus, override requester ID with the bridge's BDF
> > > upstream.  The root complex of legacy PCI system can only get
> > > requester ID from directly attached devices (including bridges).  
> > 
> > When do legacy pci systems use requester id at all?
> > PCI spec does not mention this concept.  
> 
> I see some descriptions about this in vt-d spec, e.g., chap
> 3.9.2. Maybe somewhere else too, but I cannot remember. In the spec,
> it is told something like:
> 
> "...the source-id in the DMA requests is the requester-id of the
> bridge device..."
> 
> Similar thing on interrupt remapping desc in 5.1.1.
> 
> Actually I am curious about how generic PCI system delivers
> requester ID (if there is)... For PCIe, we have encoded TLP header,
> and requester ID is filled in the specific field of the header.
> However for legacy PCI system, all the data is transmitted via the
> parallel interface (no matter 32/64 bits) and I found no place that
> the requester ID can be included. I was assuming there is some way
> for the root complex to know it (when request comes, the root
> complex should be able to know where the request come from, or say,
> its connected BDF). Never digger into the details, or am I wrong?

There's no such thing as a requester ID on conventional PCI.  We should
probably be making use of pci_bus_is_express() to determine whether we
have a valid requester ID and error if we hit pci_bus_is_root() and we
still don't have an express bus.  And as MST says, testing for
bus number zero is not a valid test for the root bus.  Thanks,

Alex

> >   
> > > If
> > > devices are attached under specific bridge (no matter  
> > 
> > should be "no matter if"
> >   
> > > there are one
> > > or more bridges), only the requester ID of the bridge that directly  
> > 
> > should be "that is directly"  
> 
> Will fix above two.
> 
> >   
> > > attached to the root complex can be recognized."
> > >   
> > > >   
> > > > > +            result = pci_get_bdf(dev);  
> > > > 
> > > > Won't dev be NULL for a root bus?  
> > > 
> > > Should not. The above while() is checking whether dev's parent bus
> > > number (N) is zero,  
> > 
> > OK but from pci perspective it's not a given that it's zero.
> > I think it isn't for pci expander.
> > BTW did you try this with expander bridges?  
> 
> Nop... I used the same test in as in v1 (Radim's one, with IR
> patchset applied, since until now IR seems the only one that uses
> this field), since I found it hard to cover all the combinations
> (include different PCI/PCIX/PCIe buses, PCI/PCIe devices, and all
> kinds of topologies, etc.).  Do you think I should do thorough tests
> for this change? If so, do you have suggestion on which test cases I
> should (at least) cover?
> 
> >   
> > > and reach here only if it's non-zero. Here, dev
> > > is already re-used to store the PCIDevice struct for the bus device,
> > > whose secondary bus number is N (as checked in the while
> > > condition). So it should never be the root pci bus (which has
> > > so-called secondary bus number 0).  
> > 
> > Pls don't make this assumption. If you want to know
> > whether it's a root, call pci_bus_is_root.  
> 
> Ah, yes, I should use that.
> 
> Thanks,
> 
> -- peterx
Michael S. Tsirkin May 16, 2016, 5:20 p.m. UTC | #6
On Mon, May 16, 2016 at 09:44:28AM -0600, Alex Williamson wrote:
> On Mon, 16 May 2016 17:58:18 +0800
> Peter Xu <peterx@redhat.com> wrote:
> 
> > On Mon, May 16, 2016 at 12:21:54PM +0300, Michael S. Tsirkin wrote:
> > [...]
> > > > "Legacy PCI bus, override requester ID with the bridge's BDF
> > > > upstream.  The root complex of legacy PCI system can only get
> > > > requester ID from directly attached devices (including bridges).  
> > > 
> > > When do legacy pci systems use requester id at all?
> > > PCI spec does not mention this concept.  
> > 
> > I see some descriptions about this in vt-d spec, e.g., chap
> > 3.9.2. Maybe somewhere else too, but I cannot remember. In the spec,
> > it is told something like:
> > 
> > "...the source-id in the DMA requests is the requester-id of the
> > bridge device..."
> > 
> > Similar thing on interrupt remapping desc in 5.1.1.
> > 
> > Actually I am curious about how generic PCI system delivers
> > requester ID (if there is)... For PCIe, we have encoded TLP header,
> > and requester ID is filled in the specific field of the header.
> > However for legacy PCI system, all the data is transmitted via the
> > parallel interface (no matter 32/64 bits) and I found no place that
> > the requester ID can be included. I was assuming there is some way
> > for the root complex to know it (when request comes, the root
> > complex should be able to know where the request come from, or say,
> > its connected BDF). Never digger into the details, or am I wrong?
> 
> There's no such thing as a requester ID on conventional PCI.  We should
> probably be making use of pci_bus_is_express() to determine whether we
> have a valid requester ID and error if we hit pci_bus_is_root() and we
> still don't have an express bus.  And as MST says, testing for
> bus number zero is not a valid test for the root bus.  Thanks,
> 
> Alex

Well MSI code sticks the requester ID in the MSI message
unconditionally. It's typically later ignored by the the
PC machine type though.

> > >   
> > > > If
> > > > devices are attached under specific bridge (no matter  
> > > 
> > > should be "no matter if"
> > >   
> > > > there are one
> > > > or more bridges), only the requester ID of the bridge that directly  
> > > 
> > > should be "that is directly"  
> > 
> > Will fix above two.
> > 
> > >   
> > > > attached to the root complex can be recognized."
> > > >   
> > > > >   
> > > > > > +            result = pci_get_bdf(dev);  
> > > > > 
> > > > > Won't dev be NULL for a root bus?  
> > > > 
> > > > Should not. The above while() is checking whether dev's parent bus
> > > > number (N) is zero,  
> > > 
> > > OK but from pci perspective it's not a given that it's zero.
> > > I think it isn't for pci expander.
> > > BTW did you try this with expander bridges?  
> > 
> > Nop... I used the same test in as in v1 (Radim's one, with IR
> > patchset applied, since until now IR seems the only one that uses
> > this field), since I found it hard to cover all the combinations
> > (include different PCI/PCIX/PCIe buses, PCI/PCIe devices, and all
> > kinds of topologies, etc.).  Do you think I should do thorough tests
> > for this change? If so, do you have suggestion on which test cases I
> > should (at least) cover?
> > 
> > >   
> > > > and reach here only if it's non-zero. Here, dev
> > > > is already re-used to store the PCIDevice struct for the bus device,
> > > > whose secondary bus number is N (as checked in the while
> > > > condition). So it should never be the root pci bus (which has
> > > > so-called secondary bus number 0).  
> > > 
> > > Pls don't make this assumption. If you want to know
> > > whether it's a root, call pci_bus_is_root.  
> > 
> > Ah, yes, I should use that.
> > 
> > Thanks,
> > 
> > -- peterx
Peter Xu May 17, 2016, 5:35 a.m. UTC | #7
On Mon, May 16, 2016 at 08:20:46PM +0300, Michael S. Tsirkin wrote:
[...]
> > > Actually I am curious about how generic PCI system delivers
> > > requester ID (if there is)... For PCIe, we have encoded TLP header,
> > > and requester ID is filled in the specific field of the header.
> > > However for legacy PCI system, all the data is transmitted via the
> > > parallel interface (no matter 32/64 bits) and I found no place that
> > > the requester ID can be included. I was assuming there is some way
> > > for the root complex to know it (when request comes, the root
> > > complex should be able to know where the request come from, or say,
> > > its connected BDF). Never digger into the details, or am I wrong?
> > 
> > There's no such thing as a requester ID on conventional PCI.  We should
> > probably be making use of pci_bus_is_express() to determine whether we
> > have a valid requester ID and error if we hit pci_bus_is_root() and we
> > still don't have an express bus.  And as MST says, testing for
> > bus number zero is not a valid test for the root bus.  Thanks,
> > 
> > Alex
> 
> Well MSI code sticks the requester ID in the MSI message
> unconditionally. It's typically later ignored by the the
> PC machine type though.

Yes, I can first detect root PCI bus type as mentioned by Alex, and
fill in SID_INVALID if it's a conventional PCI root complex.

-- peterx
diff mbox

Patch

diff --git a/hw/i386/kvm/pci-assign.c b/hw/i386/kvm/pci-assign.c
index bf425a2..c40ab36 100644
--- a/hw/i386/kvm/pci-assign.c
+++ b/hw/i386/kvm/pci-assign.c
@@ -1481,7 +1481,7 @@  static int assigned_device_pci_cap_init(PCIDevice *pci_dev, Error **errp)
          * error bits, leave the rest. */
         status = pci_get_long(pci_dev->config + pos + PCI_X_STATUS);
         status &= ~(PCI_X_STATUS_BUS | PCI_X_STATUS_DEVFN);
-        status |= pci_requester_id(pci_dev);
+        status |= pci_get_bdf(pci_dev);
         status &= ~(PCI_X_STATUS_SPL_DISC | PCI_X_STATUS_UNX_SPL |
                     PCI_X_STATUS_SPL_ERR);
         pci_set_long(pci_dev->config + pos + PCI_X_STATUS, status);
diff --git a/hw/pci/pci.c b/hw/pci/pci.c
index bb605ef..7430715 100644
--- a/hw/pci/pci.c
+++ b/hw/pci/pci.c
@@ -2498,6 +2498,35 @@  PCIDevice *pci_get_function_0(PCIDevice *pci_dev)
     }
 }
 
+/* Parse bridges up to the root complex and get final Requester ID
+ * for this device.  For PCIe-all topology, this works exactly as
+ * pci_get_bdf() does. However, several tricks are required for
+ * legacy PCI topology and PCIe-to-PCI bridges, to be better aligned
+ * with spec. */
+uint16_t pci_requester_id(PCIDevice *dev)
+{
+    uint8_t bus_n;
+    uint16_t result = pci_get_bdf(dev);
+    while ((bus_n = pci_bus_num(dev->bus))) {
+        /* We are under PCI/PCIe bridges */
+        dev = dev->bus->parent_dev;
+        if (pci_is_express(dev)) {
+            if (pcie_cap_get_type(dev) == PCI_EXP_TYPE_PCI_BRIDGE) {
+                /* When we pass through PCIe-to-PCI/PCIX bridges, we
+                 * override the requester ID using secondary bus
+                 * number with zeroed devfn (pcie-to-pci bridge spec
+                 * chap 2.3). */
+                result = PCI_BUILD_BDF(bus_n, 0);
+            }
+        } else {
+            /* Legacy PCI bus, override requester ID with the
+             * bridge's BDF upstream. */
+            result = pci_get_bdf(dev);
+        }
+    }
+    return result;
+}
+
 static const TypeInfo pci_device_type_info = {
     .name = TYPE_PCI_DEVICE,
     .parent = TYPE_DEVICE,
diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
index ef6ba51..351266c 100644
--- a/include/hw/pci/pci.h
+++ b/include/hw/pci/pci.h
@@ -15,6 +15,7 @@ 
 #define PCI_DEVFN(slot, func)   ((((slot) & 0x1f) << 3) | ((func) & 0x07))
 #define PCI_SLOT(devfn)         (((devfn) >> 3) & 0x1f)
 #define PCI_FUNC(devfn)         ((devfn) & 0x07)
+#define PCI_BUILD_BDF(bus, devfn)     ((bus << 8) | (devfn))
 #define PCI_SLOT_MAX            32
 #define PCI_FUNC_MAX            8
 
@@ -685,11 +686,13 @@  static inline uint32_t pci_config_size(const PCIDevice *d)
     return pci_is_express(d) ? PCIE_CONFIG_SPACE_SIZE : PCI_CONFIG_SPACE_SIZE;
 }
 
-static inline uint16_t pci_requester_id(PCIDevice *dev)
+static inline uint16_t pci_get_bdf(PCIDevice *dev)
 {
-    return (pci_bus_num(dev->bus) << 8) | dev->devfn;
+    return PCI_BUILD_BDF(pci_bus_num(dev->bus), dev->devfn);
 }
 
+uint16_t pci_requester_id(PCIDevice *dev);
+
 /* DMA access functions */
 static inline AddressSpace *pci_get_address_space(PCIDevice *dev)
 {