Xen devel - Aug 2013 - [PATCH] AMD IOMMU: also allocate IRTEs for HPET MSI

Omitting this was a blatant oversight of mine in commit 2ca9fbd7 ("AMD
IOMMU: allocate IRTE entries instead of using a static mapping").

This also changes a bogus inequality check into a sensible one, even
though it is already known that this will make HPET MSI unusable on
certain systems (having respective broken firmware). This, however,
seems better than failing on systems with consistent ACPI tables.

Reported-by: Sander Eikelenboom <linux@eikelenboom.it>
Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/drivers/passthrough/amd/iommu_intr.c
+++ b/xen/drivers/passthrough/amd/iommu_intr.c
@@ -595,14 +595,31 @@ void* __init amd_iommu_alloc_intremap_ta
 
 int __init amd_setup_hpet_msi(struct msi_desc *msi_desc)
 {
-    if ( (!msi_desc->hpet_id != hpet_sbdf.id) ||
-         (hpet_sbdf.iommu == NULL) )
+    spinlock_t *lock;
+    unsigned long flags;
+    int rc = 0;
+
+    if ( msi_desc->hpet_id != hpet_sbdf.id || !hpet_sbdf.iommu )
     {
-        AMD_IOMMU_DEBUG("Fail to setup HPET MSI remapping\n");
-        return 1;
+        AMD_IOMMU_DEBUG("Failed to setup HPET MSI remapping: %s\n",
+                        hpet_sbdf.iommu ? "Wrong HPET" : "No
IOMMU");
+        return -ENODEV;
     }
 
-    return 0;
+    lock = get_intremap_lock(hpet_sbdf.seg, hpet_sbdf.bdf);
+    spin_lock_irqsave(lock, flags);
+
+    msi_desc->remap_index = alloc_intremap_entry(hpet_sbdf.seg,
+                                                 hpet_sbdf.bdf, 1);
+    if ( msi_desc->remap_index >= INTREMAP_ENTRIES )
+    {
+        msi_desc->remap_index = -1;
+        rc = -ENXIO;
+    }
+
+    spin_unlock_irqrestore(lock, flags);
+
+    return rc;
 }
 
 static void dump_intremap_table(const u32 *table)





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

Jan,

I have been trying to test out this patch.  Somehow, I could not get Xen 
boot option to exercise the code path.  What boot configuration are you 
using to test this?

I''m currently using option

"loglvl=all loglvl_guest=all cpuidle cpufreq=xen debug lapic=debug 
iommu=on,debug,verbose,amd-iommu-debug apic_verbosity=debug apic=debug"

I already double check the IVRS table on my system and it looks 
correct.  Am I missing something here?

Thanks,

Suravee.

On 8/26/2013 9:20 AM, Jan Beulich wrote:
> Omitting this was a blatant oversight of mine in commit 2ca9fbd7 ("AMD
> IOMMU: allocate IRTE entries instead of using a static mapping").
>
> This also changes a bogus inequality check into a sensible one, even
> though it is already known that this will make HPET MSI unusable on
> certain systems (having respective broken firmware). This, however,
> seems better than failing on systems with consistent ACPI tables.
>
> Reported-by: Sander Eikelenboom <linux@eikelenboom.it>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>
> --- a/xen/drivers/passthrough/amd/iommu_intr.c
> +++ b/xen/drivers/passthrough/amd/iommu_intr.c
> @@ -595,14 +595,31 @@ void* __init amd_iommu_alloc_intremap_ta
>   
>   int __init amd_setup_hpet_msi(struct msi_desc *msi_desc)
>   {
> -    if ( (!msi_desc->hpet_id != hpet_sbdf.id) ||
> -         (hpet_sbdf.iommu == NULL) )
> +    spinlock_t *lock;
> +    unsigned long flags;
> +    int rc = 0;
> +
> +    if ( msi_desc->hpet_id != hpet_sbdf.id || !hpet_sbdf.iommu )
>       {
> -        AMD_IOMMU_DEBUG("Fail to setup HPET MSI remapping\n");
> -        return 1;
> +        AMD_IOMMU_DEBUG("Failed to setup HPET MSI remapping:
%s\n",
> +                        hpet_sbdf.iommu ? "Wrong HPET" :
"No IOMMU");
> +        return -ENODEV;
>       }
>   
> -    return 0;
> +    lock = get_intremap_lock(hpet_sbdf.seg, hpet_sbdf.bdf);
> +    spin_lock_irqsave(lock, flags);
> +
> +    msi_desc->remap_index = alloc_intremap_entry(hpet_sbdf.seg,
> +                                                 hpet_sbdf.bdf, 1);
> +    if ( msi_desc->remap_index >= INTREMAP_ENTRIES )
> +    {
> +        msi_desc->remap_index = -1;
> +        rc = -ENXIO;
> +    }
> +
> +    spin_unlock_irqrestore(lock, flags);
> +
> +    return rc;
>   }
>   
>   static void dump_intremap_table(const u32 *table)
>
>
>

>>> On 27.08.13 at 03:21, Suravee Suthikulpanit
<suravee.suthikulpanit@amd.com> wrote:
> I have been trying to test out this patch.  Somehow, I could not get Xen 
> boot option to exercise the code path.  What boot configuration are you 
> using to test this?
I don''t have a system suitable for testing this; Sander did. The main
requirement apart from having an IOMMU is to also have a HPET
with at least on MSI-capable channel. Otherwise code won''t get
here. The normal flow is 

hpet_broadcast_init() -> hpet_fsb_cap_lookup() -> hpet_assign_irq()
-> hpet_setup_msi_irq() -> iommu_setup_hpet_msi()

Jan

On 8/27/2013 1:41 AM, Jan Beulich wrote:
>>>> On 27.08.13 at 03:21, Suravee Suthikulpanit
<suravee.suthikulpanit@amd.com> wrote:
>> I have been trying to test out this patch.  Somehow, I could not get
Xen
>> boot option to exercise the code path.  What boot configuration are you
>> using to test this?
> I don''t have a system suitable for testing this; Sander did. The
main
> requirement apart from having an IOMMU is to also have a HPET
> with at least on MSI-capable channel. Otherwise code won''t get
> here. The normal flow is
>
> hpet_broadcast_init() -> hpet_fsb_cap_lookup() -> hpet_assign_irq()
> -> hpet_setup_msi_irq() -> iommu_setup_hpet_msi()
>
> Jan
Ah,ok.. I found out thatthisonly applies on the family10h system with 
IOMMUdue to the check in arch/x86/time.c:_disable_pit_irq

"if (!boot_cpu_has(X86_FEATURE_ARAT))"

and the condition in arch/x86/cpu/amd.c:_init_amd()

     /*
      * Family 0x12 and above processors have APIC timer
      * running in deep C states.
      */
     if (c->x86 > 0x11)
         set_bit(X86_FEATURE_ARAT, c->x86_capability);

Jan, doyou think you could also add the comment that this only 
applicable on family10h?

Acked-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>

Thank you,

Suravee.


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

>>> On 27.08.13 at 21:06, Suravee Suthikulanit
<suravee.suthikulpanit@amd.com> wrote:
> Ah,ok.. I found out thatthisonly applies on the family10h system with 
> IOMMUdue to the check in arch/x86/time.c:_disable_pit_irq
> 
> "if (!boot_cpu_has(X86_FEATURE_ARAT))"
> 
> and the condition in arch/x86/cpu/amd.c:_init_amd()
> 
>      /*
>       * Family 0x12 and above processors have APIC timer
>       * running in deep C states.
>       */
>      if (c->x86 > 0x11)
>          set_bit(X86_FEATURE_ARAT, c->x86_capability);
> 
> Jan, doyou think you could also add the comment that this only 
> applicable on family10h?
So are you saying that the check here is wrong (because if the
comment is, the check is too)? If so, I''d prefer a patch coming
from you (AMD) over cooking one myself. And if not, I need
clarification on what you''re trying to point out is wrong.

Jan