Xen devel - Sep 2013 - [PATCH 1/1 V3] x86/AMD-Vi: Add additional check for invalid special->handle

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

This patch handle additional cases for IVRS bugs where special->handle
is not correctly initialized for IOAPIC and HPETS due to firmware bugs.

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

Provide logic in "is_ioapic_overidden()"

Signed-off-by: Jan Beulich <JBeulich@suse.com>
---
Changes from V2:
	- Add logic from Jan
	- Also invalid handle for HPET
	- Clean up old logics

 xen/drivers/passthrough/amd/iommu_acpi.c | 60 +++++++++++++++++++++++++-------
 1 file changed, 47 insertions(+), 13 deletions(-)

diff --git a/xen/drivers/passthrough/amd/iommu_acpi.c
b/xen/drivers/passthrough/amd/iommu_acpi.c
index 89b359c..0e63e35 100644
--- a/xen/drivers/passthrough/amd/iommu_acpi.c
+++ b/xen/drivers/passthrough/amd/iommu_acpi.c
@@ -664,19 +664,46 @@ static void __init parse_ivrs_hpet(char *str)
 
     ASSERT(*s == ''['');
     id = simple_strtoul(s + 1, &s, 0);
-    if ( id != (typeof(hpet_sbdf.id))id || *s != '']'' || *++s
!= ''='' )
+    if ( (*s != '']'') || (*++s != ''='') )
         return;
 
     s = parse_pci(s + 1, &seg, &bus, &dev, &func);
     if ( !s || *s )
         return;
 
+    hpet_sbdf.id = id;
     hpet_sbdf.bdf = PCI_BDF(bus, dev, func);
     hpet_sbdf.seg = seg;
     hpet_sbdf.cmdline = 1;
 }
 custom_param("ivrs_hpet[", parse_ivrs_hpet);
 
+static bool_t is_ioapic_overidden(u16 seg, u16 bdf, u8 handle)
+{
+    bool_t ret = 0;
+    int apic = find_first_bit(ioapic_cmdline, ARRAY_SIZE(ioapic_sbdf));
+
+    while ( apic < ARRAY_SIZE(ioapic_sbdf) )
+    {
+        if ( ioapic_sbdf[apic].bdf == bdf &&
+             ioapic_sbdf[apic].seg == seg )
+            break;
+        apic = find_next_bit(ioapic_cmdline, ARRAY_SIZE(ioapic_sbdf),
+                             apic + 1);
+    }
+
+    if ( apic < ARRAY_SIZE(ioapic_sbdf) )
+    {
+        AMD_IOMMU_DEBUG("IVHD: Command line override present for IO-APIC
%#x "
+                        "(IVRS: %#x devID %04x:%02x:%02x.%u)\n",
+                        apic, handle, seg, PCI_BUS(bdf),
+                        PCI_SLOT(bdf), PCI_FUNC(bdf));
+        ret = 1;
+    }
+
+    return ret;
+}
+
 static u16 __init parse_ivhd_device_special(
     const struct acpi_ivrs_device8c *special, u16 seg,
     u16 header_length, u16 block_length, struct amd_iommu *iommu)
@@ -698,16 +725,18 @@ static u16 __init parse_ivhd_device_special(
         return 0;
     }
 
