[PATCH 0/3 V4] x86/AMD-Vi: Miscs patch to fix and clean up ivrs override

[PATCH 0/3 V4] x86/AMD-Vi: Miscs patch to fix and clean up ivrs override

[suravee.suthikulpanit]

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

Changes from V3:
    - Split the patches into 3 since there are changes from multiple authors
    - Clean up code and fix logics

Jan Beulich (1):
  x86/AMD-Vi: Overrides special->handle in IVRS IOAPIC

Suravee Suthikulpanit (2):
  x86/AMD-Vi: Clean up code for handling IVRS IOAPIC override
  x86/AMD-Vi: Fix IVRS HPET special->handle override

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

-- 
1.8.1.2

[PATCH 1/3] x86/AMD-Vi: Overrides special->handle in IVRS IOAPIC

[suravee.suthikulpanit]

From: Jan Beulich <JBeulich@suse.com>

This patch handles the case when IVRS IOAPIC contains invalid
special->handle, which mainly caused by firmware bugs.
This allows users to override the invalid handle with command option
ivrs_ioapic[<handle>]=<sbdf>.

Acked-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
---
 xen/drivers/passthrough/amd/iommu_acpi.c | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/xen/drivers/passthrough/amd/iommu_acpi.c b/xen/drivers/passthrough/amd/iommu_acpi.c
