On 2024-05-14 03:43, Jan Beulich wrote:
On 10.05.2024 19:40, Jason Andryuk wrote:
On 2023-12-18 09:48, Jan Beulich wrote:
--- a/xen/arch/x86/time.c
+++ b/xen/arch/x86/time.c
@@ -425,6 +425,72 @@ static struct platform_timesource __init
.resume = resume_pit,
};
+unsigned int __initdata pit_alias_mask;
+
+static void __init probe_pit_alias(void)
+{
+ unsigned int mask = 0x1c;
+ uint8_t val = 0;
+
+ if ( !opt_probe_port_aliases )
+ return;
+
+ /*
+ * Use channel 2 in mode 0 for probing. In this mode even a non-initial
+ * count is loaded independent of counting being / becoming enabled. Thus
+ * we have a 16-bit value fully under our control, to write and then check
+ * whether we can also read it back unaltered.
+ */
+
+ /* Turn off speaker output and disable channel 2 counting. */
+ outb(inb(0x61) & 0x0c, 0x61);
+
+ outb((2 << 6) | (3 << 4) | (0 << 1), PIT_MODE); /* Mode 0, LSB/MSB. */
Channel 2, Lobyte/Hibyte, 0b000 Mode 0, (Binary)
#define PIT_MODE_CH2 (2 << 6)
#define PIT_MODE0_16BIT ((3 << 4) | (0 << 1))
outb(PIT_MODE_CH2 | PIT_MODE0_16BIT, PIT_MODE);
Hmm. I can certainly see the value of introducing such #define-s, but then
while doing so one ought to also adjust other code using constants as done
here (for consistency).
I had to look up all these bit values, so I think it's nicer with
#defines-s. Particularly, using PIT_MODE0_16BIT for the programming and
checking shows the relationship. I wasn't looking to make more work for
you. This function is self-contained, so just using them here for the
time being seems reasonable.
+
+ do {
+ uint8_t val2;
+ unsigned int offs;
+
+ outb(val, PIT_CH2);
+ outb(val ^ 0xff, PIT_CH2);
+
+ /* Wait for the Null Count bit to clear. */
+ do {
+ /* Latch status. */
+ outb((3 << 6) | (1 << 5) | (1 << 3), PIT_MODE);
Read-back, Latch status, read back timer channel 2
Was this meant as a request to extend the comment? If so, not quite,
as the line doesn't include any read-back. If not, I'm in trouble seeing
what you mean to tell me here (somewhat similar also for the first line
of your earlier comment still visible in context above).
Sorry, these were my notes as I was interpreting the bits. I should
have removed them from the email before sending as they aren't
actionable comments. Read back was in reference to writing (3 << 6) to
the mode - not the action of read backing back the value.
+
+ /* Try to make sure we're actually having a PIT here. */
+ val2 = inb(PIT_CH2);
+ if ( (val2 & ~(3 << 6)) != ((3 << 4) | (0 << 1)) )
if ( (val2 & PIT_RB_MASK) != PIT_MODE0_16BIT )
I think particularly a define for PIT_MODE0_16BIT would be helpful to
show what is expected to be the same.
+ return;
+ } while ( val2 & (1 << 6) );
I think Roger might have mentioned on an earlier version - would it make
sense to have a counter to prevent looping forever?
Well, as before: The issue with bounding such loops is that the bound is
going to be entirely arbitrary (and hence easily too large / too small).
Ah, yes. Your response had slipped my mind.
Also, FYI, I tested the series. My test machine didn't show any aliasing.
That likely was an AMD one then? It's only Intel chipsets I've seen aliasing
on so far, but there it's (almost) all of them (with newer data sheets even
stating that behavior). We could, beyond shim, make the option default in
patch 1 be "false" for systems with AMD CPUs (on the assumption that those
wouldn't have Intel chipsets).
Indeed, it was an AMD system, but my sample size is 1.
I didn't realize this was motivated by aliasing being common on Intel
chipsets. I think that would be useful to include in the commit messages.
Thanks,
Jason
