[Patch 0/4] HPET general fixes

[Patch 0/4] HPET general fixes

[Andrew Cooper]

While working on the HPET interrupt issue leading to stack overflow, I have
accumulated a set of general fixes and improvements to the code which are
unrelated to main interrupt problem.

As the interrupt problem is taking longer to fix than I would have hoped, I am
submitting the general fixes ahead of time for feedback.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
CC: Keir Fraser <keir@xen.org>
CC: Jan Beulich <JBeulich@suse.com>

--
1.7.10.4

[Patch 1/4] x86/hpet: Basic cleanup

[Andrew Cooper]

* Strip trailing whitespace
* Remove redundant definitions
* Update stale documentation links
* Move hpet_address into __initdata

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
CC: Keir Fraser <keir@xen.org>
CC: Jan Beulich <JBeulich@suse.com>
---
 xen/arch/x86/hpet.c        |    6 +++---
 xen/arch/x86/hvm/hpet.c    |   18 +++++++++---------
 xen/include/asm-x86/hpet.h |    6 ++----
 3 files changed, 14 insertions(+), 16 deletions(-)

diff --git a/xen/arch/x86/hpet.c b/xen/arch/x86/hpet.c
index 7e0d332..99882b1 100644
--- a/xen/arch/x86/hpet.c
+++ b/xen/arch/x86/hpet.c
@@ -1,6 +1,6 @@
 /******************************************************************************
  * arch/x86/hpet.c
- * 
+ *
  * HPET management.
  */
 
@@ -50,7 +50,7 @@ static unsigned int __read_mostly num_hpets_used;
 
 DEFINE_PER_CPU(struct hpet_event_channel *, cpu_bc_channel);
 
-unsigned long __read_mostly hpet_address;
+unsigned long __initdata hpet_address;
 u8 __initdata hpet_blockid;
 
 /*
@@ -540,7 +540,7 @@ static void handle_rtc_once(uint8_t index, uint8_t value)
 {
     if ( index != RTC_REG_B )
         return;
-    
+
     /* RTC Reg B, contain PIE/AIE/UIE */
     if ( value & (RTC_PIE | RTC_AIE | RTC_UIE ) )
     {
diff --git a/xen/arch/x86/hvm/hpet.c b/xen/arch/x86/hvm/hpet.c
index 4b4b905..4324b52 100644
--- a/xen/arch/x86/hvm/hpet.c
+++ b/xen/arch/x86/hvm/hpet.c
@@ -47,7 +47,7 @@
 
 /* can be routed to IOAPIC.redirect_table[23..20] */
 #define HPET_TN_INT_ROUTE_CAP      (0x00f00000ULL \
-                    << HPET_TN_INT_ROUTE_CAP_SHIFT) 
+                    << HPET_TN_INT_ROUTE_CAP_SHIFT)
 
 #define HPET_TN_INT_ROUTE_CAP_MASK (0xffffffffULL \
                     << HPET_TN_INT_ROUTE_CAP_SHIFT)
@@ -79,7 +79,7 @@ static inline uint64_t hpet_read_maincounter(HPETState *h)
 
     if ( hpet_enabled(h) )
         return guest_time_hpet(h) + h->mc_offset;
-    else 
+    else
         return h->hpet.mc64;
 }
 
@@ -100,7 +100,7 @@ static uint64_t hpet_get_comparator(HPETState *h, unsigned int tn)
             h->hpet.comparator64[tn] = comparator;
         }
     }
-    
+
     /* truncate if timer is in 32 bit mode */
     if ( timer_is_32bit(h, tn) )
         comparator = (uint32_t)comparator;
@@ -249,7 +249,7 @@ static void hpet_set_timer(HPETState *h, unsigned int tn)
         irq = timer_int_route(h, tn);
 
     /*
-     * diff is the time from now when the timer should fire, for a periodic 
+     * diff is the time from now when the timer should fire, for a periodic
      * timer we also need the period which may be different because time may
      * have elapsed between the time the comparator was written and the timer
      * being enabled (now).
@@ -331,7 +331,7 @@ static int hpet_write(
         h->hpet.mc64 = new_val;
         if ( hpet_enabled(h) )
         {
-            gdprintk(XENLOG_WARNING, 
+            gdprintk(XENLOG_WARNING,
                      "HPET: writing main counter but it's not halted!\n");
             for ( i = 0; i < HPET_TIMER_NUM; i++ )
                 if ( timer_enabled(h, i) )
@@ -396,7 +396,7 @@ static int hpet_write(
              * timer's accumulator."  That is, set the comparator without
              * adjusting the period.  Much the same as just setting the
              * comparator on an enabled one-shot timer.
-             * 
+             *
              * This configuration bit clears when the comparator is written.
              */
             h->hpet.timers[tn].config &= ~HPET_TN_SETVAL;
