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

Groups > linux.kernel > #1336630 > unrolled thread

[PATCH v11 0/4] Machine check recovery when kernel accesses poison

Started byTony Luck <tony.luck@intel.com>
First post2016-02-17 19:30 +0100
Last post2016-02-18 20:00 +0100
Articles 5 on this page of 25 — 8 participants

Back to article view | Back to linux.kernel

Contents

  [PATCH v11 0/4] Machine check recovery when kernel accesses poison Tony Luck <tony.luck@intel.com> - 2016-02-17 19:30 +0100
    [PATCH v11 4/4] x86: Create a new synthetic cpu capability for  machine check recovery Tony Luck <tony.luck@intel.com> - 2016-02-17 19:30 +0100
      [tip:x86/asm] x86/cpufeature:   Create a new synthetic cpu capability for machine check recovery tip-bot for Tony Luck <tipbot@zytor.com> - 2016-02-18 11:30 +0100
    [PATCH v11 3/4] x86, mce: Add __mcsafe_copy() Tony Luck <tony.luck@intel.com> - 2016-02-17 19:30 +0100
      Re: [PATCH v11 3/4] x86, mce: Add __mcsafe_copy() Ingo Molnar <mingo@kernel.org> - 2016-02-18 09:30 +0100
        Re: [PATCH v11 3/4] x86, mce: Add __mcsafe_copy() Peter Zijlstra <peterz@infradead.org> - 2016-02-18 11:00 +0100
          Re: [PATCH v11 3/4] x86, mce: Add __mcsafe_copy() Borislav Petkov <bp@alien8.de> - 2016-02-18 11:30 +0100
          Re: [PATCH v11 3/4] x86, mce: Add __mcsafe_copy() Ingo Molnar <mingo@kernel.org> - 2016-02-18 11:30 +0100
            Re: [PATCH v11 3/4] x86, mce: Add __mcsafe_copy() Peter Zijlstra <peterz@infradead.org> - 2016-02-18 11:40 +0100
              RE: [PATCH v11 3/4] x86, mce: Add __mcsafe_copy() "Luck, Tony" <tony.luck@intel.com> - 2016-02-18 16:00 +0100
          Re: [PATCH v11 3/4] x86, mce: Add __mcsafe_copy() Ingo Molnar <mingo@kernel.org> - 2016-02-19 09:00 +0100
            Re: [PATCH v11 3/4] x86, mce: Add __mcsafe_copy() Peter Zijlstra <peterz@infradead.org> - 2016-02-19 09:50 +0100
              Re: [PATCH v11 3/4] x86, mce: Add __mcsafe_copy() Ingo Molnar <mingo@kernel.org> - 2016-02-19 11:00 +0100
        Re: [PATCH v11 3/4] x86, mce: Add __mcsafe_copy() Ingo Molnar <mingo@kernel.org> - 2016-02-18 11:40 +0100
          Re: [PATCH v11 3/4] x86, mce: Add __mcsafe_copy() Borislav Petkov <bp@alien8.de> - 2016-02-18 11:40 +0100
            Re: [PATCH v11 3/4] x86, mce: Add __mcsafe_copy() Ingo Molnar <mingo@kernel.org> - 2016-02-18 19:50 +0100
        Re: [PATCH v11 3/4] x86, mce: Add __mcsafe_copy() Borislav Petkov <bp@alien8.de> - 2016-02-18 11:40 +0100
        [PATCH v12] x86, mce: Add memcpy_trap() "Luck, Tony" <tony.luck@intel.com> - 2016-02-18 22:20 +0100
          Re: [PATCH v12] x86, mce: Add memcpy_trap() Ingo Molnar <mingo@kernel.org> - 2016-02-19 10:20 +0100
            Re: [PATCH v13] x86, mce: Add memcpy_trap() "Luck, Tony" <tony.luck@intel.com> - 2016-02-19 19:00 +0100
      Re: [PATCH v11 3/4] x86, mce: Add __mcsafe_copy() Linus Torvalds <torvalds@linux-foundation.org> - 2016-02-18 19:20 +0100
        Re: [PATCH v11 3/4] x86, mce: Add __mcsafe_copy() "Luck, Tony" <tony.luck@intel.com> - 2016-02-18 20:00 +0100
          Re: [PATCH v11 3/4] x86, mce: Add __mcsafe_copy() Ingo Molnar <mingo@kernel.org> - 2016-02-18 21:20 +0100
            Re: [PATCH v11 3/4] x86, mce: Add __mcsafe_copy() Dan Williams <dan.j.williams@intel.com> - 2016-02-18 22:40 +0100
        Re: [PATCH v11 3/4] x86, mce: Add __mcsafe_copy() Ingo Molnar <mingo@kernel.org> - 2016-02-18 20:00 +0100

