On Thu, Mar 26, 2009 at 05:08:59PM -0700, Doug Ambrisko wrote:
> Doug Ambrisko writes:
> | John Baldwin writes:
> | | On Thursday 26 March 2009 5:29:42 pm Doug Ambrisko wrote:
> | [snip]
> | | > Maybe you have another suggestion to fix this. The problem showed up
> | | > when doing a mmap of 0xcf79c000 into 0xffffffffcf79c000 also a mmap
> | | > of 0xf0000 ended up the same way. This caused it to fail. Note this
> | | > is only on amd64 with a Linux. It didn't happen with a FreeBSD i386
> | | > version on amd64. Here is a sample test program:
> | |
> | | I'm sure this can be easily fixed in the Linux mmap() handlers instead.
> Do
> | | you know if your Linux binary is using mmap2() or the old mmap()?
> |
> | I think it uses linux_mmap then bouncing it to linux_mmap_common.
> | linux_mmap_common had it right but when it mmap picked it up then
> | it was wrong in my intrumentation.
> |
> | I'll flip the l_off_t type back and then instrument it more to find
> | out when things are going bad. I missed the other usage of l_off_t
> | so I agree this is a bad change. However, I wonder if the other
> | usage of l_off_t actually works right or there is a bug with that
> | as well?
> |
> | I should be able to get something put together pretty quick and
> | send it for review.
>
> Okay, I did some more instrumenting again and found that I was
> slightly wrong. The mmap that was failing was 0xcf79c000 and not
> 0xf0000. This probably makes since since the sign bit was set
> on 0xcf79c000. However, it appear mmap doesn't really do negative
> seeks. Looking at the freebsd32_mmap the structure it uses for
> args is:
> struct freebsd6_freebsd32_mmap_args {
> char addr_l_[PADL_(caddr_t)]; caddr_t addr; char
> addr_r_[PADR_(caddr_t)];
> char len_l_[PADL_(size_t)]; size_t len; char len_r_[PADR_(size_t)];
> char prot_l_[PADL_(int)]; int prot; char prot_r_[PADR_(int)];
> char flags_l_[PADL_(int)]; int flags; char flags_r_[PADR_(int)];
> char fd_l_[PADL_(int)]; int fd; char fd_r_[PADR_(int)];
> char pad_l_[PADL_(int)]; int pad; char pad_r_[PADR_(int)];
> char poslo_l_[PADL_(u_int32_t)]; u_int32_t poslo; char
> poslo_r_[PADR_(u_int32_t)];
> char poshi_l_[PADL_(u_int32_t)]; u_int32_t poshi; char
> poshi_r_[PADR_(u_int32_t)];
> };
> with both the high and the lows being u_int32_t.
>
> So I wonder if in the linux32 the structure that is:
> struct l_mmap_argv {
> l_uintptr_t addr;
> l_size_t len;
> l_int prot;
> l_int flags;
> l_int fd;
> l_off_t pgoff;
> } __packed;
> should be uint32_t for pgoff?
>
yes, you are right. s/uint32_t/l_ulong/ :)
also remove __packed.
thnx!
> Using this patch things work okay:
>
> Index: linux.h
> ===================================================================
> RCS file: /usr/local/cvsroot/freebsd/src/sys/amd64/linux32/linux.h,v
> retrieving revision 1.24
> diff -u -p -r1.24 linux.h
> --- linux.h 26 Mar 2009 17:14:22 -0000 1.24
> +++ linux.h 27 Mar 2009 00:01:07 -0000
> @@ -79,7 +79,7 @@ typedef l_ulong l_ino_t;
> typedef l_int l_key_t;
> typedef l_longlong l_loff_t;
> typedef l_ushort l_mode_t;
> -typedef l_ulong l_off_t;
> +typedef l_long l_off_t;
> typedef l_int l_pid_t;
> typedef l_uint l_size_t;
> typedef l_long l_suseconds_t;
> Index: linux32_machdep.c
> ===================================================================
> RCS file: /usr/local/cvsroot/freebsd/src/sys/amd64/linux32/linux32_machdep.c,v
> retrieving revision 1.52
> diff -u -p -r1.52 linux32_machdep.c
> --- linux32_machdep.c 18 Feb 2009 16:11:39 -0000 1.52
> +++ linux32_machdep.c 27 Mar 2009 00:01:07 -0000
> @@ -788,6 +788,7 @@ linux_mmap(struct thread *td, struct lin
> {
> int error;
> struct l_mmap_argv linux_args;
> + uint32_t pos;
>
> error = copyin(args->ptr, &linux_args, sizeof(linux_args));
> if (error)
> @@ -801,7 +802,10 @@ linux_mmap(struct thread *td, struct lin
> #endif
> if ((linux_args.pgoff % PAGE_SIZE) != 0)
> return (EINVAL);
> - linux_args.pgoff /= PAGE_SIZE;
> + pos = linux_args.pgoff;
> + pos /= PAGE_SIZE;
> + linux_args.pgoff = pos;
> +
>
> return (linux_mmap_common(td, &linux_args));
> }
>
>
> So which should we do? The uint32_t for the /= PAGE_SIZE or in
> the mmap structure? FWIW, they are mmaping /dev/mem and grabbing
> the SMBIOS structure put at 0xcf79c000 and 0xcf7f0000 which are not
> negative offsets. linux_mmap2 and linux_common don't really have
> this problem in this case since they are using the memory address
> / PAGE_SIZE. So they don't run into this sign problem like this.
> I've confirmed that that above patch makes the Linux BMC firmware
> upgrade tool works. On a real Linux machine it also mmaps these
> addresses and it works there otherwise the program goes into the
> weeds since it can't find the IPMI controller. This change only
> mucks with linux_mmap.
>
> Thanks,
>
> Doug A.
--
Have fun!
chd
pgpKmYzPkoAqL.pgp
Description: PGP signature
