Re: XSA 154 and ISA region (640K -> 1MB) WB cache instead of UC

From: Konrad Rzeszutek Wilk
Date: Fri Aug 19 2016 - 10:53:18 EST


On Thu, Aug 18, 2016 at 04:06:33AM -0600, Jan Beulich wrote:
> >>> On 17.08.16 at 22:32, <konrad.wilk@xxxxxxxxxx> wrote:
> > One of the interesting things about XSA 154 fix ("x86: enforce consistent
> > cachability of MMIO mappings") is that when certain applications (mcelog)
> > are trying to map /dev/mmap and lurk in ISA regions - we get:
>
> DYM /dev/mem ? Most accesses to which are bogus in PV guests
> (often including Dom0) anyway.

Yes.
>
> > [ 49.399053] WARNING: CPU: 0 PID: 2471 at arch/x86/mm/pat.c:913 untrack_pfn+0x93/0xc0()
>
> What Linux version is this? untrack_pfn() doesn't span line 913 in

4.1
> 4.8-rc2. And follow_phys() appears to only check whether the write
> flag is set as expected; I can't see any cachability checks. Plus it
> gets called only when both incoming address and size are zero.

The error that happens is much sooner - that is when the VMA is setup
with the incorrect page attributes. Specifically: reserve_memtype which

548 /* Low ISA region is always mapped WB in page table. No need to track */
549 if (x86_platform.is_untracked_pat_range(start, end)) {
550 if (new_type)
551 *new_type = _PAGE_CACHE_MODE_WB;
552 return 0;
553 }

(And this for a change is v4.8-rc2)
>
> > Anyhow what I am wondering:
> >
> > a) Should we add a edge case in the hypervisor to allow multiple mappings
> > for this region? I am thinking no.. but it sounds like mapping ISA region
> > as WB is safe even in baremetal?
>
> We should never allow multiple mappings with different cachability.
> And I don't understand what makes you think the ISA region is safe
> to map WB? There might be ROMs, MMIO regions, or simply nothing
> there, neither of which is safe to map WB. ROMs certainly could be
> WP, but that would require a way to reliably size not only ISA
> extension ROMs, but also main and video BIOS.
>
> > b) Or would it be better to let Linux do its thing and treat 640KB->1MB
> > as uncached instead of writeback?
>
> According to what you wrote earlier the two parts of the sentence
> read contradictory to me.
>
> > Looking at the kernel it assumes that WB is ok for 640KB->1MB.
> > The comment says:
> > " /* Low ISA region is always mapped WB in page table. No need to track *"
>
> As per above it's not clear to me what this comment is backed by.

I was hoping you would know :-)

Ah, commit 2e5d9c857d4e6c9e7b7d8c8c86a68a7842d213d6
Author: venkatesh.pallipadi@xxxxxxxxx <venkatesh.pallipadi@xxxxxxxxx>
Date: Tue Mar 18 17:00:14 2008 -0700

x86: PAT infrastructure patch

Sets up pat_init() infrastructure.


which sets the MTRR for that region.
>
> Jan
>