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

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

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.

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

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

> 
> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
> ---
> If Xen was running on top of another instance of itself (in HVM mode,
> not PVH, i.e. not as a shim), I'm afraid our vPIT logic would not allow
> the "Try to further make sure ..." check to pass in the Xen running on
> top: We don't respect the gate bit being clear when handling counter
> reads. (There are more unhandled [and unmentioned as being so] aspects
> of PIT behavior though, yet it's unclear in how far addressing at least
> some of them would be useful.)
> 
> --- a/xen/arch/x86/dom0_build.c
> +++ b/xen/arch/x86/dom0_build.c
> @@ -504,7 +504,11 @@ int __init dom0_setup_permissions(struct
>      }
>  
>      /* Interval Timer (PIT). */
> -    rc |= ioports_deny_access(d, 0x40, 0x43);
> +    for ( offs = 0, i = pit_alias_mask & -pit_alias_mask ?: 4;
> +          offs <= pit_alias_mask; offs += i )
> +        if ( !(offs & ~pit_alias_mask) )
> +            rc |= ioports_deny_access(d, 0x40 + offs, 0x43 + offs);
> +
>      /* PIT Channel 2 / PC Speaker Control. */
>      rc |= ioports_deny_access(d, 0x61, 0x61);
>  
> --- a/xen/arch/x86/include/asm/setup.h
> +++ b/xen/arch/x86/include/asm/setup.h
> @@ -53,6 +53,7 @@ extern unsigned long highmem_start;
>  #endif
>  
>  extern unsigned int pic_alias_mask;
> +extern unsigned int pit_alias_mask;
>  
>  extern int8_t opt_smt;
>  
> --- 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...

> +
> +        /*
> +         * 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?

Thanks, Roger.