[Qemu-devel] [PATCH] pci: fix requester id with PCI bridges

[Qemu-devel] [PATCH] pci: fix requester id with PCI bridges

[Peter Xu]

Recently I encountered issue when debugging Intel IOMMU IR codes,
that interrupts are not working correctly with PCI bridges (reported
by Radim). This patch fixes the problem. I assume requester ID
should be the devfn on root PCI bus (that's how I understand it
before, and also in guest kernel, IRTE entry SID is filled in that
way), however I failed to find any good document to confirm
this. Please let me know if this is correct (or I made any
mistake).

Thanks!

Peter Xu (1):
  pci: fix requester id to be the one on root bus

 hw/pci/msi.c         | 2 +-
 hw/pci/pci.c         | 9 +++++++++
 include/hw/pci/pci.h | 2 ++
 3 files changed, 12 insertions(+), 1 deletion(-)

-- 
2.4.11

[Qemu-devel] [PATCH] pci: fix requester id to be the one on root bus

[Peter Xu]

When there are devices under PCI bridge (or bridges), PCI requester ID
should be the one that hooked on the root PCI bus, not the PCI device
itself.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 hw/pci/msi.c         | 2 +-
 hw/pci/pci.c         | 9 +++++++++
 include/hw/pci/pci.h | 2 ++
 3 files changed, 12 insertions(+), 1 deletion(-)

diff --git a/hw/pci/msi.c b/hw/pci/msi.c
index e0e64c2..1719716 100644
--- a/hw/pci/msi.c
+++ b/hw/pci/msi.c
@@ -315,7 +315,7 @@ void msi_send_message(PCIDevice *dev, MSIMessage msg)
 {
     MemTxAttrs attrs = {};
 
-    attrs.requester_id = pci_requester_id(dev);
+    attrs.requester_id = pci_requester_id_recursive(dev);
     address_space_stl_le(&dev->bus_master_as, msg.address, msg.data,
                          attrs, NULL);
 }
diff --git a/hw/pci/pci.c b/hw/pci/pci.c
index bb605ef..c14299b 100644
--- a/hw/pci/pci.c
+++ b/hw/pci/pci.c
@@ -2498,6 +2498,15 @@ PCIDevice *pci_get_function_0(PCIDevice *pci_dev)
     }
 }
 
+uint16_t pci_requester_id_recursive(PCIDevice *dev)
+{
+    while (pci_bus_num(dev->bus)) {
+        /* This is not on root PCI bus, we find its parent */
+        dev = dev->bus->parent_dev;
+    }
+    return pci_requester_id(dev);
+}
+
 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..4cb5b50 100644
--- a/include/hw/pci/pci.h
+++ b/include/hw/pci/pci.h
@@ -776,4 +776,6 @@ extern const VMStateDescription vmstate_pci_device;
     .offset     = vmstate_offset_pointer(_state, _field, PCIDevice), \
 }
 
+uint16_t pci_requester_id_recursive(PCIDevice *dev);
+
 #endif
-- 
2.4.11

Re: [Qemu-devel] [PATCH] pci: fix requester id to be the one on root bus

[Michael S. Tsirkin]

On Wed, May 11, 2016 at 02:40:31PM +0800, Peter Xu wrote:
> When there are devices under PCI bridge (or bridges), PCI requester ID
> should be the one that hooked on the root PCI bus, not the PCI device
> itself.
> 
> Signed-off-by: Peter Xu <peterx@redhat.com>

I think this is only correct for pci bridges, and wrong for pci express bridges.

How exactly do you test this?

