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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 26.10.2023 12:25, Roger Pau Monné wrote:
> On Thu, May 11, 2023 at 02:07:12PM +0200, Jan Beulich wrote:
>> ... in order to also deny Dom0 access through the alias ports. Without
>> this it is only giving the impression of denying access to PIT. Unlike
>> for CMOS/RTC, do detection pretty early, to avoid disturbing normal
>> operation later on (even if typically we won't use much of the PIT).
>>
>> Like for CMOS/RTC a fundamental assumption of the probing is that reads
>> from the probed alias port won't have side effects (beyond such that PIT
>> reads have anyway) in case it does not alias the PIT's.
>>
>> At to the port 0x61 accesses: Unlike other accesses we do, this masks
>> off the top four bits (in addition to the bottom two ones), following
>> Intel chipset documentation saying that these (read-only) bits should
>> only be written with zero.
> 
> As said in previous patches, I think this is likely too much risk for
> little benefit.  I understand the desire to uniformly deny access to
> any ports that allow interaction with devices in use by Xen (or not
> allowed to be used by dom0), but there's certainly a risk in
> configuring such devices in the way that we do by finding a register
> that can be read and written to.
> 
> I think if anything this alias detection should have a command line
> option in order to disable it.

Well, we could have command line options (for each of the RTC/CMOS,
PIC, and PIT probing allowing the alias masks to be specified (so we
don't need to probe). A value of 1 would uniformly mean "no probing,
no aliases" (as all three decode the low bit, so aliasing can happen
there). We could further make the default of these variables (yes/no,
no actual mask values of course) controllable by a Kconfig setting.

> Do you also have figures if the greatly increased amount of accesses
> add a noticeable boot time regression?

I didn't measure anything, but nothing's really noticeable, no. And
the number of accesses in all three of CMOS/RTC, PIC, and PIT is of
roughly the same order, I think (and the RTC/CMOS one has been in
for a while).

> We are usually cautious is not performing more accesses than strictly
> required.

Sadly "strictly required" is an uncertain quantity here. The more
checking we do, the less likely that we'd identify a false positive
case of aliasing.

>> --- a/xen/arch/x86/time.c
>> +++ b/xen/arch/x86/time.c
>> @@ -425,6 +425,69 @@ 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;
>> +
>> +    /*
>> +     * 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. */
>> +
>> +    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);
>> +
>> +            /* Try to make sure we're actually having a PIT here. */
>> +            val2 = inb(PIT_CH2);
>> +            if ( (val2 & ~(3 << 6)) != ((3 << 4) | (0 << 1)) )
>> +                return;
>> +        } while ( val2 & (1 << 6) );
> 
> We should have some kind of timeout here, just in case...

Hmm, I indeed did consider the need for a timeout here. With what
we've done up to here we already assume a functioning PIT, verifying
simply as we go. The issue with truly using some form of timeout is
the determination of how long to wait at most.

>> +
>> +        /*
>> +         * Try to further make sure we're actually having a PIT here.
>> +         *
>> +         * NB: Deliberately |, not ||, as we always want both reads.
>> +         */
>> +        val2 = inb(PIT_CH2);
>> +        if ( (val2 ^ val) | (inb(PIT_CH2) ^ val ^ 0xff) )
>> +            return;
>> +
>> +        for ( offs = mask & -mask; offs <= mask; offs <<= 1 )
>> +        {
>> +            if ( !(mask & offs) )
>> +                continue;
>> +            val2 = inb(PIT_CH2 + offs);
>> +            if ( (val2 ^ val) | (inb(PIT_CH2 + offs) ^ val ^ 0xff) )
>> +                mask &= ~offs;
>> +        }
>> +    } while ( mask && (val += 0x0b) );  /* Arbitrary uneven number. */
>> +
>> +    if ( mask )
>> +    {
>> +        dprintk(XENLOG_INFO, "PIT aliasing mask: %02x\n", mask);
>> +        pit_alias_mask = mask;
>> +    }
> 
> Is it fine to leave counting disabled for channel 2?
> 
> Shouldn't we restore the previous gate value?

See init_pit(), which also doesn't restore anything. The system is under
our control, and we have no other use for channel 2. (I really had
restore logic here initially, but then dropped it for said reason.)

Jan