Re: memory/virtual.c: ROUND_SIZE() bugfix + .SYS loading
Jan Kratochvil <short(a)ucw.cz> writes:
I had incorrect warnings "Section %.8s too large (%lx+%lx/%lx)" during loading of some low-level system libraries. It got fixed by the attached "wine-memory-virtual.c-too_large_section.diff" which changes (I think fixes) ROUND_SIZE() behaviour. It also comments ROUND_ADDR() and ROUND_SIZE(), I hope I guessed its intended description from the style of its usage around the code.
No, ROUND_SIZE is supposed to return an aligned size, which you use together with an aligned address. The normal pattern is to use ROUND_SIZE and ROUND_ADDR together. We don't bother to round addresses in image mapping because all the code assumes the image has correct section alignment already.
Attached "wine-memory-virtual.c-mmap_cache-preliminary.diff" is just a RFC as I found out that I was unable to load some .SYS files as they were being attempted to be loaded with page-overlapping sections, unaligned offsets and incompatible protections. Therefore I had to implement my own mapping cache and handle the overlapping/merging myself. It would be more simple/clean to use Linux /proc/$$/maps mmap map but it would be probably no-no as it is Linux-dependent.
I think this is complete overkill. Mapping unaligned binaries is a very unusual case and should be treated as such. There's no need to maintain such a complex structure, or add support for non page aligned mmaps, for such an exotic feature. Probably what you want is to create a separate map_image_unaligned function to map images that don't have a proper section alignment, using read() and not worrying about protections at all. -- Alexandre Julliard julliard(a)winehq.com
On Mon, 16 Sep 2002 21:47:23 +0200, Alexandre Julliard wrote:
Jan Kratochvil <short(a)ucw.cz> writes:
I had incorrect warnings "Section %.8s too large (%lx+%lx/%lx)" during loading of some low-level system libraries. It got fixed by the attached "wine-memory-virtual.c-too_large_section.diff" which changes (I think fixes) ROUND_SIZE() behaviour. It also comments ROUND_ADDR() and ROUND_SIZE(), I hope I guessed its intended description from the style of its usage around the code.
No, ROUND_SIZE is supposed to return an aligned size, which you use together with an aligned address. The normal pattern is to use ROUND_SIZE and ROUND_ADDR together. We don't bother to round addresses in image mapping because all the code assumes the image has correct section alignment already.
"is to use ROUND_SIZE and ROUND_ADDR together." && "We don't bother to round addresses" seems as a conflict for me - if you "don't bother" why do you use/have ROUND_ADDR at all?
From code (virtual/memory.c): DWORD base = imports->VirtualAddress & ~page_mask; DWORD end = imports->VirtualAddress + ROUND_SIZE( imports->VirtualAddress, imports->Size ); Either: There is useless "& ~page_mask" Or: There is bug during ROUND_SIZE() call as ROUND() size is used with unaligned address if "& ~page_mask" is needed.
As the current solution works for aligned executables it can be assumed that "& ~page_mask" is useless there.
Attached "wine-memory-virtual.c-mmap_cache-preliminary.diff" is just a RFC as I found out that I was unable to load some .SYS files as they were being attempted to be loaded with page-overlapping sections, unaligned offsets and incompatible protections. Therefore I had to implement my own mapping cache and handle the overlapping/merging myself. It would be more simple/clean to use Linux /proc/$$/maps mmap map but it would be probably no-no as it is Linux-dependent.
I think this is complete overkill. Mapping unaligned binaries is a very unusual case and should be treated as such. There's no need to maintain such a complex structure, or add support for non page aligned mmaps, for such an exotic feature.
"exotic"? Windows kernel is full of it. :-)
Probably what you want is to create a separate map_image_unaligned function to map images that don't have a proper section alignment, using read() and not worrying about protections at all.
Why to not mmap it when it is possible? Missing protection may complicate the debugging which follows. If such unaligned mapping would be implemented there would be no longer use for the current unaligned_mmap() as it would be just a subset of the new function (that my unaligned_mmap_cached()): naming confusion: (old) unaligned_mmap() in fact requires page-aligned addr (new) unaligned_mmap_cached() doesn't require page-aligned addr The summarizing question is whether Wine itself wants to go also to the kernel space emulation land. I found that ReactOS suits my needs better (kernel reimplementation, also it is fortunately GPL) and thus I no longer personally need my patch to Wine unaligned_mmap() although I can tune/finish it if wised. Regards, Jan Kratochvil
Jan Kratochvil <short(a)ucw.cz> writes:
"is to use ROUND_SIZE and ROUND_ADDR together." && "We don't bother to round addresses" seems as a conflict for me - if you "don't bother" why do you use/have ROUND_ADDR at all?
Because there are other places where we need to round addresses. map_image() doesn't need to bother with that, but other functions do.
From code (virtual/memory.c): DWORD base = imports->VirtualAddress & ~page_mask; DWORD end = imports->VirtualAddress + ROUND_SIZE( imports->VirtualAddress, imports->Size ); Either: There is useless "& ~page_mask" Or: There is bug during ROUND_SIZE() call as ROUND() size is used with unaligned address if "& ~page_mask" is needed.
As the current solution works for aligned executables it can be assumed that "& ~page_mask" is useless there.
Actually in that case the address is not necessarily aligned, so yes, that usage of ROUND_SIZE is slightly wrong. Thanks for spotting it.
"exotic"? Windows kernel is full of it. :-)
But loading Windows kernel binaries in Wine is exotic. There are some very specialized cases where we could actually make use of a kernel driver in user space, but there is no way Wine will provide full NT kernel support, and it's not an option to slow down the normal mmap support for that.
Why to not mmap it when it is possible? Missing protection may complicate the debugging which follows. If such unaligned mapping would be implemented there would be no longer use for the current unaligned_mmap() as it would be just a subset of the new function (that my unaligned_mmap_cached()): naming confusion: (old) unaligned_mmap() in fact requires page-aligned addr (new) unaligned_mmap_cached() doesn't require page-aligned addr
Well, you could make map_image_unaligned() use mmap if possible, and set page protections too. The important thing is that all of it is kept out of the common case. 99% of the mmap calls are aligned, and we don't want to do the work of maintaining the cache for the 1% case that can easily be done separately.
The summarizing question is whether Wine itself wants to go also to the kernel space emulation land. I found that ReactOS suits my needs better (kernel reimplementation, also it is fortunately GPL) and thus I no longer personally need my patch to Wine unaligned_mmap() although I can tune/finish it if wised.
Wine is going to remain a user-mode application. We may want to have some basic support for loading a small subset of kernel-mode drivers that can work in user mode, but a general Windows kernel emulation has to be done in the kernel, and this is a separate project. -- Alexandre Julliard julliard(a)winehq.com
participants (2)
-
Alexandre Julliard -
Jan Kratochvil