subject:"aml_rdpciaddr is busted"

Re: aml_rdpciaddr is busted

2016-10-25 Thread Mark Kettenis

> Date: Wed, 19 Oct 2016 21:51:47 +0200 (CEST)
> From: Mark Kettenis 
> 
> The bus number it reports will be totally bogus for devices behind PCI
> bridges.  As a consequence AML will peek and poke at registers of the
> wrong device.  This is what caused the suspend issues with Joris'
> Macbook.
> 
> The diff below attempts to fix this by using the mapping between AML
> nodes and PCI devices that we establish.  Because _INI methods may end
> up executing AML that ends up calling aml_rdpciaddr(), the mapping is
> now established before we execute _INI.  I'm not 100% sure this is
> correct as there is a possibility that _INI will poke some PCI bridges
> in a way that changes the mapping.  Only one way to find out...
> 
> The code also deals with devices that aren't there in a slightly
> different way.  They are now included in the node -> PCI mapping, but
> they're still not included in the list of PCI device nodes that we
> create.  Writing to config space for devices that aren't there is
> implemented as a no-op.  Reading will return an all-ones pattern.
> 
> So far I've only tested this on my x1.  More testing is needed.

This has been in snaps for a couple of days now.  Anybody bold enough
to ok this?

> Index: dev/acpi/acpi.c
> ===
> RCS file: /cvs/src/sys/dev/acpi/acpi.c,v
> retrieving revision 1.316
> diff -u -p -r1.316 acpi.c
> --- dev/acpi/acpi.c   18 Sep 2016 23:56:45 -  1.316
> +++ dev/acpi/acpi.c   19 Oct 2016 19:42:51 -
> @@ -614,6 +614,7 @@ acpi_getpci(struct aml_node *node, void 
>   pci->dev = ACPI_ADR_PCIDEV(val);
>   pci->fun = ACPI_ADR_PCIFUN(val);
>   pci->node = node;
> + node->pci = pci;
>   pci->sub = -1;
>  
>   dnprintf(10, "%.2x:%.2x.%x -> %s\n", 
> @@ -639,17 +640,12 @@ acpi_getpci(struct aml_node *node, void 
>   pci->_s4w = -1;
>  
>   /* Check if PCI device exists */
> - if (pci->dev > 0x1F || pci->fun > 7) {
> - free(pci, M_DEVBUF, sizeof(*pci));
> - return (1);
> - }
> + if (pci->dev > 31 || pci->fun > 7)
> + return 1;
>   tag = pci_make_tag(pc, pci->bus, pci->dev, pci->fun);
>   reg = pci_conf_read(pc, tag, PCI_ID_REG);
> - if (PCI_VENDOR(reg) == PCI_VENDOR_INVALID) {
> - free(pci, M_DEVBUF, sizeof(*pci));
> - return (1);
> - }
> - node->pci = pci;
> + if (PCI_VENDOR(reg) == PCI_VENDOR_INVALID)
> + return 1;
>  
>   TAILQ_INSERT_TAIL(_pcidevs, pci, next);
>  
> @@ -1066,11 +1062,11 @@ acpi_attach(struct device *parent, struc
>   config_found_sm(self, , acpi_print, acpi_submatch);
>   }
>  
> + /* get PCI mapping */
> + aml_walknodes(_root, AML_WALK_PRE, acpi_getpci, sc);
> +
>   /* initialize runtime environment */
>   aml_find_node(_root, "_INI", acpi_inidev, sc);
> -
> - /* Get PCI mapping */
> - aml_walknodes(_root, AML_WALK_PRE, acpi_getpci, sc);
>  
>   /* attach pci interrupt routing tables */
>   aml_find_node(_root, "_PRT", acpi_foundprt, sc);
> Index: dev/acpi/dsdt.c
> ===
> RCS file: /cvs/src/sys/dev/acpi/dsdt.c,v
> retrieving revision 1.225
> diff -u -p -r1.225 dsdt.c
> --- dev/acpi/dsdt.c   27 Sep 2016 10:04:19 -  1.225
> +++ dev/acpi/dsdt.c   19 Oct 2016 19:42:51 -
> @@ -2216,21 +2216,16 @@ aml_rdpciaddr(struct aml_node *pcidev, u
>  {
>   int64_t res;
>  
> - if (aml_evalinteger(acpi_softc, pcidev, "_ADR", 0, NULL, ) == 0) {
> - addr->fun = res & 0x;
> - addr->dev = res >> 16;
> - }
> - while (pcidev != NULL) {
> - /* HID device (PCI or PCIE root): eval _BBN */
> - if (__aml_search(pcidev, "_HID", 0)) {
> - if (aml_evalinteger(acpi_softc, pcidev, "_BBN", 0, 
> NULL, ) == 0) {
> - addr->bus = res;
> - break;
> - }
> - }
> - pcidev = pcidev->parent;
> - }
> - return (0);
> + if (pcidev->pci == NULL)
> + return -1;
> +
> + if (aml_evalinteger(acpi_softc, pcidev, "_ADR", 0, NULL, ))
> + return -1;
> +
> + addr->fun = res & 0x;
> + addr->dev = res >> 16;
> + addr->bus = pcidev->pci->bus;
> + return 0;
>  }
>  
>  /* Read/Write from opregion object */
> @@ -2272,7 +2267,12 @@ aml_rwgas(struct aml_value *rgn, int bpo
>  
>   if (rgn->v_opregion.iospace == GAS_PCI_CFG_SPACE) {
>   /* Get PCI Root Address for this opregion */
> - aml_rdpciaddr(rgn->node->parent, );
> + if (aml_rdpciaddr(rgn->node->parent, )) {
> + if (mode == ACPI_IOREAD)
> + pi.fun = 0x;
> + else
> + return;
> + }
>   }
>  
>   tbit

Re: aml_rdpciaddr is busted

2016-10-20 Thread Joerg Jung

On Wed, Oct 19, 2016 at 09:51:47PM +0200, Mark Kettenis wrote:
> The bus number it reports will be totally bogus for devices behind PCI
> bridges.  As a consequence AML will peek and poke at registers of the
> wrong device.  This is what caused the suspend issues with Joris'
> Macbook.
> 
> The diff below attempts to fix this by using the mapping between AML
> nodes and PCI devices that we establish.  Because _INI methods may end
> up executing AML that ends up calling aml_rdpciaddr(), the mapping is
> now established before we execute _INI.  I'm not 100% sure this is
> correct as there is a possibility that _INI will poke some PCI bridges
> in a way that changes the mapping.  Only one way to find out...
> 
> The code also deals with devices that aren't there in a slightly
> different way.  They are now included in the node -> PCI mapping, but
> they're still not included in the list of PCI device nodes that we
> create.  Writing to config space for devices that aren't there is
> implemented as a no-op.  Reading will return an all-ones pattern.
> 
> So far I've only tested this on my x1.  More testing is needed.

Tested the diff together with the timer diff from Joris on MacBookAir6,2.
Does no blow up the machine. pci/ppb lines in dmesg change with this 
(less Thunderbolt lines), dmesg diff below.

Regards,
Joerg


--- dmesg.old   Thu Oct 20 19:32:44 2016
+++ dmesg.new   Thu Oct 20 19:31:51 2016
@@ -1,4 +1,4 @@
-/bsd: OpenBSD 6.0-current (GENERIC.MP) #0: Wed Oct 19 20:30:41 CEST 2016
+/bsd: OpenBSD 6.0-current (GENERIC.MP) #0: Thu Oct 20 19:18:57 CEST 2016
 /bsd: y...@marvin.hq.umaxx.net:/usr/src/sys/arch/amd64/compile/GENERIC.MP
 /bsd: RTC BIOS diagnostic error 
ff
 /bsd: real mem = 8509276160 (8115MB)
@@ -17,7 +17,7 @@
 /bsd: acpihpet0 at acpi0: 14318179 Hz
 /bsd: acpimadt0 at acpi0 addr 0xfee0: PC-AT compat
 /bsd: cpu0 at mainbus0: apid 0 (boot processor)
-/bsd: cpu0: Intel(R) Core(TM) i7-4650U CPU @ 1.70GHz, 1600.21 MHz
+/bsd: cpu0: Intel(R) Core(TM) i7-4650U CPU @ 1.70GHz, 1600.28 MHz
 /bsd: cpu0: 
FPU,VME,DE,PSE,TSC,MSR,PAE,MCE,CX8,APIC,SEP,MTRR,PGE,MCA,CMOV,PAT,PSE36,CFLUSH,DS,ACPI,MMX,FXSR,SSE,SSE2,SS,HTT,TM,PBE,SSE3,PCLMUL,DTES64,MWAIT,DS-CPL,VMX,SMX,EST,TM2,SSSE3,SDBG,FMA3,CX16,xTPR,PDCM,PCID,SSE4.1,SSE4.2,x2APIC,MOVBE,POPCNT,DEADLINE,AES,XSAVE,AVX,F16C,RDRAND,NXE,PAGE1GB,LONG,LAHF,ABM,PERF,ITSC,FSGSBASE,BMI1,HLE,AVX2,SMEP,BMI2,ERMS,INVPCID,RTM,SENSOR,ARAT
 /bsd: cpu0: 256KB 64b/line 8-way L2 cache
 /bsd: cpu0: smt 0, core 0, package 0
@@ -25,17 +25,17 @@
 /bsd: cpu0: apic clock running at 100MHz
 /bsd: cpu0: mwait min=64, max=64, C-substates=0.2.1.2.4.1.1.1, IBE
 /bsd: cpu1 at mainbus0: apid 2 (application processor)
-/bsd: cpu1: Intel(R) Core(TM) i7-4650U CPU @ 1.70GHz, 1600.00 MHz
+/bsd: cpu1: Intel(R) Core(TM) i7-4650U CPU @ 1.70GHz, 1600.01 MHz
 /bsd: cpu1: 
FPU,VME,DE,PSE,TSC,MSR,PAE,MCE,CX8,APIC,SEP,MTRR,PGE,MCA,CMOV,PAT,PSE36,CFLUSH,DS,ACPI,MMX,FXSR,SSE,SSE2,SS,HTT,TM,PBE,SSE3,PCLMUL,DTES64,MWAIT,DS-CPL,VMX,SMX,EST,TM2,SSSE3,SDBG,FMA3,CX16,xTPR,PDCM,PCID,SSE4.1,SSE4.2,x2APIC,MOVBE,POPCNT,DEADLINE,AES,XSAVE,AVX,F16C,RDRAND,NXE,PAGE1GB,LONG,LAHF,ABM,PERF,ITSC,FSGSBASE,BMI1,HLE,AVX2,SMEP,BMI2,ERMS,INVPCID,RTM,SENSOR,ARAT
 /bsd: cpu1: 256KB 64b/line 8-way L2 cache
 /bsd: cpu1: smt 0, core 1, package 0
 /bsd: cpu2 at mainbus0: apid 1 (application processor)
-/bsd: cpu2: Intel(R) Core(TM) i7-4650U CPU @ 1.70GHz, 1600.00 MHz
+/bsd: cpu2: Intel(R) Core(TM) i7-4650U CPU @ 1.70GHz, 1600.01 MHz
 /bsd: cpu2: 
FPU,VME,DE,PSE,TSC,MSR,PAE,MCE,CX8,APIC,SEP,MTRR,PGE,MCA,CMOV,PAT,PSE36,CFLUSH,DS,ACPI,MMX,FXSR,SSE,SSE2,SS,HTT,TM,PBE,SSE3,PCLMUL,DTES64,MWAIT,DS-CPL,VMX,SMX,EST,TM2,SSSE3,SDBG,FMA3,CX16,xTPR,PDCM,PCID,SSE4.1,SSE4.2,x2APIC,MOVBE,POPCNT,DEADLINE,AES,XSAVE,AVX,F16C,RDRAND,NXE,PAGE1GB,LONG,LAHF,ABM,PERF,ITSC,FSGSBASE,BMI1,HLE,AVX2,SMEP,BMI2,ERMS,INVPCID,RTM,SENSOR,ARAT
 /bsd: cpu2: 256KB 64b/line 8-way L2 cache
 /bsd: cpu2: smt 1, core 0, package 0
 /bsd: cpu3 at mainbus0: apid 3 (application processor)
-/bsd: cpu3: Intel(R) Core(TM) i7-4650U CPU @ 1.70GHz, 1600.00 MHz
+/bsd: cpu3: Intel(R) Core(TM) i7-4650U CPU @ 1.70GHz, 1600.01 MHz
 /bsd: cpu3: 
FPU,VME,DE,PSE,TSC,MSR,PAE,MCE,CX8,APIC,SEP,MTRR,PGE,MCA,CMOV,PAT,PSE36,CFLUSH,DS,ACPI,MMX,FXSR,SSE,SSE2,SS,HTT,TM,PBE,SSE3,PCLMUL,DTES64,MWAIT,DS-CPL,VMX,SMX,EST,TM2,SSSE3,SDBG,FMA3,CX16,xTPR,PDCM,PCID,SSE4.1,SSE4.2,x2APIC,MOVBE,POPCNT,DEADLINE,AES,XSAVE,AVX,F16C,RDRAND,NXE,PAGE1GB,LONG,LAHF,ABM,PERF,ITSC,FSGSBASE,BMI1,HLE,AVX2,SMEP,BMI2,ERMS,INVPCID,RTM,SENSOR,ARAT
 /bsd: cpu3: 256KB 64b/line 8-way L2 cache
 /bsd: cpu3: smt 1, core 1, package 0
@@ -92,22 +92,9 @@
 /bsd: "Broadcom BCM4360" rev 0x03 at pci3 dev 0 function 0 not configured
 /bsd: ppb3 at pci0 dev 28 function 4 "Intel 8 Series PCIE" rev 0xe4: msi
 /bsd: pci4 at ppb3 bus 5
-/bsd: ppb4 at pci4 dev 0 function 0 "Intel DSL3510 Thunderbolt" rev 0x03
-/bsd: pci5 at ppb4 bus 6
-/bsd: ppb5 at pci5 dev 0 function 0

aml_rdpciaddr is busted

2016-10-19 Thread Mark Kettenis

The bus number it reports will be totally bogus for devices behind PCI
bridges.  As a consequence AML will peek and poke at registers of the
wrong device.  This is what caused the suspend issues with Joris'
Macbook.

The diff below attempts to fix this by using the mapping between AML
nodes and PCI devices that we establish.  Because _INI methods may end
up executing AML that ends up calling aml_rdpciaddr(), the mapping is
now established before we execute _INI.  I'm not 100% sure this is
correct as there is a possibility that _INI will poke some PCI bridges
in a way that changes the mapping.  Only one way to find out...

The code also deals with devices that aren't there in a slightly
different way.  They are now included in the node -> PCI mapping, but
they're still not included in the list of PCI device nodes that we
create.  Writing to config space for devices that aren't there is
implemented as a no-op.  Reading will return an all-ones pattern.

So far I've only tested this on my x1.  More testing is needed.


Index: dev/acpi/acpi.c
===
RCS file: /cvs/src/sys/dev/acpi/acpi.c,v
retrieving revision 1.316
diff -u -p -r1.316 acpi.c
--- dev/acpi/acpi.c 18 Sep 2016 23:56:45 -  1.316
+++ dev/acpi/acpi.c 19 Oct 2016 19:42:51 -
@@ -614,6 +614,7 @@ acpi_getpci(struct aml_node *node, void 
pci->dev = ACPI_ADR_PCIDEV(val);
pci->fun = ACPI_ADR_PCIFUN(val);
pci->node = node;
+   node->pci = pci;
pci->sub = -1;
 
dnprintf(10, "%.2x:%.2x.%x -> %s\n", 
@@ -639,17 +640,12 @@ acpi_getpci(struct aml_node *node, void 
pci->_s4w = -1;
 
/* Check if PCI device exists */
-   if (pci->dev > 0x1F || pci->fun > 7) {
-   free(pci, M_DEVBUF, sizeof(*pci));
-   return (1);
-   }
+   if (pci->dev > 31 || pci->fun > 7)
+   return 1;
tag = pci_make_tag(pc, pci->bus, pci->dev, pci->fun);
reg = pci_conf_read(pc, tag, PCI_ID_REG);
-   if (PCI_VENDOR(reg) == PCI_VENDOR_INVALID) {
-   free(pci, M_DEVBUF, sizeof(*pci));
-   return (1);
-   }
-   node->pci = pci;
+   if (PCI_VENDOR(reg) == PCI_VENDOR_INVALID)
+   return 1;
 
TAILQ_INSERT_TAIL(_pcidevs, pci, next);
 
@@ -1066,11 +1062,11 @@ acpi_attach(struct device *parent, struc
config_found_sm(self, , acpi_print, acpi_submatch);
}
 
+   /* get PCI mapping */
+   aml_walknodes(_root, AML_WALK_PRE, acpi_getpci, sc);
+
/* initialize runtime environment */
aml_find_node(_root, "_INI", acpi_inidev, sc);
-
-   /* Get PCI mapping */
-   aml_walknodes(_root, AML_WALK_PRE, acpi_getpci, sc);
 
/* attach pci interrupt routing tables */
aml_find_node(_root, "_PRT", acpi_foundprt, sc);
Index: dev/acpi/dsdt.c
===
RCS file: /cvs/src/sys/dev/acpi/dsdt.c,v
retrieving revision 1.225
diff -u -p -r1.225 dsdt.c
--- dev/acpi/dsdt.c 27 Sep 2016 10:04:19 -  1.225
+++ dev/acpi/dsdt.c 19 Oct 2016 19:42:51 -
@@ -2216,21 +2216,16 @@ aml_rdpciaddr(struct aml_node *pcidev, u
 {
int64_t res;
 
-   if (aml_evalinteger(acpi_softc, pcidev, "_ADR", 0, NULL, ) == 0) {
-   addr->fun = res & 0x;
-   addr->dev = res >> 16;
-   }
-   while (pcidev != NULL) {
-   /* HID device (PCI or PCIE root): eval _BBN */
-   if (__aml_search(pcidev, "_HID", 0)) {
-   if (aml_evalinteger(acpi_softc, pcidev, "_BBN", 0, 
NULL, ) == 0) {
-   addr->bus = res;
-   break;
-   }
-   }
-   pcidev = pcidev->parent;
-   }
-   return (0);
+   if (pcidev->pci == NULL)
+   return -1;
+
+   if (aml_evalinteger(acpi_softc, pcidev, "_ADR", 0, NULL, ))
+   return -1;
+
+   addr->fun = res & 0x;
+   addr->dev = res >> 16;
+   addr->bus = pcidev->pci->bus;
+   return 0;
 }
 
 /* Read/Write from opregion object */
@@ -2272,7 +2267,12 @@ aml_rwgas(struct aml_value *rgn, int bpo
 
if (rgn->v_opregion.iospace == GAS_PCI_CFG_SPACE) {
/* Get PCI Root Address for this opregion */
-   aml_rdpciaddr(rgn->node->parent, );
+   if (aml_rdpciaddr(rgn->node->parent, )) {
+   if (mode == ACPI_IOREAD)
+   pi.fun = 0x;
+   else
+   return;
+   }
}
 
tbit = _integer;

Re: aml_rdpciaddr is busted

Re: aml_rdpciaddr is busted

aml_rdpciaddr is busted

3 matches

Site Navigation

Mail list logo

Footer information