@@ -553,7 +553,7 @@ static int hpet_load(struct domain *d, hvm_domain_context_t *h)
         hp->hpet.timers[i].cmp = cmp;
     }
 #undef C
-    
+
     /* Recalculate the offset between the main counter and guest time */
     hp->mc_offset = hp->hpet.mc64 - guest_time_hpet(hp);
 
@@ -563,7 +563,7 @@ static int hpet_load(struct domain *d, hvm_domain_context_t *h)
         for ( i = 0; i < HPET_TIMER_NUM; i++ )
             if ( timer_enabled(hp, i) )
                 hpet_set_timer(hp, i);
- 
+
     spin_unlock(&hp->lock);
 
     return 0;
@@ -595,7 +595,7 @@ void hpet_init(struct vcpu *v)
 
     for ( i = 0; i < HPET_TIMER_NUM; i++ )
     {
-        h->hpet.timers[i].config = 
+        h->hpet.timers[i].config =
             HPET_TN_INT_ROUTE_CAP | HPET_TN_64BIT_CAP | HPET_TN_PERIODIC_CAP;
         h->hpet.timers[i].cmp = ~0ULL;
         h->pt[i].source = PTSRC_isa;
diff --git a/xen/include/asm-x86/hpet.h b/xen/include/asm-x86/hpet.h
index 98c1237..875f1de 100644
--- a/xen/include/asm-x86/hpet.h
+++ b/xen/include/asm-x86/hpet.h
@@ -3,8 +3,8 @@
 
 /*
  * Documentation on HPET can be found at:
- *      http://www.intel.com/ial/home/sp/pcmmspec.htm
- *      ftp://download.intel.com/ial/home/sp/mmts098.pdf
+ *      http://www.intel.com/content/dam/www/public/us/en/documents/
+ *      technical-specifications/software-developers-hpet-spec-1-0a.pdf
  */
 
 #define HPET_MMAP_SIZE	1024
@@ -24,9 +24,7 @@
 #define HPET_ID_NUMBER	0x00001f00
 #define HPET_ID_REV	0x000000ff
 #define	HPET_ID_NUMBER_SHIFT	8
-
 #define HPET_ID_VENDOR_SHIFT	16
-#define HPET_ID_VENDOR_8086	0x8086
 
 #define HPET_CFG_ENABLE	0x001
 #define HPET_CFG_LEGACY	0x002
-- 
1.7.10.4

[Patch 2/4] x86/hpet: Sanitise HPET ACPI table and warn about multiple tables

[Andrew Cooper]

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
CC: Keir Fraser <keir@xen.org>
CC: Jan Beulich <JBeulich@suse.com>
---
 xen/arch/x86/acpi/boot.c |   15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/xen/arch/x86/acpi/boot.c b/xen/arch/x86/acpi/boot.c
index 0e1d570..1f6cbe6 100644
--- a/xen/arch/x86/acpi/boot.c
+++ b/xen/arch/x86/acpi/boot.c
@@ -276,6 +276,21 @@ static int __init acpi_parse_hpet(struct acpi_table_header *table)
 		return -1;
 	}
 
+	if ( !hpet_tbl->address.address || !(hpet_tbl->address.address + 1) )
+	{
+		printk(KERN_WARNING PREFIX "Bad HPET address %#lx\n",
+		       hpet_tbl->address.address);
+		return -1;
+	}
+
+	/*
+	 * Hopefully someone might implement multiple HPET support in Xen.
+	 * Until then, warn the user if multiple HPET tables are found.
+	 */
+	if ( hpet_address )
+		printk(KERN_WARNING PREFIX
+		       "Xen only supports one HPET - Using latest table\n");
+
 	hpet_address = hpet_tbl->address.address;
 	hpet_blockid = hpet_tbl->sequence;
 	printk(KERN_INFO PREFIX "HPET id: %#x base: %#lx\n",
-- 
1.7.10.4

Re: [Patch 2/4] x86/hpet: Sanitise HPET ACPI table and warn about multiple tables

[Jan Beulich]

>>> On 07.10.13 at 15:26, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
> --- a/xen/arch/x86/acpi/boot.c
> +++ b/xen/arch/x86/acpi/boot.c
> @@ -276,6 +276,21 @@ static int __init acpi_parse_hpet(struct 
> acpi_table_header *table)
>  		return -1;
>  	}
>  
> +	if ( !hpet_tbl->address.address || !(hpet_tbl->address.address + 1) )
> +	{
> +		printk(KERN_WARNING PREFIX "Bad HPET address %#lx\n",
> +		       hpet_tbl->address.address);
> +		return -1;
> +	}

