Mark Kettenis wrote:
> The thing you guys are missing is that on some architectures making changes
> to instructions (PT_WRITE_I) requires some additional operations to
> guarantee that the CPU actually sees those updated instructions. Typically
> this is the case on architectures with separate data and instruction caches,
> where the instruction cache doesn't snoop the data cache. On such
> architectures (powerpc and arm are examples) you need to flush the data
> cache and invalidate the instruction cache. That may be a somewhat
> expensive operation.
> As you probably guessed, pmap_proc_iflush() is the function that takes care
> of this. Since you still call pmap_proc_iflush(), the diff isn't wrong from
> a correctness point of view, but I think we should keep the optimization of
> not calling pmap_proc_iflush() for PT_WRITE_D.
Well his makes sense. I pondered while making this change whether
or not I should change the second condition (the iflush one), obviously
I shouldn't have. Heh, I pay my lack of knowledge on !x86 arch.
Thanks for the input!
> As for the original issue. Adding UVM_IO_FIXPROT for PT_WRITE_D as well,
> means that it will now be able to make changes to read-only data. That is
> probably corrrect.
>
> So I think the only thing that should be changed is the following bit:
>
> >@@ -734,11 +724,11 @@ process_domem(struct proc *curp, struct proc *p,
> >struct uio *uio, int req)
> > vm->vm_refcnt++;
> >
> > error = uvm_io(&vm->vm_map, uio,
> >- (req == PT_WRITE_I) ? UVM_IO_FIXPROT : 0);
> >+ (uio->uio_rw == UIO_WRITE) ? UVM_IO_FIXPROT : 0);
> >
>
> Is that indeed enough to fix the original problem?
Yes this is indeed enough, and I just confirmed it with my test program.
So here is (hopefully) the latest version of this diff, which is now way
shorter.
diff --git a/sys/kern/sys_process.c b/sys/kern/sys_process.c
index 60ec50e..09e56b9 100644
--- a/sys/kern/sys_process.c
+++ b/sys/kern/sys_process.c
@@ -734,7 +734,7 @@ process_domem(struct proc *curp, struct proc *p, struct uio
*uio, int req)
vm->vm_refcnt++;
error = uvm_io(&vm->vm_map, uio,
- (req == PT_WRITE_I) ? UVM_IO_FIXPROT : 0);
+ (uio->uio_rw == UIO_WRITE) ? UVM_IO_FIXPROT : 0);
uvmspace_free(vm);
