In general this patch looks good. I can't see any issues with the extension to handle mmap more comprehensively.
As a minor comment, could you consider making using multiline comments / / for multiple line comments.

src/sim/syscall_emul.hh (Diff revision 1)

In general, perhaps the multi-line comments should use / /?

Show all issues

1. Brandon Potter Feb. 25, 2016, 4:01 p.m. (Feb. 25, 2016, 4:01 p.m.)

   I feel kind of torn about this issue, but I'm going to go ahead and drop it.
   The multiline comments are just a style preference and there's nothing in the coding standard about which one should be used. The deciding factor for me against changing the current code is that the system call code doesn't use the multiline style at all (except the copyright). If anything, I'd prefer to have the style consistent within the same file.
   If the change added anything, I'd do it, but I don't feel that it does.

2. Matt Horsnell Feb. 26, 2016, 8:18 a.m. (Feb. 26, 2016, 8:18 a.m.)

   It was a minor comment, happy for you to drop it.

Looks good. Thanks for implementing this! Just a couple minor comments below.

src/sim/syscall_emul.hh (Diff revision 1)

For readability, it would be nice to add parens around the & expressions here. Order of operations is not immediately clear.

Show all issues

src/sim/syscall_emul.hh (Diff revision 1)

Thank you for the warning here! After reading the patch description, I immediately wondered about mmapped file writes.
I'm not opening an issue here, but I'm curious about how it's possible for mmap writes to succeed in the simulated system, but not allow those updates to the file on the host system. Maybe that's default behavior for mmapping a file with PROT_READ, or does the file copy below take care of that? Either way, it might be good to have an explicit comment here that describes the process.

1. Brandon Potter Feb. 25, 2016, 5:04 p.m. (Feb. 25, 2016, 5:04 p.m.)

   TL;DR There's a solution to fix the problem, but it requires more code that's not ready for release. So, for now, I just added more comments.
   I added a comment to the code and there's an extended blurb below which tries to address your comment about the file copy.
   The file copy below is only used to initialize the mapping with the file contents. (Since there is currently no demand paging, the mapping is fully initialized within the simulated process' address space. Theoretically, we could bring in the file contents a page at a time with lazy frame allocation, but we haven't supported that yet.) The PROT_WRITE flag specifies that the file contents can be changed by modifying the mapping. However, this would happen subsequently when the system call ends. It's not possible to update the file contents from within the mmap call with any kind of copy. Modifying the mapping is the job of the application (not the system call) and this happens only after the mmap has completed.
   With a write, the modifications would need to be propagated back to the file eventually. I'm not completely sure how this works with Linux, because I haven't examined the code. I had assumed that the mapping would use the frame associated with the file in the Linux page cache; this link seems to corrborate that idea: http://duartes.org/gustavo/blog/post/page-cache-the-affair-between-memory-and-files/. (Specifically this lines: "By contrast, a shared mapping is simply mapped onto the page cache and that’s it. Updates are visible to other processes and end up in the disk.")
   So, I believe that it's true that the shared mappings all point to the same frame by using the frame associated with the page cache. With shared mappings, this makes the changes to the file-backed mapping visible to anyone else with the same mapping. Eventually, that frame will get written back to disk and the file will be permanently modified.
   How would this work with gem5? First, there's the problem that the host mappings are never directly visible to the simulated processes. All of the data becomes available to the simulated process only after the copies are done with the translating port proxies. (Essentially, the host mappings get copied into the simulated process' "physical address space" through a couple of function calls.) I don't know how to make a portion of the process' "physical address space" mirror the host's mappings; it's just weird.
   Instead, we could force gem5 applications to call msync. The msync system call would allow us to copy the mappings back with another system call. We just take the process' address space contents for the requested virtual address range, reopen the shared mapping on the host, and plop the process' contents back into the shared mapping. (However, we still need to have structures to retain virtual memory area mapping information; currently, we have internal patches for this, but they're not ready to be made public yet. The structures are also needed to implement demand paging so it's likely that they may be released in the future.) I am almost completely sure that it would work if the applications correctly called msync; msync is really the guarantee that the modified contents make it back to the file anyways.