* [RFC 1/8] x86/timing: Command line parameter to disable ARAT
2013-11-04 18:54 [RFC 0/8] Improvements to HPET interupts Andrew Cooper
@ 2013-11-04 18:54 ` Andrew Cooper
2013-11-05 10:57 ` Jan Beulich
2013-11-04 18:54 ` [RFC 2/8] x86/acpi: Warn about multiple HPET tables Andrew Cooper
` (6 subsequent siblings)
7 siblings, 1 reply; 21+ messages in thread
From: Andrew Cooper @ 2013-11-04 18:54 UTC (permalink / raw)
To: Xen-devel; +Cc: Andrew Cooper
---
xen/arch/x86/cpu/amd.c | 2 +-
xen/arch/x86/cpu/common.c | 3 +++
xen/arch/x86/cpu/cpu.h | 2 ++
xen/arch/x86/cpu/intel.c | 5 +++--
4 files changed, 9 insertions(+), 3 deletions(-)
diff --git a/xen/arch/x86/cpu/amd.c b/xen/arch/x86/cpu/amd.c
index 27b7f71..8d86d98 100644
--- a/xen/arch/x86/cpu/amd.c
+++ b/xen/arch/x86/cpu/amd.c
@@ -502,7 +502,7 @@ static void __devinit init_amd(struct cpuinfo_x86 *c)
* Family 0x12 and above processors have APIC timer
* running in deep C states.
*/
- if (c->x86 > 0x11)
+ if ( opt_arat && c->x86 > 0x11)
set_bit(X86_FEATURE_ARAT, c->x86_capability);
/*
diff --git a/xen/arch/x86/cpu/common.c b/xen/arch/x86/cpu/common.c
index e1220e6..7e829b2 100644
--- a/xen/arch/x86/cpu/common.c
+++ b/xen/arch/x86/cpu/common.c
@@ -18,6 +18,9 @@
static bool_t __cpuinitdata use_xsave = 1;
boolean_param("xsave", use_xsave);
+bool_t __cpuinitdata opt_arat = 1;
+boolean_param("arat", opt_arat);
+
unsigned int __devinitdata opt_cpuid_mask_ecx = ~0u;
integer_param("cpuid_mask_ecx", opt_cpuid_mask_ecx);
unsigned int __devinitdata opt_cpuid_mask_edx = ~0u;
diff --git a/xen/arch/x86/cpu/cpu.h b/xen/arch/x86/cpu/cpu.h
index 72c41ca..dde4d3a 100644
--- a/xen/arch/x86/cpu/cpu.h
+++ b/xen/arch/x86/cpu/cpu.h
@@ -15,6 +15,8 @@ extern unsigned int opt_cpuid_mask_ecx, opt_cpuid_mask_edx;
extern unsigned int opt_cpuid_mask_xsave_eax;
extern unsigned int opt_cpuid_mask_ext_ecx, opt_cpuid_mask_ext_edx;
+extern bool_t opt_arat;
+
extern int get_model_name(struct cpuinfo_x86 *c);
extern void display_cacheinfo(struct cpuinfo_x86 *c);
diff --git a/xen/arch/x86/cpu/intel.c b/xen/arch/x86/cpu/intel.c
index 31b69c9..fa0851b 100644
--- a/xen/arch/x86/cpu/intel.c
+++ b/xen/arch/x86/cpu/intel.c
@@ -226,8 +226,9 @@ static void __devinit init_intel(struct cpuinfo_x86 *c)
set_bit(X86_FEATURE_NONSTOP_TSC, c->x86_capability);
set_bit(X86_FEATURE_TSC_RELIABLE, c->x86_capability);
}
- if ((c->cpuid_level >= 0x00000006) &&
- (cpuid_eax(0x00000006) & (1u<<2)))
+ if ( opt_arat &&
+ (c->cpuid_level >= 0x00000006) &&
+ (cpuid_eax(0x00000006) & (1u<<2)))
set_bit(X86_FEATURE_ARAT, c->x86_capability);
}
--
1.7.10.4
^ permalink raw reply related [flat|nested] 21+ messages in thread* Re: [RFC 1/8] x86/timing: Command line parameter to disable ARAT
2013-11-04 18:54 ` [RFC 1/8] x86/timing: Command line parameter to disable ARAT Andrew Cooper
@ 2013-11-05 10:57 ` Jan Beulich
0 siblings, 0 replies; 21+ messages in thread
From: Jan Beulich @ 2013-11-05 10:57 UTC (permalink / raw)
To: Andrew Cooper; +Cc: xen-devel
>>> On 04.11.13 at 19:54, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
> --- a/xen/arch/x86/cpu/common.c
> +++ b/xen/arch/x86/cpu/common.c
> @@ -18,6 +18,9 @@
> static bool_t __cpuinitdata use_xsave = 1;
> boolean_param("xsave", use_xsave);
>
> +bool_t __cpuinitdata opt_arat = 1;
> +boolean_param("arat", opt_arat);
Two options: Either put this in a "#ifndef NDEBUG" section,
to make clear it's for debugging only. Or add it to
docs/misc/xen-command-line.markdown.
Beyond that - very much appreciated, as I too had to suppress
recognizing ARAT a number of times in the past.
Jan
^ permalink raw reply [flat|nested] 21+ messages in thread
* [RFC 2/8] x86/acpi: Warn about multiple HPET tables
2013-11-04 18:54 [RFC 0/8] Improvements to HPET interupts Andrew Cooper
2013-11-04 18:54 ` [RFC 1/8] x86/timing: Command line parameter to disable ARAT Andrew Cooper
@ 2013-11-04 18:54 ` Andrew Cooper
2013-11-05 10:58 ` Jan Beulich
2013-11-04 18:54 ` [RFC 3/8] x86/hpet: Fix ambiguity in broadcast info message Andrew Cooper
` (5 subsequent siblings)
7 siblings, 1 reply; 21+ messages in thread
From: Andrew Cooper @ 2013-11-04 18:54 UTC (permalink / raw)
To: Xen-devel; +Cc: Andrew Cooper, Keir Fraser, Jan Beulich
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 | 11 +++++++++++
1 file changed, 11 insertions(+)
diff --git a/xen/arch/x86/acpi/boot.c b/xen/arch/x86/acpi/boot.c
index df26423..63e5c3b 100644
--- a/xen/arch/x86/acpi/boot.c
+++ b/xen/arch/x86/acpi/boot.c
@@ -289,6 +289,17 @@ static int __init acpi_parse_hpet(struct acpi_table_header *table)
return -1;
}
+ /*
+ * Some BIOSes provide multiple HPET tables. Warn that we will ignore
+ * them.
+ */
+ if ( hpet_address )
+ {
+ printk(KERN_WARNING PREFIX
+ "Found multiple HPET tables. Only using first\n");
+ return -1;
+ }
+
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
^ permalink raw reply related [flat|nested] 21+ messages in thread* Re: [RFC 2/8] x86/acpi: Warn about multiple HPET tables
2013-11-04 18:54 ` [RFC 2/8] x86/acpi: Warn about multiple HPET tables Andrew Cooper
@ 2013-11-05 10:58 ` Jan Beulich
2013-11-05 16:03 ` Andrew Cooper
0 siblings, 1 reply; 21+ messages in thread
From: Jan Beulich @ 2013-11-05 10:58 UTC (permalink / raw)
To: Andrew Cooper; +Cc: xen-devel, Keir Fraser
>>> On 04.11.13 at 19:54, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
> --- a/xen/arch/x86/acpi/boot.c
> +++ b/xen/arch/x86/acpi/boot.c
> @@ -289,6 +289,17 @@ static int __init acpi_parse_hpet(struct acpi_table_header *table)
> return -1;
> }
>
> + /*
> + * Some BIOSes provide multiple HPET tables. Warn that we will ignore
> + * them.
> + */
> + if ( hpet_address )
> + {
> + printk(KERN_WARNING PREFIX
> + "Found multiple HPET tables. Only using first\n");
> + return -1;
> + }
If there really are examples of this, and if those HPETs work
properly, perhaps we should rather make use of them?
Jan
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [RFC 2/8] x86/acpi: Warn about multiple HPET tables
2013-11-05 10:58 ` Jan Beulich
@ 2013-11-05 16:03 ` Andrew Cooper
0 siblings, 0 replies; 21+ messages in thread
From: Andrew Cooper @ 2013-11-05 16:03 UTC (permalink / raw)
To: Jan Beulich; +Cc: xen-devel, Keir Fraser
On 05/11/13 10:58, Jan Beulich wrote:
>>>> On 04.11.13 at 19:54, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
>> --- a/xen/arch/x86/acpi/boot.c
>> +++ b/xen/arch/x86/acpi/boot.c
>> @@ -289,6 +289,17 @@ static int __init acpi_parse_hpet(struct acpi_table_header *table)
>> return -1;
>> }
>>
>> + /*
>> + * Some BIOSes provide multiple HPET tables. Warn that we will ignore
>> + * them.
>> + */
>> + if ( hpet_address )
>> + {
>> + printk(KERN_WARNING PREFIX
>> + "Found multiple HPET tables. Only using first\n");
>> + return -1;
>> + }
> If there really are examples of this, and if those HPETs work
> properly, perhaps we should rather make use of them?
>
> Jan
>
The cause of this was two HPET acpi tables appearing. This got 'fixed'
by a BIOS update (after which there was only a single HPET table), but I
felt it was worth warning about.
It might be nice to support multiple hpets is such a system existed. It
would however be another fairly large chunk of work and I do not believe
I have appropriate hardware to test any development work on.
~Andrew
^ permalink raw reply [flat|nested] 21+ messages in thread
* [RFC 3/8] x86/hpet: Fix ambiguity in broadcast info message.
2013-11-04 18:54 [RFC 0/8] Improvements to HPET interupts Andrew Cooper
2013-11-04 18:54 ` [RFC 1/8] x86/timing: Command line parameter to disable ARAT Andrew Cooper
2013-11-04 18:54 ` [RFC 2/8] x86/acpi: Warn about multiple HPET tables Andrew Cooper
@ 2013-11-04 18:54 ` Andrew Cooper
2013-11-05 11:02 ` Jan Beulich
2013-11-04 18:54 ` [RFC 4/8] x86/hpet: Debug and verbose hpet logging Andrew Cooper
` (4 subsequent siblings)
7 siblings, 1 reply; 21+ messages in thread
From: Andrew Cooper @ 2013-11-04 18:54 UTC (permalink / raw)
To: Xen-devel; +Cc: Andrew Cooper, Keir Fraser, Jan Beulich
"$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
^ permalink raw reply related [flat|nested] 21+ messages in thread* Re: [RFC 3/8] x86/hpet: Fix ambiguity in broadcast info message.
2013-11-04 18:54 ` [RFC 3/8] x86/hpet: Fix ambiguity in broadcast info message Andrew Cooper
@ 2013-11-05 11:02 ` Jan Beulich
2013-11-05 13:28 ` David Vrabel
0 siblings, 1 reply; 21+ messages in thread
From: Jan Beulich @ 2013-11-05 11:02 UTC (permalink / raw)
To: Andrew Cooper; +Cc: xen-devel, Keir Fraser
>>> On 04.11.13 at 19:54, 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.
>
> 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",
If you already alter this (which I'm not convinced is necessary - I
personally never considered the message ambiguous), please also
change "used" to "usable". And then maybe you agree adding the
2nd "timers" becomes unnecessary.
Jan
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [RFC 3/8] x86/hpet: Fix ambiguity in broadcast info message.
2013-11-05 11:02 ` Jan Beulich
@ 2013-11-05 13:28 ` David Vrabel
2013-11-05 13:34 ` Jan Beulich
0 siblings, 1 reply; 21+ messages in thread
From: David Vrabel @ 2013-11-05 13:28 UTC (permalink / raw)
To: Jan Beulich; +Cc: Andrew Cooper, Keir Fraser, xen-devel
On 05/11/13 11:02, Jan Beulich wrote:
>>>> On 04.11.13 at 19:54, 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.
>>
>> 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",
>
> If you already alter this (which I'm not convinced is necessary - I
> personally never considered the message ambiguous), please also
> change "used" to "usable". And then maybe you agree adding the
> 2nd "timers" becomes unnecessary.
I agree with Andrew that the original wording is ambiguous. It needs to
be clear that the second %u is a count, and not the subject of the
sub-clause.
My suggested wording would be:
"HPET: %u timers, %u are broadcast capable"
Or
"HPET: %u timers (%u are broadcast capable)"
If the broadcast capability is much less interesting that the overall
number of timers.
David
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [RFC 3/8] x86/hpet: Fix ambiguity in broadcast info message.
2013-11-05 13:28 ` David Vrabel
@ 2013-11-05 13:34 ` Jan Beulich
2013-11-05 13:36 ` David Vrabel
0 siblings, 1 reply; 21+ messages in thread
From: Jan Beulich @ 2013-11-05 13:34 UTC (permalink / raw)
To: David Vrabel; +Cc: Andrew Cooper, Keir Fraser, xen-devel
>>> On 05.11.13 at 14:28, David Vrabel <david.vrabel@citrix.com> wrote:
> On 05/11/13 11:02, Jan Beulich wrote:
>>>>> On 04.11.13 at 19:54, 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.
>>>
>>> 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",
>>
>> If you already alter this (which I'm not convinced is necessary - I
>> personally never considered the message ambiguous), please also
>> change "used" to "usable". And then maybe you agree adding the
>> 2nd "timers" becomes unnecessary.
>
> I agree with Andrew that the original wording is ambiguous. It needs to
> be clear that the second %u is a count, and not the subject of the
> sub-clause.
>
> My suggested wording would be:
>
> "HPET: %u timers, %u are broadcast capable"
>
> Or
>
> "HPET: %u timers (%u are broadcast capable)"
>
> If the broadcast capability is much less interesting that the overall
> number of timers.
In fact all we care about are those timers that can be used for
broadcast. "Boradcast capable", however, doesn't properly
express things - timers aren't "capable of", they can only be
"used for" broadcasting.
How about "HPET: %u timers usable for broadcast (%u total)"?
Jan
^ permalink raw reply [flat|nested] 21+ messages in thread
* [RFC 4/8] x86/hpet: Debug and verbose hpet logging
2013-11-04 18:54 [RFC 0/8] Improvements to HPET interupts Andrew Cooper
` (2 preceding siblings ...)
2013-11-04 18:54 ` [RFC 3/8] x86/hpet: Fix ambiguity in broadcast info message Andrew Cooper
@ 2013-11-04 18:54 ` Andrew Cooper
2013-11-04 18:54 ` [RFC 5/8] x86/msi: Refactor msi_compose_message() to not requrie an irq_desc Andrew Cooper
` (3 subsequent siblings)
7 siblings, 0 replies; 21+ messages in thread
From: Andrew Cooper @ 2013-11-04 18:54 UTC (permalink / raw)
To: Xen-devel; +Cc: Andrew Cooper, Keir Fraser, Jan Beulich
This was for debugging purposes, but might perhaps be more useful generally.
I am happy to keep none, some or all of it, depending on how useful people
think it might be.
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 | 83 +++++++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 83 insertions(+)
diff --git a/xen/arch/x86/hpet.c b/xen/arch/x86/hpet.c
index 091e624..4b08381 100644
--- a/xen/arch/x86/hpet.c
+++ b/xen/arch/x86/hpet.c
@@ -61,6 +61,36 @@ u8 __initdata hpet_blockid;
static bool_t __initdata force_hpet_broadcast;
boolean_param("hpetbroadcast", force_hpet_broadcast);
+static bool_t __read_mostly hpet_verbose;
+static bool_t __read_mostly hpet_debug;
+static void __init parse_hpet_param(char * s)
+{
+ char *ss;
+ int val;
+
+ do {
+ val = !!strncmp(s, "no-", 3);
+ if ( !val )
+ s += 3;
+
+ ss = strchr(s, ',');
+ if ( ss )
+ *ss = '\0';
+
+ if ( !strcmp(s, "verbose") )
+ hpet_verbose = val;
+ else if ( !strcmp(s, "debug") )
+ {
+ hpet_debug = val;
+ if ( val )
+ hpet_verbose = 1;
+ }
+
+ s = ss + 1;
+ } while ( ss );
+}
+custom_param("hpet", parse_hpet_param);
+
/*
* Calculate a multiplication factor for scaled math, which is used to convert
* nanoseconds based values to clock ticks:
@@ -94,6 +124,35 @@ static inline unsigned long ns2ticks(unsigned long nsec, int shift,
return (unsigned long) tmp;
}
+static void __maybe_unused dump_hpet_timer(int timer)
+{
+ u32 cfg = hpet_read32(HPET_Tn_CFG(timer));
+
+ printk(XENLOG_INFO "HPET: Timer %02u CFG: raw 0x%08"PRIx32
+ " Caps: %d %c%c", timer, cfg,
+ cfg & HPET_TN_64BIT_CAP ? 64 : 32,
+ cfg & HPET_TN_FSB_CAP ? 'M' : '-',
+ cfg & HPET_TN_PERIODIC_CAP ? 'P' : '-');
+
+ printk("\n Setup: ");
+
+ if ( (cfg & HPET_TN_FSB_CAP) && (cfg & HPET_TN_FSB) )
+ printk("FSB ");
+
+ if ( !(cfg & HPET_TN_FSB) )
+ printk("GSI %#x ",
+ (cfg & HPET_TN_ROUTE) >> HPET_TN_ROUTE_SHIFT);
+
+ if ( cfg & HPET_TN_32BIT )
+ printk("32bit ");
+
+ if ( cfg & HPET_TN_PERIODIC )
+ printk("Periodic ");
+
+ printk("%sabled ", cfg & HPET_TN_ENABLE ? "En" : "Dis");
+ printk("%s\n", cfg & HPET_TN_LEVEL ? "Level" : "Edge");
+}
+
static int hpet_next_event(unsigned long delta, int timer)
{
uint32_t cnt, cmp;
@@ -769,7 +828,14 @@ u64 __init hpet_setup(void)
unsigned int last;
if ( hpet_rate )
+ {
+ if ( hpet_debug )
+ printk(XENLOG_DEBUG "HPET: Skipping re-setup\n");
return hpet_rate;
+ }
+
+ if ( hpet_debug )
+ printk(XENLOG_DEBUG "HPET: Setting up hpet data\n");
if ( hpet_address == 0 )
return 0;
@@ -783,6 +849,20 @@ u64 __init hpet_setup(void)
return 0;
}
+ if ( hpet_verbose )
+ {
+ printk(XENLOG_INFO "HPET: Vendor: %04"PRIx16", Rev: %u, %u timers\n",
+ hpet_id >> HPET_ID_VENDOR_SHIFT,
+ hpet_id & HPET_ID_REV,
+ ((hpet_id & HPET_ID_NUMBER) >> HPET_ID_NUMBER_SHIFT) + 1);
+ printk(XENLOG_INFO "HPET: Caps: ");
+ if ( hpet_id & HPET_ID_LEGSUP )
+ printk("Legacy ");
+ if ( hpet_id & HPET_ID_64BIT )
+ printk("64bit ");
+ printk("\n");
+ }
+
/* Check for sane period (100ps <= period <= 100ns). */
hpet_period = hpet_read32(HPET_PERIOD);
if ( (hpet_period > 100000000) || (hpet_period < 100000) )
@@ -840,6 +920,9 @@ void hpet_resume(u32 *boot_cfg)
cfg &= ~HPET_TN_RESERVED;
}
hpet_write32(cfg, HPET_Tn_CFG(i));
+
+ if ( hpet_verbose )
+ dump_hpet_timer(i);
}
cfg = hpet_read32(HPET_CFG);
--
1.7.10.4
^ permalink raw reply related [flat|nested] 21+ messages in thread* [RFC 5/8] x86/msi: Refactor msi_compose_message() to not requrie an irq_desc
2013-11-04 18:54 [RFC 0/8] Improvements to HPET interupts Andrew Cooper
` (3 preceding siblings ...)
2013-11-04 18:54 ` [RFC 4/8] x86/hpet: Debug and verbose hpet logging Andrew Cooper
@ 2013-11-04 18:54 ` Andrew Cooper
2013-11-05 11:05 ` Jan Beulich
2013-11-04 18:54 ` [RFC 6/8] x86/hpet: Adjust pointer vs array semantics of hpet_boot_cfg Andrew Cooper
` (2 subsequent siblings)
7 siblings, 1 reply; 21+ messages in thread
From: Andrew Cooper @ 2013-11-04 18:54 UTC (permalink / raw)
To: Xen-devel; +Cc: Andrew Cooper, Keir Fraser, Jan Beulich
Subsequent changes will cause HPET MSIs to not have an associated IRQ.
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 +-
xen/arch/x86/msi.c | 9 ++++-----
xen/drivers/passthrough/vtd/iommu.c | 2 +-
xen/include/asm-x86/msi.h | 2 +-
4 files changed, 7 insertions(+), 8 deletions(-)
diff --git a/xen/arch/x86/hpet.c b/xen/arch/x86/hpet.c
index 4b08381..2c39808 100644
--- a/xen/arch/x86/hpet.c
+++ b/xen/arch/x86/hpet.c
@@ -390,7 +390,7 @@ static int __hpet_setup_msi_irq(struct irq_desc *desc)
{
struct msi_msg msg;
- msi_compose_msg(desc, &msg);
+ msi_compose_msg(desc->arch.vector, desc->arch.cpu_mask, &msg);
return hpet_msi_write(desc->action->dev_id, &msg);
}
diff --git a/xen/arch/x86/msi.c b/xen/arch/x86/msi.c
index b43c36a..284042e 100644
--- a/xen/arch/x86/msi.c
+++ b/xen/arch/x86/msi.c
@@ -124,13 +124,12 @@ static void msix_put_fixmap(struct arch_msix *msix, int idx)
/*
* MSI message composition
*/
-void msi_compose_msg(struct irq_desc *desc, struct msi_msg *msg)
+void msi_compose_msg(unsigned vector, const cpumask_t *cpu_mask, struct msi_msg *msg)
{
unsigned dest;
- int vector = desc->arch.vector;
memset(msg, 0, sizeof(*msg));
- if ( !cpumask_intersects(desc->arch.cpu_mask, &cpu_online_map) ) {
+ if ( !cpumask_intersects(cpu_mask, &cpu_online_map) ) {
dprintk(XENLOG_ERR,"%s, compose msi message error!!\n", __func__);
return;
}
@@ -138,7 +137,7 @@ void msi_compose_msg(struct irq_desc *desc, struct msi_msg *msg)
if ( vector ) {
cpumask_t *mask = this_cpu(scratch_mask);
- cpumask_and(mask, desc->arch.cpu_mask, &cpu_online_map);
+ cpumask_and(mask, cpu_mask, &cpu_online_map);
dest = cpu_mask_to_apicid(mask);
msg->address_hi = MSI_ADDR_BASE_HI;
@@ -491,7 +490,7 @@ int __setup_msi_irq(struct irq_desc *desc, struct msi_desc *msidesc,
desc->msi_desc = msidesc;
desc->handler = handler;
- msi_compose_msg(desc, &msg);
+ msi_compose_msg(desc->arch.vector, desc->arch.cpu_mask, &msg);
return write_msi_msg(msidesc, &msg);
}
diff --git a/xen/drivers/passthrough/vtd/iommu.c b/xen/drivers/passthrough/vtd/iommu.c
index 2dbe97a..97d5b5e 100644
--- a/xen/drivers/passthrough/vtd/iommu.c
+++ b/xen/drivers/passthrough/vtd/iommu.c
@@ -1039,7 +1039,7 @@ static void dma_msi_set_affinity(struct irq_desc *desc, const cpumask_t *mask)
return;
}
- msi_compose_msg(desc, &msg);
+ msi_compose_msg(desc->arch.vector, desc->arch.cpu_mask, &msg);
/* Are these overrides really needed? */
if (x2apic_enabled)
msg.address_hi = dest & 0xFFFFFF00;
diff --git a/xen/include/asm-x86/msi.h b/xen/include/asm-x86/msi.h
index 9eeef63..8c67ca8 100644
--- a/xen/include/asm-x86/msi.h
+++ b/xen/include/asm-x86/msi.h
@@ -231,7 +231,7 @@ struct arch_msix {
};
void early_msi_init(void);
-void msi_compose_msg(struct irq_desc *, struct msi_msg *);
+void msi_compose_msg(unsigned vector, const cpumask_t *mask, struct msi_msg *);
void __msi_set_enable(u16 seg, u8 bus, u8 slot, u8 func, int pos, int enable);
void mask_msi_irq(struct irq_desc *);
void unmask_msi_irq(struct irq_desc *);
--
1.7.10.4
^ permalink raw reply related [flat|nested] 21+ messages in thread* Re: [RFC 5/8] x86/msi: Refactor msi_compose_message() to not requrie an irq_desc
2013-11-04 18:54 ` [RFC 5/8] x86/msi: Refactor msi_compose_message() to not requrie an irq_desc Andrew Cooper
@ 2013-11-05 11:05 ` Jan Beulich
0 siblings, 0 replies; 21+ messages in thread
From: Jan Beulich @ 2013-11-05 11:05 UTC (permalink / raw)
To: Andrew Cooper; +Cc: xen-devel, Keir Fraser
>>> On 04.11.13 at 19:54, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
> --- a/xen/include/asm-x86/msi.h
> +++ b/xen/include/asm-x86/msi.h
> @@ -231,7 +231,7 @@ struct arch_msix {
> };
>
> void early_msi_init(void);
> -void msi_compose_msg(struct irq_desc *, struct msi_msg *);
> +void msi_compose_msg(unsigned vector, const cpumask_t *mask, struct msi_msg *);
Please be consistent here - either re-add a name for the last
parameter, or drop the name of the middle one. Reviewed-by
with that change.
Jan
^ permalink raw reply [flat|nested] 21+ messages in thread
* [RFC 6/8] x86/hpet: Adjust pointer vs array semantics of hpet_boot_cfg
2013-11-04 18:54 [RFC 0/8] Improvements to HPET interupts Andrew Cooper
` (4 preceding siblings ...)
2013-11-04 18:54 ` [RFC 5/8] x86/msi: Refactor msi_compose_message() to not requrie an irq_desc Andrew Cooper
@ 2013-11-04 18:54 ` Andrew Cooper
2013-11-05 11:08 ` Jan Beulich
2013-11-04 18:54 ` [RFC 7/8] x86/hpet: debug keyhandlers Andrew Cooper
2013-11-04 18:54 ` [RFC 8/8] x86/hpet: Use singe apic vector rather than irq_descs for HPET interrupts Andrew Cooper
7 siblings, 1 reply; 21+ messages in thread
From: Andrew Cooper @ 2013-11-04 18:54 UTC (permalink / raw)
To: Xen-devel; +Cc: Andrew Cooper, Keir Fraser, Jan Beulich
There should be no functional change as a result, but hpet_boot_cfg is an
array, not a pointer. Code it as such.
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 | 17 +++++++++--------
1 file changed, 9 insertions(+), 8 deletions(-)
diff --git a/xen/arch/x86/hpet.c b/xen/arch/x86/hpet.c
index 2c39808..e8a3f66 100644
--- a/xen/arch/x86/hpet.c
+++ b/xen/arch/x86/hpet.c
@@ -53,6 +53,9 @@ DEFINE_PER_CPU(struct hpet_event_channel *, cpu_bc_channel);
unsigned long __initdata hpet_address;
u8 __initdata hpet_blockid;
+/* BIOS HPET configuration */
+static u32 *hpet_boot_cfg;
+
/*
* force_hpet_broadcast: by default legacy hpet broadcast will be stopped
* if RTC interrupts are enabled. Enable this option if want to always enable
@@ -803,8 +806,8 @@ void hpet_broadcast_exit(void)
int hpet_broadcast_is_available(void)
{
- return ((hpet_events && (hpet_events->flags & HPET_EVT_LEGACY))
- || num_hpets_used > 0);
+ return ( num_hpets_used > 0 ||
+ (hpet_events && (hpet_events->flags & HPET_EVT_LEGACY)) );
}
int hpet_legacy_irq_tick(void)
@@ -819,8 +822,6 @@ int hpet_legacy_irq_tick(void)
return 1;
}
-static u32 *hpet_boot_cfg;
-
u64 __init hpet_setup(void)
{
static u64 __initdata hpet_rate;
@@ -893,7 +894,7 @@ void hpet_resume(u32 *boot_cfg)
cfg = hpet_read32(HPET_CFG);
if ( boot_cfg )
- *boot_cfg = cfg;
+ boot_cfg[0] = cfg;
cfg &= ~(HPET_CFG_ENABLE | HPET_CFG_LEGACY);
if ( cfg )
{
@@ -942,12 +943,12 @@ void hpet_disable(void)
return;
}
- hpet_write32(*hpet_boot_cfg & ~HPET_CFG_ENABLE, HPET_CFG);
+ hpet_write32(hpet_boot_cfg[0] & ~HPET_CFG_ENABLE, HPET_CFG);
id = hpet_read32(HPET_ID);
for ( i = 0; i <= ((id & HPET_ID_NUMBER) >> HPET_ID_NUMBER_SHIFT); ++i )
hpet_write32(hpet_boot_cfg[i + 1], HPET_Tn_CFG(i));
- if ( *hpet_boot_cfg & HPET_CFG_ENABLE )
- hpet_write32(*hpet_boot_cfg, HPET_CFG);
+ if ( hpet_boot_cfg[0] & HPET_CFG_ENABLE )
+ hpet_write32(hpet_boot_cfg[0], HPET_CFG);
}
--
1.7.10.4
^ permalink raw reply related [flat|nested] 21+ messages in thread* [RFC 7/8] x86/hpet: debug keyhandlers
2013-11-04 18:54 [RFC 0/8] Improvements to HPET interupts Andrew Cooper
` (5 preceding siblings ...)
2013-11-04 18:54 ` [RFC 6/8] x86/hpet: Adjust pointer vs array semantics of hpet_boot_cfg Andrew Cooper
@ 2013-11-04 18:54 ` Andrew Cooper
2013-11-05 11:11 ` Jan Beulich
2013-11-04 18:54 ` [RFC 8/8] x86/hpet: Use singe apic vector rather than irq_descs for HPET interrupts Andrew Cooper
7 siblings, 1 reply; 21+ messages in thread
From: Andrew Cooper @ 2013-11-04 18:54 UTC (permalink / raw)
To: Xen-devel; +Cc: Andrew Cooper
Debug key for dumping HPET state.
---
xen/arch/x86/hpet.c | 19 +++++++++++++++++++
1 file changed, 19 insertions(+)
diff --git a/xen/arch/x86/hpet.c b/xen/arch/x86/hpet.c
index e8a3f66..c5468fa 100644
--- a/xen/arch/x86/hpet.c
+++ b/xen/arch/x86/hpet.c
@@ -22,6 +22,8 @@
#define MAX_DELTA_NS MILLISECS(10*1000)
#define MIN_DELTA_NS MICROSECS(20)
+#include <xen/keyhandler.h>
+
#define HPET_EVT_USED_BIT 0
#define HPET_EVT_USED (1 << HPET_EVT_USED_BIT)
#define HPET_EVT_DISABLE_BIT 1
@@ -822,6 +824,21 @@ int hpet_legacy_irq_tick(void)
return 1;
}
+static void do_hpet_dump_state(unsigned char key)
+{
+ unsigned i;
+ printk("'%c' pressed - dumping HPET state\n", key);
+
+ for ( i = 0; i < num_hpets_used; ++i )
+ dump_hpet_timer(i);
+}
+
+static struct keyhandler hpet_dump_state = {
+ .irq_callback = 0,
+ .u.fn = do_hpet_dump_state,
+ .desc = "Dump hpet state"
+};
+
u64 __init hpet_setup(void)
{
static u64 __initdata hpet_rate;
@@ -879,6 +896,8 @@ u64 __init hpet_setup(void)
hpet_rate = 1000000000000000ULL; /* 10^15 */
(void)do_div(hpet_rate, hpet_period);
+ register_keyhandler('1', &hpet_dump_state);
+
return hpet_rate;
}
--
1.7.10.4
^ permalink raw reply related [flat|nested] 21+ messages in thread* Re: [RFC 7/8] x86/hpet: debug keyhandlers
2013-11-04 18:54 ` [RFC 7/8] x86/hpet: debug keyhandlers Andrew Cooper
@ 2013-11-05 11:11 ` Jan Beulich
2013-11-05 11:16 ` Andrew Cooper
0 siblings, 1 reply; 21+ messages in thread
From: Jan Beulich @ 2013-11-05 11:11 UTC (permalink / raw)
To: Andrew Cooper; +Cc: xen-devel
>>> On 04.11.13 at 19:54, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
> @@ -879,6 +896,8 @@ u64 __init hpet_setup(void)
> hpet_rate = 1000000000000000ULL; /* 10^15 */
> (void)do_div(hpet_rate, hpet_period);
>
> + register_keyhandler('1', &hpet_dump_state);
I'd prefer the numbers to be left alone if at all possible. How
about 'E'?
Jan
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [RFC 7/8] x86/hpet: debug keyhandlers
2013-11-05 11:11 ` Jan Beulich
@ 2013-11-05 11:16 ` Andrew Cooper
0 siblings, 0 replies; 21+ messages in thread
From: Andrew Cooper @ 2013-11-05 11:16 UTC (permalink / raw)
To: Jan Beulich; +Cc: xen-devel
On 05/11/13 11:11, Jan Beulich wrote:
>>>> On 04.11.13 at 19:54, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
>> @@ -879,6 +896,8 @@ u64 __init hpet_setup(void)
>> hpet_rate = 1000000000000000ULL; /* 10^15 */
>> (void)do_div(hpet_rate, hpet_period);
>>
>> + register_keyhandler('1', &hpet_dump_state);
> I'd prefer the numbers to be left alone if at all possible. How
> about 'E'?
>
> Jan
>
I wasn't intending for this keyhandler to be committed, at least
certainly not taking the place of '1' (As people might be able to guess
from similar debugging patches of mine, I regularly have many of the
numbers in use for temporary keyhandlers)
While it was very useful while working on this issue, I don't think it
would be particularly useful in general, especially given the increasing
scarcity of keys.
~Andrew
^ permalink raw reply [flat|nested] 21+ messages in thread
* [RFC 8/8] x86/hpet: Use singe apic vector rather than irq_descs for HPET interrupts
2013-11-04 18:54 [RFC 0/8] Improvements to HPET interupts Andrew Cooper
` (6 preceding siblings ...)
2013-11-04 18:54 ` [RFC 7/8] x86/hpet: debug keyhandlers Andrew Cooper
@ 2013-11-04 18:54 ` Andrew Cooper
2013-11-05 15:10 ` Jan Beulich
7 siblings, 1 reply; 21+ messages in thread
From: Andrew Cooper @ 2013-11-04 18:54 UTC (permalink / raw)
To: Xen-devel; +Cc: Andrew Cooper, Keir Fraser, Jan Beulich
This involves rewriting most of the MSI related HPET code, and as a result
this patch looks very complicated. It is probably best viewed as an end
result, with the following notes explaining what is going on.
The new logic is as follows:
* A single high priority vector is allocated and uses on all cpus.
* Reliance on the irq infrastructure is completely removed.
* Tracking of free hpet channels has changed. It is now an individual
bitmap, and allocation is based on winning a test_and_clear_bit()
operation.
* There is a notion of strict ownership of hpet channels.
** A cpu which owns an HPET channel can program it for a desired deadline.
** A cpu which can't find a free HPET channel to own may register for being
woken up by another in-use HPET which will fire at an appropriate time.
* Some functions have been renamed to be more descriptive. Some functions
have parameters changed to be more consistent.
* Any function with a __hpet prefix expectes the appropriate lock to be held.
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 | 477 +++++++++++++++++++--------------------------------
1 file changed, 181 insertions(+), 296 deletions(-)
diff --git a/xen/arch/x86/hpet.c b/xen/arch/x86/hpet.c
index c5468fa..2f4d880 100644
--- a/xen/arch/x86/hpet.c
+++ b/xen/arch/x86/hpet.c
@@ -4,54 +4,59 @@
* HPET management.
*/
-#include <xen/config.h>
+#include <xen/lib.h>
+#include <xen/init.h>
+#include <xen/cpuidle.h>
#include <xen/errno.h>
-#include <xen/time.h>
-#include <xen/timer.h>
-#include <xen/smp.h>
#include <xen/softirq.h>
-#include <xen/irq.h>
-#include <xen/numa.h>
+
+#include <mach_apic.h>
+
#include <asm/fixmap.h>
#include <asm/div64.h>
#include <asm/hpet.h>
-#include <asm/msi.h>
-#include <mach_apic.h>
-#include <xen/cpuidle.h>
#define MAX_DELTA_NS MILLISECS(10*1000)
#define MIN_DELTA_NS MICROSECS(20)
#include <xen/keyhandler.h>
-#define HPET_EVT_USED_BIT 0
-#define HPET_EVT_USED (1 << HPET_EVT_USED_BIT)
-#define HPET_EVT_DISABLE_BIT 1
+#define HPET_EVT_DISABLE_BIT 0
#define HPET_EVT_DISABLE (1 << HPET_EVT_DISABLE_BIT)
-#define HPET_EVT_LEGACY_BIT 2
+#define HPET_EVT_LEGACY_BIT 1
#define HPET_EVT_LEGACY (1 << HPET_EVT_LEGACY_BIT)
struct hpet_event_channel
{
unsigned long mult;
int shift;
+ unsigned int flags; /* HPET_EVT_x */
s_time_t next_event;
- cpumask_var_t cpumask;
+ cpumask_var_t cpumask; /* cpus wishing to be woken */
spinlock_t lock;
- void (*event_handler)(struct hpet_event_channel *);
-
unsigned int idx; /* physical channel idx */
unsigned int cpu; /* msi target */
struct msi_desc msi;/* msi state */
- unsigned int flags; /* HPET_EVT_x */
} __cacheline_aligned;
static struct hpet_event_channel *__read_mostly hpet_events;
/* msi hpet channels used for broadcast */
static unsigned int __read_mostly num_hpets_used;
-DEFINE_PER_CPU(struct hpet_event_channel *, cpu_bc_channel);
+/* High-priority vector for HPET interrupts */
+static u8 __read_mostly hpet_vector;
+
+/*
+ * HPET channel used for idling. Either the HPET channel this cpu owns
+ * (indicated by channel->cpu pointing back), or the HPET channel belonging to
+ * another cpu with which we have requested to be woken.
+ */
+static DEFINE_PER_CPU(struct hpet_event_channel *, hpet_channel);
+
+/* Bitmask of currently-free HPET channels. */
+static uint32_t free_channels;
+/* Data from the HPET ACPI table */
unsigned long __initdata hpet_address;
u8 __initdata hpet_blockid;
@@ -158,7 +163,7 @@ static void __maybe_unused dump_hpet_timer(int timer)
printk("%s\n", cfg & HPET_TN_LEVEL ? "Level" : "Edge");
}
-static int hpet_next_event(unsigned long delta, int timer)
+static int __hpet_set_counter(struct hpet_event_channel *ch, int delta)
{
uint32_t cnt, cmp;
unsigned long flags;
@@ -166,7 +171,7 @@ static int hpet_next_event(unsigned long delta, int timer)
local_irq_save(flags);
cnt = hpet_read32(HPET_COUNTER);
cmp = cnt + delta;
- hpet_write32(cmp, HPET_Tn_CMP(timer));
+ hpet_write32(cmp, HPET_Tn_CMP(ch->idx));
cmp = hpet_read32(HPET_COUNTER);
local_irq_restore(flags);
@@ -174,9 +179,8 @@ static int hpet_next_event(unsigned long delta, int timer)
return ((cmp + 2 - cnt) > delta) ? -ETIME : 0;
}
-static int reprogram_hpet_evt_channel(
- struct hpet_event_channel *ch,
- s_time_t expire, s_time_t now, int force)
+static int __program_hpet_time(struct hpet_event_channel *ch,
+ s_time_t expire, s_time_t now, int force)
{
int64_t delta;
int ret;
@@ -207,99 +211,50 @@ static int reprogram_hpet_evt_channel(
delta = max_t(int64_t, delta, MIN_DELTA_NS);
delta = ns2ticks(delta, ch->shift, ch->mult);
- ret = hpet_next_event(delta, ch->idx);
+ ret = __hpet_set_counter(ch, delta);
while ( ret && force )
{
delta += delta;
- ret = hpet_next_event(delta, ch->idx);
+ ret = __hpet_set_counter(ch, delta);
}
return ret;
}
-static void evt_do_broadcast(cpumask_t *mask)
+static void __hpet_wake_cpus(cpumask_t *mask)
{
- unsigned int cpu = smp_processor_id();
-
- if ( cpumask_test_and_clear_cpu(cpu, mask) )
- raise_softirq(TIMER_SOFTIRQ);
-
cpuidle_wakeup_mwait(mask);
if ( !cpumask_empty(mask) )
cpumask_raise_softirq(mask, TIMER_SOFTIRQ);
}
-static void handle_hpet_broadcast(struct hpet_event_channel *ch)
+static void __hpet_interrupt(struct hpet_event_channel *ch)
{
- cpumask_t mask;
- s_time_t now, next_event;
- unsigned int cpu;
- unsigned long flags;
-
- spin_lock_irqsave(&ch->lock, flags);
-
-again:
- ch->next_event = STIME_MAX;
-
- spin_unlock_irqrestore(&ch->lock, flags);
-
- next_event = STIME_MAX;
- cpumask_clear(&mask);
- now = NOW();
-
- /* find all expired events */
- for_each_cpu(cpu, ch->cpumask)
- {
- s_time_t deadline;
-
- rmb();
- deadline = per_cpu(timer_deadline, cpu);
- rmb();
- if ( !cpumask_test_cpu(cpu, ch->cpumask) )
- continue;
-
- if ( deadline <= now )
- cpumask_set_cpu(cpu, &mask);
- else if ( deadline < next_event )
- next_event = deadline;
- }
-
- /* wakeup the cpus which have an expired event. */
- evt_do_broadcast(&mask);
-
- if ( next_event != STIME_MAX )
- {
- spin_lock_irqsave(&ch->lock, flags);
-
- if ( next_event < ch->next_event &&
- reprogram_hpet_evt_channel(ch, next_event, now, 0) )
- goto again;
-
- spin_unlock_irqrestore(&ch->lock, flags);
- }
+ __hpet_wake_cpus(ch->cpumask);
+ __program_hpet_time(ch, this_cpu(timer_deadline), NOW(), 1);
+ raise_softirq(TIMER_SOFTIRQ);
}
-static void hpet_interrupt_handler(int irq, void *data,
- struct cpu_user_regs *regs)
+static void hpet_interrupt_handler(struct cpu_user_regs *regs)
{
- struct hpet_event_channel *ch = (struct hpet_event_channel *)data;
-
- this_cpu(irq_count)--;
+ unsigned int cpu = smp_processor_id();
+ struct hpet_event_channel *ch = this_cpu(hpet_channel);
- if ( !ch->event_handler )
+ if ( ch )
{
- printk(XENLOG_WARNING "Spurious HPET timer interrupt on HPET timer %d\n", ch->idx);
- return;
+ spin_lock(&ch->lock);
+ if ( ch->cpu == cpu )
+ __hpet_interrupt(ch);
+ spin_unlock(&ch->lock);
}
- ch->event_handler(ch);
+ ack_APIC_irq();
}
-static void hpet_msi_unmask(struct irq_desc *desc)
+static void __hpet_msi_unmask(struct hpet_event_channel *ch)
{
u32 cfg;
- struct hpet_event_channel *ch = desc->action->dev_id;
cfg = hpet_read32(HPET_Tn_CFG(ch->idx));
cfg |= HPET_TN_ENABLE;
@@ -307,10 +262,9 @@ static void hpet_msi_unmask(struct irq_desc *desc)
ch->msi.msi_attrib.masked = 0;
}
-static void hpet_msi_mask(struct irq_desc *desc)
+static void __hpet_msi_mask(struct hpet_event_channel *ch)
{
u32 cfg;
- struct hpet_event_channel *ch = desc->action->dev_id;
cfg = hpet_read32(HPET_Tn_CFG(ch->idx));
cfg &= ~HPET_TN_ENABLE;
@@ -318,8 +272,10 @@ static void hpet_msi_mask(struct irq_desc *desc)
ch->msi.msi_attrib.masked = 1;
}
-static int hpet_msi_write(struct hpet_event_channel *ch, struct msi_msg *msg)
+static int __hpet_msi_write(struct hpet_event_channel *ch, struct msi_msg *msg)
{
+ u32 cfg;
+
ch->msi.msg = *msg;
if ( iommu_intremap )
@@ -330,80 +286,33 @@ static int hpet_msi_write(struct hpet_event_channel *ch, struct msi_msg *msg)
return rc;
}
+ cfg = hpet_read32(HPET_Tn_CFG(ch->idx));
+ if ( cfg & HPET_TN_ENABLE )
+ hpet_write32(cfg & ~HPET_TN_ENABLE, HPET_Tn_CFG(ch->idx));
+
hpet_write32(msg->data, HPET_Tn_ROUTE(ch->idx));
hpet_write32(msg->address_lo, HPET_Tn_ROUTE(ch->idx) + 4);
- return 0;
-}
-
-static void __maybe_unused
-hpet_msi_read(struct hpet_event_channel *ch, struct msi_msg *msg)
-{
- msg->data = hpet_read32(HPET_Tn_ROUTE(ch->idx));
- msg->address_lo = hpet_read32(HPET_Tn_ROUTE(ch->idx) + 4);
- msg->address_hi = MSI_ADDR_BASE_HI;
- if ( iommu_intremap )
- iommu_read_msi_from_ire(&ch->msi, msg);
-}
+ if ( cfg & HPET_TN_ENABLE )
+ hpet_write32(cfg, HPET_Tn_CFG(ch->idx));
-static unsigned int hpet_msi_startup(struct irq_desc *desc)
-{
- hpet_msi_unmask(desc);
return 0;
}
-#define hpet_msi_shutdown hpet_msi_mask
-
-static void hpet_msi_ack(struct irq_desc *desc)
-{
- irq_complete_move(desc);
- move_native_irq(desc);
- ack_APIC_irq();
-}
-
-static void hpet_msi_set_affinity(struct irq_desc *desc, const cpumask_t *mask)
-{
- struct hpet_event_channel *ch = desc->action->dev_id;
- struct msi_msg msg = ch->msi.msg;
-
- msg.dest32 = set_desc_affinity(desc, mask);
- if ( msg.dest32 == BAD_APICID )
- return;
-
- msg.data &= ~MSI_DATA_VECTOR_MASK;
- msg.data |= MSI_DATA_VECTOR(desc->arch.vector);
- msg.address_lo &= ~MSI_ADDR_DEST_ID_MASK;
- msg.address_lo |= MSI_ADDR_DEST_ID(msg.dest32);
- if ( msg.data != ch->msi.msg.data || msg.dest32 != ch->msi.msg.dest32 )
- hpet_msi_write(ch, &msg);
-}
-
-/*
- * IRQ Chip for MSI HPET Devices,
- */
-static hw_irq_controller hpet_msi_type = {
- .typename = "HPET-MSI",
- .startup = hpet_msi_startup,
- .shutdown = hpet_msi_shutdown,
- .enable = hpet_msi_unmask,
- .disable = hpet_msi_mask,
- .ack = hpet_msi_ack,
- .set_affinity = hpet_msi_set_affinity,
-};
-
-static int __hpet_setup_msi_irq(struct irq_desc *desc)
+static int __hpet_setup_msi(struct hpet_event_channel *ch)
{
struct msi_msg msg;
- msi_compose_msg(desc->arch.vector, desc->arch.cpu_mask, &msg);
- return hpet_msi_write(desc->action->dev_id, &msg);
+ ASSERT(ch->cpu != -1);
+
+ msi_compose_msg(hpet_vector, cpumask_of(ch->cpu), &msg);
+ return __hpet_msi_write(ch, &msg);
}
-static int __init hpet_setup_msi_irq(struct hpet_event_channel *ch)
+static int __init hpet_setup_msi(struct hpet_event_channel *ch)
{
int ret;
- u32 cfg = hpet_read32(HPET_Tn_CFG(ch->idx));
- irq_desc_t *desc = irq_to_desc(ch->msi.irq);
+ u32 cfg;
if ( iommu_intremap )
{
@@ -414,14 +323,12 @@ static int __init hpet_setup_msi_irq(struct hpet_event_channel *ch)
}
/* set HPET Tn as oneshot */
+ cfg = hpet_read32(HPET_Tn_CFG(ch->idx));
cfg &= ~(HPET_TN_LEVEL | HPET_TN_PERIODIC);
cfg |= HPET_TN_FSB | HPET_TN_32BIT;
hpet_write32(cfg, HPET_Tn_CFG(ch->idx));
- desc->handler = &hpet_msi_type;
- ret = request_irq(ch->msi.irq, hpet_interrupt_handler, "HPET", ch);
- if ( ret >= 0 )
- ret = __hpet_setup_msi_irq(desc);
+ ret = __hpet_setup_msi(ch);
if ( ret < 0 )
{
if ( iommu_intremap )
@@ -429,25 +336,6 @@ static int __init hpet_setup_msi_irq(struct hpet_event_channel *ch)
return ret;
}
- desc->msi_desc = &ch->msi;
-
- return 0;
-}
-
-static int __init hpet_assign_irq(struct hpet_event_channel *ch)
-{
- int irq;
-
- if ( (irq = create_irq(NUMA_NO_NODE)) < 0 )
- return irq;
-
- ch->msi.irq = irq;
- if ( hpet_setup_msi_irq(ch) )
- {
- destroy_irq(irq);
- return -EINVAL;
- }
-
return 0;
}
@@ -468,6 +356,8 @@ static void __init hpet_fsb_cap_lookup(void)
if ( !hpet_events )
return;
+ alloc_direct_apic_vector(&hpet_vector, hpet_interrupt_handler);
+
for ( i = 0; i < num_chs && num_hpets_used < nr_cpu_ids; i++ )
{
struct hpet_event_channel *ch = &hpet_events[num_hpets_used];
@@ -490,7 +380,7 @@ static void __init hpet_fsb_cap_lookup(void)
ch->flags = 0;
ch->idx = i;
- if ( hpet_assign_irq(ch) == 0 )
+ if ( hpet_setup_msi(ch) == 0 )
num_hpets_used++;
}
@@ -498,102 +388,28 @@ static void __init hpet_fsb_cap_lookup(void)
num_chs, num_hpets_used);
}
-static struct hpet_event_channel *hpet_get_channel(unsigned int cpu)
+/*
+ * Search for, and allocate, a free HPET channel. Returns a pointer to the
+ * channel, or NULL in the case that none were free. The caller is
+ * responsible for returning the channel to the free pool.
+ */
+static struct hpet_event_channel *hpet_get_free_channel(void)
{
- static unsigned int next_channel;
- unsigned int i, next;
- struct hpet_event_channel *ch;
-
- if ( num_hpets_used == 0 )
- return hpet_events;
+ unsigned ch, tries;
- if ( num_hpets_used >= nr_cpu_ids )
- return &hpet_events[cpu];
-
- do {
- next = next_channel;
- if ( (i = next + 1) == num_hpets_used )
- i = 0;
- } while ( cmpxchg(&next_channel, next, i) != next );
-
- /* try unused channel first */
- for ( i = next; i < next + num_hpets_used; i++ )
+ for ( tries = num_hpets_used; tries; --tries )
{
- ch = &hpet_events[i % num_hpets_used];
- if ( !test_and_set_bit(HPET_EVT_USED_BIT, &ch->flags) )
- {
- ch->cpu = cpu;
- return ch;
- }
- }
-
- /* share a in-use channel */
- ch = &hpet_events[next];
- if ( !test_and_set_bit(HPET_EVT_USED_BIT, &ch->flags) )
- ch->cpu = cpu;
-
- return ch;
-}
-
-static void set_channel_irq_affinity(struct hpet_event_channel *ch)
-{
- struct irq_desc *desc = irq_to_desc(ch->msi.irq);
-
- ASSERT(!local_irq_is_enabled());
- spin_lock(&desc->lock);
- hpet_msi_mask(desc);
- hpet_msi_set_affinity(desc, cpumask_of(ch->cpu));
- hpet_msi_unmask(desc);
- spin_unlock(&desc->lock);
-
- spin_unlock(&ch->lock);
-
- /* We may have missed an interrupt due to the temporary masking. */
- if ( ch->event_handler && ch->next_event < NOW() )
- ch->event_handler(ch);
-}
-
-static void hpet_attach_channel(unsigned int cpu,
- struct hpet_event_channel *ch)
-{
- ASSERT(!local_irq_is_enabled());
- spin_lock(&ch->lock);
-
- per_cpu(cpu_bc_channel, cpu) = ch;
-
- /* try to be the channel owner again while holding the lock */
- if ( !test_and_set_bit(HPET_EVT_USED_BIT, &ch->flags) )
- ch->cpu = cpu;
-
- if ( ch->cpu != cpu )
- spin_unlock(&ch->lock);
- else
- set_channel_irq_affinity(ch);
-}
-
-static void hpet_detach_channel(unsigned int cpu,
- struct hpet_event_channel *ch)
-{
- spin_lock_irq(&ch->lock);
-
- ASSERT(ch == per_cpu(cpu_bc_channel, cpu));
+ if ( (ch = ffs(free_channels)) == 0 )
+ break;
- per_cpu(cpu_bc_channel, cpu) = NULL;
+ --ch;
+ ASSERT(ch < num_hpets_used);
- if ( cpu != ch->cpu )
- spin_unlock_irq(&ch->lock);
- else if ( cpumask_empty(ch->cpumask) )
- {
- ch->cpu = -1;
- clear_bit(HPET_EVT_USED_BIT, &ch->flags);
- spin_unlock_irq(&ch->lock);
- }
- else
- {
- ch->cpu = cpumask_first(ch->cpumask);
- set_channel_irq_affinity(ch);
- local_irq_enable();
+ if ( test_and_clear_bit(ch, &free_channels) )
+ return &hpet_events[ch];
}
+
+ return NULL;
}
#include <asm/mc146818rtc.h>
@@ -630,6 +446,7 @@ void __init hpet_broadcast_init(void)
/* Stop HPET legacy interrupts */
cfg &= ~HPET_CFG_LEGACY;
n = num_hpets_used;
+ free_channels = (1U << n) - 1;
}
else
{
@@ -673,9 +490,8 @@ void __init hpet_broadcast_init(void)
hpet_events[i].shift = 32;
hpet_events[i].next_event = STIME_MAX;
spin_lock_init(&hpet_events[i].lock);
- wmb();
- hpet_events[i].event_handler = handle_hpet_broadcast;
+ hpet_events[1].msi.irq = -1;
hpet_events[i].msi.msi_attrib.maskbit = 1;
hpet_events[i].msi.msi_attrib.pos = MSI_TYPE_HPET;
}
@@ -715,9 +531,6 @@ void hpet_broadcast_resume(void)
for ( i = 0; i < n; i++ )
{
- if ( hpet_events[i].msi.irq >= 0 )
- __hpet_setup_msi_irq(irq_to_desc(hpet_events[i].msi.irq));
-
/* set HPET Tn as oneshot */
cfg = hpet_read32(HPET_Tn_CFG(hpet_events[i].idx));
cfg &= ~(HPET_TN_LEVEL | HPET_TN_PERIODIC);
@@ -725,6 +538,7 @@ void hpet_broadcast_resume(void)
if ( !(hpet_events[i].flags & HPET_EVT_LEGACY) )
cfg |= HPET_TN_FSB;
hpet_write32(cfg, HPET_Tn_CFG(hpet_events[i].idx));
+ __hpet_setup_msi(&hpet_events[i]);
hpet_events[i].next_event = STIME_MAX;
}
@@ -760,50 +574,116 @@ void hpet_disable_legacy_broadcast(void)
void hpet_broadcast_enter(void)
{
unsigned int cpu = smp_processor_id();
- struct hpet_event_channel *ch = per_cpu(cpu_bc_channel, cpu);
+ struct hpet_event_channel *ch = this_cpu(hpet_channel);
+ s_time_t deadline = this_cpu(timer_deadline);
+
+ ASSERT(!local_irq_is_enabled());
+ ASSERT(ch == NULL);
- if ( per_cpu(timer_deadline, cpu) == 0 )
+ if ( deadline == 0 )
return;
- if ( !ch )
- ch = hpet_get_channel(cpu);
+ ch = hpet_get_free_channel();
- ASSERT(!local_irq_is_enabled());
+ if ( ch )
+ {
+ /*
+ * It is not intended to be possible to assign a legacy channel to a
+ * cpu, so getting here should be impossible.
+ */
+ ASSERT( !(ch->flags & HPET_EVT_LEGACY) );
- if ( !(ch->flags & HPET_EVT_LEGACY) )
- hpet_attach_channel(cpu, ch);
+ spin_lock(&ch->lock);
+
+ this_cpu(hpet_channel) = ch;
+ ch->cpu = cpu;
+ cpumask_set_cpu(cpu, ch->cpumask);
+
+ __hpet_setup_msi(ch);
+ __program_hpet_time(ch, deadline, NOW(), 1);
+ __hpet_msi_unmask(ch);
+
+ spin_unlock(&ch->lock);
+
+ }
+ else
+ {
+ /* TODO - this seems very ugly */
+ unsigned i;
+
+ for ( i = 0; i < num_hpets_used; ++i )
+ {
+ ch = &hpet_events[i];
+ spin_lock(&ch->lock);
+
+ if ( ch->cpu == -1 )
+ goto continue_search;
+
+ if ( ch->next_event >= deadline - MICROSECS(50) &&
+ ch->next_event <= deadline )
+ break;
+
+ continue_search:
+ spin_unlock(&ch->lock);
+ ch = NULL;
+ }
+
+ if ( ch )
+ {
+ cpumask_set_cpu(cpu, ch->cpumask);
+ this_cpu(hpet_channel) = ch;
+ spin_unlock(&ch->lock);
+ }
+ else
+ this_cpu(timer_deadline) = NOW();
+
+ }
/* Disable LAPIC timer interrupts. */
disable_APIC_timer();
- cpumask_set_cpu(cpu, ch->cpumask);
-
- spin_lock(&ch->lock);
- /* reprogram if current cpu expire time is nearer */
- if ( per_cpu(timer_deadline, cpu) < ch->next_event )
- reprogram_hpet_evt_channel(ch, per_cpu(timer_deadline, cpu), NOW(), 1);
- spin_unlock(&ch->lock);
}
void hpet_broadcast_exit(void)
{
unsigned int cpu = smp_processor_id();
- struct hpet_event_channel *ch = per_cpu(cpu_bc_channel, cpu);
+ struct hpet_event_channel *ch = this_cpu(hpet_channel);
- if ( per_cpu(timer_deadline, cpu) == 0 )
- return;
+ ASSERT(local_irq_is_enabled());
if ( !ch )
- ch = hpet_get_channel(cpu);
+ return;
- /* Reprogram the deadline; trigger timer work now if it has passed. */
- enable_APIC_timer();
- if ( !reprogram_timer(per_cpu(timer_deadline, cpu)) )
- raise_softirq(TIMER_SOFTIRQ);
+ if ( this_cpu(timer_deadline) == 0 )
+ return;
+
+ spin_lock_irq(&ch->lock);
cpumask_clear_cpu(cpu, ch->cpumask);
- if ( !(ch->flags & HPET_EVT_LEGACY) )
- hpet_detach_channel(cpu, ch);
+ /* If we own the channel, detach it */
+ if ( ch->cpu == cpu )
+ {
+ __hpet_msi_mask(ch);
+
+ /*
+ * It is not intended to be possible to assign a legacy channel to a
+ * cpu, so getting here should be impossible.
+ */
+ ASSERT( !(ch->flags & HPET_EVT_LEGACY) );
+
+ __hpet_wake_cpus(ch->cpumask);
+ ch->cpu = -1;
+ set_bit(ch->idx, &free_channels);
+ }
+
+ this_cpu(hpet_channel) = NULL;
+
+ spin_unlock_irq(&ch->lock);
+
+ /* Reprogram the deadline; trigger timer work now if it has passed. */
+ enable_APIC_timer();
+ if ( !reprogram_timer(this_cpu(timer_deadline)) )
+ raise_softirq(TIMER_SOFTIRQ);
}
int hpet_broadcast_is_available(void)
@@ -820,7 +700,12 @@ int hpet_legacy_irq_tick(void)
(hpet_events->flags & (HPET_EVT_DISABLE|HPET_EVT_LEGACY)) !=
HPET_EVT_LEGACY )
return 0;
- hpet_events->event_handler(hpet_events);
+
+ /* TODO - Does this really make sense for legacy ticks ? */
+ spin_lock_irq(&hpet_events->lock);
+ __hpet_interrupt(hpet_events);
+ spin_unlock_irq(&hpet_events->lock);
+
return 1;
}
--
1.7.10.4
^ permalink raw reply related [flat|nested] 21+ messages in thread* Re: [RFC 8/8] x86/hpet: Use singe apic vector rather than irq_descs for HPET interrupts
2013-11-04 18:54 ` [RFC 8/8] x86/hpet: Use singe apic vector rather than irq_descs for HPET interrupts Andrew Cooper
@ 2013-11-05 15:10 ` Jan Beulich
0 siblings, 0 replies; 21+ messages in thread
From: Jan Beulich @ 2013-11-05 15:10 UTC (permalink / raw)
To: Andrew Cooper; +Cc: xen-devel, Keir Fraser
>>> On 04.11.13 at 19:54, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
> This involves rewriting most of the MSI related HPET code, and as a result
> this patch looks very complicated. It is probably best viewed as an end
> result, with the following notes explaining what is going on.
Problem is - it likely won't apply without the earlier changes in the
series (I admittedly didn't try); patch 6 - which I suggested to drop -
seems like the most likely candidate for conflicts. Hence looking at
the end result makes sense only once you re-posted. I'll therefore
defer reviewing till then. The high level description sounds good in
any event.
Jan
^ permalink raw reply [flat|nested] 21+ messages in thread