Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1671857 > unrolled thread

Re: [PATCH] mm/hwpoison: Clear PRESENT bit for kernel 1:1 mappings of poison pages

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH] mm/hwpoison: Clear PRESENT bit for kernel 1:1 mappings  of poison pages "Luck, Tony" <tony.luck@intel.com> - 2017-06-21 19:50 +0200
    RE: [PATCH] mm/hwpoison: Clear PRESENT bit for kernel 1:1 mappings of  poison pages "Elliott, Robert (Persistent Memory)" <elliott@hpe.com> - 2017-06-21 22:00 +0200
      RE: [PATCH] mm/hwpoison: Clear PRESENT bit for kernel 1:1 mappings  of poison pages "Luck, Tony" <tony.luck@intel.com> - 2017-06-21 22:20 +0200
    Re: [PATCH] mm/hwpoison: Clear PRESENT bit for kernel 1:1 mappings  of poison pages Borislav Petkov <bp@suse.de> - 2017-06-22 11:40 +0200
      git send-email (w/o Cc: stable) "Luck, Tony" <tony.luck@intel.com> - 2017-06-30 00:20 +0200
        Re: git send-email (w/o Cc: stable) Borislav Petkov <bp@suse.de> - 2017-06-30 09:10 +0200
    RE: [PATCH] mm/hwpoison: Clear PRESENT bit for kernel 1:1 mappings of  poison pages "Elliott, Robert (Persistent Memory)" <elliott@hpe.com> - 2017-06-24 00:30 +0200
      Re: [PATCH] mm/hwpoison: Clear PRESENT bit for kernel 1:1 mappings of  poison pages Dan Williams <dan.j.williams@intel.com> - 2017-06-28 00:10 +0200
      RE: [PATCH] mm/hwpoison: Clear PRESENT bit for kernel 1:1 mappings  of poison pages "Luck, Tony" <tony.luck@intel.com> - 2017-06-28 00:10 +0200

#1671857 — Re: [PATCH] mm/hwpoison: Clear PRESENT bit for kernel 1:1 mappings of poison pages

On Mon, Jun 19, 2017 at 08:01:47PM +0200, Borislav Petkov wrote:
> (drop stable from CC)
> 
> You could use git's --suppress-cc= option when sending.

I would if I could work out how to use it. From reading the manual
page there seem to be a few options to this, but none of them appear
to just drop a specific address (apart from my own). :-(

> > +#ifdef CONFIG_X86_64
> > +
> > +void arch_unmap_kpfn(unsigned long pfn)
> > +{
> 
> I guess you can move the ifdeffery inside the function.

If I do, then the compiler will emit an empty function. It's only
a couple of bytes for the "ret" ... but why?  I may change it
to:

   #if defined(arch_unmap_kpfn) && defined(CONFIG_MEMORY_FAILURE)

to narrow down further when we need this.

> > +#if PGDIR_SHIFT + 9 < 63 /* 9 because cpp doesn't grok ilog2(PTRS_PER_PGD) */
> 
> Please no side comments.

Ok.

> Also, explain why the build-time check. (Sign-extension going away for VA
> space yadda yadda..., 5 2/3 level paging :-))

Will add.

> Also, I'm assuming this whole "workaround" of sorts should be Intel-only?

I'd assume that other X86 implementations would face similar issues (unless
they have extremely cautious pre-fetchers and/or no speculation).

I'm also assuming that non-X86 architectures that do recovery may want this
too ... hence hooking the arch_unmap_kpfn() function into the generic
memory_failure() code.

> > +	decoy_addr = (pfn << PAGE_SHIFT) + (PAGE_OFFSET ^ BIT(63));
> > +#else
> > +#error "no unused virtual bit available"
> > +#endif
> > +
> > +	if (set_memory_np(decoy_addr, 1))
> > +		pr_warn("Could not invalidate pfn=0x%lx from 1:1 map \n", pfn);
> 
> WARNING: unnecessary whitespace before a quoted newline
> #107: FILE: arch/x86/kernel/cpu/mcheck/mce.c:1089:
> +               pr_warn("Could not invalidate pfn=0x%lx from 1:1 map \n", pfn);

Oops!  Will fix.

-Tony

[toc] | [next] | [standalone]

#1671992 — RE: [PATCH] mm/hwpoison: Clear PRESENT bit for kernel 1:1 mappings of poison pages

> +	decoy_addr = (pfn << PAGE_SHIFT) + (PAGE_OFFSET ^ BIT(63));
> +#else
> +#error "no unused virtual bit available"
> +#endif
> +
> +	if (set_memory_np(decoy_addr, 1))
> +		pr_warn("Could not invalidate pfn=0x%lx from 1:1 map \n", pfn);

