RE: [PATCH] x86/PAT: priority the PAT warn to error to highlight the developer

From: Zhang, Jun
Date: Tue Oct 01 2019 - 01:02:49 EST

Next message: kbuild test robot: "Re: [PATCH v13 7/9] open: openat2(2) syscall"
Previous message: Alan Kao: "Re: [PATCH v2 0/3] Add support for SBI v0.2"
Next in thread: Peter Zijlstra: "Re: [PATCH] x86/PAT: priority the PAT warn to error to highlight the developer"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

Please see my comments.

Thanks,
Jun

-----Original Message-----
From: Borislav Petkov <bp@xxxxxxxxx>
Sent: Monday, September 30, 2019 8:03 PM
To: Zhang, Jun <jun.zhang@xxxxxxxxx>
Cc: dave.hansen@xxxxxxxxxxxxxxx; luto@xxxxxxxxxx; peterz@xxxxxxxxxxxxx; tglx@xxxxxxxxxxxxx; mingo@xxxxxxxxxx; hpa@xxxxxxxxx; He, Bo <bo.he@xxxxxxxxx>; x86@xxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx
Subject: Re: [PATCH] x86/PAT: priority the PAT warn to error to highlight the developer

On Sun, Sep 29, 2019 at 03:20:31PM +0800, jun.zhang@xxxxxxxxx wrote:
> From: zhang jun <jun.zhang@xxxxxxxxx>
>
> Documentation/x86/pat.txt says:
> set_memory_uc() or set_memory_wc() must use together with
> set_memory_wb()

I had to open that file to see what it actually says - btw, the filename is pat.rst now - and you're very heavily paraphrasing what is there. So try again explaining what the requirement is.
[ZJ] next parts come from pat.txt in kernel version 4.19
Drivers wanting to export some pages to userspace do it by using mmap
interface and a combination of
1) pgprot_noncached()
2) io_remap_pfn_range() or remap_pfn_range() or vm_insert_pfn()

With PAT support, a new API pgprot_writecombine is being added. So, drivers can
continue to use the above sequence, with either pgprot_noncached() or
pgprot_writecombine() in step 1, followed by step 2.

In addition, step 2 internally tracks the region as UC or WC in memtype
list in order to ensure no conflicting mapping.

Note that this set of APIs only works with IO (non RAM) regions. If driver
wants to export a RAM region, it has to do set_memory_uc() or set_memory_wc()
as step 0 above and also track the usage of those pages and use set_memory_wb()
before the page is freed to free pool.

> if break the PAT attribute, there are tons of warning like:
> [ 45.846872] x86/PAT: NDK MediaCodec_:3753 map pfn RAM range req

That's some android NDK thing, it seems: "The Android NDK is a toolset that lets you implement parts of your app in native code,... " lemme guess, they have a kernel module?
[ZJ] no, "NDK MediaCodec_" is an android codec2.0 process. It want to use WC memory.

> write-combining for [mem 0x1e7a80000-0x1e7a87fff], got write-back and
> in the extremely case, we see kernel panic unexpected like:
> list_del corruption. prev->next should be ffff88806dbe69c0, but was
> ffff888036f048c0

