Date: Sun, 17 Aug 2014 11:26:55 -0400 From: Anthony Jenkins <Anthony.B.Jenkins@att.net> To: Bruce Evans <brde@optusnet.com.au>, Ian Smith <smithi@nimnet.asn.au> Cc: freebsd-acpi@freebsd.org Subject: Re: [PATCH] ACPI CMOS region support - submission? Message-ID: <53F0C9BF.9060405@att.net> In-Reply-To: <20140806040804.P849@besplex.bde.org> References: <53DD61BD.7050508@att.net> <20140805175541.G62404@sola.nimnet.asn.au> <20140806040804.P849@besplex.bde.org>
next in thread | previous in thread | raw e-mail | index | archive | help
Thanks for the critique guys, comments inline. Sorry it took so long...
On 08/05/2014 16:49, Bruce Evans wrote:
> 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.
Ya need to complain to the folks at acpica.org, who define the types in the region access handler functions:
sys/contrib/dev/acpica/include/actypes.h:
/* Address Spaces (For Operation Regions) */
typedef
ACPI_STATUS (*ACPI_ADR_SPACE_HANDLER) (
UINT32 Function,
ACPI_PHYSICAL_ADDRESS Address,
UINT32 BitWidth,
UINT64 *Value,
void *HandlerContext,
void *RegionContext);
> 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.
I suppose I could make an atrtc_acpi.c or something and put them there...
>> 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.
I had to refactor rtcin() and writertc() because the ACPI read/write functions may be multibyte and the original functions LOCK(), read/write 1 byte, UNLOCK(). It's ACPI's variable length region handler reads/writes that prompted this change.
>> 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.
No problem... is there a FreeBSD style guide somewhere, or do I just follow "K&R style"?
>> 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?
Ehhhh... just trying to encapsulate the request that "CMOS writes should not touch certain registers". Stuck that request in a function that would return true if such a register write request was made.
It would be helpful to have a concise spec for which registers and what types of accesses should be disallowed by the CMOS region handler...
>> +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).
Yeah I'm not very consistent with those...
>> 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
> _______________________________________________
> freebsd-acpi@freebsd.org mailing list
> http://lists.freebsd.org/mailman/listinfo/freebsd-acpi
> To unsubscribe, send any mail to "freebsd-acpi-unsubscribe@freebsd.org"
>
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?53F0C9BF.9060405>