Re: [PATCH v2 3/3] x86: detect PIT aliasing on ports other than 0x4[0-3]

[Jan Beulich <jbeulich@xxxxxxxx>]

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).

>> +
>> +    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).

>> +
>> +            /* 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).

> 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).

Jan