On Sat, Jul 07, 2012 at 10:57:40AM -0500, Rob Herring wrote: > On 07/07/2012 05:36 AM, Peter Maydell wrote: > > On 6 July 2012 20:26, Dave Martin <dave.mar...@linaro.org> wrote: > >> On Fri, Jul 06, 2012 at 07:07:35PM +0100, Peter Maydell wrote: > >>> On 6 July 2012 00:27, Rob Herring <robherri...@gmail.com> wrote: > >>>> I would just change arm_add_memory to use phys_addr_t for the size > >>>> param. This ultimately calls memblock functions which use phys_addr_t > >>>> for sizes. > >>> > >>> So I have a patch that does this which basically works. However > >>> there is a bit I'm not sure about. arm_add_memory() does this: > >>> bank->size = size & PAGE_MASK; > >>> > >>> in an attempt to clear the bottom bits of the size. However, > >>> since PAGE_MASK is defined as: > >>> #define PAGE_SIZE (_AC(1,UL) << PAGE_SHIFT) > >>> #define PAGE_MASK (~(PAGE_SIZE-1)) > >>> > >>> PAGE_MASK is a 32 bit unsigned constant and so this & will > >>> clear the top 32 bits of bank->size. > >>> > >>> I'm really not sure what the best way to fix this is; suggestions? > >> > >> Maybe something like > >> > >> ~(phys_addr_t)~PAGE_MASK > >> > >> or > >> > >> ~(phys_addr_t)(PAGE_SIZE - 1) > >> > >> would work. > > > > Mmm. It feels a bit unsatisfactory that an "obviously correct" > > line of code like "size &= ~PAGE_MASK" could actually be subtly > > wrong (seems like the kind of thing that could easily slip > > through code review), so I was wondering if maybe redefining > > PAGE_MASK so it didn't do the wrong thing on 64 bit types > > would be better. (That's definitely way out of my depth though.)
That's C for you. What we really mean by "... & PAGE_MASK" is a bit-clear operation, but C doesn't have that. > While there is a mixture of usage of PAGE_MASK in the kernel, I think > correct usage is with virt addresses. For phys addresses, there is > PHYS_MASK which is sized correctly for LPAE. > > You really should post this on the lakml list. The bits masked off by PAGE_MASK are identical between physical and virtual views of the same address, so we could argue that PAGE_MASK is appropriate for both, if a suitable definition is possible. Thinking about it, using a 64-bit definition of PAGE_MASK should not break most uses. sizeof(expression involving PAGE_MASK) might unexpectedly by 8 where 4 is expected, but I suspect that such usage is rare or nonexstent. GCC seems to generate sensibly optimal code for simple operations like (unsigned long) & PAGE_MASK, equivalent to what would be generated with a 32-bit PAGE_MASK. That would need discussion on alkml to see whether other people agree. ---Dave _______________________________________________ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
