[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Xen-devel] [PATCH] AMD IOMMU: make interrupt work again



On 6/14/2013 2:14 AM, Jan Beulich wrote:
On 14.06.13 at 08:40, "Jan Beulich" <JBeulich@xxxxxxxx> wrote:
On 14.06.13 at 08:27, "Jan Beulich" <JBeulich@xxxxxxxx> wrote:
On 13.06.13 at 18:34, Suravee Suthikulanit <suravee.suthikulpanit@xxxxxxx> 
wrote:
Basically, the only different is this line that only appears in the "Bad"
version.

      (XEN)  MSI     56 vec=28  fixed  edge deassert phys    cpu
dest=00000001 mask=0/0/1
"xl debug-key i" also show the following information

      (XEN)    IRQ:  56 affinity:1 vec:28 type=AMD-IOMMU-MSI   status=00000000
mapped, unbound

There are several questionable things here:
- the interrupt being of "deassert" kind
- destination mode being physical (while all others are logical)
- the interrupt being unbound (i.e. there's no action associated
   with it, and hence no handler would ever get called)
And I think I see now at least part of what's missing: Neither
msi_compose_msg() nor write_msi_msg() get (indirectly) called
from set_iommu_interrupt_handler(). Which was that (wrong)
way already before said c/s, but got masked by
iommu_msi_set_affinity() doing a full setup rather than
incremental modification as set_msi_affinity() does. In order to
fix this reasonably cleanly I'd like to do a little bit of code
re-structuring, so it'll be more than a couple of minutes until I
could send you a patch.
So here's a first try.

Jan
This fixes the issue.  Here is the output from the xl debug-key i and M.

(XEN) MSI 56 vec=28 lowest edge assert log lowest dest=00000001 mask=0/0/?

XEN) IRQ: 56 affinity:1 vec:28 type=AMD-IOMMU-MSI status=00000000 iommu_interrupt_handler+0/0x66

Acked: - Suravee Suthikulpanit <suravee.suthikulpanit@xxxxxxx>

Thank you Jan.

Suravee


Commit 899110e3 ("AMD IOMMU: include IOMMU interrupt information in 'M'
debug key output") made the AMD IOMMU MSI setup code use more of the
generic MSI setup code (as other than for VT-d this is an ordinary MSI-
capable PCI device), but failed to notice that till now interrupt setup
there _required_ the subsequent affinity setup to be done, as that was
the only point where the MSI message would get written. The generic MSI
affinity setting routine, however, does only an incremental change,
i.e. relies on this setup to have been done before.

In order to not make the code even more clumsy, introduce a new low
level helper routine __setup_msi_irq(), thus eliminating the need for
the AMD IOMMU code to directly fiddle with the IRQ descriptor.

Reported-by: Suravee Suthikulanit <suravee.suthikulpanit@xxxxxxx>
Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>

--- a/xen/arch/x86/msi.c
+++ b/xen/arch/x86/msi.c
@@ -472,11 +472,18 @@ static struct msi_desc* alloc_msi_entry(
int setup_msi_irq(struct irq_desc *desc, struct msi_desc *msidesc)
  {
+    return __setup_msi_irq(desc, msidesc,
+                           msi_maskable_irq(msidesc) ? &pci_msi_maskable
+                                                     : &pci_msi_nonmaskable);
+}
+
+int __setup_msi_irq(struct irq_desc *desc, struct msi_desc *msidesc,
+                    hw_irq_controller *handler)
+{
      struct msi_msg msg;
desc->msi_desc = msidesc;
-    desc->handler = msi_maskable_irq(msidesc) ? &pci_msi_maskable
-                                              : &pci_msi_nonmaskable;
+    desc->handler = handler;
      msi_compose_msg(desc, &msg);
      return write_msi_msg(msidesc, &msg);
  }
--- a/xen/drivers/passthrough/amd/iommu_init.c
+++ b/xen/drivers/passthrough/amd/iommu_init.c
@@ -748,7 +748,7 @@ static void iommu_interrupt_handler(int
  static bool_t __init set_iommu_interrupt_handler(struct amd_iommu *iommu)
  {
      int irq, ret;
-    struct irq_desc *desc;
+    hw_irq_controller *handler;
      unsigned long flags;
      u16 control;
@@ -759,7 +759,6 @@ static bool_t __init set_iommu_interrupt
          return 0;
      }
- desc = irq_to_desc(irq);
      spin_lock_irqsave(&pcidevs_lock, flags);
      iommu->msi.dev = pci_get_pdev(iommu->seg, PCI_BUS(iommu->bdf),
                                    PCI_DEVFN2(iommu->bdf));
@@ -771,7 +770,6 @@ static bool_t __init set_iommu_interrupt
                          PCI_SLOT(iommu->bdf), PCI_FUNC(iommu->bdf));
          return 0;
      }
-    desc->msi_desc = &iommu->msi;
      control = pci_conf_read16(iommu->seg, PCI_BUS(iommu->bdf),
                                PCI_SLOT(iommu->bdf), PCI_FUNC(iommu->bdf),
                                iommu->msi.msi_attrib.pos + PCI_MSI_FLAGS);
@@ -781,14 +779,15 @@ static bool_t __init set_iommu_interrupt
          iommu->msi.msi_attrib.maskbit = 1;
          iommu->msi.msi.mpos = msi_mask_bits_reg(iommu->msi.msi_attrib.pos,
                                                  is_64bit_address(control));
-        desc->handler = &iommu_maskable_msi_type;
+        handler = &iommu_maskable_msi_type;
      }
      else
-        desc->handler = &iommu_msi_type;
-    ret = request_irq(irq, iommu_interrupt_handler, 0, "amd_iommu", iommu);
+        handler = &iommu_msi_type;
+    ret = __setup_msi_irq(irq_to_desc(irq), &iommu->msi, handler);
+    if ( !ret )
+        ret = request_irq(irq, iommu_interrupt_handler, 0, "amd_iommu", iommu);
      if ( ret )
      {
-        desc->handler = &no_irq_type;
          destroy_irq(irq);
          AMD_IOMMU_DEBUG("can't request irq\n");
          return 0;
--- a/xen/include/asm-x86/msi.h
+++ b/xen/include/asm-x86/msi.h
@@ -72,6 +72,7 @@ struct msi_msg {
  };
struct irq_desc;
+struct hw_interrupt_type;
  struct msi_desc;
  /* Helper functions */
  extern int pci_enable_msi(struct msi_info *msi, struct msi_desc **desc);
@@ -79,6 +80,8 @@ extern void pci_disable_msi(struct msi_d
  extern int pci_prepare_msix(u16 seg, u8 bus, u8 devfn, bool_t off);
  extern void pci_cleanup_msi(struct pci_dev *pdev);
  extern int setup_msi_irq(struct irq_desc *, struct msi_desc *);
+extern int __setup_msi_irq(struct irq_desc *, struct msi_desc *,
+                           const struct hw_interrupt_type *);
  extern void teardown_msi_irq(int irq);
  extern int msi_free_vector(struct msi_desc *entry);
  extern int pci_restore_msi_state(struct pci_dev *pdev);





_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel


 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.