Does this patch handle breaking up 512 GiB, 1 GiB or 2 MiB page mappings
if it's just trying to mark a 4 KiB page as bad?

Although the kernel doesn't use MTRRs itself anymore, what if the system
BIOS still uses them for some memory regions, and the bad address falls in
an MTRR region?

---
Robert Elliott, HPE Persistent Memory

[toc] | [prev] | [next] | [standalone]

#1672002

>> +if (set_memory_np(decoy_addr, 1))
>> +pr_warn("Could not invalidate pfn=0x%lx from 1:1 map \n", pfn);
>
> Does this patch handle breaking up 512 GiB, 1 GiB or 2 MiB page mappings
> if it's just trying to mark a 4 KiB page as bad?

Yes.  The 1:1 mappings start out using the largest supported page size.  This
call will break up huge/large pages so that only 4KB is mapped out.
[This will affect performance because of the extra levels of TLB walks]

> Although the kernel doesn't use MTRRs itself anymore, what if the system
> BIOS still uses them for some memory regions, and the bad address falls in
> an MTRR region?

This code is called after mm/memory-failure.c:memory_failure() has already
checked that the page is one managed by the kernel.  In general machine checks
from other regions are going to be called out as fatal before we get here.

-Tony

[toc] | [prev] | [next] | [standalone]

#1672466

On Wed, Jun 21, 2017 at 10:47:40AM -0700, Luck, Tony wrote:
> I would if I could work out how to use it. From reading the manual
> page there seem to be a few options to this, but none of them appear
> to just drop a specific address (apart from my own). :-(

$ git send-email --to ... --cc ... --cc ... --suppress-cc=all ...

That should send only to the ones you have in --to and --cc and suppress
the rest.

Do a

$ git send-email -v --dry-run --to ... --cc ... --cc ... --suppress-cc=all ...

to see what it is going to do.

> I'd assume that other X86 implementations would face similar issues (unless
> they have extremely cautious pre-fetchers and/or no speculation).
> 
> I'm also assuming that non-X86 architectures that do recovery may want this
> too ... hence hooking the arch_unmap_kpfn() function into the generic
> memory_failure() code.

Which means that you could move the function to generic
mm/memory_failure.c code after making the decoy_addr computation
generic.

I'd still like to hear some sort of confirmation from other
vendors/arches whether it makes sense for them too, though.

I mean, if they don't do speculative accesses, then it probably doesn't
matter even - the page is innacessible anyway but still...

-- 
Regards/Gruss,
    Boris.

SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg)
-- 

[toc] | [prev] | [next] | [standalone]

#1678239 — git send-email (w/o Cc: stable)

On Thu, Jun 22, 2017 at 11:39:05AM +0200, Borislav Petkov wrote:
> On Wed, Jun 21, 2017 at 10:47:40AM -0700, Luck, Tony wrote:
> > I would if I could work out how to use it. From reading the manual
> > page there seem to be a few options to this, but none of them appear
> > to just drop a specific address (apart from my own). :-(
> 
> $ git send-email --to ... --cc ... --cc ... --suppress-cc=all ...
> 
> That should send only to the ones you have in --to and --cc and suppress
> the rest.
> 
> Do a
> 
> $ git send-email -v --dry-run --to ... --cc ... --cc ... --suppress-cc=all ...
> 
> to see what it is going to do.

So there is a "--cc-cmd" option that can do the same as those "-cc" arguments.
Combine that with --suppress-cc=bodycc and things get a bit more automated.

In my .gitconfig:

[sendemail]
	suppresscc = bodycc
	ccCmd = /home/agluck/bin/sendemail.ccCmd

and the command is some sed(1) to grap the Cc: lines except the
stable@vger.kernel.org one:

sed -n \
	-e '/Cc: stable@vger.kernel.org/d' \
	-e '/^Cc: /s///p' \
	-e '/^---/q' $1

-Tony

[toc] | [prev] | [next] | [standalone]

#1678540 — Re: git send-email (w/o Cc: stable)

On Thu, Jun 29, 2017 at 03:11:37PM -0700, Luck, Tony wrote:
> So there is a "--cc-cmd" option that can do the same as those "-cc" arguments.
> Combine that with --suppress-cc=bodycc and things get a bit more automated.

Yeah, whatever works for you.

I did play with cc-cmd somewhat but can't be bothered to generate the CC
list per hand each time.

I'd prefer if that switch:

	--suppress-cc=<category>

had the obvious <category> of single email address too:

	--suppress-cc=stable@vger.kernel.org

so that we can send patches and unconditionally suppress only that
single recipient from the CC list.

And maybe there is a way...

Let me CC the git ML.

-- 
Regards/Gruss,
    Boris.

SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg)
-- 

