Skip to contentSkip to conversation

PCI HPMC on C240 with alternatives Patching

DISCUSSION Sven Schnelle 3 participants 7 messages May 24, 2019 PA-RISC architecture

lore
Share & tools
lore
#0 lore raw

Hi List,

i recently got my hands on an old C240. I see a Kernel oops pretty early when alternatives patching is enabled:

[   40.810794] sym53c8xx 0000:00:13.0: enabling device (0150 -> 0153)
[   40.894350] sym0: <875> rev 0x4 at pci 0000:00:13.0 irq 22
[   41.047461] sym0: No NVRAM, ID 7, Fast-20, SE, parity checking
[   50.337981] Backtrace:
[   50.366087]  [<105b61bc>] sym_hcb_attach+0x668/0x840
[   50.425150]  [<105af6c4>] sym_attach.constprop.0+0x188/0x378
[   50.492476]  [<105ae518>] sym2_probe+0x40c/0x4c4
[   50.547424]  [<104e52e4>] pci_device_probe+0xb0/0x150
[   50.607543]  [<1056cab0>] really_probe+0x2ac/0x3f8
[   50.664530]  [<1056d2f4>] driver_probe_device+0x51c/0x534
[   50.728743]  [<1056d614>] device_driver_attach+0x54/0x98
[   50.791921]  [<1056d7a0>] __driver_attach+0x148/0x160
[   50.852010]  [<1056a2dc>] bus_for_each_dev+0x7c/0xbc
[   50.911067]  [<1056c100>] driver_attach+0x2c/0x4c
[   50.967022]  [<1056b95c>] bus_add_driver+0x1b4/0x210
[   51.026071]  [<1056dfb8>] driver_register+0xdc/0x12c
[   51.085129]  [<104e4894>] __pci_register_driver+0x4c/0x6c
[   51.149385]  [<10137d4c>] sym2_init+0xc0/0x134
[   51.202244]  [<1018b81c>] do_one_initcall+0x84/0x1c0
[   51.261301]  [<1010158c>] kernel_init_freeable+0x2b8/0x2d0
[   51.326580]  [<107e6040>] kernel_init+0x20/0x140
[   51.381523]  [<1019301c>] ret_from_kernel_thread+0x1c/0x24
[   51.464420]
[   51.482098] High Priority Machine Check (HPMC): Code=1 (High-priority machine check (HPMC)) at addr 00000000
[   51.599065] CPU: 0 PID: 1 Comm: swapper Not tainted 5.2.0-rc1-32bit+ #83
[   51.678775] Hardware name: 9000/782/C240+
[   51.726427]
[   51.744090]      YZrvWESTHLNXBCVMcbcbcbcbOGFRQPDI
[   51.800018] PSW: 00000000000001001111111100001111 Not tainted
[   51.868370] r00-03  0004ff0f 109f07c4 105b61bc 2fcc09c0
[   51.930512] r04-07  2f44a000 108ced74 0098967f 00000008
[   51.992659] r08-11  00988000 01000000 00000002 2f44a3d4
[   52.054804] r12-15  10a57fc4 109e5800 000000fd f0100000
[   52.116948] r16-19  f000168c f000020c f0000204 0001602c
[   52.179094] r20-23  2f44a000 0000000f 2fc19870 2f44a920
[   52.241238] r24-27  00000040 0001602c 00016014 108fc7c4
[   52.303381] r28-31  0000001f 00000000 2fcc0a00 00000000
[   52.365519] sr00-03  00000000 00000000 00000000 00000000
[   52.428693] sr04-07  00000000 00000000 00000000 00000000
[   52.491854]
[   52.509524] IASQ: 00000000 00000000 IAOQ: 104d5708 104d570c
[   52.575798]  IIR: 48623fd9    ISR: 0024007f  IOR: 300c09ac
[   52.641036]  CPU:        0   CR30: 2fcc0000 CR31: ffff1558
[   52.706270]  ORIG_R28: 00000000
[   52.743614]  IAOQ[0]: ioread8+0x34/0x5c
[   52.789215]  IAOQ[1]: ioread8+0x38/0x5c
[   52.834821]  RP(r2): sym_hcb_attach+0x668/0x840
[   52.888676] Backtrace:
[   52.916709]  [<105b61bc>] sym_hcb_attach+0x668/0x840
[   52.975765]  [<105af6c4>] sym_attach.constprop.0+0x188/0x378
[   53.043088]  [<105ae518>] sym2_probe+0x40c/0x4c4
[   53.098009]  [<104e52e4>] pci_device_probe+0xb0/0x150
[   53.158101]  [<1056cab0>] really_probe+0x2ac/0x3f8
[   53.215080]  [<1056d2f4>] driver_probe_device+0x51c/0x534
[   53.279292]  [<1056d614>] device_driver_attach+0x54/0x98
[   53.342472]  [<1056d7a0>] __driver_attach+0x148/0x160
[   53.402557]  [<1056a2dc>] bus_for_each_dev+0x7c/0xbc
[   53.461616]  [<1056c100>] driver_attach+0x2c/0x4c
[   53.517569]  [<1056b95c>] bus_add_driver+0x1b4/0x210
[   53.576612]  [<1056dfb8>] driver_register+0xdc/0x12c
[   53.635669]  [<104e4894>] __pci_register_driver+0x4c/0x6c
[   53.699896]  [<10137d4c>] sym2_init+0xc0/0x134
[   53.752740]  [<1018b81c>] do_one_initcall+0x84/0x1c0
[   53.811785]  [<1010158c>] kernel_init_freeable+0x2b8/0x2d0
[   53.877046]  [<107e6040>] kernel_init+0x20/0x140
[   53.931969]  [<1019301c>] ret_from_kernel_thread+0x1c/0x24
[   53.997196]
[   54.016034] Kernel panic - not syncing: High Priority Machine Check (HPMC)
[   54.111012] Rebooting in 10 seconds..