index 89b359c..50b4a44 100644
--- a/xen/drivers/passthrough/amd/iommu_acpi.c
+++ b/xen/drivers/passthrough/amd/iommu_acpi.c
@@ -708,6 +708,26 @@ static u16 __init parse_ivhd_device_special(
     case ACPI_IVHD_IOAPIC:
         if ( !iommu_intremap )
             break;
+
+        /* Handling the ivrs_ioapic overriding */
+        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, special->handle, seg, PCI_BUS(bdf),
+                            PCI_SLOT(bdf), PCI_FUNC(bdf));
+            break;
+        }
+
         /*
          * Some BIOSes have IOAPIC broken entries so we check for IVRS
          * consistency here --- whether entry's IOAPIC ID is valid and
-- 
1.8.1.2

[PATCH 2/3] x86/AMD-Vi: Clean up code for handling IVRS IOAPIC override

[suravee.suthikulpanit]

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

Add more info in debug message, and remove unnecessary logic.

Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
---
 xen/drivers/passthrough/amd/iommu_acpi.c | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/xen/drivers/passthrough/amd/iommu_acpi.c b/xen/drivers/passthrough/amd/iommu_acpi.c
index 50b4a44..0778af0 100644
--- a/xen/drivers/passthrough/amd/iommu_acpi.c
+++ b/xen/drivers/passthrough/amd/iommu_acpi.c
@@ -698,9 +698,9 @@ 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 )
@@ -745,10 +745,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 )
-- 
1.8.1.2

[PATCH 3/3] x86/AMD-Vi: Fix IVRS HPET special->handle override

[suravee.suthikulpanit]

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.

Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
---
 xen/drivers/passthrough/amd/iommu_acpi.c | 22 +++++++++++++++-------
 1 file changed, 15 insertions(+), 7 deletions(-)

diff --git a/xen/drivers/passthrough/amd/iommu_acpi.c b/xen/drivers/passthrough/amd/iommu_acpi.c
index 0778af0..bfcc2eb 100644
--- a/xen/drivers/passthrough/amd/iommu_acpi.c
+++ b/xen/drivers/passthrough/amd/iommu_acpi.c
@@ -671,6 +671,7 @@ static void __init parse_ivrs_hpet(char *str)
     if ( !s || *s )
         return;
 
+    hpet_sbdf.id = id;
     hpet_sbdf.bdf = PCI_BDF(bus, dev, func);
     hpet_sbdf.seg = seg;
     hpet_sbdf.cmdline = 1;
@@ -787,19 +788,26 @@ 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) )
+        if ( hpet_sbdf.iommu )
         {
             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

Re: [PATCH 2/3] x86/AMD-Vi: Clean up code for handling IVRS IOAPIC override

[Jan Beulich]

>>> On 14.09.13 at 01:30, <suravee.suthikulpanit@amd.com> wrote:
> @@ -745,10 +745,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 )

Once again: Why is this unnecessary?

Jan

Re: [PATCH 3/3] x86/AMD-Vi: Fix IVRS HPET special->handle override

[Jan Beulich]

>>> On 14.09.13 at 01:30, <suravee.suthikulpanit@amd.com> wrote:
> 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.
> 
> Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
> ---
>  xen/drivers/passthrough/amd/iommu_acpi.c | 22 +++++++++++++++-------
>  1 file changed, 15 insertions(+), 7 deletions(-)
> 
> diff --git a/xen/drivers/passthrough/amd/iommu_acpi.c 
> b/xen/drivers/passthrough/amd/iommu_acpi.c
> index 0778af0..bfcc2eb 100644
> --- a/xen/drivers/passthrough/amd/iommu_acpi.c
> +++ b/xen/drivers/passthrough/amd/iommu_acpi.c
> @@ -671,6 +671,7 @@ static void __init parse_ivrs_hpet(char *str)
>      if ( !s || *s )
>          return;
>  
> +    hpet_sbdf.id = id;
>      hpet_sbdf.bdf = PCI_BDF(bus, dev, func);
>      hpet_sbdf.seg = seg;
>      hpet_sbdf.cmdline = 1;
> @@ -787,19 +788,26 @@ 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;

This (unconditional) assignment is what the earlier logic attempted
to avoid: We must not blindly set this (and in particular not blindly
overwrite a previously set valid value), and in order to do so we
need to know whether to trust devid or handle. I'm therefore going
to apply only the first hunk - being a clear and obvious bug fix - for
the time being.

Jan

> +            break;
> +        }
> +
>          /* set device id of hpet */
> -        if ( hpet_sbdf.iommu ||
> -             (hpet_sbdf.cmdline && hpet_sbdf.id != special->handle) )
> +        if ( hpet_sbdf.iommu )
>          {
>              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

Re: [PATCH 2/3] x86/AMD-Vi: Clean up code for handling IVRS IOAPIC override

[Suravee Suthikulpanit]

[-- Attachment #1.1: Type: text/plain, Size: 725 bytes --]

On 09/16/2013 02:29 AM, Jan Beulich wrote:
>>>> On 14.09.13 at 01:30, <suravee.suthikulpanit@amd.com> wrote:
>> @@ -745,10 +745,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 )
> Once again: Why is this unnecessary?
>
> Jan
>
>
Ok... you are right.  This part should handles the case when the IVRS 
entry has bad BDF.

Suravee

[-- Attachment #1.2: Type: text/html, Size: 1416 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

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

Re: [PATCH 3/3] x86/AMD-Vi: Fix IVRS HPET special->handle override

[Suravee Suthikulpanit]

[-- Attachment #1.1: Type: text/plain, Size: 2366 bytes --]

On 09/16/2013 03:03 AM, Jan Beulich wrote:
>>>> On 14.09.13 at 01:30, <suravee.suthikulpanit@amd.com> wrote:
>> 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.
>>
>> Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
>> ---
>>   xen/drivers/passthrough/amd/iommu_acpi.c | 22 +++++++++++++++-------
>>   1 file changed, 15 insertions(+), 7 deletions(-)
>>
>> diff --git a/xen/drivers/passthrough/amd/iommu_acpi.c
>> b/xen/drivers/passthrough/amd/iommu_acpi.c
>> index 0778af0..bfcc2eb 100644
>> --- a/xen/drivers/passthrough/amd/iommu_acpi.c
>> +++ b/xen/drivers/passthrough/amd/iommu_acpi.c
>> @@ -671,6 +671,7 @@ static void __init parse_ivrs_hpet(char *str)
>>       if ( !s || *s )
>>           return;
>>   
>> +    hpet_sbdf.id = id;
>>       hpet_sbdf.bdf = PCI_BDF(bus, dev, func);
>>       hpet_sbdf.seg = seg;
>>       hpet_sbdf.cmdline = 1;
>> @@ -787,19 +788,26 @@ 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;
> This (unconditional) assignment is what the earlier logic attempted
> to avoid: We must not blindly set this (and in particular not blindly
> overwrite a previously set valid value), and in order to do so we
> need to know whether to trust devid or handle. I'm therefore going
> to apply only the first hunk - being a clear and obvious bug fix - for
> the time being.
>
> Jan
But since we only allow one HPET in the system, and if users want to 
override the one that is in the IVRS (the buggy one)
we should allow this, right?  Otherwise, if the special->handle doesn't 
match, it would end up causing this logic to complain
about multiple HPETs.

Suravee

[-- Attachment #1.2: Type: text/html, Size: 3196 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

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

Re: [PATCH 3/3] x86/AMD-Vi: Fix IVRS HPET special->handle override

[Jan Beulich]

>>> On 17.09.13 at 16:10, Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
wrote:
> On 09/16/2013 03:03 AM, Jan Beulich wrote:
>>>>> On 14.09.13 at 01:30, <suravee.suthikulpanit@amd.com> wrote:
>>> 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.
>>>
>>> Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
>>> ---
>>>   xen/drivers/passthrough/amd/iommu_acpi.c | 22 +++++++++++++++-------
>>>   1 file changed, 15 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/xen/drivers/passthrough/amd/iommu_acpi.c
>>> b/xen/drivers/passthrough/amd/iommu_acpi.c
>>> index 0778af0..bfcc2eb 100644
>>> --- a/xen/drivers/passthrough/amd/iommu_acpi.c
>>> +++ b/xen/drivers/passthrough/amd/iommu_acpi.c
>>> @@ -671,6 +671,7 @@ static void __init parse_ivrs_hpet(char *str)
>>>       if ( !s || *s )
>>>           return;
>>>   
>>> +    hpet_sbdf.id = id;
>>>       hpet_sbdf.bdf = PCI_BDF(bus, dev, func);
>>>       hpet_sbdf.seg = seg;
>>>       hpet_sbdf.cmdline = 1;
>>> @@ -787,19 +788,26 @@ 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;
>> This (unconditional) assignment is what the earlier logic attempted
>> to avoid: We must not blindly set this (and in particular not blindly
>> overwrite a previously set valid value), and in order to do so we
>> need to know whether to trust devid or handle. I'm therefore going
>> to apply only the first hunk - being a clear and obvious bug fix - for
>> the time being.
>>
> But since we only allow one HPET in the system, and if users want to 
> override the one that is in the IVRS (the buggy one)
> we should allow this, right?  Otherwise, if the special->handle doesn't 
> match, it would end up causing this logic to complain
> about multiple HPETs.

We must not allow multiple HPET entries in the ACPI tables to
confuse us and store a bad IOMMU pointer. And yes, we also
must not wrongly complain about multiple HEPTs.

Jan

Re: [PATCH 3/3] x86/AMD-Vi: Fix IVRS HPET special->handle override

[Suravee Suthikulanit]

[-- Attachment #1.1: Type: text/plain, Size: 1454 bytes --]

On 9/17/2013 10:30 AM, Jan Beulich wrote:
>>> This (unconditional) assignment is what the earlier logic attempted
>>> >>to avoid: We must not blindly set this (and in particular not blindly
>>> >>overwrite a previously set valid value), and in order to do so we
>>> >>need to know whether to trust devid or handle. I'm therefore going
>>> >>to apply only the first hunk - being a clear and obvious bug fix - for
>>> >>the time being.
>>> >>
>> >But since we only allow one HPET in the system, and if users want to
>> >override the one that is in the IVRS (the buggy one)
>> >we should allow this, right?  Otherwise, if the special->handle doesn't
>> >match, it would end up causing this logic to complain
>> >about multiple HPETs.
> We must not allow multiple HPET entries in the ACPI tables to
> confuse us and store a bad IOMMU pointer.
I understand this part and agree.  Currently, the patch does not allow 
multiple HPET in the system.
If user specifies the ivrs_hpet, that will be the one that get used, and 
ignore the one that
is in the IVRS.  Although, it will be pointing to the IOMMU which lists 
HPET in the IVRS.

However, if IVRS is listing multiple HPETs in different IOMMUs, then it 
will just default to the first IOMMU.
I don't see this case happening though since HPET is in the Southbridge 
which only has one in the system.

Am I missing any thing?

Suravee
> And yes, we also must not wrongly complain about multiple HEPTs.
>
> Jan


[-- Attachment #1.2: Type: text/html, Size: 2749 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

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

Re: [PATCH 3/3] x86/AMD-Vi: Fix IVRS HPET special->handle override

[Jan Beulich]

>>> On 20.09.13 at 23:38, Suravee Suthikulanit <suravee.suthikulpanit@amd.com>
wrote:
> On 9/17/2013 10:30 AM, Jan Beulich wrote:
>>>> This (unconditional) assignment is what the earlier logic attempted
>>>> >>to avoid: We must not blindly set this (and in particular not blindly
>>>> >>overwrite a previously set valid value), and in order to do so we
>>>> >>need to know whether to trust devid or handle. I'm therefore going
>>>> >>to apply only the first hunk - being a clear and obvious bug fix - for
>>>> >>the time being.
>>>> >>
>>> >But since we only allow one HPET in the system, and if users want to
>>> >override the one that is in the IVRS (the buggy one)
>>> >we should allow this, right?  Otherwise, if the special->handle doesn't
>>> >match, it would end up causing this logic to complain
>>> >about multiple HPETs.
>> We must not allow multiple HPET entries in the ACPI tables to
>> confuse us and store a bad IOMMU pointer.
> I understand this part and agree.  Currently, the patch does not allow 
> multiple HPET in the system.
> If user specifies the ivrs_hpet, that will be the one that get used, and 
> ignore the one that
> is in the IVRS.  Although, it will be pointing to the IOMMU which lists 
> HPET in the IVRS.
> 
> However, if IVRS is listing multiple HPETs in different IOMMUs, then it 
> will just default to the first IOMMU.
> I don't see this case happening though since HPET is in the Southbridge 
> which only has one in the system.
> 
> Am I missing any thing?

Yes - there's no guarantee that (especially in a multi-node
system) there's just one HPET. Nor do the ACPI tables have
any indication there this would always be the case. Even if
_all_ current systems only have a single HPET (which I don't
think you can guarantee), we shouldn't code in a latent bug
like this.

Jan

Re: [PATCH 3/3] x86/AMD-Vi: Fix IVRS HPET special->handle override

[Suravee Suthikulpanit]

[-- Attachment #1.1: Type: text/plain, Size: 2289 bytes --]

On 9/23/2013 1:52 AM, Jan Beulich wrote:
>>>> On 20.09.13 at 23:38, Suravee Suthikulanit <suravee.suthikulpanit@amd.com>
> wrote:
>> On 9/17/2013 10:30 AM, Jan Beulich wrote:
>>>>> This (unconditional) assignment is what the earlier logic attempted
>>>>>>> to avoid: We must not blindly set this (and in particular not blindly
>>>>>>> overwrite a previously set valid value), and in order to do so we
>>>>>>> need to know whether to trust devid or handle. I'm therefore going
>>>>>>> to apply only the first hunk - being a clear and obvious bug fix - for
>>>>>>> the time being.
>>>>>>>
>>>>> But since we only allow one HPET in the system, and if users want to
>>>>> override the one that is in the IVRS (the buggy one)
>>>>> we should allow this, right?  Otherwise, if the special->handle doesn't
>>>>> match, it would end up causing this logic to complain
>>>>> about multiple HPETs.
>>> We must not allow multiple HPET entries in the ACPI tables to
>>> confuse us and store a bad IOMMU pointer.
>> I understand this part and agree.  Currently, the patch does not allow
>> multiple HPET in the system.
>> If user specifies the ivrs_hpet, that will be the one that get used, and
>> ignore the one that
>> is in the IVRS.  Although, it will be pointing to the IOMMU which lists
>> HPET in the IVRS.
>>
>> However, if IVRS is listing multiple HPETs in different IOMMUs, then it
>> will just default to the first IOMMU.
>> I don't see this case happening though since HPET is in the Southbridge
>> which only has one in the system.
>>
>> Am I missing any thing?
> Yes - there's no guarantee that (especially in a multi-node
> system) there's just one HPET. Nor do the ACPI tables have
> any indication there this would always be the case. Even if
> _all_ current systems only have a single HPET (which I don't
> think you can guarantee), we shouldn't code in a latent bug
> like this.
>
> Jan
I am a bit confused on what you want to do.  I believe all the systems 
at this point
should have only one HPET.  That's why the code only has one data 
structure for
initialize one HPET.  Are you expecting that certain systems could have 
more than
one HPETs?

Alternatively, I can try matching on the "SBDF" assuming that IVRS 
doesn't screw up
the SBDF part of the HPET special entry.

Suravee

[-- Attachment #1.2: Type: text/html, Size: 3708 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

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

Re: [PATCH 3/3] x86/AMD-Vi: Fix IVRS HPET special->handle override

[Andrew Cooper]

[-- Attachment #1.1: Type: text/plain, Size: 2437 bytes --]

On 23/09/13 17:48, Suravee Suthikulpanit wrote:
> On 9/23/2013 1:52 AM, Jan Beulich wrote:
>>>>> On 20.09.13 at 23:38, Suravee Suthikulanit <suravee.suthikulpanit@amd.com>
>> wrote:
>>> On 9/17/2013 10:30 AM, Jan Beulich wrote:
>>>>>> This (unconditional) assignment is what the earlier logic attempted
>>>>>>>> to avoid: We must not blindly set this (and in particular not blindly
>>>>>>>> overwrite a previously set valid value), and in order to do so we
>>>>>>>> need to know whether to trust devid or handle. I'm therefore going
>>>>>>>> to apply only the first hunk - being a clear and obvious bug fix - for
>>>>>>>> the time being.
>>>>>>>>
>>>>>> But since we only allow one HPET in the system, and if users want to
>>>>>> override the one that is in the IVRS (the buggy one)
>>>>>> we should allow this, right?  Otherwise, if the special->handle doesn't
>>>>>> match, it would end up causing this logic to complain
>>>>>> about multiple HPETs.
>>>> We must not allow multiple HPET entries in the ACPI tables to
>>>> confuse us and store a bad IOMMU pointer.
>>> I understand this part and agree.  Currently, the patch does not allow 
>>> multiple HPET in the system.
>>> If user specifies the ivrs_hpet, that will be the one that get used, and 
>>> ignore the one that
>>> is in the IVRS.  Although, it will be pointing to the IOMMU which lists 
>>> HPET in the IVRS.
>>>
>>> However, if IVRS is listing multiple HPETs in different IOMMUs, then it 
>>> will just default to the first IOMMU.
>>> I don't see this case happening though since HPET is in the Southbridge 
>>> which only has one in the system.
>>>
>>> Am I missing any thing?
>> Yes - there's no guarantee that (especially in a multi-node
>> system) there's just one HPET. Nor do the ACPI tables have
>> any indication there this would always be the case. Even if
>> _all_ current systems only have a single HPET (which I don't
>> think you can guarantee), we shouldn't code in a latent bug
>> like this.
>>
>> Jan
> I am a bit confused on what you want to do.  I believe all the systems
> at this point
> should have only one HPET.  That's why the code only has one data
> structure for
> initialize one HPET.  Are you expecting that certain systems could
> have more than
> one HPETs?

Amongst the test servers we have, I have plenty of examples of AMD
hardware with anything up to 8 HPETs in the system.  (See for reference
my HPET stack overflow thread)

~Andrew

[-- Attachment #1.2: Type: text/html, Size: 3899 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

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

Re: [PATCH 3/3] x86/AMD-Vi: Fix IVRS HPET special->handle override

[Suravee Suthikulanit]

[-- Attachment #1.1: Type: text/plain, Size: 3046 bytes --]

On 9/23/2013 12:03 PM, Andrew Cooper wrote:
> On 23/09/13 17:48, Suravee Suthikulpanit wrote:
>> On 9/23/2013 1:52 AM, Jan Beulich wrote:
>>>>>> On 20.09.13 at 23:38, Suravee Suthikulanit<suravee.suthikulpanit@amd.com>
>>> wrote:
>>>> On 9/17/2013 10:30 AM, Jan Beulich wrote:
>>>>>>> This (unconditional) assignment is what the earlier logic attempted
>>>>>>>>> to avoid: We must not blindly set this (and in particular not blindly
>>>>>>>>> overwrite a previously set valid value), and in order to do so we
>>>>>>>>> need to know whether to trust devid or handle. I'm therefore going
>>>>>>>>> to apply only the first hunk - being a clear and obvious bug fix - for
>>>>>>>>> the time being.
>>>>>>>>>
>>>>>>> But since we only allow one HPET in the system, and if users want to
>>>>>>> override the one that is in the IVRS (the buggy one)
>>>>>>> we should allow this, right?  Otherwise, if the special->handle doesn't
>>>>>>> match, it would end up causing this logic to complain
>>>>>>> about multiple HPETs.
>>>>> We must not allow multiple HPET entries in the ACPI tables to
>>>>> confuse us and store a bad IOMMU pointer.
>>>> I understand this part and agree.  Currently, the patch does not allow
>>>> multiple HPET in the system.
>>>> If user specifies the ivrs_hpet, that will be the one that get used, and
>>>> ignore the one that
>>>> is in the IVRS.  Although, it will be pointing to the IOMMU which lists
>>>> HPET in the IVRS.
>>>>
>>>> However, if IVRS is listing multiple HPETs in different IOMMUs, then it
>>>> will just default to the first IOMMU.
>>>> I don't see this case happening though since HPET is in the Southbridge
>>>> which only has one in the system.
>>>>
>>>> Am I missing any thing?
>>> Yes - there's no guarantee that (especially in a multi-node
>>> system) there's just one HPET. Nor do the ACPI tables have
>>> any indication there this would always be the case. Even if
>>> _all_ current systems only have a single HPET (which I don't
>>> think you can guarantee), we shouldn't code in a latent bug
>>> like this.
>>>
>>> Jan
>> I am a bit confused on what you want to do.  I believe all the 
>> systems at this point
>> should have only one HPET.  That's why the code only has one data 
>> structure for
>> initialize one HPET.  Are you expecting that certain systems could 
>> have more than
>> one HPETs?
>
> Amongst the test servers we have, I have plenty of examples of AMD 
> hardware with anything up to 8 HPETs in the system.  (See for 
> reference my HPET stack overflow thread)
>
> ~Andrew
Andrew,

 From the hpet-overflow-full-stackdump.tar.gz here 
(http://lists.xen.org/archives/html/xen-devel/2013-08/msg01667.html),
I inspect the files, and foundout that this is an Intel system.  Are you 
sure that you are seeing more than 1 HPET
on an AMD system?  As far as I know, I don't this that is how the 
platforms are designed.

Would you mind sending the output from "acpidump" for the "HPET" table 
from the system you have? Also, would you provide
the detail of the system?

Thank you,

Suravee

[-- Attachment #1.2: Type: text/html, Size: 5882 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

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

Re: [PATCH 3/3] x86/AMD-Vi: Fix IVRS HPET special->handle override

[Jan Beulich]

>>> On 23.09.13 at 18:48, Suravee Suthikulpanit <suravee.suthikulpanit@amd.com> wrote:
> I am a bit confused on what you want to do.  I believe all the systems 
> at this point
> should have only one HPET.  That's why the code only has one data 
> structure for
> initialize one HPET.  Are you expecting that certain systems could have 
> more than
> one HPETs?

Of course. The code assuming just one HPET is wrong in the first
place, so I'm really not looking towards making matters worse.
The only valid a priori information we have is that there's
exactly one HPET nominated as the legacy replacement one
(through the ACPI HPET table), but there could be more (as
said, this is quite likely on multi-node NUMA systems).

> Alternatively, I can try matching on the "SBDF" assuming that IVRS 
> doesn't screw up
> the SBDF part of the HPET special entry.

As long as we only support one, we should be prepared for
either ID or SBDF to be wrong, and should (for maximum
flexibility) probably allow matching in both directions. Or you'd
need to propose a new way of associating the IOMMU with
the HPET.

Jan

Re: [PATCH 3/3] x86/AMD-Vi: Fix IVRS HPET special->handle override

[Jan Beulich]

>>> On 23.09.13 at 19:03, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
> Amongst the test servers we have, I have plenty of examples of AMD
> hardware with anything up to 8 HPETs in the system.  (See for reference
> my HPET stack overflow thread)

That's HPET counters afaict, not HPETs.

Jan

Re: [PATCH 3/3] x86/AMD-Vi: Fix IVRS HPET special->handle override

[Suravee Suthikulanit]

On 9/24/2013 1:47 AM, Jan Beulich wrote:
>>>> On 23.09.13 at 18:48, Suravee Suthikulpanit <suravee.suthikulpanit@amd.com> wrote:
>> I am a bit confused on what you want to do.  I believe all the systems
>> at this point
>> should have only one HPET.  That's why the code only has one data
>> structure for
>> initialize one HPET.  Are you expecting that certain systems could have
>> more than
>> one HPETs?
> Of course. The code assuming just one HPET is wrong in the first
> place, so I'm really not looking towards making matters worse.
> The only valid a priori information we have is that there's
> exactly one HPET nominated as the legacy replacement one
> (through the ACPI HPET table), but there could be more (as
> said, this is quite likely on multi-node NUMA systems).
Ok, after looking into Intel HPET specification, I can see that the spec 
allows a particular system
to have multiple HPETs.  And Jan was correct that only one is required 
to be listed in the APCI HPET table.
The rest are listed in ACPI namespace.

I look at the "arch/x86/hpet.c" and saw that this supports discovery in 
the ACPI HPET table.
However, there is only one "hpet_address" and "hpet_blockid", which are 
initialized in the
"arch/x86/acpi/boot.c: acpi_parse_hpet()".  If the code were to support 
more than one HPET,
this would have to be changed also.  Do you expect these to change as well?

Also, I don't see the code that would walk the ACPI namespace anywhere.  
Does it exist?

FYI, I have checked with the hardware platforms team, and there is no 
AMD systems
with multiple southbridge (HPET is in the southbridge).  I also check on 
the system with
2 SR56xx chips ( each contains an IOMMU), and there is only one 
southbridge.

Suravee

Re: [PATCH 3/3] x86/AMD-Vi: Fix IVRS HPET special->handle override

[Jan Beulich]

>>> On 25.09.13 at 01:26, Suravee Suthikulanit <suravee.suthikulpanit@amd.com> wrote:
> Ok, after looking into Intel HPET specification, I can see that the spec 
> allows a particular system
> to have multiple HPETs.  And Jan was correct that only one is required 
> to be listed in the APCI HPET table.
> The rest are listed in ACPI namespace.
> 
> I look at the "arch/x86/hpet.c" and saw that this supports discovery in 
> the ACPI HPET table.
> However, there is only one "hpet_address" and "hpet_blockid", which are 
> initialized in the
> "arch/x86/acpi/boot.c: acpi_parse_hpet()".  If the code were to support 
> more than one HPET,
> this would have to be changed also.  Do you expect these to change as well?

This second paragraph contradicts what you correctly describe in
the first: There's only one required to be listed in the ACPI HPET
table, and hence that code isn't expected to change.

> Also, I don't see the code that would walk the ACPI namespace anywhere.  
> Does it exist?

In Dom0, yes. But the information not getting passed down is of
no relevance here: All we care about is how to correctly find the
IOMMU for the one HPET we use, which ought to work no matter
how many HPETs there are in the system.

Jan

Re: [PATCH 3/3] x86/AMD-Vi: Fix IVRS HPET special->handle override

[Suravee Suthikulpanit]

On 9/25/2013 1:12 AM, Jan Beulich wrote:
>>>> On 25.09.13 at 01:26, Suravee Suthikulanit <suravee.suthikulpanit@amd.com> wrote:
>> Ok, after looking into Intel HPET specification, I can see that the spec
>> allows a particular system
>> to have multiple HPETs.  And Jan was correct that only one is required
>> to be listed in the APCI HPET table.
>> The rest are listed in ACPI namespace.
>>
>> I look at the "arch/x86/hpet.c" and saw that this supports discovery in
>> the ACPI HPET table.
>> However, there is only one "hpet_address" and "hpet_blockid", which are
>> initialized in the
>> "arch/x86/acpi/boot.c: acpi_parse_hpet()".  If the code were to support
>> more than one HPET,
>> this would have to be changed also.  Do you expect these to change as well?
> This second paragraph contradicts what you correctly describe in
> the first: There's only one required to be listed in the ACPI HPET
> table, and hence that code isn't expected to change.
>
>> Also, I don't see the code that would walk the ACPI namespace anywhere.
>> Does it exist?
> In Dom0, yes. But the information not getting passed down is of
> no relevance here: All we care about is how to correctly find the
> IOMMU for the one HPET we use, which ought to work no matter
> how many HPETs there are in the system.
>
> Jan
Actually, I found out that "struct hpet_sbdf.iommu" is not used 
anywhere.  We should be able
to get rid of this, and it should now be simpler logic.  I'll remove 
this in my next patch.

Suravee