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

[Konrad Rzeszutek Wilk]

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
>