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
* 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
Andrew Cooper
2013-Oct-07 13:26 UTC
[Patch 2/4] x86/hpet: Sanitise HPET ACPI table and warn about multiple tables
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
Andrew Cooper
2013-Oct-07 13:26 UTC
[Patch 3/4] x86/hpet: Fix ambiguity in broadcast info message.
"$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
Andrew Cooper
2013-Oct-07 13:26 UTC
[Patch 4/4] x86/hpet: Don''t clear reserved bits in the General Configuration Register
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
Jan Beulich
2013-Oct-07 13:45 UTC
Re: [Patch 2/4] x86/hpet: Sanitise HPET ACPI table and warn about multiple tables
>>> 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
Jan Beulich
2013-Oct-07 13:48 UTC
Re: [Patch 3/4] x86/hpet: Fix ambiguity in broadcast info message.
>>> 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 > > > .
Andrew Cooper
2013-Oct-07 13:55 UTC
Re: [Patch 2/4] x86/hpet: Sanitise HPET ACPI table and warn about multiple tables
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
Jan Beulich
2013-Oct-07 13:55 UTC
Re: [Patch 4/4] x86/hpet: Don''t clear reserved bits in the General Configuration Register
>>> 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
Andrew Cooper
2013-Oct-07 14:02 UTC
Re: [Patch 4/4] x86/hpet: Don''t clear reserved bits in the General Configuration Register
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
Jan Beulich
2013-Oct-07 14:26 UTC
Re: [Patch 2/4] x86/hpet: Sanitise HPET ACPI table and warn about multiple tables
>>> 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
Jan Beulich
2013-Oct-07 14:28 UTC
Re: [Patch 4/4] x86/hpet: Don''t clear reserved bits in the General Configuration Register
>>> 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