* RE: [PATCH 3/3] eSDHC: fix incorrect default value of the capabilities register on P4080
From: Zang Roy-R61911 @ 2011-07-18 5:01 UTC (permalink / raw)
To: Anton Vorontsov
Cc: linuxppc-dev@lists.ozlabs.org, akpm@linux-foundation.org,
linux-mmc@vger.kernel.org
In-Reply-To: <20110705101801.GA13052@oksana.dev.rtsoft.ru>
DQoNCj4gLS0tLS1PcmlnaW5hbCBNZXNzYWdlLS0tLS0NCj4gRnJvbTogQW50b24gVm9yb250c292
IFttYWlsdG86YXZvcm9udHNvdkBtdmlzdGEuY29tXQ0KPiBTZW50OiBUdWVzZGF5LCBKdWx5IDA1
LCAyMDExIDE4OjE4IFBNDQo+IFRvOiBaYW5nIFJveS1SNjE5MTENCj4gQ2M6IGxpbnV4LW1tY0B2
Z2VyLmtlcm5lbC5vcmc7IGxpbnV4cHBjLWRldkBsaXN0cy5vemxhYnMub3JnOyBha3BtQGxpbnV4
LQ0KPiBmb3VuZGF0aW9uLm9yZw0KPiBTdWJqZWN0OiBSZTogW1BBVENIIDMvM10gZVNESEM6IGZp
eCBpbmNvcnJlY3QgZGVmYXVsdCB2YWx1ZSBvZiB0aGUNCj4gY2FwYWJpbGl0aWVzIHJlZ2lzdGVy
IG9uIFA0MDgwDQo+IA0KPiBPbiBUdWUsIEp1bCAwNSwgMjAxMSBhdCAxMjoxOTowM1BNICswODAw
LCBSb3kgWmFuZyB3cm90ZToNCj4gPiBQNDA4MCBlU0RIQyBlcnJhdGEgMTIgZGVzY3JpYmVzIGlu
Y29ycmVjdCBkZWZhdWx0IHZhbHVlIG9mIHRoZQ0KPiA+IHRoZSBob3N0IGNvbnRyb2xsZXIgY2Fw
YWJpbGl0aWVzIHJlZ2lzdGVyLg0KPiA+DQo+ID4gVGhlIGRlZmF1bHQgdmFsdWUgb2YgdGhlIFZT
MTggYW5kIFZTMzAgZmllbGRzIGluIHRoZSBob3N0IGNvbnRyb2xsZXINCj4gPiBjYXBhYmlsaXRp
ZXMgcmVnaXN0ZXIgKEhPU1RDQVBCTFQpIGFyZSBpbmNvcnJlY3QuIFRoZSBkZWZhdWx0IG9mIHRo
ZXNlIGJpdHMNCj4gPiBzaG91bGQgYmUgemVybyBpbnN0ZWFkIG9mIG9uZSBpbiB0aGUgZVNESEMg
bG9naWMuDQo+ID4NCj4gPiBUaGlzIHBhdGNoIGFkZHMgdGhlIHdvcmthcm91bmQgZm9yIHRoZXNl
IGVycmF0YS4NCj4gPg0KPiA+IFNpZ25lZC1vZmYtYnk6IFJveSBaYW5nIDx0aWUtZmVpLnphbmdA
ZnJlZXNjYWxlLmNvbT4NCj4gPiAtLS0NCj4gPiAgZHJpdmVycy9tbWMvaG9zdC9zZGhjaS1vZi1j
b3JlLmMgfCAgICAzICsrKw0KPiA+ICBkcml2ZXJzL21tYy9ob3N0L3NkaGNpLmMgICAgICAgICB8
ICAgIDYgKysrKysrDQo+ID4gIGluY2x1ZGUvbGludXgvbW1jL3NkaGNpLmggICAgICAgIHwgICAg
NCArKysrDQo+ID4gIDMgZmlsZXMgY2hhbmdlZCwgMTMgaW5zZXJ0aW9ucygrKSwgMCBkZWxldGlv
bnMoLSkNCj4gPg0KPiA+IGRpZmYgLS1naXQgYS9kcml2ZXJzL21tYy9ob3N0L3NkaGNpLW9mLWNv
cmUuYyBiL2RyaXZlcnMvbW1jL2hvc3Qvc2RoY2ktb2YtDQo+IGNvcmUuYw0KPiA+IGluZGV4IGZl
ZGU0M2QuLjliZGQzMGQgMTAwNjQ0DQo+ID4gLS0tIGEvZHJpdmVycy9tbWMvaG9zdC9zZGhjaS1v
Zi1jb3JlLmMNCj4gPiArKysgYi9kcml2ZXJzL21tYy9ob3N0L3NkaGNpLW9mLWNvcmUuYw0KPiA+
IEBAIC0xODIsNiArMTgyLDkgQEAgc3RhdGljIGludCBfX2RldmluaXQgc2RoY2lfb2ZfcHJvYmUo
c3RydWN0DQo+IHBsYXRmb3JtX2RldmljZSAqb2ZkZXYpDQo+ID4gIAlpZiAob2ZfZGV2aWNlX2lz
X2NvbXBhdGlibGUobnAsICJmc2wsZXNkaGMiKSkNCj4gPiAgCQlob3N0LT5xdWlya3MgfD0gU0RI
Q0lfUVVJUktfUU9SSVFfUFJPQ1RMX1dFSVJEOw0KPiA+DQo+ID4gKwlpZiAob2ZfZGV2aWNlX2lz
X2NvbXBhdGlibGUobnAsICJmc2wscDQwODAtZXNkaGMiKSkNCj4gPiArCQlob3N0LT5xdWlya3Mg
fD0gU0RIQ0lfUVVJUktfUU9SSVFfSE9TVENBUEJMVF9PTkxZX1ZTMzM7DQo+IA0KPiBTaG91bGQg
cmVhbGx5IHVzZSB2b2x0YWdlLXJhbmdlcywgbm90IHF1aXJrcy4NCj4gDQo+IGh0dHA6Ly93d3cu
c3Bpbmljcy5uZXQvbGlzdHMvbGludXgtbW1jL21zZzAyNzg1Lmh0bWwNCk1ha2Ugc2Vuc2UgdG8g
bWUuIFRoYW5rcy4NClJveQ0K
^ permalink raw reply
* Re: [PATCH 1/1] Fixup write permission of TLB on powerpc e500 core
From: Benjamin Herrenschmidt @ 2011-07-18 4:02 UTC (permalink / raw)
To: Peter Zijlstra
Cc: tony.luck, Peter Zijlstra, Shan Hai, linux-kernel, cmetcalf,
dhowells, paulus, tglx, walken, linuxppc-dev, akpm
In-Reply-To: <1310961196.25044.266.camel@pasglop>
On Mon, 2011-07-18 at 13:53 +1000, Benjamin Herrenschmidt wrote:
> On Mon, 2011-07-18 at 09:14 +1000, Benjamin Herrenschmidt wrote:
>
> > In fact, with such a flag, we could probably avoid the ifdef entirely, and
> > always go toward the PTE fixup path when called in such a fixup case, my gut
> > feeling is that this is going to be seldom enough not to hurt x86 measurably
> > but we'll have to try it out.
> >
> > That leads to that even less tested patch:
>
> And here's a version that builds (still not tested :-)
>
> Shan, can you verify whether that fixes the problem for you ?
>
> I also had a cursory glance at the ARM code and it seems to rely on the
> same stuff as embedded powerpc does for dirty/young updates, so in
> theory it should exhibit the same problem.
>
> I suspect the scenario is rare enough in practice in embedded workloads
> that nobody noticed until now.
Ignore that bogus send, I sent a proper one immediately after (evolution
FAIL, sorry about that)
Cheers,
Ben.
^ permalink raw reply
* Re: [PATCH v2] powerpc32: Kexec support for PPC440X chipsets
From: Suzuki Poulose @ 2011-07-18 4:04 UTC (permalink / raw)
To: Benjammin Herrenschmidt, Kumar Gala
Cc: Michal Simek, Sebastian Andrzej Siewior, kexec, lkml, Josh Boyer,
Vivek Goyal, Suzuki K. Poulose, linux ppc dev, Paul Mackerras
In-Reply-To: <20110712064356.28567.48722.stgit@suzukikp.in.ibm.com>
On 07/12/11 12:14, Suzuki K. Poulose wrote:
> Changes from V1: Uses a tmp mapping in the other address space to setup
> the 1:1 mapping (suggested by Sebastian Andrzej Siewior).
>
> Note: Should we do the same for kernel entry code for PPC44x ?
>
> This patch adds kexec support for PPC440 based chipsets.This work is based
> on the KEXEC patches for FSL BookE.
>
> The FSL BookE patch and the code flow could be found at the link below:
>
> http://patchwork.ozlabs.org/patch/49359/
>
> Steps:
>
> 1) Invalidate all the TLB entries except the one this code is run from
> 2) Create a tmp mapping for our code in the other address space and jump to it
> 3) Invalidate the entry we used
> 4) Create a 1:1 mapping for 0-2GiB in blocks of 256M
> 5) Jump to the new 1:1 mapping and invalidate the tmp mapping
>
> I have tested this patches on Ebony, Sequoia boards and Virtex on QEMU.
> It would be great if somebody could test this on the other boards.
Forgot to mention:
You would need the current snapshot of kexec-tools, available at
git://git.kernel.org/pub/scm/utils/kernel/kexec/kexec-tools.git
The following commits(in the current tree) are needed for the support of ppc32.
commit 13f6d71bdf9836b90ae4ec21209383f1a3c56b0f
kexec-tools: ppc32: Fixup ThreadPointer for purgatory code
commit 9ec3fac7e8840fb31891ba49a626c5dd33e09e86
kexec-tools: powerpc: Use the #address-cells information to parsememory/reg
commit 806ef8870539a6c74a2a98188d0207a038b16f77
Fix memory errors on ppc
Thanks
Suzuki
^ permalink raw reply
* Re: [PATCH 1/1] Fixup write permission of TLB on powerpc e500 core
From: Benjamin Herrenschmidt @ 2011-07-18 4:01 UTC (permalink / raw)
To: Peter Zijlstra
Cc: tony.luck, Peter Zijlstra, Shan Hai, linux-kernel, cmetcalf,
dhowells, paulus, tglx, walken, linuxppc-dev, akpm
In-Reply-To: <1310944453.25044.262.camel@pasglop>
On Mon, 2011-07-18 at 09:14 +1000, Benjamin Herrenschmidt wrote:
> In fact, with such a flag, we could probably avoid the ifdef entirely, and
> always go toward the PTE fixup path when called in such a fixup case, my gut
> feeling is that this is going to be seldom enough not to hurt x86 measurably
> but we'll have to try it out.
>
> That leads to that even less tested patch:
And here's a version that builds and fixes a bug or two
(still not tested :-)
Shan, can you verify whether that fixes the problem for you ?
I also had a cursory glance at the ARM code and it seems to rely on the
same stuff as embedded powerpc does for dirty/young updates, so in
theory it should exhibit the same problem.
I suspect the scenario is rare enough in practice in embedded workloads
that nobody noticed until now.
Cheers,
Ben.
mm/futex: Fix use of gup() to "fixup" failing atomic user accesses
The futex code uses atomic (page fault disabled) accesses to user space,
and when they fail, uses get_user_pages() to "fixup" the PTE and try again.
However, on arch with SW tracking of the dirty and young bits, this will
not work properly as neither of the above will perform the necessary fixup
of those bits.
There's also a possible corner cases with archs who rely on
handle_pte_fault() to invalidate the TLB for "spurrious" faults (though
I don't know which arch actually needs that). Those would break the
same way.
This fixes it by factoring out the fixup code from handle_pte_fault() into
a separate function, and use it from within gup as well, whenever the
FOLL_FIXFAULT flag has been passed to it. The futex code is modified to
pass that flag.
This doesn't change the "normal" gup case (and thus avoids the overhead
of doing that tracking)
Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
---
diff --git a/include/linux/mm.h b/include/linux/mm.h
index 9670f71..8a76694 100644
--- a/include/linux/mm.h
+++ b/include/linux/mm.h
@@ -1546,6 +1546,7 @@ struct page *follow_page(struct vm_area_struct *, unsigned long address,
#define FOLL_MLOCK 0x40 /* mark page as mlocked */
#define FOLL_SPLIT 0x80 /* don't return transhuge pages, split them */
#define FOLL_HWPOISON 0x100 /* check page is hwpoisoned */
+#define FOLL_FIXFAULT 0x200 /* fixup after a fault (PTE dirty/young upd) */
typedef int (*pte_fn_t)(pte_t *pte, pgtable_t token, unsigned long addr,
void *data);
diff --git a/kernel/futex.c b/kernel/futex.c
index fe28dc2..02adff7 100644
--- a/kernel/futex.c
+++ b/kernel/futex.c
@@ -355,8 +355,8 @@ static int fault_in_user_writeable(u32 __user *uaddr)
int ret;
down_read(&mm->mmap_sem);
- ret = get_user_pages(current, mm, (unsigned long)uaddr,
- 1, 1, 0, NULL, NULL);
+ ret = __get_user_pages(current, mm, (unsigned long)uaddr, 1,
+ FOLL_WRITE | FOLL_FIXFAULT, NULL, NULL, NULL);
up_read(&mm->mmap_sem);
return ret < 0 ? ret : 0;
diff --git a/mm/memory.c b/mm/memory.c
index 40b7531..3c4d502 100644
--- a/mm/memory.c
+++ b/mm/memory.c
@@ -1419,6 +1419,29 @@ int zap_vma_ptes(struct vm_area_struct *vma, unsigned long address,
}
EXPORT_SYMBOL_GPL(zap_vma_ptes);
+static void handle_pte_sw_young_dirty(struct vm_area_struct *vma,
+ unsigned long address,
+ pte_t *ptep, int write)
+{
+ pte_t entry = *ptep;
+
+ if (write)
+ pte_mkdirty(entry);
+ entry = pte_mkyoung(entry);
+ if (ptep_set_access_flags(vma, address, ptep, entry, write)) {
+ update_mmu_cache(vma, address, ptep);
+ } else {
+ /*
+ * This is needed only for protection faults but the arch code
+ * is not yet telling us if this is a protection fault or not.
+ * This still avoids useless tlb flushes for .text page faults
+ * with threads.
+ */
+ if (write)
+ flush_tlb_fix_spurious_fault(vma, address);
+ }
+}
+
/**
* follow_page - look up a page descriptor from a user-virtual address
* @vma: vm_area_struct mapping @address
@@ -1514,6 +1537,10 @@ split_fallthrough:
if (flags & FOLL_GET)
get_page(page);
+
+ if (flags & FOLL_FIXFAULT)
+ handle_pte_sw_young_dirty(vma, address, ptep,
+ flags & FOLL_WRITE);
if (flags & FOLL_TOUCH) {
if ((flags & FOLL_WRITE) &&
!pte_dirty(pte) && !PageDirty(page))
@@ -1525,6 +1552,7 @@ split_fallthrough:
*/
mark_page_accessed(page);
}
+
if ((flags & FOLL_MLOCK) && (vma->vm_flags & VM_LOCKED)) {
/*
* The preliminary mapping check is mainly to avoid the
@@ -3358,21 +3386,8 @@ int handle_pte_fault(struct mm_struct *mm,
if (!pte_write(entry))
return do_wp_page(mm, vma, address,
pte, pmd, ptl, entry);
- entry = pte_mkdirty(entry);
- }
- entry = pte_mkyoung(entry);
- if (ptep_set_access_flags(vma, address, pte, entry, flags & FAULT_FLAG_WRITE)) {
- update_mmu_cache(vma, address, pte);
- } else {
- /*
- * This is needed only for protection faults but the arch code
- * is not yet telling us if this is a protection fault or not.
- * This still avoids useless tlb flushes for .text page faults
- * with threads.
- */
- if (flags & FAULT_FLAG_WRITE)
- flush_tlb_fix_spurious_fault(vma, address);
}
+ handle_pte_sw_young_dirty(vma, address, pte, flags & FAULT_FLAG_WRITE);
unlock:
pte_unmap_unlock(pte, ptl);
return 0;
^ permalink raw reply related
* Re: [PATCH 1/1] Fixup write permission of TLB on powerpc e500 core
From: Benjamin Herrenschmidt @ 2011-07-18 3:53 UTC (permalink / raw)
To: Peter Zijlstra
Cc: tony.luck, Peter Zijlstra, Shan Hai, linux-kernel, cmetcalf,
dhowells, paulus, tglx, walken, linuxppc-dev, akpm
In-Reply-To: <1310944453.25044.262.camel@pasglop>
On Mon, 2011-07-18 at 09:14 +1000, Benjamin Herrenschmidt wrote:
> In fact, with such a flag, we could probably avoid the ifdef entirely, and
> always go toward the PTE fixup path when called in such a fixup case, my gut
> feeling is that this is going to be seldom enough not to hurt x86 measurably
> but we'll have to try it out.
>
> That leads to that even less tested patch:
And here's a version that builds (still not tested :-)
Shan, can you verify whether that fixes the problem for you ?
I also had a cursory glance at the ARM code and it seems to rely on the
same stuff as embedded powerpc does for dirty/young updates, so in
theory it should exhibit the same problem.
I suspect the scenario is rare enough in practice in embedded workloads
that nobody noticed until now.
Cheers,
Ben.
diff --git a/drivers/ata/sata_sil24.c b/drivers/ata/sata_sil24.c
index 06c564e..e8ef0e6 100644
--- a/drivers/ata/sata_sil24.c
+++ b/drivers/ata/sata_sil24.c
@@ -1296,7 +1296,7 @@ static int sil24_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
DRV_NAME);
if (rc)
return rc;
- iomap = pcim_iomap_table(pdev);
+ iomap = pcim_iomap_table~(pdev);
/* apply workaround for completion IRQ loss on PCI-X errata */
if (pi.flags & SIL24_FLAG_PCIX_IRQ_WOC) {
diff --git a/include/linux/mm.h b/include/linux/mm.h
index 9670f71..8a76694 100644
--- a/include/linux/mm.h
+++ b/include/linux/mm.h
@@ -1546,6 +1546,7 @@ struct page *follow_page(struct vm_area_struct *, unsigned long address,
#define FOLL_MLOCK 0x40 /* mark page as mlocked */
#define FOLL_SPLIT 0x80 /* don't return transhuge pages, split them */
#define FOLL_HWPOISON 0x100 /* check page is hwpoisoned */
+#define FOLL_FIXFAULT 0x200 /* fixup after a fault (PTE dirty/young upd) */
typedef int (*pte_fn_t)(pte_t *pte, pgtable_t token, unsigned long addr,
void *data);
diff --git a/kernel/futex.c b/kernel/futex.c
index fe28dc2..02adff7 100644
--- a/kernel/futex.c
+++ b/kernel/futex.c
@@ -355,8 +355,8 @@ static int fault_in_user_writeable(u32 __user *uaddr)
int ret;
down_read(&mm->mmap_sem);
- ret = get_user_pages(current, mm, (unsigned long)uaddr,
- 1, 1, 0, NULL, NULL);
+ ret = __get_user_pages(current, mm, (unsigned long)uaddr, 1,
+ FOLL_WRITE | FOLL_FIXFAULT, NULL, NULL, NULL);
up_read(&mm->mmap_sem);
return ret < 0 ? ret : 0;
diff --git a/mm/memory.c b/mm/memory.c
index 40b7531..94b1d3f 100644
--- a/mm/memory.c
+++ b/mm/memory.c
@@ -1419,6 +1419,29 @@ int zap_vma_ptes(struct vm_area_struct *vma, unsigned long address,
}
EXPORT_SYMBOL_GPL(zap_vma_ptes);
+static void handle_pte_sw_young_dirty(struct vm_area_struct *vma,
+ unsigned long address,
+ pte_t *ptep, int write)
+{
+ pte_t entry = *ptep;
+
+ if (write)
+ pte_mkdirty(entry);
+ entry = pte_mkyoung(entry);
+ if (ptep_set_access_flags(vma, address, ptep, entry, write)) {
+ update_mmu_cache(vma, address, ptep);
+ } else {
+ /*
+ * This is needed only for protection faults but the arch code
+ * is not yet telling us if this is a protection fault or not.
+ * This still avoids useless tlb flushes for .text page faults
+ * with threads.
+ */
+ if (write)
+ flush_tlb_fix_spurious_fault(vma, address);
+ }
+}
+
/**
* follow_page - look up a page descriptor from a user-virtual address
* @vma: vm_area_struct mapping @address
@@ -1514,16 +1537,22 @@ split_fallthrough:
if (flags & FOLL_GET)
get_page(page);
- if (flags & FOLL_TOUCH) {
- if ((flags & FOLL_WRITE) &&
- !pte_dirty(pte) && !PageDirty(page))
- set_page_dirty(page);
- /*
- * pte_mkyoung() would be more correct here, but atomic care
- * is needed to avoid losing the dirty bit: it is easier to use
- * mark_page_accessed().
- */
- mark_page_accessed(page);
+
+ if (!pte_young(pte) || ((flags & FOLL_WRITE) && !pte_dirty(pte))) {
+ if (flags & FOLL_FIXFAULT)
+ handle_pte_sw_young_dirty(vma, address, ptep,
+ flags & FOLL_WRITE);
+ else if (flags & FOLL_TOUCH) {
+ if ((flags & FOLL_WRITE) &&
+ !pte_dirty(pte) && !PageDirty(page))
+ set_page_dirty(page);
+ /*
+ * pte_mkyoung() would be more correct here, but atomic care
+ * is needed to avoid losing the dirty bit: it is easier to use
+ * mark_page_accessed().
+ */
+ mark_page_accessed(page);
+ }
}
if ((flags & FOLL_MLOCK) && (vma->vm_flags & VM_LOCKED)) {
/*
@@ -3358,21 +3387,8 @@ int handle_pte_fault(struct mm_struct *mm,
if (!pte_write(entry))
return do_wp_page(mm, vma, address,
pte, pmd, ptl, entry);
- entry = pte_mkdirty(entry);
- }
- entry = pte_mkyoung(entry);
- if (ptep_set_access_flags(vma, address, pte, entry, flags & FAULT_FLAG_WRITE)) {
- update_mmu_cache(vma, address, pte);
- } else {
- /*
- * This is needed only for protection faults but the arch code
- * is not yet telling us if this is a protection fault or not.
- * This still avoids useless tlb flushes for .text page faults
- * with threads.
- */
- if (flags & FAULT_FLAG_WRITE)
- flush_tlb_fix_spurious_fault(vma, address);
}
+ handle_pte_sw_young_dirty(vma, address, pte, flags & FAULT_FLAG_WRITE);
unlock:
pte_unmap_unlock(pte, ptl);
return 0;
^ permalink raw reply related
* Re: [v2 PATCH 1/1] powerpc/4xx: enable and fix pcie gen1/gen2 on the 460sx
From: Tony Breeds @ 2011-07-18 4:01 UTC (permalink / raw)
To: Ayman Elkhashab
Cc: Ayman El-Khashab, Paul Mackerras, linuxppc-dev, linux-kernel
In-Reply-To: <1310748027-31956-1-git-send-email-aymane@elkhashab.com>
On Fri, Jul 15, 2011 at 11:40:27AM -0500, Ayman Elkhashab wrote:
> @@ -1582,8 +1628,8 @@ static int __init ppc4xx_setup_one_pciex_POM(struct ppc4xx_pciex_port *port,
> dcr_write(port->dcrs, DCRO_PEGPL_OMR2BAH, lah);
> dcr_write(port->dcrs, DCRO_PEGPL_OMR2BAL, lal);
> dcr_write(port->dcrs, DCRO_PEGPL_OMR2MSKH, 0x7fffffff);
> - /* Note that 3 here means enabled | single region */
> - dcr_write(port->dcrs, DCRO_PEGPL_OMR2MSKL, sa | 3);
> + dcr_write(port->dcrs, DCRO_PEGPL_OMR2MSKL,
> + sa | DCRO_PEGPL_OMRxMSKL_VAL);
Didn't you just change "sa | 3" to "sa | 1" ?
<snip>
> index 56d9e5d..61b3659 100644
> --- a/arch/powerpc/sysdev/ppc4xx_pci.h
> +++ b/arch/powerpc/sysdev/ppc4xx_pci.h
> @@ -464,6 +464,18 @@
> #define PECFG_POM2LAL 0x390
> #define PECFG_POM2LAH 0x394
>
> +/* 460sx only */
> +#define PECFG_460SX_DLLSTA 0x3f8
> +
> +/* 460sx Bit Mappings */
> +#define PECFG_460SX_DLLSTA_LINKUP 0x00000010
> +#define DCRO_PEGPL_460SX_OMR1MSKL_UOT 0x00000004
> +
> +/* PEGPL Bit Mappings */
> +#define DCRO_PEGPL_OMRxMSKL_VAL 0x00000001
> +#define DCRO_PEGPL_OMR1MSKL_UOT 0x00000002
> +#define DCRO_PEGPL_OMR3MSKL_IO 0x00000002
> +
> /* SDR Bit Mappings */
> #define PESDRx_RCSSET_HLDPLB 0x10000000
> #define PESDRx_RCSSET_RSTGU 0x01000000
Yours Tony
^ permalink raw reply
* [PATCH v2 5/5] powerpc: pSeries reconfig notifier error injection
From: Akinobu Mita @ 2011-07-18 1:16 UTC (permalink / raw)
To: linux-kernel, akpm; +Cc: linuxppc-dev, Paul Mackerras, Akinobu Mita
In-Reply-To: <1310951766-3840-1-git-send-email-akinobu.mita@gmail.com>
This provides the ability to inject artifical errors to pSeries reconfig
notifier chain callbacks. It is controlled through debugfs interface
under /sys/kernel/debug/pSeries-reconfig-notifier-error-inject/
Each of the files in the directory represents an event which can be
failed and contains the error code. If the notifier call chain should
be failed with some events notified, write the error code to the files.
Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Cc: Paul Mackerras <paulus@samba.org>
Cc: linuxppc-dev@lists.ozlabs.org
---
- Nothing changed from v1
arch/powerpc/platforms/pseries/reconfig.c | 31 +++++++++++++++++++++++++++++
lib/Kconfig.debug | 9 ++++++++
2 files changed, 40 insertions(+), 0 deletions(-)
diff --git a/arch/powerpc/platforms/pseries/reconfig.c b/arch/powerpc/platforms/pseries/reconfig.c
index 168651a..31d9b0f 100644
--- a/arch/powerpc/platforms/pseries/reconfig.c
+++ b/arch/powerpc/platforms/pseries/reconfig.c
@@ -117,6 +117,37 @@ int pSeries_reconfig_notify(unsigned long action, void *p)
return notifier_to_errno(err);
}
+#ifdef CONFIG_PSERIES_RECONFIG_NOTIFIER_ERROR_INJECTION
+
+static struct err_inject_notifier_block err_inject_reconfig_nb = {
+ .actions = {
+ { ERR_INJECT_NOTIFIER_ACTION(PSERIES_RECONFIG_ADD) },
+ { ERR_INJECT_NOTIFIER_ACTION(PSERIES_RECONFIG_REMOVE) },
+ { ERR_INJECT_NOTIFIER_ACTION(PSERIES_DRCONF_MEM_ADD) },
+ { ERR_INJECT_NOTIFIER_ACTION(PSERIES_DRCONF_MEM_REMOVE) },
+ {}
+ }
+};
+
+static int __init err_inject_reconfig_notifier_init(void)
+{
+ int err;
+
+ err = err_inject_notifier_block_init(&err_inject_reconfig_nb,
+ "pSeries-reconfig-notifier-error-inject", -1);
+ if (err)
+ return err;
+
+ err = pSeries_reconfig_notifier_register(&err_inject_reconfig_nb.nb);
+ if (err)
+ err_inject_notifier_block_cleanup(&err_inject_reconfig_nb);
+
+ return err;
+}
+late_initcall(err_inject_reconfig_notifier_init);
+
+#endif /* CONFIG_PSERIES_RECONFIG_NOTIFIER_ERROR_INJECTION */
+
static int pSeries_reconfig_add_node(const char *path, struct property *proplist)
{
struct device_node *np;
diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
index 8f5c380..d713000 100644
--- a/lib/Kconfig.debug
+++ b/lib/Kconfig.debug
@@ -1080,6 +1080,15 @@ config MEMORY_NOTIFIER_ERROR_INJECTION
# echo offline > /sys/devices/system/memory/memoryXXX/state
bash: echo: write error: Cannot allocate memory
+config PSERIES_RECONFIG_NOTIFIER_ERROR_INJECTION
+ bool "pSeries reconfig notifier error injection"
+ depends on PPC_PSERIES && NOTIFIER_ERROR_INJECTION
+ help
+ This option provides the ability to inject artifical errors to
+ pSeries reconfig notifier chain callbacks. It is controlled
+ through debugfs interface under
+ /sys/kernel/debug/pSeries-reconfig-notifier-error-inject/
+
config CPU_NOTIFIER_ERROR_INJECT
tristate "CPU notifier error injection module"
depends on HOTPLUG_CPU && DEBUG_KERNEL
--
1.7.4.4
^ permalink raw reply related
* [PATCH v2 1/5] fault-injection: notifier error injection
From: Akinobu Mita @ 2011-07-18 1:16 UTC (permalink / raw)
To: linux-kernel, akpm
Cc: Greg Kroah-Hartman, Akinobu Mita, Rafael J. Wysocki, linux-mm,
Paul Mackerras, Pavel Machek, linux-pm, linuxppc-dev
In-Reply-To: <1310951766-3840-1-git-send-email-akinobu.mita@gmail.com>
The notifier error injection provides the ability to inject artifical
errors to specified notifier chain callbacks. It is useful to test
the error handling of notifier call chain failures.
This adds common basic functions to define which type of events can be
fail and to initialize the debugfs interface to control what error code
should be returned and which event should be failed.
Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
Cc: Pavel Machek <pavel@ucw.cz>
Cc: "Rafael J. Wysocki" <rjw@sisk.pl>
Cc: linux-pm@lists.linux-foundation.org
Cc: Greg Kroah-Hartman <gregkh@suse.de>
Cc: linux-mm@kvack.org
Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Cc: Paul Mackerras <paulus@samba.org>
Cc: linuxppc-dev@lists.ozlabs.org
---
* v2
- put a comment in err_inject_notifier_block_init()
- only allow valid errno to be injected (-MAX_ERRNO <= errno <= 0)
include/linux/notifier.h | 25 ++++++++++++++
kernel/notifier.c | 81 ++++++++++++++++++++++++++++++++++++++++++++++
lib/Kconfig.debug | 11 ++++++
3 files changed, 117 insertions(+), 0 deletions(-)
diff --git a/include/linux/notifier.h b/include/linux/notifier.h
index c0688b0..51882d6 100644
--- a/include/linux/notifier.h
+++ b/include/linux/notifier.h
@@ -278,5 +278,30 @@ extern struct blocking_notifier_head reboot_notifier_list;
#define VT_UPDATE 0x0004 /* A bigger update occurred */
#define VT_PREWRITE 0x0005 /* A char is about to be written to the console */
+#ifdef CONFIG_NOTIFIER_ERROR_INJECTION
+
+struct err_inject_notifier_action {
+ unsigned long val;
+ int error;
+ const char *name;
+};
+
+#define ERR_INJECT_NOTIFIER_ACTION(action) \
+ .name = #action, .val = (action),
+
+struct err_inject_notifier_block {
+ struct notifier_block nb;
+ struct dentry *dir;
+ struct err_inject_notifier_action actions[];
+ /* The last slot must be terminated with zero sentinel */
+};
+
+extern int err_inject_notifier_block_init(struct err_inject_notifier_block *enb,
+ const char *name, int priority);
+extern void err_inject_notifier_block_cleanup(
+ struct err_inject_notifier_block *enb);
+
+#endif /* CONFIG_NOTIFIER_ERROR_INJECTION */
+
#endif /* __KERNEL__ */
#endif /* _LINUX_NOTIFIER_H */
diff --git a/kernel/notifier.c b/kernel/notifier.c
index 2488ba7..8824ae3 100644
--- a/kernel/notifier.c
+++ b/kernel/notifier.c
@@ -5,6 +5,7 @@
#include <linux/rcupdate.h>
#include <linux/vmalloc.h>
#include <linux/reboot.h>
+#include <linux/debugfs.h>
/*
* Notifier list for kernel code which wants to be called
@@ -584,3 +585,83 @@ int unregister_die_notifier(struct notifier_block *nb)
return atomic_notifier_chain_unregister(&die_chain, nb);
}
EXPORT_SYMBOL_GPL(unregister_die_notifier);
+
+#ifdef CONFIG_NOTIFIER_ERROR_INJECTION
+
+static int debugfs_errno_set(void *data, u64 val)
+{
+ *(int *)data = clamp_t(int, val, -MAX_ERRNO, 0);
+ return 0;
+}
+
+static int debugfs_errno_get(void *data, u64 *val)
+{
+ *val = *(int *)data;
+ return 0;
+}
+
+DEFINE_SIMPLE_ATTRIBUTE(fops_errno, debugfs_errno_get, debugfs_errno_set,
+ "%lld\n");
+
+static struct dentry *debugfs_create_errno(const char *name, mode_t mode,
+ struct dentry *parent, int *value)
+{
+ return debugfs_create_file(name, mode, parent, value, &fops_errno);
+}
+
+static int err_inject_notifier_callback(struct notifier_block *nb,
+ unsigned long val, void *p)
+{
+ int err = 0;
+ struct err_inject_notifier_block *enb =
+ container_of(nb, struct err_inject_notifier_block, nb);
+ struct err_inject_notifier_action *action;
+
+ for (action = enb->actions; action->name; action++) {
+ if (action->val == val) {
+ err = action->error;
+ break;
+ }
+ }
+ if (err) {
+ printk(KERN_INFO "Injecting error (%d) to %s\n",
+ err, action->name);
+ }
+
+ return notifier_from_errno(err);
+}
+
+int err_inject_notifier_block_init(struct err_inject_notifier_block *enb,
+ const char *name, int priority)
+{
+ struct err_inject_notifier_action *action;
+ mode_t mode = S_IFREG | S_IRUSR | S_IWUSR;
+
+ enb->nb.notifier_call = err_inject_notifier_callback;
+ enb->nb.priority = priority;
+
+ enb->dir = debugfs_create_dir(name, NULL);
+ if (!enb->dir)
+ return -ENOMEM;
+
+ for (action = enb->actions; action->name; action++) {
+ /*
+ * Create debugfs r/w file containing action->error. If
+ * notifier call chain is called with action->val, it will
+ * fail with the error code
+ */
+ if (!debugfs_create_errno(action->name, mode, enb->dir,
+ &action->error)) {
+ debugfs_remove_recursive(enb->dir);
+ return -ENOMEM;
+ }
+ }
+ return 0;
+}
+
+void err_inject_notifier_block_cleanup(struct err_inject_notifier_block *enb)
+{
+ debugfs_remove_recursive(enb->dir);
+}
+
+#endif /* CONFIG_NOTIFIER_ERROR_INJECTION */
diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
index dd373c8..8c6ce7e 100644
--- a/lib/Kconfig.debug
+++ b/lib/Kconfig.debug
@@ -1018,6 +1018,17 @@ config LKDTM
Documentation on how to use the module can be found in
Documentation/fault-injection/provoke-crashes.txt
+config NOTIFIER_ERROR_INJECTION
+ bool "Notifier error injection"
+ depends on DEBUG_KERNEL
+ select DEBUG_FS
+ help
+ This option provides the ability to inject artifical errors to
+ specified notifier chain callbacks. It is useful to test the error
+ handling of notifier call chain failures.
+
+ Say N if unsure.
+
config CPU_NOTIFIER_ERROR_INJECT
tristate "CPU notifier error injection module"
depends on HOTPLUG_CPU && DEBUG_KERNEL
--
1.7.4.4
^ permalink raw reply related
* Re: [PATCH 1/1] Fixup write permission of TLB on powerpc e500 core
From: Benjamin Herrenschmidt @ 2011-07-17 23:14 UTC (permalink / raw)
To: Peter Zijlstra
Cc: tony.luck, Peter Zijlstra, Shan Hai, linux-kernel, cmetcalf,
dhowells, paulus, tglx, walken, linuxppc-dev, akpm
In-Reply-To: <1310912990.25044.203.camel@pasglop>
On Mon, 2011-07-18 at 00:29 +1000, Benjamin Herrenschmidt wrote:
> A better approach might be a flag to pass to gup (via the "write"
> argument ? top bits ?) to tell it to immediately perform dirty/young
> updates.
So I dug a bit now that it's not 1am anymore :-)
Looks like gup changed a lot since I last looked. In fact, it already
has a very similar logic to what I want, with FOLL_TOUCH (which is set
by gup always and passed to __gup):
if (flags & FOLL_TOUCH) {
if ((flags & FOLL_WRITE) &&
!pte_dirty(pte) && !PageDirty(page))
set_page_dirty(page);
/*
* pte_mkyoung() would be more correct here, but atomic care
* is needed to avoid losing the dirty bit: it is easier to use
* mark_page_accessed().
*/
mark_page_accessed(page);
}
The problem here, is that we assume that having the struct page bits is
enough and we don't bother setting the PTE for either bits.
The problem with setting the PTE here is that while it would be
perfectly ok to do so under the PTL for archs that maintain dirty and
young in SW, for archs that do it in HW, this needs to be done in a way
that will be atomic vs. potential concurrent HW updates.
This could be done, I believe by using ptep_set_access_flags() but that
would be a waste on things like x86 or hash based powerpc who don't need
the PTE to be updated (x86 because of HW dirty/young updates, hash based
powerpc because our hash code does the updates and so looks to Linux
like it is HW updates).
At this point, I believe, we need to introduce a different behaviour
between architectures depending on how their mm works.
Peter, what do you reckon ? We could just have an
_ARCH_NEEDS_GUP_PTE_UPDATES and call ptep_set_access_flags() on those, I
believe that would be enough (it, it would mimmic what
handle_pte_fault() does to do the updates).
Something (not even compile tested) like:
diff --git a/mm/memory.c b/mm/memory.c
index 40b7531..32024ac 100644
--- a/mm/memory.c
+++ b/mm/memory.c
@@ -1515,6 +1515,17 @@ split_fallthrough:
if (flags & FOLL_GET)
get_page(page);
if (flags & FOLL_TOUCH) {
+#ifdef _ARCH_NEEDS_GUP_PTE_UPDATES
+ if (!pte_young(pte) ||
+ ((flags & FOLL_WRITE) && !pte_dirty(pte))) {
+ pte_t new_pte = pte_mkyoung(pte);
+
+ if (flags & FOLL_WRITE)
+ new_pte = pte_mkdirty(pte);
+ ptep_set_access_flags(vma, address, ptep, pte,
+ flags & FOLL_WRITE);
+ }
+#else
if ((flags & FOLL_WRITE) &&
!pte_dirty(pte) && !PageDirty(page))
set_page_dirty(page);
@@ -1524,6 +1535,7 @@ split_fallthrough:
* mark_page_accessed().
*/
mark_page_accessed(page);
+#endif
}
if ((flags & FOLL_MLOCK) && (vma->vm_flags & VM_LOCKED)) {
/*
I though we could try to factor the young/dirty update from handle_pte_fault
into a separate function and call it there, but I'm not sure whether we want
gup to go to the else case in there for flushing spurrious mappings.... actually,
thinking baout it:
That leads to another potential issue with the way we use gup
here to "fixup" atomic user access (ie, fake fault)... this call to
flush_tlb_fix_spurious_fault(), I'm not entirely certain what it's
doing, ie, it shouldn't be necessary on powerpc and is #ifdef'ed out on
x86, but I suppose -some- archs at least may lazily fixup permissions in a way
that requires it.
That means that an arch that needs that fixup will potentially also break with
the way the futex code relies on gup to do the faulting, since here too, we are
in a situation where gup -will- find a valid struct page and valid write
permission, but some kind of fixup is still needed.
It looks like a more robust fix would be to indeed factor out that code from
handle_pte_fault() and call it from gup as well, at least if the arch requires
it (and we can make "safe" archs like x86 not require it).
We do want to avoid that flush spurrious mapping on common gup's however,
it's going to be a killer. That means we need to inform gup that it's been
called in order to fixup a previously EFAULT'ing atomic user access, and
thus that we require it to perform all the necessary fixups.
In fact, with such a flag, we could probably avoid the ifdef entirely, and
always go toward the PTE fixup path when called in such a fixup case, my gut
feeling is that this is going to be seldom enough not to hurt x86 measurably
but we'll have to try it out.
That leads to that even less tested patch:
diff --git a/include/linux/mm.h b/include/linux/mm.h
index 9670f71..8a76694 100644
--- a/include/linux/mm.h
+++ b/include/linux/mm.h
@@ -1546,6 +1546,7 @@ struct page *follow_page(struct vm_area_struct *, unsigned long address,
#define FOLL_MLOCK 0x40 /* mark page as mlocked */
#define FOLL_SPLIT 0x80 /* don't return transhuge pages, split them */
#define FOLL_HWPOISON 0x100 /* check page is hwpoisoned */
+#define FOLL_FIXFAULT 0x200 /* fixup after a fault (PTE dirty/young upd) */
typedef int (*pte_fn_t)(pte_t *pte, pgtable_t token, unsigned long addr,
void *data);
diff --git a/kernel/futex.c b/kernel/futex.c
index fe28dc2..7480a93 100644
--- a/kernel/futex.c
+++ b/kernel/futex.c
@@ -355,8 +355,8 @@ static int fault_in_user_writeable(u32 __user *uaddr)
int ret;
down_read(&mm->mmap_sem);
- ret = get_user_pages(current, mm, (unsigned long)uaddr,
- 1, 1, 0, NULL, NULL);
+ ret = __get_user_pages(current, mm, (unsigned long)uaddr,
+ FOLL_WRITE | FOLL_FIXFAULT, NULL, NULL, NULL);
up_read(&mm->mmap_sem);
return ret < 0 ? ret : 0;
diff --git a/mm/memory.c b/mm/memory.c
index 40b7531..c61fddc 100644
--- a/mm/memory.c
+++ b/mm/memory.c
@@ -1419,6 +1419,29 @@ int zap_vma_ptes(struct vm_area_struct *vma, unsigned long address,
}
EXPORT_SYMBOL_GPL(zap_vma_ptes);
+static void handle_pte_sw_young_dirty(struct vm_area_struct *vma,
+ unsigned long address,
+ pte_t *ptep, int write)
+{
+ pte_t entry = *ptep;
+
+ if (write)
+ pte_mkdirty(entry);
+ entry = pte_mkyoung(entry);
+ if (ptep_set_access_flags(vma, address, pte, entry, write)) {
+ update_mmu_cache(vma, address, pte);
+ } else if (fault) {
+ /*
+ * This is needed only for protection faults but the arch code
+ * is not yet telling us if this is a protection fault or not.
+ * This still avoids useless tlb flushes for .text page faults
+ * with threads.
+ */
+ if (write)
+ flush_tlb_fix_spurious_fault(vma, address);
+ }
+}
+
/**
* follow_page - look up a page descriptor from a user-virtual address
* @vma: vm_area_struct mapping @address
@@ -1514,16 +1537,22 @@ split_fallthrough:
if (flags & FOLL_GET)
get_page(page);
- if (flags & FOLL_TOUCH) {
- if ((flags & FOLL_WRITE) &&
- !pte_dirty(pte) && !PageDirty(page))
- set_page_dirty(page);
- /*
- * pte_mkyoung() would be more correct here, but atomic care
- * is needed to avoid losing the dirty bit: it is easier to use
- * mark_page_accessed().
- */
- mark_page_accessed(page);
+
+ if (!pte_young(pte) || ((flags & FOLL_WRITE) && !pte_dirty(pte))) {
+ if (flags & FOLL_FIXFAULT)
+ handle_pte_sw_young_dirty(vma, address, ptep,
+ flags & FOLL_WRITE);
+ else if (flags & FOLL_TOUCH) {
+ if ((flags & FOLL_WRITE) &&
+ !pte_dirty(pte) && !PageDirty(page))
+ set_page_dirty(page);
+ /*
+ * pte_mkyoung() would be more correct here, but atomic care
+ * is needed to avoid losing the dirty bit: it is easier to use
+ * mark_page_accessed().
+ */
+ mark_page_accessed(page);
+ }
}
if ((flags & FOLL_MLOCK) && (vma->vm_flags & VM_LOCKED)) {
/*
@@ -3358,21 +3387,8 @@ int handle_pte_fault(struct mm_struct *mm,
if (!pte_write(entry))
return do_wp_page(mm, vma, address,
pte, pmd, ptl, entry);
- entry = pte_mkdirty(entry);
- }
- entry = pte_mkyoung(entry);
- if (ptep_set_access_flags(vma, address, pte, entry, flags & FAULT_FLAG_WRITE)) {
- update_mmu_cache(vma, address, pte);
- } else {
- /*
- * This is needed only for protection faults but the arch code
- * is not yet telling us if this is a protection fault or not.
- * This still avoids useless tlb flushes for .text page faults
- * with threads.
- */
- if (flags & FAULT_FLAG_WRITE)
- flush_tlb_fix_spurious_fault(vma, address);
}
+ handle_pte_sw_young_dirty(vma, address, pte, flags & FAULT_FLAG_WRITE);
unlock:
pte_unmap_unlock(pte, ptl);
return 0;
Any comment ?
Cheers,
Ben.
^ permalink raw reply related
* Re: [PATCH 1/1] Fixup write permission of TLB on powerpc e500 core
From: Benjamin Herrenschmidt @ 2011-07-17 22:34 UTC (permalink / raw)
To: Shan Hai
Cc: tony.luck, Peter Zijlstra, linux-kernel, cmetcalf, dhowells,
paulus, tglx, walken, linuxppc-dev, akpm
In-Reply-To: <4E230270.20209@gmail.com>
On Sun, 2011-07-17 at 23:40 +0800, Shan Hai wrote:
> On 07/17/2011 10:48 PM, Benjamin Herrenschmidt wrote:
> > On Sun, 2011-07-17 at 21:33 +0800, Shan Hai wrote:
> >> On ARM you could not protect pages from supervisor-mode writes,
> >> isn't it? That means, all writable user pages are writable for
> >> supervisor too, but its not hold for at least x86 and powerpc,
> >> x86 and powerpc can be configured to protect pages from
> >> supervisor-mode writes.
> > That doesn't sound right... how would put_user() work properly then ? A
> > cursory glance at the ARM code doesn't show it doing anything "special",
> > just stores ... but I might have missing something.
> >
>
> That's real for ARM, for the reason put_user() work properly is that
> the first time access to the write protected page triggers a page
> fault, and the handle_mm_fault() will fix up the write permission
> for the kernel, because at this time no one disabled the page fault
> as done in the futex case.
Sorry, you don't make sense here, you first say that ARM cannot protect
pages from supervisor writes, and then you write that put_user() will
work because it triggers a page fault :-) Those are mutually exclusive.
If you have a read-only PTE present, put_user() will trigger a page
fault on writes because the supervisor sees the same "write" protection
as userspace, for user pages that is, at least that's how it works on
most archs and I didn't think ARM was any different.
Note that things are different for -kernel- pages (ie, linear mapping,
vmalloc, etc...) for some archs. For example, on hash-table based
powerpc MMUs, it's not always possible to create a kernel-only read-only
mapping. But user mappings (below PAGE_OFFSET) always honor userspace
protections regardless of whether the CPU is in supervisor or user mode.
Anyway, we are getting on a side track here. Let's sort out our original
futex problem first.
> So, for the reason the SW tracked dirty/young and supervisor protected
> pages has potential effects on not only *futex* but also on other components
> of the kernel which might access the non-dirty supervisor protected page,
> in my opinion it might be more sensible to fix it from gup instead of fixing
> it in the futex.
Well, again it depends. First let's not use "supervisor protected" here.
Those pages are user pages. The supervisor just honors the user
protection from what I can tell. Real "supervisor protected" (such as
read-only kernel text pages etc...) are a completely different beast.
Secondly, we don't need anything special for the "normal" user access
cases, which are get/put_user, copy_tofrom_user, or gup followed with
access to the pages directly via the linear mapping, kmap, etc...
The problem is specific to code path doing user accesses within
pagefault_disable() sections -and- expecting to use gup to "fixup"
things when they fail.
Do we have many more than futex here ?
I -do- tend to prefer the fix inside gup approach for different reasons,
simply because gup already walks the page tables (well, follow_pages()
does) and we already have usable "flags" arguments to both gup and
follow_pages() that we can hijack to add our "update dirty & young now"
attribute.
So it should be a simple patch, provided Peter etc... agree in principle
with the approach.
Cheers,
Ben.
> Thanks
> Shan Hai
>
> > Cheers,
> > Ben.
> >
> >
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
^ permalink raw reply
* Re: [PATCH 1/1] Fixup write permission of TLB on powerpc e500 core
From: Shan Hai @ 2011-07-17 15:40 UTC (permalink / raw)
To: Benjamin Herrenschmidt
Cc: tony.luck, Peter Zijlstra, linux-kernel, cmetcalf, dhowells,
paulus, tglx, walken, linuxppc-dev, akpm
In-Reply-To: <1310914117.25044.216.camel@pasglop>
On 07/17/2011 10:48 PM, Benjamin Herrenschmidt wrote:
> On Sun, 2011-07-17 at 21:33 +0800, Shan Hai wrote:
>> On ARM you could not protect pages from supervisor-mode writes,
>> isn't it? That means, all writable user pages are writable for
>> supervisor too, but its not hold for at least x86 and powerpc,
>> x86 and powerpc can be configured to protect pages from
>> supervisor-mode writes.
> That doesn't sound right... how would put_user() work properly then ? A
> cursory glance at the ARM code doesn't show it doing anything "special",
> just stores ... but I might have missing something.
>
That's real for ARM, for the reason put_user() work properly is that
the first time access to the write protected page triggers a page
fault, and the handle_mm_fault() will fix up the write permission
for the kernel, because at this time no one disabled the page fault
as done in the futex case.
>> Think about the following situation,
>> a page fault occurs on the kernel trying to write to a writable shared
>> user page which is read only to the kernel, the following conditions
>> hold,
>> - the page is *present*, because its a shared page
>> - the page is *writable*, because demand paging sets up the pte for
>> the current process to so
>>
>> The follow_page() called in the __get_user_page() returns non NULL
>> to its caller on the above mentioned *present* and *writable* page,
>> so the gup(.write=1) has no chance to set pte dirty by calling
>> handle_mm_fault,
>> the follow_page() has no knowledge of supervisor-mode write protected
>> pages,
>> that's the culprit in the bug discussed here.
> Right, the problem is with writable pages that have "lost" (or never had
> but usually it's lost, due to swapping for example) their dirty bit, or
> any page that has lost young.
>
> From what I can tell, we need to either fix those bits from the caller
> of gup (futex code), which sound nasty, or more easily fix those from
> gup itself, possibly under control of flags in the "write" argument to
> avoid breaking code relying on the existing behaviour, expecially vs.
> dirty.
>
So, for the reason the SW tracked dirty/young and supervisor protected
pages has potential effects on not only *futex* but also on other components
of the kernel which might access the non-dirty supervisor protected page,
in my opinion it might be more sensible to fix it from gup instead of fixing
it in the futex.
Thanks
Shan Hai
> Cheers,
> Ben.
>
>
^ permalink raw reply
* Re: [PATCH 1/1] Fixup write permission of TLB on powerpc e500 core
From: Benjamin Herrenschmidt @ 2011-07-17 14:48 UTC (permalink / raw)
To: Shan Hai
Cc: tony.luck, Peter Zijlstra, linux-kernel, cmetcalf, dhowells,
paulus, tglx, walken, linuxppc-dev, akpm
In-Reply-To: <4E22E4AC.7040600@gmail.com>
On Sun, 2011-07-17 at 21:33 +0800, Shan Hai wrote:
>
> On ARM you could not protect pages from supervisor-mode writes,
> isn't it? That means, all writable user pages are writable for
> supervisor too, but its not hold for at least x86 and powerpc,
> x86 and powerpc can be configured to protect pages from
> supervisor-mode writes.
That doesn't sound right... how would put_user() work properly then ? A
cursory glance at the ARM code doesn't show it doing anything "special",
just stores ... but I might have missing something.
> Think about the following situation,
> a page fault occurs on the kernel trying to write to a writable shared
> user page which is read only to the kernel, the following conditions
> hold,
> - the page is *present*, because its a shared page
> - the page is *writable*, because demand paging sets up the pte for
> the current process to so
>
> The follow_page() called in the __get_user_page() returns non NULL
> to its caller on the above mentioned *present* and *writable* page,
> so the gup(.write=1) has no chance to set pte dirty by calling
> handle_mm_fault,
> the follow_page() has no knowledge of supervisor-mode write protected
> pages,
> that's the culprit in the bug discussed here.
Right, the problem is with writable pages that have "lost" (or never had
but usually it's lost, due to swapping for example) their dirty bit, or
any page that has lost young.
>From what I can tell, we need to either fix those bits from the caller
of gup (futex code), which sound nasty, or more easily fix those from
gup itself, possibly under control of flags in the "write" argument to
avoid breaking code relying on the existing behaviour, expecially vs.
dirty.
Cheers,
Ben.
^ permalink raw reply
* Re: [PATCH 1/1] Fixup write permission of TLB on powerpc e500 core
From: Benjamin Herrenschmidt @ 2011-07-17 14:34 UTC (permalink / raw)
To: Peter Zijlstra
Cc: tony.luck, Shan Hai, linux-kernel, cmetcalf, dhowells, paulus,
tglx, walken, linuxppc-dev, akpm
In-Reply-To: <1310900561.13765.22.camel@twins>
On Sun, 2011-07-17 at 13:02 +0200, Peter Zijlstra wrote:
>
> Again, _WHY_ isn't gup(.write=1) a complete write fault? Its supposed to
> be, it needs to break COW, do dirty page tracking and call page_mkwrite.
> I'm still thinking this e500 stuff is smoking crack.
>
> ARM has no hardware dirty bit either, and yet it works for them. I can't
> exactly tell how because I got lost in there, but it does, again,
> suggest e500 is on crack.
Because gup won't set dirty for a page that is already writable but
whose dirty bit has been "harvested" by the VM already. Same with young.
Maybe nobody sees it on ARM because nobody ever swaps on it ? :-) Or
they have some different way of handling dirty/young updates.. I don't
know.
e500 isn't the only one who will be affected. All the non-hash powerpc
subarchs will (I wrote a lot of that mm code so it's all my fault :-)
That includes 4xx and 64-bit BookE.
Cheers,
Ben.
^ permalink raw reply
* Re: [PATCH 1/1] Fixup write permission of TLB on powerpc e500 core
From: Benjamin Herrenschmidt @ 2011-07-17 14:29 UTC (permalink / raw)
To: Peter Zijlstra
Cc: tony.luck, Peter Zijlstra, Shan Hai, linux-kernel, cmetcalf,
dhowells, paulus, tglx, walken, linuxppc-dev, akpm
In-Reply-To: <4b337921-d430-4b63-bc36-ad31753cf800@email.android.com>
On Sun, 2011-07-17 at 11:38 +0200, Peter Zijlstra wrote:
> Whats this talk about interrup context? There is non of that involved.
Ok, let's not mix things here. I was going to fast and may have murkied
the waters. First let's get back to the futex code issue with gup:
> Furthermore, I still dont see the problem. The futex code is
> optimistically trying to poke at user memory while holding spinlocks.
>
> We fully expect that to fail, hence the error path that drops all
> locks and does the gup(.write=1) to fix up the mapping after which we
> try again.
See below
> This has worked for years, its by no means new code. Nor do I see how
> its broken.
No it hasn't worked for years, it's been broken for years on some archs
but nobody noticed :-)
The problem I see is that gup doesn't set dirty (or young) itself. It
requires the caller to set dirty before releasing the pages basically
and there's no provision for young. Afaik, callers set it directly in
the struct page and not the PTE too (which means a spurrious fault on
subsequent access for archs that do dirty tracking).
On archs where dirty and young are SW tracked, pages that don't have
them set in the PTE will fault on access. The lack of dirty means the
page is effectively going to be read-only and the lack of young means
the page will be inaccessible.
gup itself isn't mean to fix those, at least the way it's been used so
far, the caller of gup is.
Thus the only "proper" way to fix that is to have the futex code itself
perform dirty and accessed updates, which sucks (means going back down
the page tables, taking the PTL and doing the deed).
Having the actual fault handlers do the fixup even when in atomic isn't
a good option for us:
I don't know what other archs that rely on that SW tracking do, but in
the case of powerpc, that would be problematic due to the fact that
those archs have been written with the assumption that all changes to
PTEs are done under the PTL (which allows to simplify code and thus make
things faster).
Among others, that also means no changes at interrupt time. Enabling our
fault handlers to update the dirty & young bits even while in "atomic"
context would potentially open the door to things like interrupt-time
perf backtraces to cause PTE updates, etc... (in addition to generally
breaking the locking rules for PTE modifications).
Now, I suppose we -could- differentiate preempt disabled from real
interrupt time in the fault handlers, tho that's somewhat sucky. It
might require moving the dirty/young updates from generic code to arch
code, I suppose.
I'm also not sure how risky it would be to take the PTL in that case...
code doing user accesses within pagefault_disable() might be written
with the assumption that the PTL won't be taken (it might be already
held ? I don't know what all the users are at this point, too late to
grep, I can have a look tomorrow). It makes me a bit nervous too.
A better approach might be a flag to pass to gup (via the "write"
argument ? top bits ?) to tell it to immediately perform dirty/young
updates.
Cheers,
Ben.
^ permalink raw reply
* Re: [PATCH 1/1] Fixup write permission of TLB on powerpc e500 core
From: Shan Hai @ 2011-07-17 13:33 UTC (permalink / raw)
To: Peter Zijlstra
Cc: tony.luck, linux-kernel, cmetcalf, dhowells, paulus, tglx, walken,
linuxppc-dev, akpm
In-Reply-To: <1310900561.13765.22.camel@twins>
On 07/17/2011 07:02 PM, Peter Zijlstra wrote:
> On Sun, 2011-07-17 at 09:49 +1000, Benjamin Herrenschmidt wrote:
>> In the meantime, other than rewriting the futex code to not require
>> those in-atomic accesses (can't it just access the pages via the linear
>> mapping and/or kmap after the gup ?),
> That'll wreck performance on things like ARM and SPARC that have to deal
> with cache aliasing.
>
>> all I see would be a way to force
>> dirty and young after gup, with appropriate locks, or a variant of gup
>> (via a flag ?) to require it to do so.
> Again, _WHY_ isn't gup(.write=1) a complete write fault? Its supposed to
> be, it needs to break COW, do dirty page tracking and call page_mkwrite.
> I'm still thinking this e500 stuff is smoking crack.
>
> ARM has no hardware dirty bit either, and yet it works for them. I can't
> exactly tell how because I got lost in there, but it does, again,
> suggest e500 is on crack.
Ok, the following feature of the architecture causes failure of
gup(.write=1) on dirtying pages,
- allows pages to be protected from supervisor-mode writes
On ARM you could not protect pages from supervisor-mode writes,
isn't it? That means, all writable user pages are writable for
supervisor too, but its not hold for at least x86 and powerpc,
x86 and powerpc can be configured to protect pages from
supervisor-mode writes.
Think about the following situation,
a page fault occurs on the kernel trying to write to a writable shared
user page which is read only to the kernel, the following conditions hold,
- the page is *present*, because its a shared page
- the page is *writable*, because demand paging sets up the pte for
the current process to so
The follow_page() called in the __get_user_page() returns non NULL
to its caller on the above mentioned *present* and *writable* page,
so the gup(.write=1) has no chance to set pte dirty by calling
handle_mm_fault,
the follow_page() has no knowledge of supervisor-mode write protected pages,
that's the culprit in the bug discussed here.
Thanks
Shan Hai
^ permalink raw reply
* Re: [PATCH] dtc: Remove unused check variable
From: Jon Loeliger @ 2011-07-17 12:36 UTC (permalink / raw)
To: David Gibson; +Cc: linuxppc-dev, Josh Boyer
In-Reply-To: <20110715135732.GC4368@yookeroo.fritz.box>
> On Tue, Jun 28, 2011 at 08:47:09AM -0400, Josh Boyer wrote:
> > Commit 376ab6f2 removed the old style check functionality from DTC,
> > however the check option and variable were not removed. This leads to
> > build failures when -Werror=unused-but-set-variable is specified:
> >
> > dtc.c: In function 'main':
> > dtc.c:102:17: error: variable 'check' set but not used [-Werror=unused-but-set-variable]
> > cc1: all warnings being treated as errors
> > make: *** [dtc.o] Error 1
> > make: *** Waiting for unfinished jobs....
> >
> > Remove the check variable.
> >
> > Signed-off-by: Josh Boyer <jwboyer@linux.vnet.ibm.com>
>
> Acked-by: David Gibson <david@gibson.dropbear.id.au>
Applied.
jdl
^ permalink raw reply
* Re: [PATCH v2] dtc: Remove unused variable in flat_read_mem_reserve
From: Jon Loeliger @ 2011-07-17 12:36 UTC (permalink / raw)
To: David Gibson; +Cc: linuxppc-dev, Josh Boyer
In-Reply-To: <20110715135755.GD4368@yookeroo.fritz.box>
> On Tue, Jun 28, 2011 at 09:47:11AM -0400, Josh Boyer wrote:
> > The *p variable is declared and used to save inb->ptr, however p is
> > later never used. This has been the case since commit 6c0f3676 and can
> > lead to build failures with -Werror=unused-but-set-variable:
> >
> > flattree.c: In function 'flat_read_mem_reserve':
> > flattree.c:700:14: error: variable 'p' set but not used [-Werror=unused-but-set-variable]
> > cc1: all warnings being treated as errors
> > make: *** [flattree.o] Error 1
> >
> > Remove the variable.
> >
> > Signed-off-by: Josh Boyer <jwboyer@linux.vnet.ibm.com>
>
> Acked-by: David Gibson <david@gibson.dropbear.id.au>
Applied.
jdl
^ permalink raw reply
* Re: [PATCH 1/1] Fixup write permission of TLB on powerpc e500 core
From: Peter Zijlstra @ 2011-07-17 11:02 UTC (permalink / raw)
To: Benjamin Herrenschmidt
Cc: tony.luck, Shan Hai, linux-kernel, cmetcalf, dhowells, paulus,
tglx, walken, linuxppc-dev, akpm
In-Reply-To: <1310860194.25044.17.camel@pasglop>
On Sun, 2011-07-17 at 09:49 +1000, Benjamin Herrenschmidt wrote:
> In the meantime, other than rewriting the futex code to not require
> those in-atomic accesses (can't it just access the pages via the linear
> mapping and/or kmap after the gup ?),
That'll wreck performance on things like ARM and SPARC that have to deal
with cache aliasing.
> all I see would be a way to force
> dirty and young after gup, with appropriate locks, or a variant of gup
> (via a flag ?) to require it to do so.=20
Again, _WHY_ isn't gup(.write=3D1) a complete write fault? Its supposed to
be, it needs to break COW, do dirty page tracking and call page_mkwrite.
I'm still thinking this e500 stuff is smoking crack.
ARM has no hardware dirty bit either, and yet it works for them. I can't
exactly tell how because I got lost in there, but it does, again,
suggest e500 is on crack.
^ permalink raw reply
* Re: [PATCH 1/1] Fixup write permission of TLB on powerpc e500 core
From: Peter Zijlstra @ 2011-07-17 9:38 UTC (permalink / raw)
To: Benjamin Herrenschmidt, Shan Hai
Cc: tony.luck, Peter Zijlstra, linux-kernel, cmetcalf, dhowells,
paulus, tglx, walken, linuxppc-dev, akpm
In-Reply-To: <1310860194.25044.17.camel@pasglop>
Benjamin Herrenschmidt <benh@kernel.crashing.org> wrote:
>On Sat, 2011-07-16 at 10:50 -0400, Shan Hai wrote:
>>
>> That's right the gup(.write=1) want to do the same thing as the
>> above code snippet, but it failed for the following reason:
>> because the get_user_pages() would not dirty pte for the reason
>> the follow_page() returns not NULL on *present* and *writable*
>> page, the page which holds the lock is present because its a shared
>> page,
>> writable because demand paging set that up so for shared
>> writable page, so the handle_mm_fault() in the __get_user_page()
>> could not be called.
>>
>> Why the above code could do the same task, because by calling
>> handle_mm_fault() will set pte dirty by
>> [do_annonymous_page(), memory.c]
>> if (vma->vm_flags & VM_WRITE)
>> entry = pte_mkwrite(pte_mkdirty(entry));
>>
>
>Right. gup won't set page_dirty, it expects the caller to do so (in
>case
>it doesn't dirty all the gup'ed pages a suppose).
>
>You could probably fix the problem here by setting dirty after gup in
>the futex code if you know you're going to write. You must do that with
>the PTE lock held though and -not- at interrupt time.
>
>Note however that the exact same problem exist with normal "read"
>accesses and page_young (_PAGE_ACCESSED on powerpc). The page will not
>be accessible until that bit is set and it's set by SW.
>
>As I wrote earlier, fixing that by making "atomic" page faults perform
>the dirty/accessed tracking is not right, since such faults can happen
>at interrupt time and the PTE lock cannot be taken at interrupt time.
>
>IE. The implementation of those "SW" TLB archs heavily relies on the
>PTE
>lock to serialize write access to the PTE and writing it outside of
>that
>lock would do really bad things.
>
>So there's a deeper problem here. The whole user access "in atomic"
>concept is by itself a violation of some of the basic access rules of
>user memory that have existed from day 1 of the kernel. That we allow
>it
>for semi-harmless (and allowed to fail) things like snapshot of
>backtraces for perf is one thing, but relying on it for the futex case
>like that is not going to fly very well. I sincerely hope that this
>kind
>of usage is not going to become a habit.
>
>In the meantime, other than rewriting the futex code to not require
>those in-atomic accesses (can't it just access the pages via the linear
>mapping and/or kmap after the gup ?), all I see would be a way to force
>dirty and young after gup, with appropriate locks, or a variant of gup
>(via a flag ?) to require it to do so.
>
>Cheers,
>Ben.
Whats this talk about interrup context? There is non of that involved.
Furthermore, I still dont see the problem. The futex code is optimistically trying to poke at user memory while holding spinlocks.
We fully expect that to fail, hence the error path that drops all locks and does the gup(.write=1) to fix up the mapping after which we try again.
This has worked for years, its by no means new code. Nor do I see how its broken.
--
Sent from my Android phone with K-9 Mail. Please excuse my brevity.
^ permalink raw reply
* Re: [PATCH 1/1] Fixup write permission of TLB on powerpc e500 core
From: Benjamin Herrenschmidt @ 2011-07-16 23:49 UTC (permalink / raw)
To: Shan Hai
Cc: tony.luck, Peter Zijlstra, linux-kernel, cmetcalf, dhowells,
paulus, tglx, walken, linuxppc-dev, akpm
In-Reply-To: <4E21A526.8010904@gmail.com>
On Sat, 2011-07-16 at 10:50 -0400, Shan Hai wrote:
>
> That's right the gup(.write=1) want to do the same thing as the
> above code snippet, but it failed for the following reason:
> because the get_user_pages() would not dirty pte for the reason
> the follow_page() returns not NULL on *present* and *writable*
> page, the page which holds the lock is present because its a shared
> page,
> writable because demand paging set that up so for shared
> writable page, so the handle_mm_fault() in the __get_user_page()
> could not be called.
>
> Why the above code could do the same task, because by calling
> handle_mm_fault() will set pte dirty by
> [do_annonymous_page(), memory.c]
> if (vma->vm_flags & VM_WRITE)
> entry = pte_mkwrite(pte_mkdirty(entry));
>
Right. gup won't set page_dirty, it expects the caller to do so (in case
it doesn't dirty all the gup'ed pages a suppose).
You could probably fix the problem here by setting dirty after gup in
the futex code if you know you're going to write. You must do that with
the PTE lock held though and -not- at interrupt time.
Note however that the exact same problem exist with normal "read"
accesses and page_young (_PAGE_ACCESSED on powerpc). The page will not
be accessible until that bit is set and it's set by SW.
As I wrote earlier, fixing that by making "atomic" page faults perform
the dirty/accessed tracking is not right, since such faults can happen
at interrupt time and the PTE lock cannot be taken at interrupt time.
IE. The implementation of those "SW" TLB archs heavily relies on the PTE
lock to serialize write access to the PTE and writing it outside of that
lock would do really bad things.
So there's a deeper problem here. The whole user access "in atomic"
concept is by itself a violation of some of the basic access rules of
user memory that have existed from day 1 of the kernel. That we allow it
for semi-harmless (and allowed to fail) things like snapshot of
backtraces for perf is one thing, but relying on it for the futex case
like that is not going to fly very well. I sincerely hope that this kind
of usage is not going to become a habit.
In the meantime, other than rewriting the futex code to not require
those in-atomic accesses (can't it just access the pages via the linear
mapping and/or kmap after the gup ?), all I see would be a way to force
dirty and young after gup, with appropriate locks, or a variant of gup
(via a flag ?) to require it to do so.
Cheers,
Ben.
^ permalink raw reply
* Re: [PATCH 1/1] Fixup write permission of TLB on powerpc e500 core
From: Shan Hai @ 2011-07-16 15:36 UTC (permalink / raw)
To: Peter Zijlstra
Cc: tony.luck, linux-kernel, cmetcalf, dhowells, paulus, tglx, walken,
linuxppc-dev, akpm
In-Reply-To: <1310743477.2586.358.camel@twins>
On 07/15/2011 11:24 AM, Peter Zijlstra wrote:
> On Fri, 2011-07-15 at 11:18 -0400, Shan Hai wrote:
>
>>>> + vma = find_vma(mm, address);
>>> Uhm, find_vma() needs mmap_sem, and futex_atomic_cmpxchg_inatomic() is
>>> most certainly not called with that lock held.
>>>
>> My fault, that will be fixed in the V2 patch.
> But you cannot, the function isn't called _atomic_ just for kicks, its
> used while holding spinlocks.
>
Yes we can do that, _atomic_ here is just atomic for cmpxchg
implemented by the combination of 'lwarx' and 'stwcx.' instructions
as done in the spin lock implementation, so even we hold the
mmap_sem that has no impact on the _atomic_ feature of the
futex_atomic_cmpxchg_inatomic().
>>>> + if (likely(vma)) {
>>>> + /* only fixup present page */
>>>> + if (follow_page(vma, address, FOLL_WRITE)) {
>>>> + handle_mm_fault(mm, vma, address, FAULT_FLAG_WRITE);
>>> So how can this toggle your sw dirty/young tracking, that's pretty much
>>> what gup(.write=1) does too!
>>>
>> because of the kernel read only permission of the page is transparent
>> to the follow_page(), the handle_mm_fault() is not to be activated
>> in the __get_use_pages(), so the gup(.write=1) could not help to fixup
>> the write permission.
> So why do you need the vma? Is it like I wrote earlier that you don't
> have spare PTE bits and need the vma flags to see if it may become
> writable?
>
Need vma for the reason to call handle_mm_fault(), that's all.
> gup(.write=1) not triggering this is a serious problem though, not
> something you can just paper over. I wouldn't be at all surprised to
> find there's more things broken because of that.
In my opinion another solution might be check the read only for kernel
feature of a page in the follow_page() on gup(.write=1) to avoid this
problem on all architectures.
Thanks
Shan Hai
^ permalink raw reply
* Re: [PATCH 0/1] Fixup write permission of TLB on powerpc e500 core
From: Shan Hai @ 2011-07-16 15:03 UTC (permalink / raw)
To: Benjamin Herrenschmidt
Cc: tony.luck, Peter Zijlstra, linux-kernel, cmetcalf, dhowells,
David Laight, paulus, tglx, walken, linuxppc-dev, akpm
In-Reply-To: <1310775649.25044.5.camel@pasglop>
On 07/15/2011 08:20 PM, Benjamin Herrenschmidt wrote:
> On Fri, 2011-07-15 at 11:32 -0400, Shan Hai wrote:
>> I agree with you, the problem could be triggered by accessing
>> any user space page which has kernel read only permission
>> in the page fault disabled context, the problem also affects
>> architectures which depend on SW dirty/young tracking as
>> stated by Benjamin in this thread.
>>
>> In the e500 case, the commit 6cfd8990e27d3a491c1c605d6cbc18a46ae51fef
>> removed the write permission fixup from TLB miss handlers and left it to
>> generic code, so it might be right time to fixup the write permission here
>> in the generic code.
> But we can't. The must not modify the PTE from an interrupt context and
> the "atomic" variants of user accesses can be called in such contexts.
>
> I think the problem is that we try to actually do things other than just
> "peek" at user memory (for backtraces etc...) but actually useful things
> in page fault disabled contexts. That's bad and various archs mm were
> designed with the assumption that this never happens.
>
Yes I understood, the *here* above means 'generic code' like futex code,
I am sorry for my ambiguous description.
> If the futex case is seldom here, we could probably find a way to work
> around in that specific case.
>
That's what my patch wants to do.
> However, I -still- don't understand why gup didn't fixup the write
> permission. gup doesn't set dirty ?
>
Yep, gup doesn't set dirty, because when the page fault
occurs on the kernel accessing a user page which is
read only to the kernel the following conditions hold,
- the page is present, because its a shared page
- the page is writable, because demand paging
sets up the pte for the current process to so
The follow_page() called in the __get_user_page()
returns non NULL to its caller on the above mentioned
present and writable page, so the gup(.write=1) has no
chance to set pte dirty by calling handle_mm_fault
Thanks
Shan Hai
s
> Cheers,
> Ben.
>
>
^ permalink raw reply
* Re: [PATCH 1/1] Fixup write permission of TLB on powerpc e500 core
From: Shan Hai @ 2011-07-16 14:50 UTC (permalink / raw)
To: Peter Zijlstra
Cc: tony.luck, linux-kernel, cmetcalf, dhowells, paulus, tglx, walken,
linuxppc-dev, akpm
In-Reply-To: <1310725418.2586.309.camel@twins>
On 07/15/2011 06:23 AM, Peter Zijlstra wrote:
> On Fri, 2011-07-15 at 16:07 +0800, Shan Hai wrote:
>> The kernel has no write permission on COW pages by default on e500 core, this
>> will cause endless loop in futex_lock_pi, because futex code assumes the kernel
>> has write permission on COW pages. Grant write permission to the kernel on COW
>> pages when access violation page fault occurs.
>>
>> Signed-off-by: Shan Hai<haishan.bai@gmail.com>
>> ---
>> arch/powerpc/include/asm/futex.h | 11 ++++++++++-
>> arch/powerpc/include/asm/tlb.h | 25 +++++++++++++++++++++++++
>> 2 files changed, 35 insertions(+), 1 deletions(-)
>>
>> diff --git a/arch/powerpc/include/asm/futex.h b/arch/powerpc/include/asm/futex.h
>> index c94e4a3..54c3e74 100644
>> --- a/arch/powerpc/include/asm/futex.h
>> +++ b/arch/powerpc/include/asm/futex.h
>> @@ -8,6 +8,7 @@
>> #include<asm/errno.h>
>> #include<asm/synch.h>
>> #include<asm/asm-compat.h>
>> +#include<asm/tlb.h>
>>
>> #define __futex_atomic_op(insn, ret, oldval, uaddr, oparg) \
>> __asm__ __volatile ( \
>> @@ -113,7 +114,15 @@ futex_atomic_cmpxchg_inatomic(u32 *uval, u32 __user *uaddr,
>> : "cc", "memory");
>>
>> *uval = prev;
>> - return ret;
>> +
>> + /* Futex assumes the kernel has permission to write to
>> + * COW pages, grant the kernel write permission on COW
>> + * pages because it has none by default.
>> + */
>> + if (ret == -EFAULT)
>> + __tlb_fixup_write_permission(current->mm, (unsigned long)uaddr);
>> +
>> + return ret;
>> }
>>
>> #endif /* __KERNEL__ */
>> diff --git a/arch/powerpc/include/asm/tlb.h b/arch/powerpc/include/asm/tlb.h
>> index e2b428b..3863c6a 100644
>> --- a/arch/powerpc/include/asm/tlb.h
>> +++ b/arch/powerpc/include/asm/tlb.h
>> @@ -45,5 +45,30 @@ static inline void __tlb_remove_tlb_entry(struct mmu_gather *tlb, pte_t *ptep,
>> #endif
>> }
>>
>> +/* Grant write permission to the kernel on a page. */
>> +static inline void __tlb_fixup_write_permission(struct mm_struct *mm,
>> + unsigned long address)
>> +{
>> +#if defined(CONFIG_FSL_BOOKE)
>> + /* Grant write permission to the kernel on a page by setting TLB.SW
>> + * bit, the bit setting operation is tricky here, calling
>> + * handle_mm_fault with FAULT_FLAG_WRITE causes _PAGE_DIRTY bit of
>> + * the pte to be set, the _PAGE_DIRTY of the pte is translated into
>> + * TLB.SW on Powerpc e500 core.
>> + */
>> +
>> + struct vm_area_struct *vma;
>> +
>> + vma = find_vma(mm, address);
> Uhm, find_vma() needs mmap_sem, and futex_atomic_cmpxchg_inatomic() is
> most certainly not called with that lock held.
>
>> + if (likely(vma)) {
>> + /* only fixup present page */
>> + if (follow_page(vma, address, FOLL_WRITE)) {
>> + handle_mm_fault(mm, vma, address, FAULT_FLAG_WRITE);
> So how can this toggle your sw dirty/young tracking, that's pretty much
> what gup(.write=1) does too!
>
That's right the gup(.write=1) want to do the same thing as the
above code snippet, but it failed for the following reason:
because the get_user_pages() would not dirty pte for the reason
the follow_page() returns not NULL on *present* and *writable*
page, the page which holds the lock is present because its a shared page,
writable because demand paging set that up so for shared
writable page, so the handle_mm_fault() in the __get_user_page()
could not be called.
Why the above code could do the same task, because by calling
handle_mm_fault() will set pte dirty by
[do_annonymous_page(), memory.c]
if (vma->vm_flags & VM_WRITE)
entry = pte_mkwrite(pte_mkdirty(entry));
Thanks
Shan Hai
>> + flush_tlb_page(vma, address);
>> + }
>> + }
>> +#endif
>> +}
>> +
>> #endif /* __KERNEL__ */
>> #endif /* __ASM_POWERPC_TLB_H */
^ permalink raw reply
* [PATCH] powerpc: Exporting boot_cpuid_phys
From: Andrew Gabbasov @ 2011-07-16 13:22 UTC (permalink / raw)
To: benh; +Cc: openmcapi-dev, linuxppc-dev
Kernel loadable module can use hard_smp_processor_id() if building with SMP
kernel. In order to make it work for UP kernels too, boot_cpuid_phys
symbol (which is what hard_smp_processor_id() macro resolves to
in non-SMP configuration) must be exported.
Signed-off-by: Andrew Gabbasov <andrew_gabbasov@mentor.com>
---
arch/powerpc/kernel/setup_32.c | 1 +
1 files changed, 1 insertions(+), 0 deletions(-)
diff --git a/arch/powerpc/kernel/setup_32.c b/arch/powerpc/kernel/setup_32.c
index 1d2fbc9..3dffce6 100644
--- a/arch/powerpc/kernel/setup_32.c
+++ b/arch/powerpc/kernel/setup_32.c
@@ -49,6 +49,7 @@ extern void bootx_init(unsigned long r4, unsigned long phys);
int boot_cpuid = -1;
EXPORT_SYMBOL_GPL(boot_cpuid);
int boot_cpuid_phys;
+EXPORT_SYMBOL_GPL(boot_cpuid_phys);
int smp_hw_index[NR_CPUS];
^ permalink raw reply related
* RE: [v3 PATCH 1/1] booke/kprobe: make program exception to use one dedicated exception stack
From: Chen, Tiejun @ 2011-07-16 3:25 UTC (permalink / raw)
To: Scott Wood; +Cc: linuxppc-dev@ozlabs.org
In-Reply-To: <20110715134232.56373e03@schlenkerla.am.freescale.net>
> -----Original Message-----
> From: Scott Wood [mailto:scottwood@freescale.com]=20
> Sent: Saturday, July 16, 2011 2:43 AM
> To: Chen, Tiejun
> Cc: Kumar Gala; linuxppc-dev@ozlabs.org
> Subject: Re: [v3 PATCH 1/1] booke/kprobe: make program=20
> exception to use one dedicated exception stack
>=20
> On Fri, 15 Jul 2011 13:28:15 +0800
> tiejun.chen <tiejun.chen@windriver.com> wrote:
>=20
> > Kumar Gala wrote:
> > > I'm still very confused why we need a unique stack frame=20
> for kprobe/program exceptions on book-e devices.
> >=20
> > Its a bug at least for Book-E.
>=20
> But why only booke? There's nothing booke-specific about the=20
I don't mean this is reproduced only on booke, so I use 'at least' carefull=
y to notice we really see this problem on booke.
> stwu instruction.
Please note this root cause to this bug is not related to how to emulate st=
wu instruction. That should be issued from the overlap between an exception=
frame and the kprobed function stack frame on booke. Would you like to see=
that example I showed?
>=20
> > And if you'd like to check another topic thread,
> > "[BUG?]3.0-rc4+ftrace+kprobe: set kprobe at instruction=20
> 'stwu' lead to=20
> > system crash/freeze", you can know this story completely :)
> >=20
> > This bug should not be reproduced on PPC64 with the exception=20
> > prolog/endlog dedicated to PPC64.
>=20
> What if the function you're stepping through is creating a=20
> stack frame that is larger than the 288 bytes that get skipped?
>From that bug email looks PPC64 should be good from some feedbacks. So here=
I think guys who are very familiar with PPC64 can reply this rather than m=
e :)
>=20
> > But I have no enough time to check other PPC32 & !BOOKE so I'm not=20
> > sure if we should extend this modification.
>=20
> Someone should.
This is fine to me.
But currently we have to fix this based on booke firstly.
Tiejun=20
>=20
> -Scott
>=20
> =
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox