NetBSD Problem Report #55139

From tsutsui@ceres.dti.ne.jp  Fri Apr  3 18:00:47 2020
Return-Path: <tsutsui@ceres.dti.ne.jp>
Received: from mail.netbsd.org (mail.netbsd.org [199.233.217.200])
	(using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits))
	(Client CN "mail.NetBSD.org", Issuer "mail.NetBSD.org CA" (not verified))
	by mollari.NetBSD.org (Postfix) with ESMTPS id AA2111A9213
	for <gnats-bugs@gnats.NetBSD.org>; Fri,  3 Apr 2020 18:00:47 +0000 (UTC)
Message-Id: <202004031800.033I0cFw003643@ceres.dti.ne.jp>
Date: Sat, 4 Apr 2020 03:00:38 +0900 (JST)
From: Izumi Tsutsui <tsutsui@ceres.dti.ne.jp>
Reply-To: tsutsui@ceres.dti.ne.jp
To: gnats-bugs@NetBSD.org
Cc: tsutsui@ceres.dti.ne.jp
Subject: Misunderstanding on R5000 L2 cache Page_Invalidate(S) op size
X-Send-Pr-Version: 3.95

>Number:         55139
>Category:       port-mips
>Synopsis:       Misunderstanding on R5000 L2 cache Page_Invalidate(S) op size
>Confidential:   no
>Severity:       non-critical
>Priority:       medium
>Responsible:    port-mips-maintainer
>State:          closed
>Class:          sw-bug
>Submitter-Id:   net
>Arrival-Date:   Fri Apr 03 18:05:00 +0000 2020
>Closed-Date:    Sun Jun 21 07:12:06 +0000 2020
>Last-Modified:  Sun Jun 21 07:12:06 +0000 2020
>Originator:     Izumi Tsutsui
>Release:        NetBSD 9.0
>Organization:
>Environment:
System: NetBSD 9.0
Architecture: mips
Machine: R5000 mips
>Description:
In src/sys/arch/mips/mips/cache_r5k.c, there is inconsistent size
definitions for block size of cache flush ops:

---
#define	mips_r5k_round_page(x)	round_line(x, PAGE_SIZE)
#define	mips_r5k_trunc_page(x)	trunc_line(x, PAGE_SIZE)

void
r5k_sdcache_wbinv_range(register_t va, vsize_t size)
{
	uint32_t ostatus, taglo;
	register_t eva = mips_r5k_round_page(va + size);

	va = mips_r5k_trunc_page(va);

	ostatus = mips_cp0_status_read();
	mips_cp0_status_write(ostatus & ~MIPS_SR_INT_IE);

	__asm volatile("mfc0 %0, $28" : "=r"(taglo));
	__asm volatile("mtc0 $0, $28");

	for (; va < eva; va += (128 * 32)) {
		cache_op_r4k_line(va, CACHEOP_R4K_HIT_WB_INV|CACHE_R4K_SD);
	}

	mips_cp0_status_write(ostatus);
	__asm volatile("mtc0 %0, $28; nop" :: "r"(taglo));
}
---

I.e. the start address and end address are truncated/rounded up
per PAGE_SIZE:
>> #define	mips_r5k_round_page(x)	round_line(x, PAGE_SIZE)
>> #define	mips_r5k_trunc_page(x)	trunc_line(x, PAGE_SIZE)

but "(128 * 32)" (i.e. 4096) is used in the flush address iteration:
>>	for (; va < eva; va += (128 * 32)) {
>>		cache_op_r4k_line(va, CACHEOP_R4K_HIT_WB_INV|CACHE_R4K_SD);
>>	}

The former part was changed in rev 1.17:
 http://cvsweb.netbsd.org/bsdweb.cgi/src/sys/arch/mips/mips/cache_r5k.c#rev1.17
>> - don't clear KX when disabling interrupts
>> - sign extend addresses as needed
>> - use PAGE_SIZE instead of blindly assuming 4KB
>> now n32 kernels work again on my R5k SGIs

Currently MIPS3 kernels seem to use 8192 as PAGE_SIZE by default,
and if the latter (128 * 32) is changed to PAGE_SIZE, most DMA ops
(ahc(4) disk xfers) fail on O2 (DMA ops on O2 are not cache coherent).

According to "User's Manual VR5000 TM, VR10000 TM 64-BIT MICROPROCESSOR
INSTRUCTION" (Document No. U12754EJ) by NEC, the "Page Invalidate (S)"
(R5000 only) op is described as:
---
1.4.19 Page Invalidate (S) (R5000 only)
 The processor will generate a page invalidate by doing a burst of
 128 line invalidates to the secondary cache at the page specified
 by the effective address generated by the CACHE instruction, which
 must be page aligned. Interrupts are deferred during page invalidates.
---

R5000 L2 cacheline size is 32, so the the original implementation
"(128 * 32)" shall be a correct for R5000 Page Invalidate (S) op,
and PAGE_SIZE is not correct. Extra truncation/roundup aginst flush
regions would be harmless except small performance penalty, though.

>How-To-Repeat:
Code inspection.

>Fix:
Revert PAGE_SIZE changes to (128 * 32), or
define and use proper macro for a size of R5K_Page_Invalidate_S op
as Linux does?
 https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/tree/arch/mips/mm/sc-r5k.c?h=v5.6.1#n22
>> #define SC_LINE 32
>> #define SC_PAGE (128*SC_LINE)

>Release-Note:

>Audit-Trail:
From: Nick Hudson <nick.hudson@gmx.co.uk>
To: gnats-bugs@netbsd.org, port-mips-maintainer@netbsd.org,
 gnats-admin@netbsd.org, netbsd-bugs@netbsd.org
Cc: 
Subject: Re: port-mips/55139: Misunderstanding on R5000 L2 cache
 Page_Invalidate(S) op size
Date: Sat, 4 Apr 2020 11:26:48 +0100

 On 03/04/2020 19:05, Izumi Tsutsui wrote:
 >> Number:         55139
 [snip]

 > but "(128 * 32)" (i.e. 4096) is used in the flush address iteration:
 >>> 	for (; va < eva; va += (128 * 32)) {
 >>> 		cache_op_r4k_line(va, CACHEOP_R4K_HIT_WB_INV|CACHE_R4K_SD);
 >>> 	}

 [snip]

 why isn't this correct? I didn't follow your logic


 > R5000 L2 cacheline size is 32, so the the original implementation
 > "(128 * 32)" shall be a correct for R5000 Page Invalidate (S) op,
 > and PAGE_SIZE is not correct. Extra truncation/roundup aginst flush
 > regions would be harmless except small performance penalty, though.

 sys/arch/mips/mips/cache_r5k.c:1.17 just did the extra-truncation/roundup.

 I'm obviously missing something here...

 Nick

From: Izumi Tsutsui <tsutsui@ceres.dti.ne.jp>
To: nick.hudson@gmx.co.uk
Cc: gnats-bugs@netbsd.org, tsutsui@ceres.dti.ne.jp
Subject: Re: port-mips/55139: Misunderstanding on R5000 L2 cachePage_Invalidate(S)
	 op size
Date: Sat, 4 Apr 2020 22:21:00 +0900

 > why isn't this correct? I didn't follow your logic

 There are two points:

 1) trunc/round macros were changed from "(128 * 32)" to "PAGE_SIZE", but
    a corresponding increment count in iteration was left as "(128 * 32)".
    (I'm not sure if it was intentional or not)

 2) R5000 Page_Invalidate_S op flushes fixed 4096 bytes (128 lines),
    but "PAGE_SIZE" on MIPS3 is no longer 4096 (== 128 * 32) by default:

  https://nxr.netbsd.org/xref/src/sys/arch/mips/include/mips_param.h?r=1.38#87
 ---
 #ifdef ENABLE_MIPS_16KB_PAGE
 #define	PGSHIFT		14		/* LOG2(NBPG) */
 #elif defined(ENABLE_MIPS_8KB_PAGE) \
     || (!defined(ENABLE_MIPS_4KB_PAGE) && __mips >= 3)
 #define	PGSHIFT		13		/* LOG2(NBPG) */
 #else
 #define	PGSHIFT		12		/* LOG2(NBPG) */
 #endif
 ---

 ---
 Izumi Tsustui