The full dmesg can be found at https://stackframe.org/crashlog.txt

This also happens sometimes with the tulip driver, so it's likely not related to sym53c8xx itself. The crash location in source is:

0x1059e394 is in sym_hcb_attach (/home/svens/parisc-linux/src/drivers/scsi/sym53c8xx_2/sym_hipd.c:1038).
1029 /*
1030 * Start script (exchange values)
1031 */
1032 OUTL(np, nc_dsa, np->hcb_ba);
1033 OUTL_DSP(np, pc);
1034 /*
1035 * Wait 'til done (with timeout)
1036 */
1037 for (i=0; i<SYM_SNOOP_TIMEOUT; i++)
1038 if (INB(np, nc_istat) & (INTF|SIP|DIP)) <-- crash
1039 break;
1040 if (i>=SYM_SNOOP_TIMEOUT) {
1041 printf ("CACHE TEST FAILED: timeout.\n");
1042 return (0x20);

My (wild) guess is that we're patching away some memory barrier or cache flush so the SCRIPTS engine in the SCSI controller starts executing garbage and triggers a PCI bus read/write to an invalid address. The reason the INB() is given as the HPMC location is likely caused by the delay between writing DSPS and the chip actually starting to fetch insn/data.

Does that ring any bell for someone on the list? Otherwise i can check the alternatives patching over the weekend, i think there are not that many locations.

The good thing is it's reproducible - it always crashes. Either in SCSI or in Tulip.

Regards
Sven

#1 lore raw
Quoted 11 lines · Sven Schnelle

Hi List,

i recently got my hands on an old C240. I see a Kernel oops pretty early when alternatives patching is enabled:
[..]
My (wild) guess is that we're patching away some memory barrier or cache flush so the SCRIPTS engine in the SCSI controller starts executing garbage and triggers a PCI bus read/write to an invalid address. The reason the INB() is given as the HPMC location is likely caused by the delay between writing DSPS and the chip actually starting to fetch insn/data.

Quoted 5 lines

Does that ring any bell for someone on the list? Otherwise i can check the alternatives patching over the weekend, i think there are not that many locations.

The good thing is it's reproducible - it always crashes. Either in SCSI or in Tulip.

Did a quick test, removing ALT_COND_N_IOC_FDC from asm_io_fdc() seems to fix this issue. Haven't looked in more detail into this though.

index 73ca89a47f49..d83b1adf2f3f 100644

arch/parisc/include/asm/cache.h +0 -1
@@ -52,7 +52,6 @@ void parisc_setup_cache_timing(void);
 
 #define asm_io_fdc(addr) asm volatile("fdc %%r0(%0)" \
                        ALTERNATIVE(ALT_COND_NO_DCACHE, INSN_NOP) \
-                       ALTERNATIVE(ALT_COND_NO_IOC_FDC, INSN_NOP) \
                        : : "r" (addr) : "memory")
 #define asm_io_sync()  asm volatile("sync" \
                        ALTERNATIVE(ALT_COND_NO_DCACHE, INSN_NOP) \

Sven

#2 lore raw

Hi List,

Quoted 12 lines · Sven Schnelle
Quoted 11 lines · Sven Schnelle

Hi List,

i recently got my hands on an old C240. I see a Kernel oops pretty early when alternatives patching is enabled:
[..]
My (wild) guess is that we're patching away some memory barrier or cache flush so the SCRIPTS engine in the SCSI controller starts executing garbage and triggers a PCI bus read/write to an invalid address. The reason the INB() is given as the HPMC location is likely caused by the delay between writing DSPS and the chip actually starting to fetch insn/data.

