Date: Wed, 6 Aug 2014 06:49:12 +1000 (EST) From: Bruce Evans <brde@optusnet.com.au> To: Ian Smith <smithi@nimnet.asn.au> Cc: Anthony Jenkins <Anthony.B.Jenkins@att.net>, freebsd-acpi@freebsd.org Subject: Re: [PATCH] ACPI CMOS region support - submission? Message-ID: <20140806040804.P849@besplex.bde.org> In-Reply-To: <20140805175541.G62404@sola.nimnet.asn.au> References: <53DD61BD.7050508@att.net> <20140805175541.G62404@sola.nimnet.asn.au>
next in thread | previous in thread | raw e-mail | index | archive | help
On Tue, 5 Aug 2014, Ian Smith wrote:
> On Sat, 2 Aug 2014 18:10:05 -0400, Anthony Jenkins wrote:
>
> > I made a few minor changes since the last incarnation:
> >
> > - Defined the CMOS address/data register addresses as macros
> > - Defined the (apparent) I/O delay as a macro
>
> Looks good, Anthony.
>
> > I also verified the ACPI CMOS region code only accesses up to
> > register 63 (0x3F - in previous emails I mistakenly said 0x7F).
>
> Is 0x3f what the ACPI spec limits ACPI access to?
>
> This is mildly surprising, as all modern RTC chips are at least 128
> bytes; noone's actually used a MC146818 in over ten years, I gather.
BIOSes at least used to use much more. It is good for ACPI to not allow
clobbering the BIOS state.
> > If/when this gets in, I'd like to add sysctl controls to e.g. allow
> > ACPI access to the date/time registers (I currently return failure
> > when attempting to write them via ACPI). I don't see anything in the
> > spec (after re-reading it) that disallows ACPI from touching those.
>
> I don't know about the spec, but I can't see allowing ACPI write access
> to time/alarm/date registers as being a sensible idea; this could allow
> completely out-of-band modification of time/alarm/date regs without the
> necessary precautions needed for setting these, namely use of the SET
> bit in status reg B (0x0b) to disable updates while setting time & date,
> and anyway why allow changing time/date without the OS knowing about it?
> Reading should be no problem, though without proper observance of UIP
> bit timing, data may not be consistent.
They might need to be read on resume. I can't see how resume can possibly
work without reading them or some clock to reset the time. But this should
be done by calling the standard time reading functions, not at a low level.
> I guess there may be a case for messing with the alarm bytes, though we
> don't currently allow use of the alarm for wake from poweroff (S5) or
> STR (S3) states, as doth Linux. I looked into this some years ago when
> there were a few requests for the feature, however it would involve some
> major reworking to always preserve the alarm interrupt bit through init
> and suspend/resume, and appropriate clearing of same after use, along
> with a utility to setup the alarm .. a potential to leave open, perhaps?
I use the seconds alarm for pps, but don't use suspend.
> Which leads to my other concern here: that you are allowing out-of-band
> access to especially status reg C (0x0c), which when read clears all
> enabled interrupts, and the other status regs whose settings are also
> too important to allow screwing with.
Yes, reading it would break interrupt handling.
> We used to use the RTC periodic interrupt as a clock source for a 128Hz
> interrupt (1024Hz while profiling) but since mav@'s reworking of all the
> clocks and timers sometime prior to 9.0-REL (IIRC), that's now rarely if
> ever used as such, but remains an available clock or timer source.
>
> Reg A (0x0a) is r/w and sets clock divider and rate selection bits. Do
> we want ACPI to be able to mess with these? I think not. Readonly, ok.
>
> Reg B (0x0b) is r/w and contains the aforementioned SET bit, the three
> interrupt enable bits (PIE, AIE, UIE), the SQWE, DM, 24/12 and DSE bits,
> again none of which should be allowed to be written to out-of-band.
>
> And reg C (0x0c) is read-only, and as mentioned clears interrupts; again
> not something that should be permitted without the OS knowing about it.
>
> I suppose you have the MC146818 spec to hand?
>
> Couple of things from your patch:
>
> -static int rtc_reg = -1;
>
> Indeed; I never could figure out the advantage in this, as how rarely
> would you want to read or write the same reg twice in a row anyway?
About 99.9999% of accesses are to the status register for handling RTC
interrupts. The RTC is almost never used except for the 128Hz interrupt,
so the index register is normally constant at the single value used by
this interrupt (RTC_INTR). With mav's changes, it is now rarely used.
Removing this is a good re-pessimization. rtcin() does 4 i/o's with the
pessimization. This takes about 5 usec. At 128 Hz this only wastes
0.064% of a CPU. At 1024 Hz it wastes 0.512%. The non-pessimized
version only does 1 i/o so it wastes 1/4 as much. The wastage should
be much larger. 1024 Hz is far too slow for profiling a modern system.
It is about right for a 4.77 MHz 8086, except the RTC overhead is too
high for that (I used the i8254). Scaling this for a 2 GHz modern CPU
gives 429 kHz (or more, since instructions per cycle is more). The
overhead for that with the pessimization would be 214% of a CPU.
Without the pessimization, it would only be 53.7% of a CPU. Still too
much. The RTC is just unsuitable for profiling interrupts at a useful
rate. The i8254 is better and the lapic timer is much better, but
profhz has only been increased to 8192. Other things don't scale so
well. There are other i/o's for interrupts that make it difficult
to go much higher than 429 KHz though 429 Khz is certainly possible
using less than 100% of a CPU.
> static u_char rtc_statusa = RTCSA_DIVIDER | RTCSA_NOPROF;
> static u_char rtc_statusb = RTCSB_24HR;
>
> +static void acpi_cmos_read(ACPI_PHYSICAL_ADDRESS address, UINT8 *buf, UINT32 buflen)
> {
> + UINT32 offset;
Please no Windows typedefs in non-Windows code.
It is a minor layering violation to have acpi functions in non-acpi drivers.
At least use the driver's spellings and not the Windows ones in drivers.
>
> RTC_LOCK;
>
> + for (offset = 0U; offset < buflen; ++offset) {
> + IO_DELAY();
> + outb(IO_RTC_ADDR, ((address + offset) | 0x80) & 0xFF);
> + IO_DELAY();
> + buf[offset] = inb(IO_RTC_DATA);
This should just call rtcin() in a loop instead of breaking it. Then
there would be no need to put this in the driver.
>
> Sorry, but I don't get '| 0x80' there, and below in acpi_cmos_write ?
0x80 disables NMI on some systems (mostly old ones?). Old books (Van
Gilluwe) say to use it. FreeBSD never bothered with it. Locking makes
it unecessary, modulo likely bugs in the NMI handler.
It is also necessary disable ordinary interrupts and be careful about
reenabling NMIs. FreeBSD's locking disables ordinary interrupts as a
side effect of using spinlocks, except in my version where spinlocks
don't block fast interrupt handlers. The remaining care is just to
turn NMIs back on in the RTC at the end of the loop before unlocking
(normally) reenables ordinary interrupts. This releases any pending
NMIs.
Spinlocks don't block NMIs, so the NMI handler must be careful not to
block endlessly on a spinlock or call any code that depends on spinlocks
for locking, like the code here. Fast interrupt handlers should be
similarly careful, but aren't except in my version.
NMIs are rare except for pmc and SMP stopping, and the NMI handler never
bothered about correctness in FreeBSD. isa_nmi() for x86 normally just
prints things (using log()) and returns for the system to panic.
Printing is supposed to work when called in the "any" context, but it
is quite broken now. It has locks galore. When an NMI occurs while
one of these locks is held, then the NMI handler deadlocks on the lock
if it is a (non-recursive) spinlock held by the same CPU. If the lock
is held by another CPU, then there is some chance that the lock will
go away, and the bug is just that the printing is delayed. If the
lock is a recursive spinlock held by the same CPU, then the locking
just doesn't work. The buggy locking in printf() mostly uses non-
recursive spinlocks.
The sloppiness in the NMI handler is not much of a problem for the RTC.
The NMI handler just doesn't normally go near the RTC code, so it won't
deadlock. Unless there is something like a resettodr() call for shutdown
and panic() stumbles into this.
>
> +int
> +rtcin(int reg)
> +{
> + u_char val;
> +
> + acpi_cmos_read(reg & 0xFF, &val, 1);
rtcin() is pessimized too.
The pessimization factor must also be increased from 4 to 5 or 6 (1
more i/o to turn off the NMI disable bit at the end, and probably
another to delay after this).
> ...
> static int
> +is_datetime_reg(ACPI_PHYSICAL_ADDRESS address)
> +{
> + return address == 0x00 ||
> + address == 0x02 ||
> + address == 0x04 ||
> + address == 0x04 ||
> + (address >= 0x06 && address <= 0x09);
> +}
Lots of style bugs. The KNF style is:
<empty line after null declarations>
return (address == 0x00 || address == 0x02 || address == 0x04 ||
address == 0x04 || (address >= 0x06 && address <= 0x09));
This includes:
- silly parentheses around return values
- no splitting of statements after every subexpression to make split
statements even more split
- continuation indent is 4 spaces.
> I guess that second 0x04 should be 0x06? But why are you limiting to
> even addresses, ie time regs but not the alarm regs? If anything, the
> alarm regs seem more likely something BIOS/AML may? want to know about?
>
> +static ACPI_STATUS
> +acpi_rtc_cmos_handler(UINT32 function, ACPI_PHYSICAL_ADDRESS address, UINT32 width,
> + UINT64 *value, void *context, void *region_context)
More style bugs. Now the line splitting doesn't even keep the line
short enough, and the continuation indentation isn't even 1 tab or gnu style
(-lp).
> +{
> + struct atrtc_softc *sc;
> +
> + sc = (struct atrtc_softc *)context;
> + if (!value || !sc)
> + return AE_BAD_PARAMETER;
> + if (width > 32 || (width & 0x07) || address >= 64U)
> + return AE_BAD_PARAMETER;
Missing some Windows bad style (U suffix on 64 but not on 32).
>
> Ok, address is limited to 0x3f here, though strictly address should be
> less than (64 - width>>3) for multibyte access? I guess non-ACPI access
> via rtcin() and writertc() will still be able to access registers, well,
> from 0 to 0xff ..
>
> + if (function == ACPI_WRITE &&
> + (is_datetime_reg(address) ||
> + (width > 8 && address <= 0x09)))
> + return AE_BAD_PARAMETER;
Larger indentation errors than above. The missing silly parentheses on
return values seem to be consistent.
>
> I'd exclude AT LEAST 0x0a - 0x0d for write, certainly 0x0c from read.
>
> + printf("%s: %-5s%02u address=%04lx value=%08x\n", + __FUNCTION__,
> function == ACPI_READ ? "READ" : "WRITE", + width >> 3, address,
> *((UINT32 *)value));
> + return AE_OK;
>
> The printf is good to see. Maybe you'd eventually want to limit this to
> when verbose booting is on? It would be useful to see with suspend and
> resume messages, or anywhere else ACPI wanted R/W access to 'CMOS' RAM?
It's so ugly it is obviously only for debugging :-).
> I tried hunting down a list of what CMOS locations various systems use,
> and find some anecdotal info but nothing like 'manuf X uses bytes Y-Z'.
Why do we have to care about acpi doing unsafe accesses? Only the
block access seems dangerous. With scattered accesses like
rtcin(RTC_FOO) using the system (non-acpi) spelling for 'FOO', it is
very easy to see any dangerous ones. rtcin() used to be only used for
memory sizing and in the fd driver (for dubious disk equipment byte
reads; the equipment bytes just tell you about equipment that systems
might have had in 1985, so it is almost useless, while other BIOS bytes
are even more useless since they are not standard). It is now used in:
- nvram driver. This seems to allow anyone with access to the device
to read and/or write the offsets 14-127. So they can manage or
corrupt some BIOS bytes but not the ones used by FreeBSD :-). acpia
should be even more trustable, but might need read access to the
clock bytes.
- fb driver. This reads the video equipment byte. This is so hackish
that RTC_EQUIPMENT is still not defined in rtc.h but is defined here.
EQUIPMENT is the standard name for this byte (see van Gilluwe), but
it is not the only equipment-related byte. There is also the diskette
equipment byte. Many other equipment-related bytes that worked in
1985 are also not defined in rtc.h. They are also not used in FreeBSD.
- acpi_support/icpi_ibm.c uses a large subset of RTC space. It uses
rtcin(), but has its own defines for 6 RTC registers between 0x64 and
0x6e (for things like brightness and volume). Obviously very
vendor-specific.
- i386/xen/clock.c is a copy of the old x86 clock.c. It duplicates
almost everything including the badly named readrtc() BCD translation
wrapper, but uses the systtem rtcin() and writertc().
Write accesses used to be protected by writerc() being static, but this
was broken by exporting it for nvram and xen.
Bruce
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20140806040804.P849>