From: "Izumi Tsutsui" <tsutsui@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc: 
Subject: PR/55139 CVS commit: src/sys/arch/mips
Date: Sun, 14 Jun 2020 15:12:57 +0000

 Module Name:	src
 Committed By:	tsutsui
 Date:		Sun Jun 14 15:12:56 UTC 2020

 Modified Files:
 	src/sys/arch/mips/include: cache_r5k.h
 	src/sys/arch/mips/mips: cache_r5k.c cache_r5k_subr.S

 Log Message:
 Use proper "page" alignments for R5k Page Invalidate(S) op.  PR/55139

 According to NEC "User's Manual VR5000, VR1000 64-BIT MICROPROCESSOR
 INSTRUCTION" (U12754EJ1V0UMJ1), R5000 Page Invalidate (S) op does
 "a page invalidate by doing a burst of 128 line invalidates to
 the secondary cache at the page specified by the effective address
 generated by the CACHE instruction, which must be page aligned."

 This description looks a bit confusing, but "page" used here
 implies fixed 32 byte cacheline * 128 lines == 4096 bytes,
 not our variable "PAGE_SIZE" used in VM paging ops.  Note
 the current default PAGE_SIZE for MIPS3 has been changed to 8192.

 While here, also define and use proper macro for the "page" and CACHEOP
 arg for the R5k Page_Invalidate_S op, as the manual also describes
 the cache op field 10111 as "Page Invalidate" for the secondary cache.

 No visible regression on Cobalt Qube 2700 (Rm5230) through
 whole installation using netbsd-9 based Cobalt RestoreCD/USB.


 To generate a diff of this commit:
 cvs rdiff -u -r1.4 -r1.5 src/sys/arch/mips/include/cache_r5k.h
 cvs rdiff -u -r1.20 -r1.21 src/sys/arch/mips/mips/cache_r5k.c
 cvs rdiff -u -r1.3 -r1.4 src/sys/arch/mips/mips/cache_r5k_subr.S

 Please note that diffs are not public domain; they are subject to the
 copyright notices on the relevant files.

State-Changed-From-To: open->pending-pullups
State-Changed-By: tsutsui@NetBSD.org
State-Changed-When: Thu, 18 Jun 2020 17:38:15 +0000
State-Changed-Why:
[pullup-9 #965]


From: "Martin Husemann" <martin@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc: 
Subject: PR/55139 CVS commit: [netbsd-9] src/sys/arch/mips
Date: Sat, 20 Jun 2020 16:38:42 +0000

 Module Name:	src
 Committed By:	martin
 Date:		Sat Jun 20 16:38:42 UTC 2020

 Modified Files:
 	src/sys/arch/mips/include [netbsd-9]: cache_r5k.h
 	src/sys/arch/mips/mips [netbsd-9]: cache_r5k.c cache_r5k_subr.S

 Log Message:
 Pull up following revision(s) (requested by tsutsui in ticket #965):

 	sys/arch/mips/include/cache_r5k.h: revision 1.5
 	sys/arch/mips/mips/cache_r5k_subr.S: revision 1.4
 	sys/arch/mips/mips/cache_r5k.c: revision 1.21

 Use proper "page" alignments for R5k Page Invalidate(S) op.  PR/55139

 According to NEC "User's Manual VR5000, VR1000 64-BIT MICROPROCESSOR
 INSTRUCTION" (U12754EJ1V0UMJ1), R5000 Page Invalidate (S) op does
 "a page invalidate by doing a burst of 128 line invalidates to
 the secondary cache at the page specified by the effective address
 generated by the CACHE instruction, which must be page aligned."

 This description looks a bit confusing, but "page" used here
 implies fixed 32 byte cacheline * 128 lines == 4096 bytes,
 not our variable "PAGE_SIZE" used in VM paging ops.  Note
 the current default PAGE_SIZE for MIPS3 has been changed to 8192.

 While here, also define and use proper macro for the "page" and CACHEOP
 arg for the R5k Page_Invalidate_S op, as the manual also describes
 the cache op field 10111 as "Page Invalidate" for the secondary cache.

 No visible regression on Cobalt Qube 2700 (Rm5230) through
 whole installation using netbsd-9 based Cobalt RestoreCD/USB.


 To generate a diff of this commit:
 cvs rdiff -u -r1.4 -r1.4.22.1 src/sys/arch/mips/include/cache_r5k.h
 cvs rdiff -u -r1.20 -r1.20.14.1 src/sys/arch/mips/mips/cache_r5k.c
 cvs rdiff -u -r1.3 -r1.3.60.1 src/sys/arch/mips/mips/cache_r5k_subr.S

 Please note that diffs are not public domain; they are subject to the
 copyright notices on the relevant files.

State-Changed-From-To: pending-pullups->closed
State-Changed-By: tsutsui@NetBSD.org
State-Changed-When: Sun, 21 Jun 2020 07:12:06 +0000
State-Changed-Why:
Pullup complete.


>Unformatted:

Home	PR Database Search