Did you really encounter a system where this would trigger?

> +
> +	/*
> +	 * Hopefully someone might implement multiple HPET support in Xen.
> +	 * Until then, warn the user if multiple HPET tables are found.
> +	 */
> +	if ( hpet_address )
> +		printk(KERN_WARNING PREFIX
> +		       "Xen only supports one HPET - Using latest table\n");
> +

You perhaps miunderstood how multiple HPETs would be surfaced
by firmware: Not via multiple HPET tables, but via objects in the
ACPI object namespace. With that, a similar question to the above
arises: Have you seen a system where multiple HPET tables get
surfaced?

Jan

Re: [Patch 2/4] x86/hpet: Sanitise HPET ACPI table and warn about multiple tables

[Andrew Cooper]

On 07/10/13 14:45, Jan Beulich wrote:
>>>> On 07.10.13 at 15:26, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
>> --- a/xen/arch/x86/acpi/boot.c
>> +++ b/xen/arch/x86/acpi/boot.c
>> @@ -276,6 +276,21 @@ static int __init acpi_parse_hpet(struct 
>> acpi_table_header *table)
>>  		return -1;
>>  	}
>>  
>> +	if ( !hpet_tbl->address.address || !(hpet_tbl->address.address + 1) )
>> +	{
>> +		printk(KERN_WARNING PREFIX "Bad HPET address %#lx\n",
>> +		       hpet_tbl->address.address);
>> +		return -1;
>> +	}
> Did you really encounter a system where this would trigger?
>
>> +
>> +	/*
>> +	 * Hopefully someone might implement multiple HPET support in Xen.
>> +	 * Until then, warn the user if multiple HPET tables are found.
>> +	 */
>> +	if ( hpet_address )
>> +		printk(KERN_WARNING PREFIX
>> +		       "Xen only supports one HPET - Using latest table\n");
>> +
> You perhaps miunderstood how multiple HPETs would be surfaced
> by firmware: Not via multiple HPET tables, but via objects in the
> ACPI object namespace. With that, a similar question to the above
> arises: Have you seen a system where multiple HPET tables get
> surfaced?
>
> Jan
>

We had a system which had two HPET tables.  A BIOS update fixed the
issue, but the fact remains that in the case of multiple HPET tables, we
blindly go with the latest table.

Perhaps the message could change to "Multiple tables found (BIOS BUG?). 
Using information from latest" ?

~Andrew

Re: [Patch 2/4] x86/hpet: Sanitise HPET ACPI table and warn about multiple tables

[Jan Beulich]

>>> On 07.10.13 at 15:55, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
> On 07/10/13 14:45, Jan Beulich wrote:
>>>>> On 07.10.13 at 15:26, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
>>> --- a/xen/arch/x86/acpi/boot.c
>>> +++ b/xen/arch/x86/acpi/boot.c
>>> @@ -276,6 +276,21 @@ static int __init acpi_parse_hpet(struct 
>>> acpi_table_header *table)
>>>  		return -1;
>>>  	}
>>>  
>>> +	if ( !hpet_tbl->address.address || !(hpet_tbl->address.address + 1) )
>>> +	{
>>> +		printk(KERN_WARNING PREFIX "Bad HPET address %#lx\n",
>>> +		       hpet_tbl->address.address);
>>> +		return -1;
>>> +	}
>> Did you really encounter a system where this would trigger?
>>
>>> +
>>> +	/*
>>> +	 * Hopefully someone might implement multiple HPET support in Xen.
>>> +	 * Until then, warn the user if multiple HPET tables are found.
>>> +	 */
>>> +	if ( hpet_address )
>>> +		printk(KERN_WARNING PREFIX
>>> +		       "Xen only supports one HPET - Using latest table\n");
>>> +
>> You perhaps miunderstood how multiple HPETs would be surfaced
>> by firmware: Not via multiple HPET tables, but via objects in the
>> ACPI object namespace. With that, a similar question to the above
>> arises: Have you seen a system where multiple HPET tables get
>> surfaced?
> 
> We had a system which had two HPET tables.  A BIOS update fixed the
> issue, but the fact remains that in the case of multiple HPET tables, we
> blindly go with the latest table.