Quoted 8 lines
Quoted 5 lines

Does that ring any bell for someone on the list? Otherwise i can check the alternatives patching over the weekend, i think there are not that many locations.

The good thing is it's reproducible - it always crashes. Either in SCSI or in Tulip.

Did a quick test, removing ALT_COND_N_IOC_FDC from asm_io_fdc() seems to fix this issue. Haven't looked in more detail into this though.

Added some debugging:

[   25.405365] boot_cpu_data.pdc_capabilities: 2

So PDC says IO-PDIR fetches are not performed coherently, *BUT*:

When this bit is clear, flushes and syncs are not required. This bit is only applicable to SBAs, and does not apply to Legacy IOAs.

With my limited understand i would think that C240 has a 'Legacy IOA' while C3xxx has SBA? So i think we would need to add some check whether we have an IOA or SBA in the alternatives code?

Sven

#3 lore raw

Hi List,

Quoted 7 lines · Sven Schnelle
Quoted 2 lines

Did a quick test, removing ALT_COND_N_IOC_FDC from asm_io_fdc() seems to fix this issue. Haven't looked in more detail into this though.

Added some debugging:

[   25.405365] boot_cpu_data.pdc_capabilities: 2
Quoted 8 lines

So PDC says IO-PDIR fetches are not performed coherently, *BUT*:

When this bit is clear, flushes and syncs are not required. This bit is only applicable to SBAs, and does not apply to Legacy IOAs.

With my limited understand i would think that C240 has a 'Legacy IOA' while C3xxx has SBA? So i think we would need to add some check whether we have an IOA or SBA in the alternatives code?

I did the patch below to check for legacy IO Adapters. Is HW_BCPORT the right type? On my C240 both GSC Adapters are HW_BCPORT.

From a5a444d0eb4960d7a1c7c4acf5eeb86b4e11e358 Mon Sep 17 00:00:00 2001
From: Sven Schnelle <[email protected]>
Date: Fri, 24 May 2019 17:33:28 +0200
Subject: [PATCH] parisc: fix alternative patching for Legacy IO systems

On systems with legacy IO Adapters we must ignore the IO-PDIR bit we get in the PDC_MODEL response. This fixes booting on HP9000/C240.

---
 arch/parisc/include/asm/hardware.h |  1 +
 arch/parisc/kernel/alternative.c   |  8 +++++---
 arch/parisc/kernel/drivers.c       | 14 ++++++++++++++
 3 files changed, 20 insertions(+), 3 deletions(-)
arch/parisc/include/asm/hardware.h +1 -0
index 9d3d7737c58b..5fb7a3c3eb46 100644
@@ -121,6 +121,7 @@ extern void init_parisc_bus(void);
 extern struct device *hwpath_to_device(struct hardware_path *modpath);
 extern void device_to_hwpath(struct device *dev, struct hardware_path *path);
 extern int machine_has_merced_bus(void);
+extern int machine_has_ioa(void);
 
 /* inventory.c: */
 extern void do_memory_inventory(void);
