----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviews.gem5.org/r/1735/#review4515 -----------------------------------------------------------
Ship it! Thanks for the change. It looks good to me. Ali src/sim/syscall_emul.hh <http://reviews.gem5.org/r/1735/#comment4234> This should be new_length = roundUp(new_length, TheISA::VMPageSize); src/sim/syscall_emul.hh <http://reviews.gem5.org/r/1735/#comment4233> This should be length = roundUp(length, TheISA::VMPageSize); - Ali Saidi On Feb. 22, 2013, 9:44 a.m., Tom Jablin wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://reviews.gem5.org/r/1735/ > ----------------------------------------------------------- > > (Updated Feb. 22, 2013, 9:44 a.m.) > > > Review request for Default. > > > Repository: gem5 > > > Description > ------- > > Presently, the alignment checks in the mmap and mremap implementations in > syscall_emul.hh are wrong. The checks are implemented as: > > if ((start % TheISA::VMPageSize) != 0 || > (length % TheISA::VMPageSize) != 0) { > warn("mmap failing: arguments not page-aligned: " > "start 0x%x length 0x%x", > start, length); > return -EINVAL; > } > > This checks that both the start and the length arguments of the mmap syscall > are checked for page-alignment. However, the POSIX specification says: > > The off argument is constrained to be aligned and sized according to the > value returned by sysconf() when passed _SC_PAGESIZE or _SC_PAGE_SIZE. When > MAP_FIXED is specified, the application shall ensure that the argument addr > also meets these constraints. The implementation performs mapping operations > over whole pages. Thus, while the argument len need not meet a size or > alignment constraint, the implementation shall include, in any mapping > operation, any partial page specified by the range [pa,pa+len). > > So the length parameter should not be checked for page-alignment. By > contrast, the current implementation fails to check the offset argument, > which must be page aligned. > > I do not have commit privileges, so can a reviewer please push this patch for > me? > > > Diffs > ----- > > src/sim/syscall_emul.hh e6347e559e8f > > Diff: http://reviews.gem5.org/r/1735/diff/ > > > Testing > ------- > > #include <stdio.h> > #include <unistd.h> > #include <sys/mman.h> > > int main(int argc, char **argv) { > > void *addr1 = > mmap(0, 1, PROT_NONE, MAP_PRIVATE | MAP_ANONYMOUS, -1, 0); > printf("Test unaligned lengths: %s\n", > addr1 == (void *) -1 ? "Failed" : "Passed"); > > size_t pageSize = (size_t) sysconf(_SC_PAGE_SIZE); > void *addr2 = > mmap(0, pageSize, PROT_NONE, MAP_PRIVATE | MAP_ANONYMOUS, -1, 1); > printf("Test unaligned offsets: %s\n", > addr2 == (void *) -1 ? "Passed" : "Failed"); > > return 0; > } > > > Thanks, > > Tom Jablin > > _______________________________________________ gem5-dev mailing list [email protected] http://m5sim.org/mailman/listinfo/gem5-dev
