From mboxrd@z Thu Jan 1 00:00:00 1970 From: Suravee Suthikulanit Subject: Re: [PATCH] AMD IOMMU: make interrupt work again Date: Fri, 14 Jun 2013 11:10:31 -0500 Message-ID: <51BB4077.4010106@amd.com> References: <1370840751-11277-1-git-send-email-suravee.suthikulpanit@amd.com> <51B5CCF002000078000DC9A5@nat28.tlf.novell.com> <20130610121855.GE8802@ocelot.phlegethon.org> <51B5E58802000078000DCA7E@nat28.tlf.novell.com> <51B6E40A02000078000DCF6C@nat28.tlf<51B6E40A02000078000DCF6C@nat28.tlf.novell.com> <51B7ACB5.7070805@amd.com> <51B8303302000078000DD6B6@nat28.tlf.novell.com> <51B8F814.70202@amd.com> <51B923F1.7000802@amd.com> <51BA082C02000078000DE0C0@nat28.tlf.novell.com> <51B9F481.50204@amd.com> <51BAD3FE02000078000DE433@nat28.tlf.novell.com> <51BAD70402000078000DE43E@nat28.tlf.novell.com> <51BADEFB02000078000DE458@nat28.tlf.novell.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <51BADEFB02000078000DE458@nat28.tlf.novell.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Jan Beulich Cc: Tim Deegan , "xen-devel@lists.xen.org" , "Shin, Jacob" , "Hurwitz, Sherry" List-Id: xen-devel@lists.xenproject.org On 6/14/2013 2:14 AM, Jan Beulich wrote: >>>> On 14.06.13 at 08:40, "Jan Beulich" wrote: >>>>> On 14.06.13 at 08:27, "Jan Beulich" wrote: >>>>>> On 13.06.13 at 18:34, Suravee Suthikulanit 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 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 > Signed-off-by: Jan Beulich > > --- 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); > >