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

[Jason Andryuk <jason.andryuk@xxxxxxx>]

On 2024-05-14 03:43, Jan Beulich wrote:
> 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).
I had to look up all these bit values, so I think it's nicer with 
#defines-s.  Particularly, using PIT_MODE0_16BIT for the programming and 
checking shows the relationship.  I wasn't looking to make more work for 
you.  This function is self-contained, so just using them here for the 
time being seems reasonable.
> > > +
> > > +    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).
Sorry, these were my notes as I was interpreting the bits.  I should 
have removed them from the email before sending as they aren't 
actionable comments.  Read back was in reference to writing (3 << 6) to 
the mode - not the action of read backing back the value.
> > > +
> > > +            /* 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).
Ah, yes.  Your response had slipped my mind.

> > 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).
Indeed, it was an AMD system, but my sample size is 1.

I didn't realize this was motivated by aliasing being common on Intel 
chipsets.  I think that would be useful to include in the commit messages.
Thanks,
Jason