Page 2 of 2 — ← Prev page 1 [2]

#1337605 — Re: [PATCH v11 3/4] x86, mce: Add __mcsafe_copy()

FromLinus Torvalds <torvalds@linux-foundation.org>
Date2016-02-18 19:20 +0100
SubjectRe: [PATCH v11 3/4] x86, mce: Add __mcsafe_copy()
Message-ID<r3Blf-16f-11@gated-at.bofh.it>
In reply to#1336637
On Wed, Feb 17, 2016 at 10:20 AM, Tony Luck <tony.luck@intel.com> wrote:
>
> If we faulted during the copy, then 'trapnr' will say which type
> of trap (X86_TRAP_PF or X86_TRAP_MC) and 'remain' says how many
> bytes were not copied.

So apart from the naming, a couple of questions:

 - I'd like to see the actual *use* case explained, not just what it does.

 - why does this use the complex - and slower, on modern machines -
unrolled manual memory copy, when you might as well just use a single

     rep ; movsb

    which not only makes it smaller, but makes the exception fixup trivial.

 - why not make the "bytes remaining" the same as for a user-space
copy (ie return it as the return value)?

 - at that point, it ends up looking a *lot* like uaccess_try/catch,
which gets the error code from current_thread_info()->uaccess_err

Hmm?

          Linus

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

#1337626 — Re: [PATCH v11 3/4] x86, mce: Add __mcsafe_copy()

From"Luck, Tony" <tony.luck@intel.com>
Date2016-02-18 20:00 +0100
SubjectRe: [PATCH v11 3/4] x86, mce: Add __mcsafe_copy()
Message-ID<r3BXY-1pz-15@gated-at.bofh.it>
In reply to#1337605
On Thu, Feb 18, 2016 at 10:12:42AM -0800, Linus Torvalds wrote:
> On Wed, Feb 17, 2016 at 10:20 AM, Tony Luck <tony.luck@intel.com> wrote:
> >
> > If we faulted during the copy, then 'trapnr' will say which type
> > of trap (X86_TRAP_PF or X86_TRAP_MC) and 'remain' says how many
> > bytes were not copied.
> 
> So apart from the naming, a couple of questions:
> 
>  - I'd like to see the actual *use* case explained, not just what it does.

First user is libnvdimm. Dan Williams already has code to use this
so that kernel code accessing persistent memory can return -EIO to
a user instead of crashing the system if the cpu runs into an
uncorrected error during the copy.

I would also lkie use this for a machine check aware
copy_from_user() which would avoid crashing the kernel 
when the uncorrected error is in a user page (we can SIGBUS
the user just like we do if the user touched the poison themself).

copy_to_user() is also interesting if the source address is the
page cache. I think we can also avoid crashing the kernel in this
case too - but I haven't thought that all the way through.

>  - why does this use the complex - and slower, on modern machines -
> unrolled manual memory copy, when you might as well just use a single
> 
>      rep ; movsb
> 
>     which not only makes it smaller, but makes the exception fixup trivial.

Because current generation cpus don't give a recoverable machine
check if we consume with a "rep ; movsb" :-(
When we have that we can pick the best copy function based
on the capabilities of the cpu we are running on.

>  - why not make the "bytes remaining" the same as for a user-space
> copy (ie return it as the return value)?
> 
>  - at that point, it ends up looking a *lot* like uaccess_try/catch,
> which gets the error code from current_thread_info()->uaccess_err

For my copy_from_user/copy_to_user cases we need to know both the
number of remaining bytes and also *why* we stopped copying. We
might have #PF, in which case we return -EFAULT to the user, if
we have #MC then the recovery path is different (need to offline
the page, SIGBUS the user, ...)