This is not really helpful. You need to explain what exactly you're doing - not shortening the error messages.
[ZJ] android codec2.0 want to use WC memory. Which use ion to allocate memory. So, we enable drivers/staging/android/ion, which work well except X86, x86 need to set_memory_wc().
So there are tons of warning, then list_del corruption. I use this patch(https://www.lkml.org/lkml/2019/9/29/25), list crash disappear.
Next is error message.
<4>[49967.389732] x86/PAT: NDK MediaCodec_:10602 map pfn RAM range req write-combining for [mem 0x1091f8000-0x1091f8fff], got write-back
<4>[49967.389747] x86/PAT: .vorbis.decoder:10606 map pfn RAM range req write-combining for [mem 0x1091f4000-0x1091f7fff], got write-back
<4>[49967.390622] x86/PAT: .vorbis.decoder:10606 map pfn RAM range req write-combining for [mem 0x109090000-0x109090fff], got write-back
<4>[49967.390687] x86/PAT: .vorbis.decoder:10606 map pfn RAM range req write-combining for [mem 0x1091f8000-0x1091fbfff], got write-back
<4>[49967.390855] x86/PAT: NDK MediaCodec_:10602 map pfn RAM range req write-combining for [mem 0x1091f4000-0x1091f4fff], got write-back
<4>[49967.391405] x86/PAT: .vorbis.decoder:10606 map pfn RAM range req write-combining for [mem 0x109098000-0x109098fff], got write-back
<4>[49967.391454] x86/PAT: NDK MediaCodec_:10602 map pfn RAM range req write-combining for [mem 0x1091f8000-0x1091f8fff], got write-back
<4>[49967.391474] x86/PAT: .vorbis.decoder:10606 map pfn RAM range req write-combining for [mem 0x1091f4000-0x1091f7fff], got write-back
<4>[49967.392641] x86/PAT: .vorbis.decoder:10606 map pfn RAM range req write-combining for [mem 0x14eb68000-0x14eb68fff], got write-back
<4>[49967.392708] x86/PAT: .vorbis.decoder:10606 map pfn RAM range req write-combining for [mem 0x1091f8000-0x1091fbfff], got write-back
<4>[49967.393001] x86/PAT: NDK MediaCodec_:10602 map pfn RAM range req write-combining for [mem 0x1091f4000-0x1091f4fff], got write-back
<4>[49967.394066] x86/PAT: NDK MediaCodec_:10602 map pfn RAM range req write-combining for [mem 0x1091f8000-0x1091f8fff], got write-back
<6>[50045.677129] binder: 3390:3390 transaction failed 29189/-22, size 88-0 line 3131
<3>[50046.153621] list_del corruption. prev->next should be ffff89598004c960, but was ffff895ad46e4590
<4>[50046.163464] invalid opcode: 0000 [#1] PREEMPT SMP NOPTI
<4>[50046.169297] CPU: 1 PID: 18792 Comm: Binder:3390_1B Tainted: G U O 4.19.68-PKT-190905T163945Z-00031-g9de920e66b4e #1
<4>[50046.182213] RIP: 0010:__list_del_entry_valid+0x78/0x90
<4>[50046.187947] Code: e8 00 a1 c1 ff 0f 0b 48 89 fe 48 c7 c7 60 1a b6 a5 e8 ef a0 c1 ff 0f 0b 48 89 f2 48 89 fe 48 c7 c7 98 1a b6 a5 e8 db a0 c1 ff <0f> 0b 48 c7 c7 d8 1a b6 a5 e8 cd a0 c1 ff 0f 0b 90 90 90 90 90 90
<4>[50046.208906] RSP: 0018:ffffa5d5c85cbc88 EFLAGS: 00010282
<4>[50046.214733] RAX: 0000000000000054 RBX: ffff89598004c960 RCX: 0000000000000000
<4>[50046.222689] RDX: 0000000000000000 RSI: ffffffffa5b3c047 RDI: 00000000ffffffff
<4>[50046.230645] RBP: ffffa5d5c85cbc88 R08: 00000000001d8f83 R09: 0000000000000036
<4>[50046.238608] R10: ffffa5d5c7e97b08 R11: ffffffffa5b10d5e R12: ffff895ad46e4400
<4>[50046.246570] R13: ffff8959d9237e48 R14: ffff8959d9237e00 R15: ffff895ad46e4400
<4>[50046.254535] FS: 0000736cc6941010(0000) GS:ffff895afba80000(0000) knlGS:0000736ccaaa9400
<4>[50046.263568] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
<4>[50046.269982] CR2: 00000000133bb018 CR3: 000000014e91c000 CR4: 00000000003406e0
<4>[50046.277950] Call Trace:
<4>[50046.280680] binder_dequeue_work_head_ilocked+0x1f/0x50
<4>[50046.286512] binder_thread_read+0x2c3/0x12e0
<4>[50046.291277] ? binder_alloc_free_buf+0x2a/0x30
<4>[50046.296238] ? finish_wait+0x90/0x90
<4>[50046.300226] ? __might_sleep+0x1b/0x20
<4>[50046.304404] binder_ioctl+0x79d/0xa46
<4>[50046.308489] do_vfs_ioctl+0xa9/0x6d0
<4>[50046.312480] ? _raw_spin_unlock_irqrestore+0x28/0x50
<4>[50046.318021] ksys_ioctl+0x75/0x80
<4>[50046.321719] __x64_sys_ioctl+0x1a/0x20
<4>[50046.325902] do_syscall_64+0x55/0x100
<4>[50046.329984] entry_SYSCALL_64_after_hwframe+0x49/0xbe
<4>[50046.335620] RIP: 0033:0x736dae4f57b7
<4>[50046.339606] Code: cc cc cc b8 60 00 00 00 0f 05 48 3d 01 f0 ff ff 72 09 f7 d8 89 c7 e8 c8 fc ff ff c3 cc cc cc cc cc cc cc b8 10 00 00 00 0f 05 <48> 3d 01 f0 ff ff 72 09 f7 d8 89 c7 e8 a8 fc ff ff c3 cc cc cc cc
<4>[50046.360566] RSP: 002b:0000736cc6940aa8 EFLAGS: 00000206 ORIG_RAX: 0000000000000010
<4>[50046.369018] RAX: ffffffffffffffda RBX: 0000000000000000 RCX: 0000736dae4f57b7
<4>[50046.376979] RDX: 0000736cc6940ba0 RSI: 00000000c0306201 RDI: 0000000000000021
<4>[50046.384940] RBP: 00000000fffffff7 R08: 0000000000000000 R09: 0000000000000000
<4>[50046.392903] R10: 0000000000000000 R11: 0000000000000206 R12: 0000736d20c68a00
<4>[50046.400863] R13: 0000736cc6940ba0 R14: 0000736d20c68aa8 R15: 0000736d20c68b20
<4>[50046.408826] Modules linked in: hci_uart bluetooth ecdh_generic rfkill_gpio dwc3_pci dwc3 pcie8xxx(O) snd_usb_audio xhci_pci tpm_crb mei_me xhci_hcd snd_usbmidi_lib tpm mlan(O) mei snd_hwdep igb_avb(O) cfg80211 brd snd_soc_sst_bxt_tdf8532 snd_soc_tdf8532 snd_soc_skl trusty_timer snd_soc_skl_ipc trusty_wall snd_soc_sst_ipc trusty_log snd_soc_sst_dsp trusty_virtio trusty_ipc trusty_mem snd_hda_ext_core trusty snd_hda_core virtio_ring virtio videobuf2_dma_sg
<4>[50046.454020] ---[ end trace 66821e043574850e ]---

> so it's better to priority the warn to error to highlight to remind
> the developer.

Whut?

From reading what is trying hard to be a sentence, I can only guess what you're trying to say here. And it doesn't make it clear why is pr_warn() not enough and it has to be pr_err().
[ZJ] usually warning don't output to console, and could be ignored. I think PAT error could cause crash, so it is very serious.

> Signed-off-by: zhang jun <jun.zhang@xxxxxxxxx>
> Signed-off-by: he, bo <bo.he@xxxxxxxxx>

And this SOB chain is wrong.
[ZJ] What is SOB? I use git format-patch -1. Checkpatch.pl, then git send-email.

> ---
> arch/x86/mm/pat.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/arch/x86/mm/pat.c b/arch/x86/mm/pat.c index
> d9fbd4f69920..43a4dfdcedc8 100644
> --- a/arch/x86/mm/pat.c
> +++ b/arch/x86/mm/pat.c
> @@ -897,7 +897,7 @@ static int reserve_pfn_range(u64 paddr, unsigned
> long size, pgprot_t *vma_prot,
>
> pcm = lookup_memtype(paddr);
> if (want_pcm != pcm) {
> - pr_warn("x86/PAT: %s:%d map pfn RAM range req %s for [mem %#010Lx-%#010Lx], got %s\n",
> + pr_err("x86/PAT: %s:%d map pfn RAM range req %s for [mem
> +%#010Lx-%#010Lx], got %s!!!\n",

Three "!!!" would make this more urgent, huh?

How about you make the error message more informative and user-friendly, instead?
[ZJ] the whole log, please see the attachment.

Thx.

--
Regards/Gruss,
Boris.

https://people.kernel.org/tglx/notes-about-netiquette

PAT (Page Attribute Table)

x86 Page Attribute Table (PAT) allows for setting the memory attribute at the
page level granularity. PAT is complementary to the MTRR settings which allows
for setting of memory types over physical address ranges. However, PAT is
more flexible than MTRR due to its capability to set attributes at page level
and also due to the fact that there are no hardware limitations on number of
such attribute settings allowed. Added flexibility comes with guidelines for
not having memory type aliasing for the same physical memory with multiple
virtual addresses.

PAT allows for different types of memory attributes. The most commonly used
ones that will be supported at this time are Write-back, Uncached,
Write-combined, Write-through and Uncached Minus.

PAT APIs
--------

There are many different APIs in the kernel that allows setting of memory
attributes at the page level. In order to avoid aliasing, these interfaces
should be used thoughtfully. Below is a table of interfaces available,
their intended usage and their memory attribute relationships. Internally,
these APIs use a reserve_memtype()/free_memtype() interface on the physical
address range to avoid any aliasing.

-------------------------------------------------------------------
API | RAM | ACPI,... | Reserved/Holes |
-----------------------|----------|------------|------------------|
| | | |
ioremap | -- | UC- | UC- |
| | | |
ioremap_cache | -- | WB | WB |
| | | |
ioremap_uc | -- | UC | UC |
| | | |
ioremap_nocache | -- | UC- | UC- |
| | | |
ioremap_wc | -- | -- | WC |
| | | |
ioremap_wt | -- | -- | WT |
| | | |
set_memory_uc | UC- | -- | -- |
set_memory_wb | | | |
| | | |
set_memory_wc | WC | -- | -- |
set_memory_wb | | | |
| | | |
set_memory_wt | WT | -- | -- |
set_memory_wb | | | |
| | | |
pci sysfs resource | -- | -- | UC- |
| | | |
pci sysfs resource_wc | -- | -- | WC |
is IORESOURCE_PREFETCH| | | |
| | | |
pci proc | -- | -- | UC- |
!PCIIOC_WRITE_COMBINE | | | |
| | | |
pci proc | -- | -- | WC |
PCIIOC_WRITE_COMBINE | | | |
| | | |
/dev/mem | -- | WB/WC/UC- | WB/WC/UC- |
read-write | | | |
| | | |
/dev/mem | -- | UC- | UC- |
mmap SYNC flag | | | |
| | | |
/dev/mem | -- | WB/WC/UC- | WB/WC/UC- |
mmap !SYNC flag | |(from exist-| (from exist- |
and | | ing alias)| ing alias) |
any alias to this area| | | |
| | | |
/dev/mem | -- | WB | WB |
mmap !SYNC flag | | | |
no alias to this area | | | |
and | | | |
MTRR says WB | | | |
| | | |
/dev/mem | -- | -- | UC- |
mmap !SYNC flag | | | |
no alias to this area | | | |
and | | | |
MTRR says !WB | | | |
| | | |
-------------------------------------------------------------------

Advanced APIs for drivers
-------------------------
A. Exporting pages to users with remap_pfn_range, io_remap_pfn_range,
vm_insert_pfn

Drivers wanting to export some pages to userspace do it by using mmap
interface and a combination of
1) pgprot_noncached()
2) io_remap_pfn_range() or remap_pfn_range() or vm_insert_pfn()

With PAT support, a new API pgprot_writecombine is being added. So, drivers can
continue to use the above sequence, with either pgprot_noncached() or
pgprot_writecombine() in step 1, followed by step 2.

In addition, step 2 internally tracks the region as UC or WC in memtype
list in order to ensure no conflicting mapping.

Note that this set of APIs only works with IO (non RAM) regions. If driver
wants to export a RAM region, it has to do set_memory_uc() or set_memory_wc()
as step 0 above and also track the usage of those pages and use set_memory_wb()
before the page is freed to free pool.

MTRR effects on PAT / non-PAT systems
-------------------------------------

The following table provides the effects of using write-combining MTRRs when
using ioremap*() calls on x86 for both non-PAT and PAT systems. Ideally
mtrr_add() usage will be phased out in favor of arch_phys_wc_add() which will
be a no-op on PAT enabled systems. The region over which a arch_phys_wc_add()
is made, should already have been ioremapped with WC attributes or PAT entries,
this can be done by using ioremap_wc() / set_memory_wc(). Devices which
combine areas of IO memory desired to remain uncacheable with areas where
write-combining is desirable should consider use of ioremap_uc() followed by
set_memory_wc() to white-list effective write-combined areas. Such use is
nevertheless discouraged as the effective memory type is considered
implementation defined, yet this strategy can be used as last resort on devices
with size-constrained regions where otherwise MTRR write-combining would
otherwise not be effective.

----------------------------------------------------------------------
MTRR Non-PAT PAT Linux ioremap value Effective memory type
----------------------------------------------------------------------
Non-PAT | PAT
PAT
|PCD
||PWT
|||
WC 000 WB _PAGE_CACHE_MODE_WB WC | WC
WC 001 WC _PAGE_CACHE_MODE_WC WC* | WC
WC 010 UC- _PAGE_CACHE_MODE_UC_MINUS WC* | UC
WC 011 UC _PAGE_CACHE_MODE_UC UC | UC
----------------------------------------------------------------------

(*) denotes implementation defined and is discouraged

Notes:

-- in the above table mean "Not suggested usage for the API". Some of the --'s
are strictly enforced by the kernel. Some others are not really enforced
today, but may be enforced in future.

For ioremap and pci access through /sys or /proc - The actual type returned
can be more restrictive, in case of any existing aliasing for that address.
For example: If there is an existing uncached mapping, a new ioremap_wc can
return uncached mapping in place of write-combine requested.

set_memory_[uc|wc|wt] and set_memory_wb should be used in pairs, where driver
will first make a region uc, wc or wt and switch it back to wb after use.

Over time writes to /proc/mtrr will be deprecated in favor of using PAT based
interfaces. Users writing to /proc/mtrr are suggested to use above interfaces.

Drivers should use ioremap_[uc|wc] to access PCI BARs with [uc|wc] access
types.

Drivers should use set_memory_[uc|wc|wt] to set access type for RAM ranges.

PAT debugging
-------------

With CONFIG_DEBUG_FS enabled, PAT memtype list can be examined by

# mount -t debugfs debugfs /sys/kernel/debug
# cat /sys/kernel/debug/x86/pat_memtype_list
PAT memtype list:
uncached-minus @ 0x7fadf000-0x7fae0000
uncached-minus @ 0x7fb19000-0x7fb1a000
uncached-minus @ 0x7fb1a000-0x7fb1b000
uncached-minus @ 0x7fb1b000-0x7fb1c000
uncached-minus @ 0x7fb1c000-0x7fb1d000
uncached-minus @ 0x7fb1d000-0x7fb1e000
uncached-minus @ 0x7fb1e000-0x7fb25000
uncached-minus @ 0x7fb25000-0x7fb26000
uncached-minus @ 0x7fb26000-0x7fb27000
uncached-minus @ 0x7fb27000-0x7fb28000
uncached-minus @ 0x7fb28000-0x7fb2e000
uncached-minus @ 0x7fb2e000-0x7fb2f000
uncached-minus @ 0x7fb2f000-0x7fb30000
uncached-minus @ 0x7fb31000-0x7fb32000
uncached-minus @ 0x80000000-0x90000000

This list shows physical address ranges and various PAT settings used to
access those physical address ranges.

Another, more verbose way of getting PAT related debug messages is with
"debugpat" boot parameter. With this parameter, various debug messages are
printed to dmesg log.

PAT Initialization
------------------

The following table describes how PAT is initialized under various
configurations. The PAT MSR must be updated by Linux in order to support WC
and WT attributes. Otherwise, the PAT MSR has the value programmed in it
by the firmware. Note, Xen enables WC attribute in the PAT MSR for guests.

MTRR PAT Call Sequence PAT State PAT MSR
=========================================================
E E MTRR -> PAT init Enabled OS
E D MTRR -> PAT init Disabled -
D E MTRR -> PAT disable Disabled BIOS
D D MTRR -> PAT disable Disabled -
- np/E PAT -> PAT disable Disabled BIOS
- np/D PAT -> PAT disable Disabled -
E !P/E MTRR -> PAT init Disabled BIOS
D !P/E MTRR -> PAT disable Disabled BIOS
!M !P/E MTRR stub -> PAT disable Disabled BIOS

Legend
------------------------------------------------
E Feature enabled in CPU
D Feature disabled/unsupported in CPU
np "nopat" boot option specified
!P CONFIG_X86_PAT option unset
!M CONFIG_MTRR option unset
Enabled PAT state set to enabled
Disabled PAT state set to disabled
OS PAT initializes PAT MSR with OS setting
BIOS PAT keeps PAT MSR with BIOS setting

Attachment: apanic_console
Description: apanic_console

Next message: kbuild test robot: "Re: [PATCH v13 7/9] open: openat2(2) syscall"
Previous message: Alan Kao: "Re: [PATCH v2 0/3] Add support for SBI v0.2"
Next in thread: Peter Zijlstra: "Re: [PATCH] x86/PAT: priority the PAT warn to error to highlight the developer"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]