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 <jbeul...@suse.com>
> ---
> 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.
