Le 03/02/2020 à 01:46, Russell Currey a écrit :
On Wed, 2020-01-08 at 13:52 +0100, Christophe Leroy wrote:
Le 24/12/2019 à 06:55, Russell Currey a écrit :
diff --git a/arch/powerpc/mm/Makefile b/arch/powerpc/mm/Makefile
index 5e147986400d..d0a0bcbc9289 100644
--- a/arch/powerpc/mm/Makefile
+++ b/arch/powerpc/mm/Makefile
@@ -20,3 +20,4 @@ obj-$(CONFIG_HIGHMEM) += highmem.o
obj-$(CONFIG_PPC_COPRO_BASE) += copro_fault.o
obj-$(CONFIG_PPC_PTDUMP) += ptdump/
obj-$(CONFIG_KASAN) += kasan/
+obj-$(CONFIG_ARCH_HAS_SET_MEMORY) += pageattr.o
CONFIG_ARCH_HAS_SET_MEMORY is set inconditionnally, I think you
should
add pageattr.o to obj-y instead. CONFIG_ARCH_HAS_XXX are almost
never
used in Makefiles
Fair enough, will keep that in mind
I forgot I commented that. I'll do it in v3.
+ pte_t pte_val;
+
+ // invalidate the PTE so it's safe to modify
+ pte_val = ptep_get_and_clear(&init_mm, addr, ptep);
+ flush_tlb_kernel_range(addr, addr + PAGE_SIZE);
Why flush a range for a single page ? On most targets this will do a
tlbia which is heavy, while a tlbie would suffice.
I think flush_tlb_kernel_range() should be replaced by something
flushing only a single page.
You might be able to help me out here, I wanted to do that but the only
functions I could find that flushed single pages needed a
vm_area_struct, which I can't get.
I sent out two patches for that, one for book3s/32 and one for nohash:
https://patchwork.ozlabs.org/patch/1231983/
https://patchwork.ozlabs.org/patch/1232223/
Maybe one for book3s/64 would be needed as well ? Can you do it if needed ?
+
+ // modify the PTE bits as desired, then apply
+ switch (action) {
+ case SET_MEMORY_RO:
+ pte_val = pte_wrprotect(pte_val);
+ break;
+ case SET_MEMORY_RW:
+ pte_val = pte_mkwrite(pte_val);
+ break;
+ case SET_MEMORY_NX:
+ pte_val = pte_exprotect(pte_val);
+ break;
+ case SET_MEMORY_X:
+ pte_val = pte_mkexec(pte_val);
+ break;
+ default:
+ WARN_ON(true);
+ return -EINVAL;
Is it worth checking that the action is valid for each page ? I
think
validity of action should be checked in change_memory_attr(). All
other
functions are static so you know they won't be called from outside.
Once done, you can squash __change_page_attr() into
change_page_attr(),
remove the ret var and return 0 all the time.
Makes sense to fold things into a single function, but in terms of
performance it shouldn't make a difference, right? I still have to
check the action to determine what to change (unless I replace passing
SET_MEMORY_RO into apply_to_page_range() with a function pointer to
pte_wrprotect() for example).
pte_wrprotect() is a static inline.
+ }
+
+ set_pte_at(&init_mm, addr, ptep, pte_val);
+
+ return 0;
+}
+
+static int change_page_attr(pte_t *ptep, unsigned long addr, void
*data)
+{
+ int ret;
+
+ spin_lock(&init_mm.page_table_lock);
+ ret = __change_page_attr(ptep, addr, data);
+ spin_unlock(&init_mm.page_table_lock);
+
+ return ret;
+}
+
+int change_memory_attr(unsigned long addr, int numpages, int
action)
+{
+ unsigned long start = ALIGN_DOWN(addr, PAGE_SIZE);
+ unsigned long size = numpages * PAGE_SIZE;
+
+ if (!numpages)
+ return 0;
+
+ return apply_to_page_range(&init_mm, start, size,
change_page_attr, &action);
Use (void*)action instead of &action (see upper comment)
To get this to work I had to use (void *)(size_t)action to stop the
compiler from complaining about casting an int to a void*, is there a
better way to go about it? Works fine, just looks gross.
Yes, use long instead (see my v3)
Christophe