Interesting.

> Perhaps the message could change to "Multiple tables found (BIOS BUG?). 
> Using information from latest" ?

Something along those lines. In any case this would raise the
question whether going with the first, last, or any intermediate
table would be the safest route then. I'd be inclined to use the
first in that case rather than the last.

Jan

[Patch 3/4] x86/hpet: Fix ambiguity in broadcast info message.

[Andrew Cooper]

"$N will be used for broadcast" is ambiguous between "$N timers" or "timer
$N", particuarly when N is 0.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
CC: Keir Fraser <keir@xen.org>
CC: Jan Beulich <JBeulich@suse.com>
---
 xen/arch/x86/hpet.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/xen/arch/x86/hpet.c b/xen/arch/x86/hpet.c
index 99882b1..091e624 100644
--- a/xen/arch/x86/hpet.c
+++ b/xen/arch/x86/hpet.c
@@ -430,7 +430,7 @@ static void __init hpet_fsb_cap_lookup(void)
             num_hpets_used++;
     }
 
-    printk(XENLOG_INFO "HPET: %u timers (%u will be used for broadcast)\n",
+    printk(XENLOG_INFO "HPET: %u timers (%u timers used for broadcast)\n",
            num_chs, num_hpets_used);
 }
 
-- 
1.7.10.4

Re: [Patch 3/4] x86/hpet: Fix ambiguity in broadcast info message.

[Jan Beulich]

>>> On 07.10.13 at 15:26, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
> "$N will be used for broadcast" is ambiguous between "$N timers" or "timer
> $N", particuarly when N is 0.

Honestly I never considered this to possibly mean the second of the
alternatives you present.

Jan

> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> CC: Keir Fraser <keir@xen.org>
> CC: Jan Beulich <JBeulich@suse.com>
> ---
>  xen/arch/x86/hpet.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/xen/arch/x86/hpet.c b/xen/arch/x86/hpet.c
> index 99882b1..091e624 100644
> --- a/xen/arch/x86/hpet.c
> +++ b/xen/arch/x86/hpet.c
> @@ -430,7 +430,7 @@ static void __init hpet_fsb_cap_lookup(void)
>              num_hpets_used++;
>      }
>  
> -    printk(XENLOG_INFO "HPET: %u timers (%u will be used for broadcast)\n",
> +    printk(XENLOG_INFO "HPET: %u timers (%u timers used for broadcast)\n",
>             num_chs, num_hpets_used);
>  }
>  
> -- 
> 1.7.10.4
> 
> 
> .

[Patch 4/4] x86/hpet: Don't clear reserved bits in the General Configuration Register

[Andrew Cooper]

It is a violation of the specification.

The reserved bits in the General Configuration Register, unlike all other
reserved bits I have found in the spec, are specified as 'must never be
changed by the OS'.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
 xen/arch/x86/hpet.c        |   10 +++-------
 xen/include/asm-x86/hpet.h |    2 ++
 2 files changed, 5 insertions(+), 7 deletions(-)

diff --git a/xen/arch/x86/hpet.c b/xen/arch/x86/hpet.c
index 091e624..eb48f84 100644
--- a/xen/arch/x86/hpet.c
+++ b/xen/arch/x86/hpet.c
@@ -814,15 +814,11 @@ void hpet_resume(u32 *boot_cfg)
     cfg = hpet_read32(HPET_CFG);
     if ( boot_cfg )
         *boot_cfg = cfg;
-    cfg &= ~(HPET_CFG_ENABLE | HPET_CFG_LEGACY);
-    if ( cfg )
-    {
+
+    if ( cfg & HPET_CFG_RESERVED )
         printk(XENLOG_WARNING
                "HPET: reserved bits %#x set in global config register\n",
-               cfg);
-        cfg = 0;
-    }
-    hpet_write32(cfg, HPET_CFG);
+               cfg & HPET_CFG_RESERVED);
 
     hpet_id = hpet_read32(HPET_ID);
     last = (hpet_id & HPET_ID_NUMBER) >> HPET_ID_NUMBER_SHIFT;