> ---
>  hw/pci/msi.c         | 2 +-
>  hw/pci/pci.c         | 9 +++++++++
>  include/hw/pci/pci.h | 2 ++
>  3 files changed, 12 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/pci/msi.c b/hw/pci/msi.c
> index e0e64c2..1719716 100644
> --- a/hw/pci/msi.c
> +++ b/hw/pci/msi.c
> @@ -315,7 +315,7 @@ void msi_send_message(PCIDevice *dev, MSIMessage msg)
>  {
>      MemTxAttrs attrs = {};
>  
> -    attrs.requester_id = pci_requester_id(dev);
> +    attrs.requester_id = pci_requester_id_recursive(dev);
>      address_space_stl_le(&dev->bus_master_as, msg.address, msg.data,
>                           attrs, NULL);
>  }
> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> index bb605ef..c14299b 100644
> --- a/hw/pci/pci.c
> +++ b/hw/pci/pci.c
> @@ -2498,6 +2498,15 @@ PCIDevice *pci_get_function_0(PCIDevice *pci_dev)
>      }
>  }
>  
> +uint16_t pci_requester_id_recursive(PCIDevice *dev)
> +{
> +    while (pci_bus_num(dev->bus)) {
> +        /* This is not on root PCI bus, we find its parent */
> +        dev = dev->bus->parent_dev;
> +    }
> +    return pci_requester_id(dev);
> +}
> +
>  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..4cb5b50 100644
> --- a/include/hw/pci/pci.h
> +++ b/include/hw/pci/pci.h
> @@ -776,4 +776,6 @@ extern const VMStateDescription vmstate_pci_device;
>      .offset     = vmstate_offset_pointer(_state, _field, PCIDevice), \
>  }
>  
> +uint16_t pci_requester_id_recursive(PCIDevice *dev);
> +
>  #endif
> -- 
> 2.4.11

Re: [Qemu-devel] [PATCH] pci: fix requester id to be the one on root bus

[Peter Xu]

On Wed, May 11, 2016 at 04:53:54PM +0300, Michael S. Tsirkin wrote:
> On Wed, May 11, 2016 at 02:40:31PM +0800, Peter Xu wrote:
> > When there are devices under PCI bridge (or bridges), PCI requester ID
> > should be the one that hooked on the root PCI bus, not the PCI device
> > itself.
> > 
> > Signed-off-by: Peter Xu <peterx@redhat.com>
> 
> I think this is only correct for pci bridges, and wrong for pci express bridges.
> 
> How exactly do you test this?

I was using Radim's test case:

bin=x86_64-softmmu/qemu-system-x86_64
$bin -machine q35,iommu=on,intremap=on,kernel-irqchip=split \
     -smp cpus=2 -m 1024 -enable-kvm \
     -device i82801b11-bridge,id=pci.1,bus=pcie.0,addr=0x1e \
     -device pci-bridge,chassis_nr=2,id=pci.2,bus=pci.1,addr=0x0 \
     -netdev user,id=user.0,hostfwd=tcp::5555-:22 \
     -device e1000,netdev=user.0,bus=pci.2,addr=0x1 \
     -drive file=/var/lib/libvirt/images/vm1.qcow2,format=qcow2,if=none,id=drive-virtio-disk0 \
     -device virtio-blk-pci,scsi=off,bus=pci.2,addr=0x3,drive=drive-virtio-disk0,id=virtio-disk0,bootindex=1

This does not boot on v6 series, but can boot with the patch
mentioned.

Do you know where I can find any document on related topics?

Thanks in advance.

-- peterx

Re: [Qemu-devel] [PATCH] pci: fix requester id to be the one on root bus

[Alex Williamson]

On Thu, 12 May 2016 10:40:57 +0800
Peter Xu <peterx@redhat.com> wrote:

> On Wed, May 11, 2016 at 04:53:54PM +0300, Michael S. Tsirkin wrote:
> > On Wed, May 11, 2016 at 02:40:31PM +0800, Peter Xu wrote:  
> > > When there are devices under PCI bridge (or bridges), PCI requester ID
> > > should be the one that hooked on the root PCI bus, not the PCI device
> > > itself.
> > > 
> > > Signed-off-by: Peter Xu <peterx@redhat.com>  
> > 
> > I think this is only correct for pci bridges, and wrong for pci express bridges.
> > 
> > How exactly do you test this?  
> 
> I was using Radim's test case:
> 
> bin=x86_64-softmmu/qemu-system-x86_64
> $bin -machine q35,iommu=on,intremap=on,kernel-irqchip=split \
>      -smp cpus=2 -m 1024 -enable-kvm \
>      -device i82801b11-bridge,id=pci.1,bus=pcie.0,addr=0x1e \
>      -device pci-bridge,chassis_nr=2,id=pci.2,bus=pci.1,addr=0x0 \
>      -netdev user,id=user.0,hostfwd=tcp::5555-:22 \
>      -device e1000,netdev=user.0,bus=pci.2,addr=0x1 \
>      -drive file=/var/lib/libvirt/images/vm1.qcow2,format=qcow2,if=none,id=drive-virtio-disk0 \
>      -device virtio-blk-pci,scsi=off,bus=pci.2,addr=0x3,drive=drive-virtio-disk0,id=virtio-disk0,bootindex=1
> 
> This does not boot on v6 series, but can boot with the patch
> mentioned.
> 
> Do you know where I can find any document on related topics?

