Re: [PATCH] AMD IOMMU: make interrupt work again

[Suravee Suthikulanit <suravee.suthikulpanit@amd.com>]

On 6/14/2013 2:14 AM, Jan Beulich wrote:
>>>> On 14.06.13 at 08:40, "Jan Beulich" <JBeulich@suse.com> wrote:
>>>>> On 14.06.13 at 08:27, "Jan Beulich" <JBeulich@suse.com> wrote:
>>>>>> On 13.06.13 at 18:34, Suravee Suthikulanit <suravee.suthikulpanit@amd.com> 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@amd.com>

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@amd.com>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>
> --- 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);
>
>