cvs commit: src/sys/pci agp.c

[Seigo Tanimura]

On Fri, 27 Oct 2006 05:16:37 +0900 (JST),
  Hiroki Sato <hrs at FreeBSD.org> said:

hrs> Seigo Tanimura <tanimura at FreeBSD.org> wrote
hrs>   in <200610150504.k9F548ld008933 at repoman.freebsd.org>:

ta> tanimura    2006-10-15 05:04:07 UTC
ta> 
ta> FreeBSD src repository
ta> 
ta> Modified files:
ta> sys/pci              agp.c
ta> Log:
ta> Fix the wraparound of memsize >=2GB.
ta> 
ta> Revision  Changes    Path
ta> 1.54      +3 -2      src/sys/pci/agp.c

hrs>  I have doubt about this change because int memsize->u_int memsize
hrs>  does not solve the problem directly; memsize never occurs wraparound
hrs>  actually and an implicit cast to unsigned int just makes the problem
hrs>  invisible.  The questionable code fragment in agp.c is the following:

hrs>  memsize = ptoa(Maxmem) >> 20;
hrs>  for (i = 0; i < agp_max_size; i++) {
hrs>          if (memsize <= agp_max[i][0])
hrs>                  break;
hrs>  }

hrs>  ptoa(Maxmem)>>20 will occur a wraparound problem when Maxmem>=2GB, so
hrs>  this part should be fixed instead.  BTW, this should be a problem
hrs>  only on i386 since the definition of ptoa() is "#define ptoa(x) ((x)
hrs>  << PAGE_SHIFT)".  The other platforms use a cast like "#define
hrs>  ptoa(x) ((unsigned long)(x) << PAGE_SHIFT)".

hrs>  I think it can be solved by using "ptoa((unsigned long)Maxmem)" or
hrs>  so, but I am not sure if this is reasonable because there are more
hrs>  notional types like vm_paddr_t.  If "#define ptoa(x) ((vm_paddr_t)(x)
hrs>  << PAGE_SHIFT)" works fine on all platforms, it looks more reasonable
hrs>  to me, but I have a misunderstanding?

ptoa() is machine-dependent; (unsigned long) should be OK.
(vm_paddr_t) would mess up the namespace.

What hassles me is that Maxmem is *NOT* unsigned, grrr...

hrs> --
hrs> | Hiroki SATO

-- 
Seigo Tanimura <tanimura at tanimura.dyndns.org> <tanimura at FreeBSD.org>