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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 26.10.2023 17:13, Roger Pau Monné wrote:
> On Thu, Oct 26, 2023 at 05:10:41PM +0200, Jan Beulich wrote:
>> On 26.10.2023 15:57, Roger Pau Monné wrote:
>>> On Thu, Oct 26, 2023 at 02:31:27PM +0200, Jan Beulich wrote:
>>>> 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.
>>>
>>> If you want to make this more fine grained, or even allow the user to
>>> provide custom masks that's all fine, but there's already
>>> dom0_ioports_disable that allows disabling a list of IO port ranges.
>>>
>>> What I would require is a way to avoid all the probing, so that we
>>> could return to the previous behavior.
>>>
>>>>>> --- 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.
>>>
>>> I would likely make it based on iterations, could you get some figures
>>> on how many iterations it takes for the bit to be clear?
>>>
>>> I would think something like 1000 should be enough, but really have no
>>> idea.
>>
>> Except that how long a given number of iterations takes is unknown. 1000
>> may be enough today or on the systems we test, but may not be tomorrow
>> or on other peoples' systems. Hence why I'm hesitant ...
> 
> Hm, but getting stuck in a loop here can't be good either.

I certainly understand that. The command line option I've just added in
a prereq patch would allow bypassing the probing, but of course I agree
that we want to avoid hanging here nevertheless (if we can).

>  Let's do
> it time wise if you prefer, 1s should be more than enough I would
> think.

Yet time-wise is also problematic ahead of us having calibrated clocks.
And using the PIT itself (which runs at a known frequency) doesn't look
to be a good idea when we mean to deal with the case of a broken PIT,
or none at all.

Jan