[toc] | [prev] | [next] | [standalone]

#1673924 — RE: [PATCH] mm/hwpoison: Clear PRESENT bit for kernel 1:1 mappings of poison pages

> > > +	if (set_memory_np(decoy_addr, 1))
> > > +		pr_warn("Could not invalidate pfn=0x%lx from 1:1 map \n",

Another concept to consider is mapping the page as UC rather than
completely unmapping it.

The uncorrectable error scope could be smaller than a page size, like:
* memory ECC width (e.g., 8 bytes)
* cache line size (e.g., 64 bytes)
* block device logical block size (e.g., 512 bytes, for persistent memory)

UC preserves the ability to access adjacent data within the page that
hasn't gone bad, and is particularly useful for persistent memory.

---
Robert Elliott, HPE Persistent Memory

[toc] | [prev] | [next] | [standalone]

#1676210 — Re: [PATCH] mm/hwpoison: Clear PRESENT bit for kernel 1:1 mappings of poison pages

On Tue, Jun 27, 2017 at 3:04 PM, Luck, Tony <tony.luck@intel.com> wrote:
>> > > > +if (set_memory_np(decoy_addr, 1))
>> > > > +pr_warn("Could not invalidate pfn=0x%lx from 1:1 map \n",
>>
>> Another concept to consider is mapping the page as UC rather than
>> completely unmapping it.
>
> UC would also avoid the speculative prefetch issue.  The Vol 3, Section 11.3 SDM says:
>
> Strong Uncacheable (UC) -System memory locations are not cached. All reads and writes
> appear on the system bus and are executed in program order without reordering. No speculative
> memory accesses, pagetable walks, or prefetches of speculated branch targets are made.
> This type of cache-control is useful for memory-mapped I/O devices. When used with normal
> RAM, it greatly reduces processor performance.
>
> But then I went and read the code for set_memory_uc() ... which calls "reserve_memtyep()"
> which does all kinds of things to avoid issues with MTRRs and other stuff.  Which all looks
> really more complex that we need just here.
>
>> The uncorrectable error scope could be smaller than a page size, like:
>> * memory ECC width (e.g., 8 bytes)
>> * cache line size (e.g., 64 bytes)
>> * block device logical block size (e.g., 512 bytes, for persistent memory)
>>
>> UC preserves the ability to access adjacent data within the page that
>> hasn't gone bad, and is particularly useful for persistent memory.
>
> If you want to dig into the non-poisoned pieces of the page later it might be
> better to set up a new scratch UC mapping to do that.
>
> My takeaway from Dan's comments on unpoisoning is that this isn't the context
> that he wants to do that.  He'd rather wait until he has somebody overwriting the
> page with fresh data.
>
> So I think I'd like to keep the patch as-is.

Yes, the persistent-memory poison interactions should be handled
separately and not hold up this patch for the normal system-memory
case. We might dove-tail support for this into stray write protection
where we unmap all of pmem while nothing in the kernel is actively
accessing it.

[toc] | [prev] | [next] | [standalone]

#1676212

> > > > +if (set_memory_np(decoy_addr, 1))
> > > > +pr_warn("Could not invalidate pfn=0x%lx from 1:1 map \n",
>
> Another concept to consider is mapping the page as UC rather than
> completely unmapping it.

UC would also avoid the speculative prefetch issue.  The Vol 3, Section 11.3 SDM says:

Strong Uncacheable (UC) -System memory locations are not cached. All reads and writes
appear on the system bus and are executed in program order without reordering. No speculative
memory accesses, pagetable walks, or prefetches of speculated branch targets are made.
This type of cache-control is useful for memory-mapped I/O devices. When used with normal
RAM, it greatly reduces processor performance.

But then I went and read the code for set_memory_uc() ... which calls "reserve_memtyep()"
which does all kinds of things to avoid issues with MTRRs and other stuff.  Which all looks
really more complex that we need just here.

> The uncorrectable error scope could be smaller than a page size, like:
> * memory ECC width (e.g., 8 bytes)
> * cache line size (e.g., 64 bytes)
> * block device logical block size (e.g., 512 bytes, for persistent memory)
>
> UC preserves the ability to access adjacent data within the page that
> hasn't gone bad, and is particularly useful for persistent memory.

If you want to dig into the non-poisoned pieces of the page later it might be
better to set up a new scratch UC mapping to do that.

My takeaway from Dan's comments on unpoisoning is that this isn't the context
that he wants to do that.  He'd rather wait until he has somebody overwriting the
page with fresh data.

So I think I'd like to keep the patch as-is.

-Tony

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web