From owner-freebsd-acpi@FreeBSD.ORG Tue Aug 5 20:49:23 2014 Return-Path: Delivered-To: freebsd-acpi@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [8.8.178.115]) (using TLSv1 with cipher ADH-AES256-SHA (256/256 bits)) (No client certificate requested) by hub.freebsd.org (Postfix) with ESMTPS id CA93D1D5 for ; Tue, 5 Aug 2014 20:49:23 +0000 (UTC) Received: from mail107.syd.optusnet.com.au (mail107.syd.optusnet.com.au [211.29.132.53]) by mx1.freebsd.org (Postfix) with ESMTP id 510F724A2 for ; Tue, 5 Aug 2014 20:49:22 +0000 (UTC) Received: from c122-106-147-133.carlnfd1.nsw.optusnet.com.au (c122-106-147-133.carlnfd1.nsw.optusnet.com.au [122.106.147.133]) by mail107.syd.optusnet.com.au (Postfix) with ESMTPS id 1C135D41C08; Wed, 6 Aug 2014 06:49:13 +1000 (EST) Date: Wed, 6 Aug 2014 06:49:12 +1000 (EST) From: Bruce Evans X-X-Sender: bde@besplex.bde.org To: Ian Smith Subject: Re: [PATCH] ACPI CMOS region support - submission? In-Reply-To: <20140805175541.G62404@sola.nimnet.asn.au> Message-ID: <20140806040804.P849@besplex.bde.org> References: <53DD61BD.7050508@att.net> <20140805175541.G62404@sola.nimnet.asn.au> MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII; format=flowed X-Optus-CM-Score: 0 X-Optus-CM-Analysis: v=2.1 cv=eojmkOZX c=1 sm=1 tr=0 a=7NqvjVvQucbO2RlWB8PEog==:117 a=PO7r1zJSAAAA:8 a=yaUwhfENFygA:10 a=qMAOWPgdKHIA:10 a=kj9zAlcOel0A:10 a=JzwRw_2MAAAA:8 a=QhA1Jl_5v47fETCu2T4A:9 a=yLazs6WoQpZzPl8J:21 a=vI0KxA2gom9bJNVb:21 a=CjuIK1q_8ugA:10 Cc: Anthony Jenkins , freebsd-acpi@freebsd.org X-BeenThere: freebsd-acpi@freebsd.org X-Mailman-Version: 2.1.18 Precedence: list List-Id: ACPI and power management development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 05 Aug 2014 20:49:23 -0000 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: 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