| Message ID | 08b293b3-e04a-9230-e7ec-dd7da9d198b1@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | x86/time: minor adjustments to init_pit() | expand |
On 17/01/2022 10:36, Jan Beulich wrote: > For one, "using_pit" shouldn't be set ahead of the function's last > (for now: only) error path. Otherwise "clocksource=pit" on the command > line can lead to misbehavior when actually taking that error path. > > And then make an implicit assumption explicit: CALIBRATE_FRAC cannot, > for example, simply be changed to 10. The way init_pit() works, the > upper bound on the calibration period is about 54ms. > > Signed-off-by: Jan Beulich <jbeulich@suse.com> Acked-by: Andrew Cooper <andrew.cooper3@citrix.com> > --- > Really I've noticed this while considering what would happen if someone > specified "clocksource=pit" on the shim's command line. Unlike "hpet" > and "acpi", "pit" presently wouldn't be (explicitly) ignored. While, > aiui, right now the only error path would be taken (due to port 0x61 > reads being supposed to get back 0xff), I don't think we can build on > that longer term: Seeing what we use port 0x61 for in traps.c, I think > sooner or later we will need to have some form of emulation for it. Such > emulation is then not unlikely to continuously report 0 in the bit in > question. That would leed to an infinite loop here. If we're not already doing it, pv shim really ought to set the FADT hardware reduced bits. There should be no need to depend on heuristics around ~0. I do suspect that the emulation for port 0x61 is obsolete enough for us to consider dropping. ~Andrew
On 20.01.2022 17:17, Andrew Cooper wrote: > On 17/01/2022 10:36, Jan Beulich wrote: >> For one, "using_pit" shouldn't be set ahead of the function's last >> (for now: only) error path. Otherwise "clocksource=pit" on the command >> line can lead to misbehavior when actually taking that error path. >> >> And then make an implicit assumption explicit: CALIBRATE_FRAC cannot, >> for example, simply be changed to 10. The way init_pit() works, the >> upper bound on the calibration period is about 54ms. >> >> Signed-off-by: Jan Beulich <jbeulich@suse.com> > > Acked-by: Andrew Cooper <andrew.cooper3@citrix.com> Thanks. >> --- >> Really I've noticed this while considering what would happen if someone >> specified "clocksource=pit" on the shim's command line. Unlike "hpet" >> and "acpi", "pit" presently wouldn't be (explicitly) ignored. While, >> aiui, right now the only error path would be taken (due to port 0x61 >> reads being supposed to get back 0xff), I don't think we can build on >> that longer term: Seeing what we use port 0x61 for in traps.c, I think >> sooner or later we will need to have some form of emulation for it. Such >> emulation is then not unlikely to continuously report 0 in the bit in >> question. That would leed to an infinite loop here. > > If we're not already doing it, pv shim really ought to set the FADT > hardware reduced bits. There should be no need to depend on heuristics > around ~0. Before forcing this flag onto "others", I guess we'd better first start properly honoring this mode ourselves? Outside of ACPICA code there has been only a single use of this FADT bit so far ... > I do suspect that the emulation for port 0x61 is obsolete enough for us > to consider dropping. Well, as always - I'm hesitant to drop code which we don't know for sure cannot possibly be of use to anyone anymore, and which also isn't known to cause (significant) harm. Jan
On Thu, Jan 20, 2022 at 04:17:37PM +0000, Andrew Cooper wrote: > On 17/01/2022 10:36, Jan Beulich wrote: > > For one, "using_pit" shouldn't be set ahead of the function's last > > (for now: only) error path. Otherwise "clocksource=pit" on the command > > line can lead to misbehavior when actually taking that error path. > > > > And then make an implicit assumption explicit: CALIBRATE_FRAC cannot, > > for example, simply be changed to 10. The way init_pit() works, the > > upper bound on the calibration period is about 54ms. > > > > Signed-off-by: Jan Beulich <jbeulich@suse.com> > > Acked-by: Andrew Cooper <andrew.cooper3@citrix.com> > > > --- > > Really I've noticed this while considering what would happen if someone > > specified "clocksource=pit" on the shim's command line. Unlike "hpet" > > and "acpi", "pit" presently wouldn't be (explicitly) ignored. While, > > aiui, right now the only error path would be taken (due to port 0x61 > > reads being supposed to get back 0xff), I don't think we can build on > > that longer term: Seeing what we use port 0x61 for in traps.c, I think > > sooner or later we will need to have some form of emulation for it. Such > > emulation is then not unlikely to continuously report 0 in the bit in > > question. That would leed to an infinite loop here. > > If we're not already doing it, pv shim really ought to set the FADT > hardware reduced bits. There should be no need to depend on heuristics > around ~0. We have talked about setting the hardware reduced flag for PVH, it's however tricky. For once hardware-reduced ACPI mandates the usage of UEFI firmware, which we don't yet fully support for PVH. And then we cannot set that flag for a PVH dom0, because we use a mix of crafted and native ACPI tables, so we risk that setting the bit creates incompatibilities with the native tables we expose. Roger.
--- a/xen/arch/x86/time.c +++ b/xen/arch/x86/time.c @@ -333,8 +333,6 @@ static s64 __init init_pit(struct platfo u64 start, end; unsigned long count; - using_pit = true; - /* Set the Gate high, disable speaker. */ outb((portb & ~0x02) | 0x01, 0x61); @@ -344,6 +342,7 @@ static s64 __init init_pit(struct platfo * (LSB and MSB) to begin countdown. */ #define CALIBRATE_LATCH CALIBRATE_VALUE(CLOCK_TICK_RATE) + BUILD_BUG_ON(CALIBRATE_LATCH >> 16); outb(0xb0, PIT_MODE); /* binary, mode 0, LSB/MSB, Ch 2 */ outb(CALIBRATE_LATCH & 0xff, PIT_CH2); /* LSB of count */ outb(CALIBRATE_LATCH >> 8, PIT_CH2); /* MSB of count */ @@ -361,6 +360,8 @@ static s64 __init init_pit(struct platfo if ( count == 0 ) return 0; + using_pit = true; + return (end - start) * CALIBRATE_FRAC; }
For one, "using_pit" shouldn't be set ahead of the function's last (for now: only) error path. Otherwise "clocksource=pit" on the command line can lead to misbehavior when actually taking that error path. And then make an implicit assumption explicit: CALIBRATE_FRAC cannot, for example, simply be changed to 10. The way init_pit() works, the upper bound on the calibration period is about 54ms. Signed-off-by: Jan Beulich <jbeulich@suse.com> --- Really I've noticed this while considering what would happen if someone specified "clocksource=pit" on the shim's command line. Unlike "hpet" and "acpi", "pit" presently wouldn't be (explicitly) ignored. While, aiui, right now the only error path would be taken (due to port 0x61 reads being supposed to get back 0xff), I don't think we can build on that longer term: Seeing what we use port 0x61 for in traps.c, I think sooner or later we will need to have some form of emulation for it. Such emulation is then not unlikely to continuously report 0 in the bit in question. That would leed to an infinite loop here.