diff --git a/xen/include/asm-x86/hpet.h b/xen/include/asm-x86/hpet.h
index 875f1de..22a11da 100644
--- a/xen/include/asm-x86/hpet.h
+++ b/xen/include/asm-x86/hpet.h
@@ -28,6 +28,8 @@
 
 #define HPET_CFG_ENABLE	0x001
 #define HPET_CFG_LEGACY	0x002
+#define HPET_CFG_RESERVED 0xfffffffc
+
 #define	HPET_LEGACY_8254	2
 #define	HPET_LEGACY_RTC		8
 
-- 
1.7.10.4

Re: [Patch 4/4] x86/hpet: Don't clear reserved bits in the General Configuration Register

[Jan Beulich]

>>> On 07.10.13 at 15:26, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
> It is a violation of the specification.
> 
> The reserved bits in the General Configuration Register, unlike all other
> reserved bits I have found in the spec, are specified as 'must never be
> changed by the OS'.

Mind pointing out where exactly you found this? I only find the
usual "should not modify" statements, and it is really unclear
whether leaving the bits alone is more compatible than clearing
them (since a bit of unknown function being set may easily mean
the HPET behaves in a way we don't expect).

Jan

Re: [Patch 4/4] x86/hpet: Don't clear reserved bits in the General Configuration Register

[Andrew Cooper]

On 07/10/13 14:55, Jan Beulich wrote:
>>>> On 07.10.13 at 15:26, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
>> It is a violation of the specification.
>>
>> The reserved bits in the General Configuration Register, unlike all other
>> reserved bits I have found in the spec, are specified as 'must never be
>> changed by the OS'.
> Mind pointing out where exactly you found this? I only find the
> usual "should not modify" statements, and it is really unclear
> whether leaving the bits alone is more compatible than clearing
> them (since a bit of unknown function being set may easily mean
> the HPET behaves in a way we don't expect).
>
> Jan
>

Hpet spec 1-0a.pdf Page 12

"General Configuration Register Bit Definitions"

For bits 63:2, (ignoring the spec reserved vs firmware reserved bits),
the requirement states:

"In order to preserve usage of these bits in the future, software should
not modify the value in
these bits until they are defined. This is done by doing a
“read-modify-write” to this
register."

In most cases Xen does correctly perform a read-modify-write, but not on
initialize examination of the hpet where it blindly tries to clear bits
it doesn't understand.

I did find it strange at the difference in the spec; All other reserved
bits I can find are specified as "must write 0".

~Andrew

Re: [Patch 4/4] x86/hpet: Don't clear reserved bits in the General Configuration Register

[Jan Beulich]

>>> On 07.10.13 at 16:02, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
> On 07/10/13 14:55, Jan Beulich wrote:
>>>>> On 07.10.13 at 15:26, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
>>> It is a violation of the specification.
>>>
>>> The reserved bits in the General Configuration Register, unlike all other
>>> reserved bits I have found in the spec, are specified as 'must never be
>>> changed by the OS'.
>> Mind pointing out where exactly you found this? I only find the
>> usual "should not modify" statements, and it is really unclear
>> whether leaving the bits alone is more compatible than clearing
>> them (since a bit of unknown function being set may easily mean
>> the HPET behaves in a way we don't expect).
>>
>> Jan
>>
> 
> Hpet spec 1-0a.pdf Page 12
> 
> "General Configuration Register Bit Definitions"
> 
> For bits 63:2, (ignoring the spec reserved vs firmware reserved bits),
> the requirement states:
> 
> "In order to preserve usage of these bits in the future, software should

"should" != "must never"

> not modify the value in
> these bits until they are defined. This is done by doing a
> “read-modify-write” to this
> register."
> 
> In most cases Xen does correctly perform a read-modify-write, but not on
> initialize examination of the hpet where it blindly tries to clear bits
> it doesn't understand.
> 
> I did find it strange at the difference in the spec; All other reserved
> bits I can find are specified as "must write 0".

Right. As said before - it's all but clear whether leaving the bits
alone is indeed better than clearing them.

Jan

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