arch/parisc/kernel/alternative.c +5 -3
index bf2274e01a96..ca6368e6e96a 100644
@@ -25,6 +25,7 @@ void __init_or_module apply_alternatives(struct alt_instr *start,
 	struct alt_instr *entry;
 	int index = 0, applied = 0;
 	int num_cpus = num_online_cpus();
+	int has_ioa = machine_has_ioa();
 
 	for (entry = start; entry < end; entry++, index++) {
 
@@ -53,10 +54,11 @@ void __init_or_module apply_alternatives(struct alt_instr *start,
 		/*
 		 * If the PDC_MODEL capabilities has Non-coherent IO-PDIR bit
 		 * set (bit #61, big endian), we have to flush and sync every
-		 * time IO-PDIR is changed in Ike/Astro.
+		 * time IO-PDIR is changed in Ike/Astro. If legacy IOAs are
+		 * present we're not allowed to skip these flushes/syncs.
 		 */
-		if ((cond & ALT_COND_NO_IOC_FDC) &&
-			(boot_cpu_data.pdc.capabilities & PDC_MODEL_IOPDIR_FDC))
+		if (((cond & ALT_COND_NO_IOC_FDC) && (has_ioa ||
+		      (boot_cpu_data.pdc.capabilities & PDC_MODEL_IOPDIR_FDC))))
 			continue;
 
 		/* Want to replace pdtlb by a pdtlb,l instruction? */
arch/parisc/kernel/drivers.c +14 -0
index 00a181f1ecc6..4c93b416df99 100644
@@ -282,6 +282,20 @@ int __init machine_has_merced_bus(void)
 	return ret ? 1 : 0;
 }
 
+static int __init is_IOA_device(struct device *dev, void *data)
+{
+	struct parisc_device *pdev = to_parisc_device(dev);
+
+	if (!check_dev(dev))
+		return 0;
+	return pdev->id.hw_type == HPHW_BCPORT;
+}
+
+int __init machine_has_ioa(void)
+{
+	return !!for_each_padev(is_IOA_device, NULL);
+}
+
 /**
  * find_pa_parent_type - Find a parent of a specific type
  * @dev: The device to start searching from
Signature · 1 line
2.20.1
#4 lore raw

arch/parisc/include/asm/hardware.h extern void init_parisc_bus(void); · line 124

Quoted 10 lines
(quoted diff) +0 -0

b/arch/parisc/include/asm/hardware.h index 9d3d7737c58b..5fb7a3c3eb46 100644

arch/parisc/include/asm/hardware.h +1 -0
@@ -121,6 +121,7 @@ extern void init_parisc_bus(void);
 extern struct device *hwpath_to_device(struct hardware_path *modpath);
 extern void device_to_hwpath(struct device *dev, struct hardware_path
 *path); extern int machine_has_merced_bus(void);
+extern int machine_has_ioa(void);

This could return bool.

maintainer
#5 lore raw
In reply to Sven Schnelle

* Sven Schnelle <[email protected]>:

Quoted 6 lines · Sven Schnelle
Quoted 5 lines
Quoted 2 lines

Did a quick test, removing ALT_COND_N_IOC_FDC from asm_io_fdc() seems to fix this issue. Haven't looked in more detail into this though.

Added some debugging:

[   25.405365] boot_cpu_data.pdc_capabilities: 2

In case it would have booted, one could see that via:

# grep capabilities /proc/cpuinfo
capabilities    : os64 iopdir_fdc needs_equivalent_aliasing (0x35)
Quoted 11 lines
Quoted 8 lines

So PDC says IO-PDIR fetches are not performed coherently, *BUT*:

When this bit is clear, flushes and syncs are not required. This bit is only applicable to SBAs, and does not apply to Legacy IOAs.

With my limited understand i would think that C240 has a 'Legacy IOA' while C3xxx has SBA? So i think we would need to add some check whether we have an IOA or SBA in the alternatives code?

I did the patch below to check for legacy IO Adapters. Is HW_BCPORT the right type? On my C240 both GSC Adapters are HW_BCPORT.

I'm not sure.
Seems to be dependend on the CPU.
See comment in drivers/parisc/ccio-dma.c, line 607ff:

/* FIXME: PCX_W platforms don't need FDC/SYNC. (eg C360)
** PCX-U/U+ do. (eg C200/C240)
** PCX-T'? Don't know. (eg C110 or similar K-class)
**
** See PDC_MODEL/option 0/SW_CAP word for "Non-coherent IO-PDIR bit". **
** "Since PCX-U employs an offset hash that is incompatible with ** the real mode coherence index generation of U2, the PDIR entry ** must be flushed to memory to retain coherence."

Can you try this patch instead?

arch/parisc/kernel/alternative.c +2 -1
index bf2274e01a96..7c574b21f834 100644
@@ -56,7 +56,8 @@ void __init_or_module apply_alternatives(struct alt_instr *start,
 		 * time IO-PDIR is changed in Ike/Astro.
 		 */
 		if ((cond & ALT_COND_NO_IOC_FDC) &&
-			(boot_cpu_data.pdc.capabilities & PDC_MODEL_IOPDIR_FDC))
+			((boot_cpu_data.cpu_type < pcxw) ||
+			 (boot_cpu_data.pdc.capabilities & PDC_MODEL_IOPDIR_FDC)))
 			continue;
 
 		/* Want to replace pdtlb by a pdtlb,l instruction? */

Helge

#6 lore raw

Hi Helge,

Quoted 18 lines · Helge Deller
Quoted 2 lines

I did the patch below to check for legacy IO Adapters. Is HW_BCPORT the right type? On my C240 both GSC Adapters are HW_BCPORT.

I'm not sure.
Seems to be dependend on the CPU.
See comment in drivers/parisc/ccio-dma.c, line 607ff:

/* FIXME: PCX_W platforms don't need FDC/SYNC. (eg C360)
** PCX-U/U+ do. (eg C200/C240)
** PCX-T'? Don't know. (eg C110 or similar K-class)
**
** See PDC_MODEL/option 0/SW_CAP word for "Non-coherent IO-PDIR bit". **
** "Since PCX-U employs an offset hash that is incompatible with ** the real mode coherence index generation of U2, the PDIR entry ** must be flushed to memory to retain coherence."

Quoted 2 lines

Can you try this patch instead?
[..]

Works on my C240 and C3750. Thanks!

Regards
Sven