http://defect.opensolaris.org/bz/show_bug.cgi?id=6499
--- Comment #8 from Bill Holler <bill.holler at sun.com> 2009-02-13 19:03:31
---
(In reply to comment #2)
> More feedback:
>
> cpu_idle.c:
> * start the LAPIC Timer agin before leaving this function.
> again
done
> hpet_acpi.c:
>
> hpet_init_proxys()
> proxies
>
> As we discussed on the phone, something in a "top-of-file" comment about
> 1) HPET interrupt doesn't cause interrupt on the CPU, but rather "wake from
> HLT", until the "code after HLT" reenables IF, and 2) one cpu is tasked with
> managing HPET wakeups for all C-stated CPUs
done. Big block comment placed in hpet_acpi.c which includes this info.
> Presumably apic_regops.c should not be in this wad?
This file will be removed from the changeset.
> cpuid.c:
>
> 1) we really weren't looking for CPUID_TSC_INVARIANCE before? Doesn't Mark
> look for that for p-state support?
No. C-state and P-state Invariance are two seperate CPU properties.
CPUs can have any combination of these properties.
P-states used family/model to determine P-state invariant TSC.
For example Intel Penryn processors have P-state invariant TSC and
C-state variant TSC.
The define what changed to include CSTATE in the name, so this will be
more clear.
> 2) TSC_INVARIANCE applies to C-, P-, and T- state TSC invariance; the comment
> should note that
As above: C-state and P-state Invariance are two seperate CPU properties.
Comment changed to be more C-state specific.
> 3) why is the #define here, and not in, say, x86_archext.h?
done. Moved into x86_archext.h
> cpu_idle.c
>
> 211 * Set our mcpu_mwait here, so we can tell if anyone trys to
> tries
done
> 223 * If this CPU is online, and there's multiple CPUs
> there are
done
> 224 * in the system, then we should notate our halting
> note
done
> 238 * (if anyone) should be awoken. We therefore need to first
> I would say "awakened", but I see both in online dictionaries. I dunno.
done
> 274 * start the LAPIC Timer agin before leaving this function.
> "again" again. You might want to global-search for 'agin'.
done
> 350 /* DTRACE_PROBE1(idle__state__transition, uint_t,
> (uint_t)cs_type); */
> Is this an old version of the probe, or?...(ah, see mp_machdep.c below)
done. Bringup code deleted.
> mp_machdep.c:
> I don't these are in your wad, but I notice
> * Set our mcpu_mwait here, so we can tell if anyone trys to
> tries
done
> * in the system, then we should notate our halting
> note
done
> * (if anyone) should be awoken. We therefore need to first
> awakened
done
> I sense a theme :)
>
> In your changes:
>
> + * XXX: for now this in not guarded from xpv.
>
> Why not? and should it be before putback?
done. Moved into guard. This was discussed with XEN team a while back.
Guarding C-state implementation from xpv is prefered.
> XXX_highbit looks vestigial; presumably lint catches this
Done. Bringup code deleted.
> + * XXX: DTRACE_PROBE1 did not work in acpi_cpu_idle which is linked into
>
> ah, now I understand line 350 in cpu_idle.c above
done. Removed.
> Can cstate_wakeup() take over from cpu_wakeup()?
We do not know what AMD will do with C-state support.
-bash-3.2$
-bash-3.2$ vi /tmp
-bash-3.2$ vi /tmp/t
-bash-3.2$ more /tmp/t
(In reply to comment #4)
> More feedback:
>
> 365 * The HPET Spec claims the Main Counter is initially halted and
> zeroed.
> 366 * This is true on most test machines....
> 367 * A Dell 690 was observed to have an initially running Main
> Counter.
> 368 * ENABLE_CNF bit was originally set and the main counter was
> running.
>
> seems unnecessary level of detail. Moderately interesting, but irrelevant.
done. removed.
> 377 /*
> 378 * Read main counter 10 times in a loop.
> 379 */
> is one of those "obvious comments that don't help". The comment should
> be *why* are we reading the counter ten times, and what happens next.
> The code after that is quite complex, and really needs explication.
done. Bringup code removed. Retained some debugging code, but it is
much simpler now.
> 418 * The HPET spec allows for MSI. In the future MSI may be prefered.
> preferred
done
> hpet_allocate_proxy says that it sets up the timer before programming the
> I/O APIC, but where is the I/O APIC programmed?...is it not before that,
> in hpet_install_interrupt_handler->add_avintr? Moreover, "allocate" here
> includes initing and adding an interrupt handler. It sounds more like "setup"
> or "init" or something to me.
done. name changed.
The I/O APIC is programmed later in boot when APIC initializes it.
> Do we need to do anything to protect against races in
> hpet_uninstall_interrupt_handler? What if an interrupt is active when
> it's called, or just after?
No. The only place hpet_uninstall_interrupt_handler() is called is after
CPR if we fail to start the Main Counter. The HPET will not generate
interrupts until after the Main Counter has been started.
> 687 * Guaranty the high 64-bits do not increment after reading lower
> "Guarantee". Guaranty is a noun.
>
> On this: why not #ifdef amd64 for a true-64-bit read?
We discussed this in email. done. Bus to HPET does not guarantee
atomic 64-bit reads.
> Similar comments for the one-line worker read/write routines; why all
> the way into oneline functions?...(710-727, 739-744)
A design decission.
HPET register reads are extremely slow compared to the overhead
of a function call.
> 851 static int
> 852 hpet_timer_available(uint32_t allocated_timers, uint32_t n)
> 853 {
> 854 return (~(allocated_timers & (1 << n)));
> 855 }
> That's not right; when allocated, you'll have a mask with one bit set,
> and ~ also has bits set. You probably want '!', but I'd use "== 0" for
> clarity anyway.
done.
> 880 new_conf = hpet_read_timer_N_config(hip, timer_n);
> 881
> 882 /*
> 883 * Verify INT_ROUTE_CNF was set as expected:
> 884 */
> 885 return ((conf & new_conf) == conf);
> Is there something specific to HPET that it won't accept config bits here,
> or in general? If so, why is this the only verify?..
done. Removed extremely paranoid bringup code.
> 893 * guaranty the interrupt is handled after this function returns.
> guarantee
done
> 949 * Timer registers including main counter may not be preserved through
> ACPI
> "including the main counter" needs () or ,, to set it off
done
> 955 * XXX: Future projects beware the HPET Main Counter is not saved/restore
d
> for
> 956 * Deep C-state. That is not needed to use the HPET as an interval timer
.
>
> First, "beware: ", I think. Second: the HPET doesn't stop during deep c-state
,
> does it? If not, why is this something to beware of?
done. Comment changed for clarity. HPET Spec does not define the Main Counter
during CPR, and we do not rely on either implementation.
> 958 * Note that pause_cpus() called later by the CPR framework ensures all
> CPUs
> "called later by the CPR framework" needs either () or ,,
done
> 961 * It is safe to leave the HPET running as the system suspends.
> but, 1), above, we say that HPET state isn't saved on s3-5, and, 2),
> it sure looks like the routine is stopping the HPET...so ???
done. Comment updated.
> What makes sure the CPUs have processed their poke_cpus() before
> continuing past hpet_expire_all(), or why isn't it necessary?
Comment changed to explain why it is not necessary.
> 992 /*
> 993 * The ACPI and HPET specifications do not mention a need
> to
> 994 * re-read ACPI tables nor re-map HPET physical memory
> during
> 995 * resume.
> 996 */
>
> Since mapping of the tables is only for the OS to be able to refer to
> physical addresses, it wouldn't (unless you mean "the tables don't change
> position in physical memory"), but there's plenty that depends on
> the state of ACPI tables being consistent on S3 transition; mentioning it
> here isn't the right place, I wouldn't think.
done. Removed comment.
> 1029 cmn_err(CE_NOTE, "!hpet resume main counter failed");
> 1047 cmn_err(CE_WARN, "!hpet_acpi: timer setup failed.");
> Both of these are in a routine named "hpet_resume", and should probably
> say so.
done
> 1090 * The order of these opperations is important to avoid
> operations
done
> if we're going to use B_TRUE/B_FALSE, why not for proxy_installed as well?
done
> 1091 * lost wakeups: Set a flag to refuse all future LAPIC
> Timer
> 1092 * proxy requests, then waking up all CPUs from deep
> C-state,
> 1093 * and finally disable the HPET interrupt generating
> timer.
> tenses: "set, waking, disable". Probably "waking" should be "wake". Also
> "interrupt-generating" needs a hyphen.
done
> 1098 etc.: what happens if we never get the lock? This CPU is screwed...
> with no warning. counter and message?...
done. Code added to kick us out of these spin loops with cmn_err messages.
Stress tested change.
> 1124 * Interrupt Service Routine for HPET I/O APIC generated interrupts.
> 1125 * Used to wakeup CPUs from Deep C-state when there Local APIC Timer
> stops.
>
> "I/O-APIC-generated", and "their". And I wouldn't capitalize Service Routine
> or Local [APIC] Timer".
done
> 1137 * We are using a level triggered interrupt.
> level-triggered
done
> 1158 * CPUs in deep c-states do not enable interrupts until after
> 1159 * performing idle cleanup which includes descheduling itself fro
m
>
> number. Either "CPU...does" or "themselves"
done
> So, I don't get this: the HPET interrupt *is* what moves us out of
> deep c-state, right? What does the HPET do if not cause an interrupt to
> awake from c-state?
done. Discussed in email etc.
--
Configure bugmail: http://defect.opensolaris.org/bz/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are on the CC list for the bug.
