Date: Tue, 05 Aug 2014 09:04:21 -0400 From: Anthony Jenkins <Anthony.B.Jenkins@att.net> To: Ian Smith <smithi@nimnet.asn.au> Cc: freebsd-acpi@freebsd.org Subject: Re: [PATCH] ACPI CMOS region support - submission? Message-ID: <53E0D655.6090701@att.net> 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 08/05/2014 06:46, 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?
Yes, just for this specific device with PNP ID PNP0B00.
> 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.
True... the ACPI spec (3.0-5.0 at least) also added PNP0B01 and PNP0B02 to cover two (apparently common) CMOS implementations which are 128+ bytes. To cover these devices I'd have to sorta overhaul the atrtc.c driver (or add different drivers). I'm just focusing on the ubiquitous PNP0B00 for now...
> > 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.
That's fair, I also don't foresee any reason for ACPI to twiddle with the date/time CMOS regs (though there /is/ an open FreeBSD bug about ACPI wake from alarm not supported). As proposed, my code disallows write access to these regs; I just wanted to cover the case where somebody interpreted the spec as I did and designed their system to write to those registers. Also, to me the authors of the ACPI code are the same ones who designed the system and selected the south bridge part which provides the PNP0B00 device; personally I have no problem letting them stomp around in the date/time/alarm regs :-) Maybe it's sufficient to add a compile time flag like ALLOW_ACPI_CMOS_DATETIME_WRITES or something to enable the iffy behaviour.
I need to find out where I got the idea that those regs were excluded from ACPI writes...
> 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?
>
> 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.
>
> 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;
Yeah I actually couldn't figure out the purpose of that variable until recently, then it just seemed an unnecessary optimization.
> 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?
>
> 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;
>
> 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);
>
> Sorry, but I don't get '| 0x80' there, and below in acpi_cmos_write ?
Right, I just wanted to ensure bit 7 was always set (or was it clear?). Does that bit still control NMI? Maybe that should've been & (~0x80)... good catch! My system seems to not care about that bit...
> +int
> +rtcin(int reg)
> +{
> + u_char val;
> +
> + acpi_cmos_read(reg & 0xFF, &val, 1);
>
> +void
> +writertc(int reg, u_char val)
> +{
> + acpi_cmos_write(reg & 0xFF, &val, 1);
> +}
>
> Reg is already masked to 0xFF here anyway, and AFAICT every other access
> system-wide is via rtcin() and writertc().
>
> static int
> +is_datetime_reg(ACPI_PHYSICAL_ADDRESS address)
> +{
> + return address == 0x00 ||
> + address == 0x02 ||
> + address == 0x04 ||
> + address == 0x04 ||
> + (address >= 0x06 && address <= 0x09);
> +}
>
> I guess that second 0x04 should be 0x06?
Right, copy/paste error.
> 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?
That function just encapsulates identifying which registers are "read-only" (i.e. the date/time regs); I did not want to limit ACPI from writing the alarm regs.
> +static ACPI_STATUS
> +acpi_rtc_cmos_handler(UINT32 function, ACPI_PHYSICAL_ADDRESS address, UINT32 width,
> + UINT64 *value, void *context, void *region_context)
> +{
> + 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;
>
> Ok, address is limited to 0x3f here, though strictly address should be
> less than (64 - width>>3) for multibyte access?
Ahh yes, didn't catch that.
> I guess non-ACPI access
> via rtcin() and writertc() will still be able to access registers, well,
> from 0 to 0xff ..
Yup, tried to make sure not to change the behavior of rtcin() and writertc() (I did with the 0x80 bit masking, but it seemed to be a no-op).
> + if (function == ACPI_WRITE &&
> + (is_datetime_reg(address) ||
> + (width > 8 && address <= 0x09)))
> + return AE_BAD_PARAMETER;
>
> I'd exclude AT LEAST 0x0a - 0x0d for write, certainly 0x0c from read.
I'll add those exclusions, possibly providing knobs for people to enable them if they so wish.
> + 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?
>
> 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'.
>
> Probably I'm missing something really major here, on recent form :)
Good comments, thanks! I'll try to make the changes tonight (but I'm slammed this week with a business trip tomorrow).
Anthony
>
> cheers, Ian
>
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?53E0D655.6090701>