PCI Express to PCI/PCI-X Bridge Specification rev 1.0
2.3 Assignment of Requester ID and Tag by the Bridge

PCIe-to-PCI bridges assign a requester ID composed of the secondary bus
number with devfn = 0.  Although often on real hardware, the root
complex PCI bridge uses the actual bridge requester ID even though
it's actually a PCIe bridge.  Linux assume that if a bridge has a PCIe
capability with type PCIe-to-PCI/X bridge we use the secondary bus
requester ID, if it has a PCIe capability with type PCI/X-to-PCIe, we
use the bridge requester ID.  If it does not have a PCIe capability we
use the bridge ID except for a few quirked devices known to use the
secondary bus ID.  Yay standards!  Thanks,

Alex

Re: [Qemu-devel] [PATCH] pci: fix requester id to be the one on root bus

[Peter Xu]

On Wed, May 11, 2016 at 09:22:03PM -0600, Alex Williamson wrote:
[...]
> PCI Express to PCI/PCI-X Bridge Specification rev 1.0
> 2.3 Assignment of Requester ID and Tag by the Bridge
> 
> PCIe-to-PCI bridges assign a requester ID composed of the secondary bus
> number with devfn = 0.  Although often on real hardware, the root
> complex PCI bridge uses the actual bridge requester ID even though
> it's actually a PCIe bridge.  Linux assume that if a bridge has a PCIe
> capability with type PCIe-to-PCI/X bridge we use the secondary bus
> requester ID, if it has a PCIe capability with type PCI/X-to-PCIe, we
> use the bridge requester ID.  If it does not have a PCIe capability we
> use the bridge ID except for a few quirked devices known to use the
> secondary bus ID.  Yay standards!  Thanks,

Thanks Alex!

I have found pci and pci-to-pci bridge specs, which seems useful to
me.  However, I still cannot find pcie-to-pci bridge spec online (as
you have mentioned above).  Is that only for registered users?

-- peterx

Re: [Qemu-devel] [PATCH] pci: fix requester id with PCI bridges

[Peter Xu]

On Wed, May 11, 2016 at 02:40:30PM +0800, Peter Xu wrote:
> Recently I encountered issue when debugging Intel IOMMU IR codes,
> that interrupts are not working correctly with PCI bridges (reported
> by Radim). This patch fixes the problem. I assume requester ID
> should be the devfn on root PCI bus (that's how I understand it
> before, and also in guest kernel, IRTE entry SID is filled in that
> way), however I failed to find any good document to confirm
> this. Please let me know if this is correct (or I made any
> mistake).

One thing to mention is that, this patch does *not* fix the problem
if directly applied, because IR patchset introduced another patch
that also need a similar fix. In case if there is someone (Radim?)
who would like to try this patch, we need to apply this patch as
well:

---8<---

