Date: Sat, 10 Jul 2010 09:20:58 +0530 From: "Jayachandran C." <c.jayachandran@gmail.com> To: "M. Warner Losh" <imp@bsdimp.com> Cc: freebsd-mips@freebsd.org Subject: Re: Merging 64 bit changes to -HEAD - part 4 Message-ID: <AANLkTikwMzIIp-gykkhvbX9fbmxpNYdLXvNrAxTvkUte@mail.gmail.com> In-Reply-To: <20100709.144631.1141490504242621103.imp@bsdimp.com> References: <20100708.021250.1099368555950605809.imp@bsdimp.com> <AANLkTilFZ4zAKqGcDKie6og5Nhrh9PU_Ta3uDoq7yexA@mail.gmail.com> <AANLkTik8fBlxMyZ3AZETF_7DvlSpPsx2hozIWinRy0U0@mail.gmail.com> <20100709.144631.1141490504242621103.imp@bsdimp.com>
next in thread | previous in thread | raw e-mail | index | archive | help
On Sat, Jul 10, 2010 at 2:16 AM, M. Warner Losh <imp@bsdimp.com> wrote:
> In message: <AANLkTik8fBlxMyZ3AZETF_7DvlSpPsx2hozIWinRy0U0@mail.gmail.com=
>
> =A0 =A0 =A0 =A0 =A0 =A0"Jayachandran C." <c.jayachandran@gmail.com> write=
s:
> : On Thu, Jul 8, 2010 at 2:52 PM, Jayachandran C.
> : <c.jayachandran@gmail.com> wrote:
> : > On Thu, Jul 8, 2010 at 1:42 PM, M. Warner Losh <imp@bsdimp.com> wrote=
:
> : >> In message: <AANLkTikSVi27V2UICgLvKd8Bk7v6tuGty9YX6-C6-21H@mail.gmai=
l.com>
> : >> =A0 =A0 =A0 =A0 =A0 =A0"Jayachandran C." <c.jayachandran@gmail.com> =
writes:
> : [...]
> : >> : pmap-n64.patch
> : >> : =A0The main n64 patch (from Juli's branch)
> : >> : =A0 This still uses the old 2-level page tables. But this adds oth=
er
> : >> : pmap code to support n64. I have re-arranged some of =A0Juli's cod=
e to
> : >> : reduce #ifdefs.
> : >>
> : >> I think this could be done in smaller bites. =A0At least the
> : >> MIPS_{PYHS,KSEGx}_TO_{KSEGx,PHYS} stuff can be done as a separate
> : >> patch.
> : [...]
> : > Thanks for the review, I'll try to get the patches other than pmap-n6=
4
> : > in first (with mentor approval). I'll post a new split version of
> : > pmap-n64 after that.
> :
> : Here are the pmap changes again, along with a ddb patch.
> :
> : kseg-def-to-cpu.h.patch
> : Move KSEG address definitions from cpu.h to cpuregs.h with the other
> : definitions, add some =A0XKPHYS related definitions.
>
> This looks good.
>
> : mips-pmap-intial-n64.patch (from Juli's branch)
> : Initial changes to pmap.c for n64 support. When compiled for n64, pmap
> : can use XKPHYS for a lot of its operations. The page tables are still
> : 2-level.
>
> +#if !defined(__mips_n64)
> + =A0 =A0 =A0 if (phys_avail[i - 1] >=3D MIPS_KSEG0_LARGEST_PHYS)
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 memory_larger_than_512meg =3D 1;
> +#endif
>
> We never set memory_larger_than_512meg then for __mips_n64. =A0Do we
> need to ifdef both the declaration of this variable, as well as the
> if (memory_larger_than_512mb) clauses later in the function?
I thought I will prevent more ifdefs. Since the variable is zero, the
compiler should optimize it out for n64 - but I have not actually
looked at the generated code yet. I can add a comment to the variable
if needed.
> pmap_map should likely have a comment to the effect:
> =A0* For memory that's directly mappable, we return the direct map
> =A0* address. =A0For other addresses, we create a map.
>
> A similar comment is needed for pmap_kenter_temporary, pmap_zero_page,
> pmap_zero_page_area, pmap_zero_page_idle, pmap_copy_page, pmap_mapdev,
> pmap_unmapdev, pmap_kextract. =A0Hmmm, or maybe a general comment?
Will look at this.
> : mips-tlb64.patch (from Juli's branch)
> : 64 bit TLB definitions.
>
> This looks good.
>
> : mips-ddb-64.patch (from Juli's branch)
> : Minor fixups for ddb support.
>
> Can you explain the casting for all the calls to kbdpeek*?
I had looked at cleaning it up (this is the reason I did not sent this
patch earlier). But after looking at it, looked like we needed to
revisit it, and just took the changes from Juli's tree.
I can move all the casts to one line where we can define a temp
variable of the needed type.
> : This should get n64 up to mountroot - let me know you comments.
>
> Cool!
>
> thanks for your work. =A0Also, I've been providing reviews on many of
> these commits, but I haven't seen a 'Reviewed by: imp' in the commit
> messages. =A0I'd appreciate it if you could add then when I provide a
> review.
I had added reviewed by for 2 changesets for which you said OK. In the
other changesets I thought your comments were more neutral so I was
not sure if I could add the reviewed by. Will do this in further
changes.
Thanks
JC.
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?AANLkTikwMzIIp-gykkhvbX9fbmxpNYdLXvNrAxTvkUte>