On Wed, Jul 29, 2020 at 08:29:31PM +1000, Jonathan Matthew wrote:
> On Wed, Jul 29, 2020 at 10:06:14AM +0200, Mark Kettenis wrote:
> > > Date: Wed, 29 Jul 2020 10:38:55 +1000
> > > From: Jonathan Matthew <jonat...@d14n.org>
> > >
> > > On Tue, Jul 28, 2020 at 07:30:36PM +0200, Mark Kettenis wrote:
> > > > > Date: Tue, 28 Jul 2020 21:42:46 +1000
> > > > > From: Jonathan Matthew <jonat...@d14n.org>
> > > > >
> > > > > On Tue, Jul 28, 2020 at 11:12:21AM +0200, Mark Kettenis wrote:
> > > > > > > Date: Tue, 28 Jul 2020 13:46:34 +1000
> > > > > > > From: Jonathan Matthew <jonat...@d14n.org>
> > > > > > >
> > > > > > > On Mon, Jul 27, 2020 at 05:16:47PM +0200, Mark Kettenis wrote:
> > > > > > > > > Date: Mon, 27 Jul 2020 17:02:41 +0200 (CEST)
> > > > > > > > > From: Mark Kettenis <mark.kette...@xs4all.nl>
> > > > > > > > >
> > > > > > > > > Recent ACPI versions have deprecated "Processor()" nodes in
> > > > > > > > > favout of
> > > > > > > > > "Device()" nodes with a _HID() method that returns
> > > > > > > > > "ACPI0007". This
> > > > > > > > > diff tries to support machines with firmware that implements
> > > > > > > > > this. If
> > > > > > > > > you see something like:
> > > > > > > > >
> > > > > > > > > "ACPI0007" at acpi0 not configured
> > > > > > > > >
> > > > > > > > > please try the following diff and report back with an updated
> > > > > > > > > dmesg.
> > > > > > > > >
> > > > > > > > > Cheers,
> > > > > > > > >
> > > > > > > > > Mark
> > > > > > > >
> > > > > > > > And now with the right diff...
> > > > > > >
> > > > > > > On a dell r6415, it looks like this:
> > > > > > >
> > > > > > > acpicpu0 at acpi0copyvalue: 6: C1(@1 halt!)
> > > > > > > all the way up to
> > > > > > > acpicpu127 at acpi0copyvalue: 6: no cpu matching ACPI ID 127
> > > > > > >
> > > > > > > which I guess means aml_copyvalue() needs to learn how to copy
> > > > > > > AML_OBJTYPE_DEVICE.
> > > > > >
> > > > > > Yes. It is not immediately obvious how this should work. Do we
> > > > > > need
> > > > > > to copy the aml_node pointer or not? We don't do that for
> > > > > > AML_OBJTYPE_PROCESSOR and AML_OBJTYPE_POWERRSRC types which are
> > > > > > similar to AML_OBJTYPE_DEVICE. But AML_OBJTYPE_DEVICE object don't
> > > > > > carry any additional information. So we end up with just an empty
> > > > > > case to avoid the warning.
> > > > > >
> > > > > > Does this work on the Dell machines?
> > > > >
> > > > > We've seen crashes in pool_cache_get() in various places after all
> > > > > the acpicpus
> > > > > attach, which we haven't seen before on these machines, so I think
> > > > > it's
> > > > > corrupting memory somehow.
> > > >
> > > > Does that happen with only the acpicpu(4) diff?
> > >
> > > Yes. Looking at this a bit more, in the case where aml_evalnode() can't
> > > copy the result value, it leaves it uninitialised, which means we'll call
> > > aml_freevalue(&res) where res is stack junk. memset(&res, 0, sizeof(res))
> > > seems to fix it.
> >
> > Eh, where exactly?
>
> I had it just before the call to aml_evalnode(), but that can't be it,
> since aml_evalnode() does the same thing.
Much better theory: the acpicpu_sc array has MAXCPUS elements, but on this
system (and all R6415s, as far as I can tell) we have more acpicpu devices
than that. I suppose we should just make acpicpu_match fail if cf->cf_unit
is >= MAXCPUS as we do with the actual cpu devices.
Index: acpicpu.c
===================================================================
RCS file: /cvs/src/sys/dev/acpi/acpicpu.c,v
retrieving revision 1.85
diff -u -p -r1.85 acpicpu.c
--- acpicpu.c 27 May 2020 05:02:21 -0000 1.85
+++ acpicpu.c 1 Aug 2020 08:18:49 -0000
@@ -186,6 +186,11 @@ struct cfdriver acpicpu_cd = {
NULL, "acpicpu", DV_DULL
};
+const char *acpicpu_hids[] = {
+ "ACPI0007",
+ NULL
+};
+
extern int setperf_prio;
struct acpicpu_softc *acpicpu_sc[MAXCPUS];
@@ -650,6 +655,12 @@ acpicpu_match(struct device *parent, voi
struct acpi_attach_args *aa = aux;
struct cfdata *cf = match;
+ if (cf->cf_unit >= MAXCPUS)
+ return (0);
+
+ if (acpi_matchhids(aa, acpicpu_hids, cf->cf_driver->cd_name))
+ return (1);
+
/* sanity */
if (aa->aaa_name == NULL ||
strcmp(aa->aaa_name, cf->cf_driver->cd_name) != 0 ||
@@ -665,6 +676,7 @@ acpicpu_attach(struct device *parent, st
struct acpicpu_softc *sc = (struct acpicpu_softc *)self;
struct acpi_attach_args *aa = aux;
struct aml_value res;
+ int64_t uid;
int i;
uint32_t status = 0;
CPU_INFO_ITERATOR cii;
@@ -675,6 +687,10 @@ acpicpu_attach(struct device *parent, st
acpicpu_sc[sc->sc_dev.dv_unit] = sc;
SLIST_INIT(&sc->sc_cstates);
+
+ if (aml_evalinteger(sc->sc_acpi, sc->sc_devnode,
+ "_UID", 0, NULL, &uid) == 0)
+ sc->sc_cpu = uid;
if (aml_evalnode(sc->sc_acpi, sc->sc_devnode, 0, NULL, &res) == 0) {
if (res.type == AML_OBJTYPE_PROCESSOR) {