-    AMD_IOMMU_DEBUG("IVHD Special: %04x:%02x:%02x.%u variety %#x handle
%#x\n",
+    AMD_IOMMU_DEBUG("IVHD Special: %04x:%02x:%02x.%u variety %#x handle
%#x used_id %#x\n",
                     seg, PCI_BUS(bdf), PCI_SLOT(bdf), PCI_FUNC(bdf),
-                    special->variety, special->handle);
+                    special->variety, special->handle,
special->used_id);
     add_ivrs_mapping_entry(bdf, bdf, special->header.data_setting, iommu);
 
     switch ( special->variety )
     {
     case ACPI_IVHD_IOAPIC:
-        if ( !iommu_intremap )
+        if ( !iommu_intremap ||
+             is_ioapic_overidden(seg, bdf, special->handle) )
             break;
+
         /*
          * Some BIOSes have IOAPIC broken entries so we check for IVRS
          * consistency here --- whether entry''s IOAPIC ID is valid and
@@ -725,10 +754,7 @@ static u16 __init parse_ivhd_device_special(
                 return 0;
             }
 
-            if ( test_bit(special->handle, ioapic_cmdline) )
-                AMD_IOMMU_DEBUG("IVHD: Command line override present for
IO-APIC %#x\n",
-                                special->handle);
-            else if ( ioapic_sbdf[special->handle].pin_2_idx )
+            if ( ioapic_sbdf[special->handle].pin_2_idx )
             {
                 if ( ioapic_sbdf[special->handle].bdf == bdf &&
                      ioapic_sbdf[special->handle].seg == seg )
@@ -770,6 +796,16 @@ 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));
+            hpet_sbdf.iommu = iommu;
+            break;
+        }
+
         /* set device id of hpet */
         if ( hpet_sbdf.iommu ||
              (hpet_sbdf.cmdline && hpet_sbdf.id != special->handle)
)
@@ -777,12 +813,10 @@ static u16 __init parse_ivhd_device_special(
             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.bdf = bdf;
+        hpet_sbdf.seg = seg;
         hpet_sbdf.iommu = iommu;
         break;
     default:
-- 
1.8.1.2

>>> On 12.09.13 at 19:00, <suravee.suthikulpanit@amd.com> wrote:
> --- a/xen/drivers/passthrough/amd/iommu_acpi.c
> +++ b/xen/drivers/passthrough/amd/iommu_acpi.c
> @@ -664,19 +664,46 @@ static void __init parse_ivrs_hpet(char *str)
>  
>      ASSERT(*s == ''['');
>      id = simple_strtoul(s + 1, &s, 0);
> -    if ( id != (typeof(hpet_sbdf.id))id || *s != '']'' ||
*++s != ''='' )
> +    if ( (*s != '']'') || (*++s != ''='')
)
No, unless you have a very good reason.
>          return;
>  
>      s = parse_pci(s + 1, &seg, &bus, &dev, &func);
>      if ( !s || *s )
>          return;
>  
> +    hpet_sbdf.id = id;
In essence this is the only change not contained in the patch I sent.
So I''d be inclined to commit this (and perhaps the one debug
message adjustment below) under your name, and my proposed
change as a separate one.
>      hpet_sbdf.bdf = PCI_BDF(bus, dev, func);
>      hpet_sbdf.seg = seg;
>      hpet_sbdf.cmdline = 1;
>  }
>  custom_param("ivrs_hpet[", parse_ivrs_hpet);
>  
> +static bool_t is_ioapic_overidden(u16 seg, u16 bdf, u8 handle)
Missing __init annotation. And anyway, I can''t really see why
putting this in a separate function is a significant benefit. It''s
only being used in one place afaics.
> +{
> +    bool_t ret = 0;
> +    int apic = find_first_bit(ioapic_cmdline, ARRAY_SIZE(ioapic_sbdf));
> +
> +    while ( apic < ARRAY_SIZE(ioapic_sbdf) )
> +    {
> +        if ( ioapic_sbdf[apic].bdf == bdf &&
> +             ioapic_sbdf[apic].seg == seg )
> +            break;
> +        apic = find_next_bit(ioapic_cmdline, ARRAY_SIZE(ioapic_sbdf),
> +                             apic + 1);
> +    }
> +
> +    if ( apic < ARRAY_SIZE(ioapic_sbdf) )
> +    {
> +        AMD_IOMMU_DEBUG("IVHD: Command line override present for
IO-APIC %#x "
> +                        "(IVRS: %#x devID %04x:%02x:%02x.%u)\n",
> +                        apic, handle, seg, PCI_BUS(bdf),
> +                        PCI_SLOT(bdf), PCI_FUNC(bdf));
> +        ret = 1;
> +    }
> +
> +    return ret;
> +}
> +
>  static u16 __init parse_ivhd_device_special(
>      const struct acpi_ivrs_device8c *special, u16 seg,
>      u16 header_length, u16 block_length, struct amd_iommu *iommu)
> @@ -698,16 +725,18 @@ static u16 __init parse_ivhd_device_special(
>          return 0;
>      }
>  
> -    AMD_IOMMU_DEBUG("IVHD Special: %04x:%02x:%02x.%u variety %#x
handle %#x\n",
> +    AMD_IOMMU_DEBUG("IVHD Special: %04x:%02x:%02x.%u variety %#x
handle %#x used_id %#x\n",
>                      seg, PCI_BUS(bdf), PCI_SLOT(bdf), PCI_FUNC(bdf),
> -                    special->variety, special->handle);
> +                    special->variety, special->handle,
special->used_id);
>      add_ivrs_mapping_entry(bdf, bdf, special->header.data_setting,
iommu);
>  
>      switch ( special->variety )
>      {
>      case ACPI_IVHD_IOAPIC:
> -        if ( !iommu_intremap )
> +        if ( !iommu_intremap ||
> +             is_ioapic_overidden(seg, bdf, special->handle) )
>              break;
> +
>          /*
>           * Some BIOSes have IOAPIC broken entries so we check for IVRS
>           * consistency here --- whether entry''s IOAPIC ID is
valid and
> @@ -725,10 +754,7 @@ static u16 __init parse_ivhd_device_special(
>                  return 0;
>              }
>  
> -            if ( test_bit(special->handle, ioapic_cmdline) )
> -                AMD_IOMMU_DEBUG("IVHD: Command line override present
for IO-APIC %#x\n",
> -                                special->handle);
> -            else if ( ioapic_sbdf[special->handle].pin_2_idx )
> +            if ( ioapic_sbdf[special->handle].pin_2_idx )
Again - no, unless you have a very good reason.
>              {
>                  if ( ioapic_sbdf[special->handle].bdf == bdf &&
>                       ioapic_sbdf[special->handle].seg == seg )
> @@ -770,6 +796,16 @@ 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));
> +            hpet_sbdf.iommu = iommu;
> +            break;
> +        }
> +
>          /* set device id of hpet */
>          if ( hpet_sbdf.iommu ||
>               (hpet_sbdf.cmdline && hpet_sbdf.id !=
special->handle) )
> @@ -777,12 +813,10 @@ static u16 __init parse_ivhd_device_special(
>              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.bdf = bdf;
> +        hpet_sbdf.seg = seg;
I don''t see what benefit these HPET related changes provide. Or
if there is any that I overlook, then the previous uses of
hpet_sbdf.cmdline should all be eliminated.

Jan

On 9/13/2013 4:24 AM, Jan Beulich wrote:
>>>> On 12.09.13 at 19:00, <suravee.suthikulpanit@amd.com>
wrote:
>> --- a/xen/drivers/passthrough/amd/iommu_acpi.c
>> +++ b/xen/drivers/passthrough/amd/iommu_acpi.c
>> @@ -664,19 +664,46 @@ static void __init parse_ivrs_hpet(char *str)
>>   
>>       ASSERT(*s == ''['');
>>       id = simple_strtoul(s + 1, &s, 0);
>> -    if ( id != (typeof(hpet_sbdf.id))id || *s != '']''
|| *++s != ''='' )
>> +    if ( (*s != '']'') || (*++s !=
''='') )
> No, unless you have a very good reason.
Oh, sorry. This is my mistake.
>
>>           return;
>>   
>>       s = parse_pci(s + 1, &seg, &bus, &dev, &func);
>>       if ( !s || *s )
>>           return;
>>   
>> +    hpet_sbdf.id = id;
> In essence this is the only change not contained in the patch I sent.
> So I''d be inclined to commit this (and perhaps the one debug
> message adjustment below) under your name, and my proposed
> change as a separate one.
Ok.
>
>>       hpet_sbdf.bdf = PCI_BDF(bus, dev, func);
>>       hpet_sbdf.seg = seg;
>>       hpet_sbdf.cmdline = 1;
>>   }
>>   custom_param("ivrs_hpet[", parse_ivrs_hpet);
>>   
>> +static bool_t is_ioapic_overidden(u16 seg, u16 bdf, u8 handle)
> Missing __init annotation. And anyway, I can''t really see why
> putting this in a separate function is a significant benefit. It''s
> only being used in one place afaics.
Sorry, I forgot the __init.  I normally just like to keep to code that 
does a specific
thing inside it''s own function for the ease of reading.  But this is 
purely coding style.
I can put it in the caller function if you prefer.
>
>> +{
>> +    bool_t ret = 0;
>> +    int apic = find_first_bit(ioapic_cmdline,
ARRAY_SIZE(ioapic_sbdf));
>> +
>> +    while ( apic < ARRAY_SIZE(ioapic_sbdf) )
>> +    {
>> +        if ( ioapic_sbdf[apic].bdf == bdf &&
>> +             ioapic_sbdf[apic].seg == seg )
>> +            break;
>> +        apic = find_next_bit(ioapic_cmdline, ARRAY_SIZE(ioapic_sbdf),
>> +                             apic + 1);
>> +    }
>> +
>> +    if ( apic < ARRAY_SIZE(ioapic_sbdf) )
>> +    {
>> +        AMD_IOMMU_DEBUG("IVHD: Command line override present for
IO-APIC %#x "
>> +                        "(IVRS: %#x devID
%04x:%02x:%02x.%u)\n",
>> +                        apic, handle, seg, PCI_BUS(bdf),
>> +                        PCI_SLOT(bdf), PCI_FUNC(bdf));
>> +        ret = 1;
>> +    }
>> +
>> +    return ret;
>> +}
>> +
>>   static u16 __init parse_ivhd_device_special(
>>       const struct acpi_ivrs_device8c *special, u16 seg,
>>       u16 header_length, u16 block_length, struct amd_iommu *iommu)
>> @@ -698,16 +725,18 @@ static u16 __init parse_ivhd_device_special(
>>           return 0;
>>       }
>>   
>> -    AMD_IOMMU_DEBUG("IVHD Special: %04x:%02x:%02x.%u variety %#x
handle %#x\n",
>> +    AMD_IOMMU_DEBUG("IVHD Special: %04x:%02x:%02x.%u variety %#x
handle %#x used_id %#x\n",
>>                       seg, PCI_BUS(bdf), PCI_SLOT(bdf), PCI_FUNC(bdf),
>> -                    special->variety, special->handle);
>> +                    special->variety, special->handle,
special->used_id);
>>       add_ivrs_mapping_entry(bdf, bdf, special->header.data_setting,
iommu);
>>   
>>       switch ( special->variety )
>>       {
>>       case ACPI_IVHD_IOAPIC:
>> -        if ( !iommu_intremap )
>> +        if ( !iommu_intremap ||
>> +             is_ioapic_overidden(seg, bdf, special->handle) )
>>               break;
>> +
>>           /*
>>            * Some BIOSes have IOAPIC broken entries so we check for
IVRS
>>            * consistency here --- whether entry''s IOAPIC ID is
valid and
>> @@ -725,10 +754,7 @@ static u16 __init parse_ivhd_device_special(
>>                   return 0;
>>               }
>>   
>> -            if ( test_bit(special->handle, ioapic_cmdline) )
>> -                AMD_IOMMU_DEBUG("IVHD: Command line override
present for IO-APIC %#x\n",
>> -                                special->handle);
>> -            else if ( ioapic_sbdf[special->handle].pin_2_idx )
>> +            if ( ioapic_sbdf[special->handle].pin_2_idx )
> Again - no, unless you have a very good reason.
I think with the check for command line stuff already handle prior to 
this point,
I don''t see why we still need to keep this here.  It should not need to
go through
the logic here.
>
>>               {
>>                   if ( ioapic_sbdf[special->handle].bdf == bdf
&&
>>                        ioapic_sbdf[special->handle].seg == seg )
>> @@ -770,6 +796,16 @@ 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));
>> +            hpet_sbdf.iommu = iommu;
>> +            break;
>> +        }
>> +
>>           /* set device id of hpet */
>>           if ( hpet_sbdf.iommu ||
>>                (hpet_sbdf.cmdline && hpet_sbdf.id !=
special->handle) )
>> @@ -777,12 +813,10 @@ static u16 __init parse_ivhd_device_special(
>>               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.bdf = bdf;
>> +        hpet_sbdf.seg = seg;
> I don''t see what benefit these HPET related changes provide. Or
> if there is any that I overlook, then the previous uses of
> hpet_sbdf.cmdline should all be eliminated.
>
> Jan
>
I found an issue when I was testing.  if the "handle" for the IVRS
HPET
is not the same where:
     - IVRS Table HPET handle = 0x0
     - HPET Table = 0x2

This throws the following error message:
(XEN) AMD-Vi: Failed to setup HPET MSI remapping: Wrong HPET

In this case, the handle is wrong and I am trying to change it.
When I am trying to specify the command line option
"ivrs_hpet[2]=00:14.0"
this does not get used because the in the parse_ivrs_hpet() did not 
store the
override id.  So, I modify the logic in the parse_ivrs_hpet() to 
override the hpet_sbdf.bdf
with the new ID specified from command line.  Then it doesn''t need to
go
though this logic here.
Also, the check "hpet_sbdf.cmdline && hpet_sbdf.id !=
special->handle"
should also be eliminated.

I will send out V4 with updates, and thank you for your patience.

Suravee

On 9/13/2013 4:24 AM, Jan Beulich wrote:
>>>> On 12.09.13 at 19:00, <suravee.suthikulpanit@amd.com>
wrote:
>> --- a/xen/drivers/passthrough/amd/iommu_acpi.c
>> +++ b/xen/drivers/passthrough/amd/iommu_acpi.c
>> @@ -664,19 +664,46 @@ static void __init parse_ivrs_hpet(char *str)
>>   
>>       ASSERT(*s == ''['');
>>       id = simple_strtoul(s + 1, &s, 0);
>> -    if ( id != (typeof(hpet_sbdf.id))id || *s != '']''
|| *++s != ''='' )
>> +    if ( (*s != '']'') || (*++s !=
''='') )
> No, unless you have a very good reason.
Oh, sorry. This is my mistake.
>
>>           return;
>>   
>>       s = parse_pci(s + 1, &seg, &bus, &dev, &func);
>>       if ( !s || *s )
>>           return;
>>   
>> +    hpet_sbdf.id = id;
> In essence this is the only change not contained in the patch I sent.
> So I''d be inclined to commit this (and perhaps the one debug
> message adjustment below) under your name, and my proposed
> change as a separate one.
Ok.
>
>>       hpet_sbdf.bdf = PCI_BDF(bus, dev, func);
>>       hpet_sbdf.seg = seg;
>>       hpet_sbdf.cmdline = 1;
>>   }
>>   custom_param("ivrs_hpet[", parse_ivrs_hpet);
>>   
>> +static bool_t is_ioapic_overidden(u16 seg, u16 bdf, u8 handle)
> Missing __init annotation. And anyway, I can''t really see why
> putting this in a separate function is a significant benefit. It''s
> only being used in one place afaics.
Sorry, I forgot the __init.  I normally just like to keep to code that 
does a specific
thing inside it''s own function for the ease of reading.  But this is 
purely coding style.
I can put it in the caller function if you prefer.
>
>> +{
>> +    bool_t ret = 0;
>> +    int apic = find_first_bit(ioapic_cmdline,
ARRAY_SIZE(ioapic_sbdf));
>> +
>> +    while ( apic < ARRAY_SIZE(ioapic_sbdf) )
>> +    {
>> +        if ( ioapic_sbdf[apic].bdf == bdf &&
>> +             ioapic_sbdf[apic].seg == seg )
>> +            break;
>> +        apic = find_next_bit(ioapic_cmdline, ARRAY_SIZE(ioapic_sbdf),
>> +                             apic + 1);
>> +    }
>> +
>> +    if ( apic < ARRAY_SIZE(ioapic_sbdf) )
>> +    {
>> +        AMD_IOMMU_DEBUG("IVHD: Command line override present for
IO-APIC %#x "
>> +                        "(IVRS: %#x devID
%04x:%02x:%02x.%u)\n",
>> +                        apic, handle, seg, PCI_BUS(bdf),
>> +                        PCI_SLOT(bdf), PCI_FUNC(bdf));
>> +        ret = 1;
>> +    }
>> +
>> +    return ret;
>> +}
>> +
>>   static u16 __init parse_ivhd_device_special(
>>       const struct acpi_ivrs_device8c *special, u16 seg,
>>       u16 header_length, u16 block_length, struct amd_iommu *iommu)
>> @@ -698,16 +725,18 @@ static u16 __init parse_ivhd_device_special(
>>           return 0;
>>       }
>>   
>> -    AMD_IOMMU_DEBUG("IVHD Special: %04x:%02x:%02x.%u variety %#x
handle %#x\n",
>> +    AMD_IOMMU_DEBUG("IVHD Special: %04x:%02x:%02x.%u variety %#x
handle %#x used_id %#x\n",
>>                       seg, PCI_BUS(bdf), PCI_SLOT(bdf), PCI_FUNC(bdf),
>> -                    special->variety, special->handle);
>> +                    special->variety, special->handle,
special->used_id);
>>       add_ivrs_mapping_entry(bdf, bdf, special->header.data_setting,
iommu);
>>   
>>       switch ( special->variety )
>>       {
>>       case ACPI_IVHD_IOAPIC:
>> -        if ( !iommu_intremap )
>> +        if ( !iommu_intremap ||
>> +             is_ioapic_overidden(seg, bdf, special->handle) )
>>               break;
>> +
>>           /*
>>            * Some BIOSes have IOAPIC broken entries so we check for
IVRS
>>            * consistency here --- whether entry''s IOAPIC ID is
valid and
>> @@ -725,10 +754,7 @@ static u16 __init parse_ivhd_device_special(
>>                   return 0;
>>               }
>>   
>> -            if ( test_bit(special->handle, ioapic_cmdline) )
>> -                AMD_IOMMU_DEBUG("IVHD: Command line override
present for IO-APIC %#x\n",
>> -                                special->handle);
>> -            else if ( ioapic_sbdf[special->handle].pin_2_idx )
>> +            if ( ioapic_sbdf[special->handle].pin_2_idx )
> Again - no, unless you have a very good reason.
I think with the check for command line stuff already handle prior to 
this point,
I don''t see why we still need to keep this here.  It should not need to
go through
the logic here.
>
>>               {
>>                   if ( ioapic_sbdf[special->handle].bdf == bdf
&&
>>                        ioapic_sbdf[special->handle].seg == seg )
>> @@ -770,6 +796,16 @@ 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));
>> +            hpet_sbdf.iommu = iommu;
>> +            break;
>> +        }
>> +
>>           /* set device id of hpet */
>>           if ( hpet_sbdf.iommu ||
>>                (hpet_sbdf.cmdline && hpet_sbdf.id !=
special->handle) )
>> @@ -777,12 +813,10 @@ static u16 __init parse_ivhd_device_special(
>>               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.bdf = bdf;
>> +        hpet_sbdf.seg = seg;
> I don''t see what benefit these HPET related changes provide. Or
> if there is any that I overlook, then the previous uses of
> hpet_sbdf.cmdline should all be eliminated.
>
> Jan
>
I found an issue when I was testing.  if the "handle" for the IVRS
HPET
is not the same where:
     - IVRS Table HPET handle = 0x0
     - HPET Table = 0x2

This throws the following error message:
(XEN) AMD-Vi: Failed to setup HPET MSI remapping: Wrong HPET

In this case, the handle is wrong and I am trying to change it.
When I am trying to specify the command line option
"ivrs_hpet[2]=00:14.0"
this does not get used because the in the parse_ivrs_hpet() did not 
store the
override id.  So, I modify the logic in the parse_ivrs_hpet() to 
override the hpet_sbdf.bdf
with the new ID specified from command line.  Then it doesn''t need to
go
though this logic here.
Also, the check "hpet_sbdf.cmdline && hpet_sbdf.id !=
special->handle"
should also be eliminated.

I will split the patch send out V4 with updates, and thank you for your 
patience.

Suravee