-Tony

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

#1337674 — Re: [PATCH v11 3/4] x86, mce: Add __mcsafe_copy()

FromIngo Molnar <mingo@kernel.org>
Date2016-02-18 21:20 +0100
SubjectRe: [PATCH v11 3/4] x86, mce: Add __mcsafe_copy()
Message-ID<r3Ddo-2wj-17@gated-at.bofh.it>
In reply to#1337626
* Luck, Tony <tony.luck@intel.com> wrote:

> On Thu, Feb 18, 2016 at 10:12:42AM -0800, Linus Torvalds wrote:
> > On Wed, Feb 17, 2016 at 10:20 AM, Tony Luck <tony.luck@intel.com> wrote:
> > >
> > > If we faulted during the copy, then 'trapnr' will say which type
> > > of trap (X86_TRAP_PF or X86_TRAP_MC) and 'remain' says how many
> > > bytes were not copied.
> > 
> > So apart from the naming, a couple of questions:
> > 
> >  - I'd like to see the actual *use* case explained, not just what it does.
> 
> First user is libnvdimm. Dan Williams already has code to use this so that 
> kernel code accessing persistent memory can return -EIO to a user instead of 
> crashing the system if the cpu runs into an uncorrected error during the copy.

Are these the memcpy_*_pmem() calls in drivers/nvdimm/pmem.c? Is there any actual 
patch to look at?

Thanks,

	Ingo

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

#1337745 — Re: [PATCH v11 3/4] x86, mce: Add __mcsafe_copy()

FromDan Williams <dan.j.williams@intel.com>
Date2016-02-18 22:40 +0100
SubjectRe: [PATCH v11 3/4] x86, mce: Add __mcsafe_copy()
Message-ID<r3EsO-3h6-1@gated-at.bofh.it>
In reply to#1337674
On Thu, Feb 18, 2016 at 12:14 PM, Ingo Molnar <mingo@kernel.org> wrote:
>
> * Luck, Tony <tony.luck@intel.com> wrote:
>
>> On Thu, Feb 18, 2016 at 10:12:42AM -0800, Linus Torvalds wrote:
>> > On Wed, Feb 17, 2016 at 10:20 AM, Tony Luck <tony.luck@intel.com> wrote:
>> > >
>> > > If we faulted during the copy, then 'trapnr' will say which type
>> > > of trap (X86_TRAP_PF or X86_TRAP_MC) and 'remain' says how many
>> > > bytes were not copied.
>> >
>> > So apart from the naming, a couple of questions:
>> >
>> >  - I'd like to see the actual *use* case explained, not just what it does.
>>
>> First user is libnvdimm. Dan Williams already has code to use this so that
>> kernel code accessing persistent memory can return -EIO to a user instead of
>> crashing the system if the cpu runs into an uncorrected error during the copy.
>
> Are these the memcpy_*_pmem() calls in drivers/nvdimm/pmem.c? Is there any actual
> patch to look at?
>

Here's the integration patch I had from the version of mcsafe_copy()
at the beginning of January.  Pardon the whitespace damage... the
original thread is here: [1].  Note that the "badblocks" intergration
portion of that set went upstream in v4.5-rc1.

[1]: https://lists.01.org/pipermail/linux-nvdimm/2016-January/003864.html

---

Subject: x86, pmem: use __mcsafe_copy() for memcpy_from_pmem()

