Mark Kettenis <mark.kette...@xs4all.nl> writes:
>> From: Jeremie Courreges-Anglas <j...@wxcvbn.org>
>> Date: Mon, 30 May 2016 19:30:27 +0200
>>
>> Mark Kettenis <mark.kette...@xs4all.nl> writes:
>>
>> > Mathieu - schreef op 2016-05-28 13:05:
>> >> Martin Natano wrote:
>> >>> The diff reads fine to me, however it is incomplete. There are some
>> >>> callers of process_domem() in arch/. They will need to be changed too.
>> >>> req seems to be in sync with uio_rw in all the cases, so just removing
>> >>> the last argument should do it.
>> >>>
>> >>
>> >> Thanks for the feedback. The missing callers where an overlook on my
>> >> part, sorry for that.
>> >>
>> >> Here is a regenerated diff including all the call site. As a side note,
>> >> obviously every one of them was using PT_WRITE_I, that's why it went
>> >> unnoticed.
>> >
>> > 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.
>>
>> Since PT_WRITE_I and PT_WRITE_D are documented as strictly equivalent
>> since rev. 1.1, I doubt that such an optimization is a good idea.
>
> A clear case where the documentation is wrong.
It is wrong since the change that happened in rev. 1.33; before,
procfs_domem() used:
error = uvm_io(&p->p_vmspace->vm_map, uio,
uio->uio_rw == UIO_WRITE ? UVM_IO_FIXPROT : 0);
uvmspace_free(p->p_vmspace);
if (error == 0 && uio->uio_rw == UIO_WRITE)
pmap_proc_iflush(p, addr, len);
Looks like you are the one who added the pmap_proc_iflush call, btw.
The documentation may have been wrong for some time on some archs, it
feels like making PT_WRITE_D and PT_WRITE_I equivalent was deemed
useful at one point. Given that Free and NetBSD document the same
guarantee, I personally don't feel comfortable changing that, but YMMV.
--
jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF DDCC 0DFA 74AE 1524 E7EE
