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

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

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

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

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

>>> 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

>>> 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

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


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

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


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

>>> 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

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


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

>>> 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

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


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

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


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

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


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

>>> 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

>>> 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

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

>>> 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

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