In support of large capacity persistent memory use __mcsafe_copy() for
pmem I/O.  This allows the pmem driver to support an error model similar
to disks when machine check recovery is available.

Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: "H. Peter Anvin" <hpa@zytor.com>
Cc: x86@kernel.org
Cc: Borislav Petkov <bp@suse.de>
Cc: Tony Luck <tony.luck@intel.com>
Cc: Andy Lutomirski <luto@amacapital.net>
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
 arch/x86/include/asm/pmem.h |   16 ++++++++++++++++
 drivers/nvdimm/Kconfig      |    1 +
 drivers/nvdimm/pmem.c       |   10 ++++++----
 include/linux/pmem.h        |   17 +++++++++++++----
 4 files changed, 36 insertions(+), 8 deletions(-)

diff --git a/arch/x86/include/asm/pmem.h b/arch/x86/include/asm/pmem.h
index d8ce3ec816ab..4ef301e78a2b 100644
--- a/arch/x86/include/asm/pmem.h
+++ b/arch/x86/include/asm/pmem.h
@@ -17,6 +17,7 @@
 #include <asm/cacheflush.h>
 #include <asm/cpufeature.h>
 #include <asm/special_insns.h>
+#include <asm/string.h>

 #ifdef CONFIG_ARCH_HAS_PMEM_API
 /**
@@ -47,6 +48,21 @@ static inline void arch_memcpy_to_pmem(void __pmem
*dst, const void *src,
                BUG();
 }

+static inline int arch_memcpy_from_pmem(void *dst, const void __pmem *src,
+               size_t n)
+{
+       if (IS_ENABLED(CONFIG_MCE_KERNEL_RECOVERY)) {
+               struct mcsafe_ret ret;
+
+               ret = __mcsafe_copy(dst, (void __force *) src, n);
+               if (ret.remain)
+                       return -EIO;
+               return 0;
+       }
+       memcpy(dst, (void __force *) src, n);
+       return 0;
+}
+
 /**
  * arch_wmb_pmem - synchronize writes to persistent memory
  *
diff --git a/drivers/nvdimm/Kconfig b/drivers/nvdimm/Kconfig
index 53c11621d5b1..fe5885d01fd8 100644
--- a/drivers/nvdimm/Kconfig
+++ b/drivers/nvdimm/Kconfig
@@ -22,6 +22,7 @@ config BLK_DEV_PMEM
        depends on HAS_IOMEM
        select ND_BTT if BTT
        select ND_PFN if NVDIMM_PFN
+       select MCE_KERNEL_RECOVERY if X86_MCE && X86_64
        help
          Memory ranges for PMEM are described by either an NFIT
          (NVDIMM Firmware Interface Table, see CONFIG_NFIT_ACPI), a
diff --git a/drivers/nvdimm/pmem.c b/drivers/nvdimm/pmem.c
index 8744235b5be2..d8e14e962327 100644
--- a/drivers/nvdimm/pmem.c
+++ b/drivers/nvdimm/pmem.c
@@ -62,6 +62,7 @@ static bool is_bad_pmem(struct badblocks *bb,
sector_t sector, unsigned int len)
 static int pmem_do_bvec(struct block_device *bdev, struct page *page,
                unsigned int len, unsigned int off, int rw, sector_t sector)
 {
+       int rc = 0;
        void *mem = kmap_atomic(page);
        struct gendisk *disk = bdev->bd_disk;
        struct pmem_device *pmem = disk->private_data;
@@ -71,7 +72,7 @@ static int pmem_do_bvec(struct block_device *bdev,
struct page *page,
        if (rw == READ) {
                if (unlikely(is_bad_pmem(disk->bb, sector, len)))
                        return -EIO;
-               memcpy_from_pmem(mem + off, pmem_addr, len);
+               rc = memcpy_from_pmem(mem + off, pmem_addr, len);
                flush_dcache_page(page);
        } else {
                flush_dcache_page(page);
@@ -79,7 +80,7 @@ static int pmem_do_bvec(struct block_device *bdev,
struct page *page,
        }

        kunmap_atomic(mem);
-       return 0;
+       return rc;
 }

 static blk_qc_t pmem_make_request(struct request_queue *q, struct bio *bio)
@@ -237,6 +238,7 @@ static int pmem_rw_bytes(struct nd_namespace_common *ndns,
                resource_size_t offset, void *buf, size_t size, int rw)
 {
        struct pmem_device *pmem = dev_get_drvdata(ndns->claim);
+       int rc = 0;

        if (unlikely(offset + size > pmem->size)) {
                dev_WARN_ONCE(&ndns->dev, 1, "request out of range\n");
@@ -244,13 +246,13 @@ static int pmem_rw_bytes(struct nd_namespace_common *ndns,
        }

        if (rw == READ)
-               memcpy_from_pmem(buf, pmem->virt_addr + offset, size);
+               rc = memcpy_from_pmem(buf, pmem->virt_addr + offset, size);
        else {
                memcpy_to_pmem(pmem->virt_addr + offset, buf, size);
                wmb_pmem();
        }

-       return 0;
+       return rc;
 }

 static int nd_pfn_init(struct nd_pfn *nd_pfn)
diff --git a/include/linux/pmem.h b/include/linux/pmem.h
index acfea8ce4a07..0e57a5beab21 100644
--- a/include/linux/pmem.h
+++ b/include/linux/pmem.h
@@ -42,6 +42,13 @@ static inline void arch_memcpy_to_pmem(void __pmem
*dst, const void *src,
        BUG();
 }

+static inline int arch_memcpy_from_pmem(void *dst,
+               const void __pmem *src, size_t n)
+{
+       BUG();
+       return 0;
+}
+
 static inline size_t arch_copy_from_iter_pmem(void __pmem *addr, size_t bytes,
                struct iov_iter *i)
 {
@@ -57,12 +64,14 @@ static inline void arch_clear_pmem(void __pmem
*addr, size_t size)

 /*
  * Architectures that define ARCH_HAS_PMEM_API must provide
- * implementations for arch_memcpy_to_pmem(), arch_wmb_pmem(),
- * arch_copy_from_iter_pmem(), arch_clear_pmem() and arch_has_wmb_pmem().
+ * implementations for arch_memcpy_to_pmem(), arch_memcpy_from_pmem(),
+ * arch_wmb_pmem(), arch_copy_from_iter_pmem(), arch_clear_pmem() and
+ * arch_has_wmb_pmem().
  */
