Xen devel - Sep 2013 - [PATCH V2] x86/AMD-Vi: Fix IVRS HPET special->handle override

From: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>

The current logic does not handle the case when HPET special->handle
is invalid in IVRS. On such system, the following message is shown:

(XEN) AMD-Vi: Failed to setup HPET MSI remapping: Wrong HPET

This patch will allow the ivrs_hpet[<handle>]=<sbdf> to override the
IVRS.  Also, it removes struct hpet_sbdf.iommu since it is not
used anywhere in the code.

Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
---
Changes from V1:
        * Remove the struct hpet_sbdf.iommu

 xen/drivers/passthrough/amd/iommu_acpi.c      | 21 +++++++++++++--------
 xen/drivers/passthrough/amd/iommu_intr.c      |  5 ++---
 xen/include/asm-x86/hvm/svm/amd-iommu-proto.h |  1 -
 3 files changed, 15 insertions(+), 12 deletions(-)

diff --git a/xen/drivers/passthrough/amd/iommu_acpi.c
b/xen/drivers/passthrough/amd/iommu_acpi.c
index c3b9631..d1a82c9 100644
--- a/xen/drivers/passthrough/amd/iommu_acpi.c
+++ b/xen/drivers/passthrough/amd/iommu_acpi.c
@@ -789,20 +789,25 @@ static u16 __init parse_ivhd_device_special(
         }
         break;
     case ACPI_IVHD_HPET:
+        if ( hpet_sbdf.cmdline )
+        {
+            AMD_IOMMU_DEBUG("IVHD: Command line override present for HPET
%#x "
+                            "(IVRS: %#x devID %04x:%02x:%02x.%u)\n",
+                            hpet_sbdf.id, special->handle, seg,
PCI_BUS(bdf),
+                            PCI_SLOT(bdf), PCI_FUNC(bdf));
+            break;
+        }
+
         /* set device id of hpet */
-        if ( hpet_sbdf.iommu ||
-             (hpet_sbdf.cmdline && hpet_sbdf.id != special->handle)
)
+        if ( hpet_sbdf.id != 0 )
         {
             printk(XENLOG_WARNING "Only one IVHD HPET entry is
supported\n");
             break;
         }
+
         hpet_sbdf.id = special->handle;
-        if ( !hpet_sbdf.cmdline )
-        {
-            hpet_sbdf.bdf = bdf;
-            hpet_sbdf.seg = seg;
-        }
-        hpet_sbdf.iommu = iommu;
+        hpet_sbdf.bdf = bdf;
+        hpet_sbdf.seg = seg;
         break;
     default:
         printk(XENLOG_ERR "Unrecognized IVHD special variety %#x\n",
diff --git a/xen/drivers/passthrough/amd/iommu_intr.c
b/xen/drivers/passthrough/amd/iommu_intr.c
index 213f4d7..3e29824 100644
--- a/xen/drivers/passthrough/amd/iommu_intr.c
+++ b/xen/drivers/passthrough/amd/iommu_intr.c
@@ -598,10 +598,9 @@ int __init amd_setup_hpet_msi(struct msi_desc *msi_desc)
     unsigned long flags;
     int rc = 0;
 
-    if ( msi_desc->hpet_id != hpet_sbdf.id || !hpet_sbdf.iommu )
+    if ( msi_desc->hpet_id != hpet_sbdf.id )
     {
-        AMD_IOMMU_DEBUG("Failed to setup HPET MSI remapping: %s\n",
-                        hpet_sbdf.iommu ? "Wrong HPET" : "No
IOMMU");
+        AMD_IOMMU_DEBUG("Failed to setup HPET MSI remapping. Mismatch IVRS
info.\n");
         return -ENODEV;
     }
 
diff --git a/xen/include/asm-x86/hvm/svm/amd-iommu-proto.h
b/xen/include/asm-x86/hvm/svm/amd-iommu-proto.h
index 3e6961d..f14dc68 100644
--- a/xen/include/asm-x86/hvm/svm/amd-iommu-proto.h
+++ b/xen/include/asm-x86/hvm/svm/amd-iommu-proto.h
@@ -109,7 +109,6 @@ extern struct ioapic_sbdf {
 extern struct hpet_sbdf {
     u16 bdf, seg, id;
     bool_t cmdline;
-    struct amd_iommu *iommu;
 } hpet_sbdf;
 
 extern void *shared_intremap_table;
-- 
1.8.1.2

>>> On 25.09.13 at 21:15, <suravee.suthikulpanit@amd.com> wrote:
> --- a/xen/drivers/passthrough/amd/iommu_acpi.c
> +++ b/xen/drivers/passthrough/amd/iommu_acpi.c
> @@ -789,20 +789,25 @@ static u16 __init parse_ivhd_device_special(
>          }
>          break;
>      case ACPI_IVHD_HPET:
> +        if ( hpet_sbdf.cmdline )
> +        {
> +            AMD_IOMMU_DEBUG("IVHD: Command line override present for
HPET %#x "
> +                            "(IVRS: %#x devID
%04x:%02x:%02x.%u)\n",
> +                            hpet_sbdf.id, special->handle, seg,
PCI_BUS(bdf),
> +                            PCI_SLOT(bdf), PCI_FUNC(bdf));
> +            break;
> +        }
> +
>          /* set device id of hpet */
> -        if ( hpet_sbdf.iommu ||
> -             (hpet_sbdf.cmdline && hpet_sbdf.id !=
special->handle) )
> +        if ( hpet_sbdf.id != 0 )
Surely there''s no guarantee that special->handle in non-zero. After
all that''s a firmware allocated value. What might be acceptable is
assuming bdf to be non-zero, but even that would look sort of
hackish. Perhaps you''d be better off converting .cmdline to an
enum with states none, cmdline, and ivhd, and use a switch() here.
>          {
>              printk(XENLOG_WARNING "Only one IVHD HPET entry is
supported\n");
>              break;
>          }
> +
>          hpet_sbdf.id = special->handle;
> -        if ( !hpet_sbdf.cmdline )
> -        {
> -            hpet_sbdf.bdf = bdf;
> -            hpet_sbdf.seg = seg;
> -        }
> -        hpet_sbdf.iommu = iommu;
> +        hpet_sbdf.bdf = bdf;
> +        hpet_sbdf.seg = seg;
>          break;
>      default:
>          printk(XENLOG_ERR "Unrecognized IVHD special variety
%#x\n",
> --- a/xen/drivers/passthrough/amd/iommu_intr.c
> +++ b/xen/drivers/passthrough/amd/iommu_intr.c
> @@ -598,10 +598,9 @@ int __init amd_setup_hpet_msi(struct msi_desc
*msi_desc)
>      unsigned long flags;
>      int rc = 0;
>  
> -    if ( msi_desc->hpet_id != hpet_sbdf.id || !hpet_sbdf.iommu )
> +    if ( msi_desc->hpet_id != hpet_sbdf.id )
>      {
> -        AMD_IOMMU_DEBUG("Failed to setup HPET MSI remapping:
%s\n",
> -                        hpet_sbdf.iommu ? "Wrong HPET" :
"No IOMMU");
> +        AMD_IOMMU_DEBUG("Failed to setup HPET MSI remapping. Mismatch
IVRS info.\n");
This will need to get adjusted too - instead of !hpet_sbdf.iommu
you''d now need to check the new enum to be other than "none".

Jan