diff --git a/target-i386/kvm.c b/target-i386/kvm.c
index 80b3251..0876a1c 100644
--- a/target-i386/kvm.c
+++ b/target-i386/kvm.c
@@ -3342,7 +3342,8 @@ int kvm_arch_fixup_msi_route(struct kvm_irq_routing_entry *route,
         src.data = route->u.msi.data;

         ret = class->int_remap(iommu, &src, &dst, dev ? \
-                               pci_requester_id(dev) : X86_IOMMU_SID_INVALID);
+                               pci_requester_id_recursive(dev) : \
+                               X86_IOMMU_SID_INVALID);
         if (ret) {
             trace_kvm_x86_fixup_msi_error(route->gsi);
             return 1;

--->8---

This should be able to be applied directly onto IR v6 patchset as
well.

Thanks,

-- peterx

Re: [Qemu-devel] [PATCH] pci: fix requester id with PCI bridges

[Michael S. Tsirkin]

On Wed, May 11, 2016 at 02:53:18PM +0800, Peter Xu wrote:
> On Wed, May 11, 2016 at 02:40:30PM +0800, Peter Xu wrote:
> > Recently I encountered issue when debugging Intel IOMMU IR codes,
> > that interrupts are not working correctly with PCI bridges (reported
> > by Radim). This patch fixes the problem. I assume requester ID
> > should be the devfn on root PCI bus (that's how I understand it
> > before, and also in guest kernel, IRTE entry SID is filled in that
> > way), however I failed to find any good document to confirm
> > this. Please let me know if this is correct (or I made any
> > mistake).
> 
> One thing to mention is that, this patch does *not* fix the problem
> if directly applied, because IR patchset introduced another patch
> that also need a similar fix. In case if there is someone (Radim?)
> who would like to try this patch, we need to apply this patch as
> well:
> 
> ---8<---
> 
> diff --git a/target-i386/kvm.c b/target-i386/kvm.c
> index 80b3251..0876a1c 100644
> --- a/target-i386/kvm.c
> +++ b/target-i386/kvm.c
> @@ -3342,7 +3342,8 @@ int kvm_arch_fixup_msi_route(struct kvm_irq_routing_entry *route,
>          src.data = route->u.msi.data;
> 
>          ret = class->int_remap(iommu, &src, &dst, dev ? \
> -                               pci_requester_id(dev) : X86_IOMMU_SID_INVALID);
> +                               pci_requester_id_recursive(dev) : \
> +                               X86_IOMMU_SID_INVALID);
>          if (ret) {
>              trace_kvm_x86_fixup_msi_error(route->gsi);
>              return 1;
> 
> --->8---
> 
> This should be able to be applied directly onto IR v6 patchset as
> well.
> 
> Thanks,
> 
> -- peterx

I really dislike the name pci_requester_id_recursive.
Are there cases where we need the original requester id value?
I am guessing not, and if I'm right we should just change the implementation
of pci_requester_id to DTRT.

Re: [Qemu-devel] [PATCH] pci: fix requester id with PCI bridges

[Peter Xu]

On Thu, May 12, 2016 at 10:11:31AM +0300, Michael S. Tsirkin wrote:
> On Wed, May 11, 2016 at 02:53:18PM +0800, Peter Xu wrote:
> > On Wed, May 11, 2016 at 02:40:30PM +0800, Peter Xu wrote:
> > > Recently I encountered issue when debugging Intel IOMMU IR codes,
> > > that interrupts are not working correctly with PCI bridges (reported
> > > by Radim). This patch fixes the problem. I assume requester ID
> > > should be the devfn on root PCI bus (that's how I understand it
> > > before, and also in guest kernel, IRTE entry SID is filled in that
> > > way), however I failed to find any good document to confirm
> > > this. Please let me know if this is correct (or I made any
> > > mistake).
> > 
> > One thing to mention is that, this patch does *not* fix the problem
> > if directly applied, because IR patchset introduced another patch
> > that also need a similar fix. In case if there is someone (Radim?)
> > who would like to try this patch, we need to apply this patch as
> > well:
> > 
> > ---8<---
> > 
> > diff --git a/target-i386/kvm.c b/target-i386/kvm.c
> > index 80b3251..0876a1c 100644
> > --- a/target-i386/kvm.c
> > +++ b/target-i386/kvm.c
> > @@ -3342,7 +3342,8 @@ int kvm_arch_fixup_msi_route(struct kvm_irq_routing_entry *route,
> >          src.data = route->u.msi.data;
> > 
> >          ret = class->int_remap(iommu, &src, &dst, dev ? \
> > -                               pci_requester_id(dev) : X86_IOMMU_SID_INVALID);
> > +                               pci_requester_id_recursive(dev) : \
> > +                               X86_IOMMU_SID_INVALID);
> >          if (ret) {
> >              trace_kvm_x86_fixup_msi_error(route->gsi);
> >              return 1;
> > 
> > --->8---
> > 
> > This should be able to be applied directly onto IR v6 patchset as
> > well.
> > 
> > Thanks,
> > 
> > -- peterx
> 
> I really dislike the name pci_requester_id_recursive.

Do you have better suggestion on this? :)

> Are there cases where we need the original requester id value?
> I am guessing not, and if I'm right we should just change the implementation
> of pci_requester_id to DTRT.

There are two other callers for it (besides the MSI one):

- assigned_device_pci_cap_init()
- do_pcie_aer_inject_error()

For both the cases, I am not sure whether we can do the replacement.

-- peterx

Re: [Qemu-devel] [PATCH] pci: fix requester id with PCI bridges

[Michael S. Tsirkin]

On Thu, May 12, 2016 at 03:32:03PM +0800, Peter Xu wrote:
> On Thu, May 12, 2016 at 10:11:31AM +0300, Michael S. Tsirkin wrote:
> > On Wed, May 11, 2016 at 02:53:18PM +0800, Peter Xu wrote:
> > > On Wed, May 11, 2016 at 02:40:30PM +0800, Peter Xu wrote:
> > > > Recently I encountered issue when debugging Intel IOMMU IR codes,
> > > > that interrupts are not working correctly with PCI bridges (reported
> > > > by Radim). This patch fixes the problem. I assume requester ID
> > > > should be the devfn on root PCI bus (that's how I understand it
> > > > before, and also in guest kernel, IRTE entry SID is filled in that
> > > > way), however I failed to find any good document to confirm
> > > > this. Please let me know if this is correct (or I made any
> > > > mistake).
> > > 
> > > One thing to mention is that, this patch does *not* fix the problem
> > > if directly applied, because IR patchset introduced another patch
> > > that also need a similar fix. In case if there is someone (Radim?)
> > > who would like to try this patch, we need to apply this patch as
> > > well:
> > > 
> > > ---8<---
> > > 
> > > diff --git a/target-i386/kvm.c b/target-i386/kvm.c
> > > index 80b3251..0876a1c 100644
> > > --- a/target-i386/kvm.c
> > > +++ b/target-i386/kvm.c
> > > @@ -3342,7 +3342,8 @@ int kvm_arch_fixup_msi_route(struct kvm_irq_routing_entry *route,
> > >          src.data = route->u.msi.data;
> > > 
> > >          ret = class->int_remap(iommu, &src, &dst, dev ? \
> > > -                               pci_requester_id(dev) : X86_IOMMU_SID_INVALID);
> > > +                               pci_requester_id_recursive(dev) : \
> > > +                               X86_IOMMU_SID_INVALID);
> > >          if (ret) {
> > >              trace_kvm_x86_fixup_msi_error(route->gsi);
> > >              return 1;
> > > 
> > > --->8---
> > > 
> > > This should be able to be applied directly onto IR v6 patchset as
> > > well.
> > > 
> > > Thanks,
> > > 
> > > -- peterx
> > 
> > I really dislike the name pci_requester_id_recursive.
> 
> Do you have better suggestion on this? :)
> 
> > Are there cases where we need the original requester id value?
> > I am guessing not, and if I'm right we should just change the implementation
> > of pci_requester_id to DTRT.
> 
> There are two other callers for it (besides the MSI one):
> 
> - assigned_device_pci_cap_init()
> - do_pcie_aer_inject_error()
> 
> For both the cases, I am not sure whether we can do the replacement.
> 
> -- peterx

Read the specs please. We can't just pile on APIs.

-- 
MST

Re: [Qemu-devel] [PATCH] pci: fix requester id with PCI bridges

[Peter Xu]

On Thu, May 12, 2016 at 10:51:43AM +0300, Michael S. Tsirkin wrote:
> On Thu, May 12, 2016 at 03:32:03PM +0800, Peter Xu wrote:
[...]
> > > Are there cases where we need the original requester id value?
> > > I am guessing not, and if I'm right we should just change the implementation
> > > of pci_requester_id to DTRT.
> > 
> > There are two other callers for it (besides the MSI one):
> > 
> > - assigned_device_pci_cap_init()

This should use the new interface.

> > - do_pcie_aer_inject_error()

This should use the old one, which is to get BDF only.

> > 
> > For both the cases, I am not sure whether we can do the replacement.
> > 
> > -- peterx
> 
> Read the specs please. We can't just pile on APIs.

IIUC, we may still need to add a new API here, because not all of
them are looking for requester IDs. I just posted v2 for this patch.
Instead of renaming the new function, I renamed the old one to
pci_get_bdf(). Hope that works.

Thanks,

-- peterx