-static inline void memcpy_from_pmem(void *dst, void __pmem const
*src, size_t size)
+static inline int memcpy_from_pmem(void *dst, void __pmem const *src,
+               size_t size)
 {
-       memcpy(dst, (void __force const *) src, size);
+       return arch_memcpy_from_pmem(dst, src, size);
 }

 static inline bool arch_has_pmem_api(void)

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

#1337627 — Re: [PATCH v11 3/4] x86, mce: Add __mcsafe_copy()

FromIngo Molnar <mingo@kernel.org>
Date2016-02-18 20:00 +0100
SubjectRe: [PATCH v11 3/4] x86, mce: Add __mcsafe_copy()
Message-ID<r3BXY-1pz-19@gated-at.bofh.it>
In reply to#1337605
* Linus Torvalds <torvalds@linux-foundation.org> wrote:

> On Wed, Feb 17, 2016 at 10:20 AM, Tony Luck <tony.luck@intel.com> wrote:
> >
> > If we faulted during the copy, then 'trapnr' will say which type
> > of trap (X86_TRAP_PF or X86_TRAP_MC) and 'remain' says how many
> > bytes were not copied.
> 
> So apart from the naming, a couple of questions:
> 
>  - I'd like to see the actual *use* case explained, not just what it does.
> 
>  - why does this use the complex - and slower, on modern machines -
> unrolled manual memory copy, when you might as well just use a single
> 
>      rep ; movsb
> 
>     which not only makes it smaller, but makes the exception fixup trivial.
>
>  - why not make the "bytes remaining" the same as for a user-space
> copy (ie return it as the return value)?
> 
>  - at that point, it ends up looking a *lot* like uaccess_try/catch,
> which gets the error code from current_thread_info()->uaccess_err
> 
> Hmm?

memcpy_try()/memcpy_catch() definitely has a nice ring to it.

Thanks,

	Ingo

[toc] | [prev] | [standalone]

Page 2 of 2 — ← Prev page 1 [2]

Back to top | Article view | linux.kernel

csiph-web