Hello Ben,
On Wednesday, December 10, 2008 you wrote:
> Hi Ilya !
> Looks good overall. A few minor comments.
>> +config PPC_4K_PAGES
>> + bool "4k page size"
>> +
>> +config PPC_16K_PAGES
>> + bool "16k page size" if 44x
>> +
>> +config PPC_64K_PAGES
>> + bool "64k page size" if 44x || PPC64
>> + select PPC_HAS_HASH_64K if PPC64
> I'd rather if the PPC64 references were instead PPC_STD_MMU_64 (which
> may or may not be defined in Kconfig depending on what you are based on,
> but is trivial to add.
> I want to clearly differenciate what is MMU from what CPU architecture
> and there may (will ... ahem) at some point be 64-bit BookE.
Understood. We'll fix this, and re-post the patch then.
[snip]
>> diff --git a/arch/powerpc/include/asm/highmem.h
>> b/arch/powerpc/include/asm/highmem.h
>> index 91c5895..9875540 100644
>> --- a/arch/powerpc/include/asm/highmem.h
>> +++ b/arch/powerpc/include/asm/highmem.h
>> @@ -38,9 +38,20 @@ extern pte_t *pkmap_page_table;
>> * easily, subsequent pte tables have to be allocated in one physical
>> * chunk of RAM.
>> */
>> -#define LAST_PKMAP (1 << PTE_SHIFT)
>> -#define LAST_PKMAP_MASK (LAST_PKMAP-1)
>> +/*
>> + * We use one full pte table with 4K pages. And with 16K/64K pages pte
>> + * table covers enough memory (32MB and 512MB resp.) that both FIXMAP
>> + * and PKMAP can be placed in single pte table. We use 1024 pages for
>> + * PKMAP in case of 16K/64K pages.
>> + */
>> +#define PKMAP_ORDER min(PTE_SHIFT, 10)
>> +#define LAST_PKMAP (1 << PKMAP_ORDER)
>> +#if !defined(CONFIG_PPC_4K_PAGES)
>> +#define PKMAP_BASE (FIXADDR_START - PAGE_SIZE*(LAST_PKMAP + 1))
>> +#else
>> #define PKMAP_BASE ((FIXADDR_START - PAGE_SIZE*(LAST_PKMAP + 1)) &
>> PMD_MASK)
>> +#endif
> I'm not sure about the above & PMD_MASK. Shouldn't we instead make it
> not build if (PKMAP_BASE & PMD_MASK) != 0 ?
We separated the !4K_PAGES case here exactly because (PKMAP_BASE &
PMD_MASK) != 0 [see the comment to this chunk - why]. So, this'll turn
out to be broken if we follow your suggestion. Are there any reasons
why we should have PKMAP_BASE aligned on the PMD_SIZE boundary ?
> IE, somebody set
> FIXADDR_START to something wrong... and avoid the ifdef alltogether ? Or
> am I missing something ? (it's early morning and I may not have all my
> wits with me right now !)
[snip]
>> diff --git a/arch/powerpc/include/asm/pgtable.h
>> b/arch/powerpc/include/asm/pgtable.h
>> index dbb8ca1..a202043 100644
>> --- a/arch/powerpc/include/asm/pgtable.h
>> +++ b/arch/powerpc/include/asm/pgtable.h
>> @@ -39,6 +39,8 @@ extern void paging_init(void);
>>
>> #include <asm-generic/pgtable.h>
>>
>> +#define PGD_T_LOG2 (__builtin_ffs(sizeof(pgd_t)) - 1)
>> +#define PTE_T_LOG2 (__builtin_ffs(sizeof(pte_t)) - 1)
> I'm surprised the above actually work :-) Why not having these next to the
> definition of pte_t in page_32.h ?
These definitions seem to be related to the page table, so, as for me,
then pgtable.h is the better place for them. Though, as you want;
we'll move this to page_32.h.
> Also, you end up having to do an asm-offset trick to get those to asm, I
> wonder if it's worth it or if we aren't better off just #defining the sizes
> with actual numbers next to the type definitions. No big deal either way.
>> /*
>> * To support >32-bit physical addresses, we use an 8KB pgdir.
>> diff --git a/arch/powerpc/kernel/misc_32.S b/arch/powerpc/kernel/misc_32.S
>> index bdc8b0e..42f99d2 100644
>> --- a/arch/powerpc/kernel/misc_32.S
>> +++ b/arch/powerpc/kernel/misc_32.S
>> @@ -647,8 +647,8 @@ _GLOBAL(__flush_dcache_icache)
>> BEGIN_FTR_SECTION
>> blr
>> END_FTR_SECTION_IFSET(CPU_FTR_COHERENT_ICACHE)
>> - rlwinm r3,r3,0,0,19 /* Get page base address */
>> - li r4,4096/L1_CACHE_BYTES /* Number of lines in a page */
>> + rlwinm r3,r3,0,0,PPC44x_RPN_MASK /* Get page base address */
>> + li r4,PAGE_SIZE/L1_CACHE_BYTES /* Number of lines in a page */
> Now, the problem here is the name of the constant. IE. This is more or
> less generic ppc code and you are using something called
> "PPC4xx_RPN_MASK", doesn't look right.
> I'r rather you do the right arithmetic using PAGE_SHIFT straight in
> here. In general, those _MASK constants you use aren't really masks,
> they are bit numbers in PPC notation which is very confusing. Maybe you
> should call those constants something like
> PPC_xxxx_MASK_BIT
OK.
> Dunno ... it's a bit verbose. But I'm not too happy with that naming at
> this stage. In any case, the above is definitely wrong in misc_32.S
>> mtctr r4
>> mr r6,r3
>> 0: dcbst 0,r3 /* Write line to ram */
>> @@ -688,8 +688,8 @@ END_FTR_SECTION_IFSET(CPU_FTR_COHERENT_ICACHE)
>> rlwinm r0,r10,0,28,26 /* clear DR */
>> mtmsr r0
>> isync
>> - rlwinm r3,r3,0,0,19 /* Get page base address */
>> - li r4,4096/L1_CACHE_BYTES /* Number of lines in a page */
>> + rlwinm r3,r3,0,0,PPC44x_RPN_MASK /* Get page base address */
>> + li r4,PAGE_SIZE/L1_CACHE_BYTES /* Number of lines in a page */
> Same comment.
>> pgd_t *pgd_alloc(struct mm_struct *mm)
>> {
>> pgd_t *ret;
>>
>> - ret = (pgd_t *)__get_free_pages(GFP_KERNEL|__GFP_ZERO, PGDIR_ORDER);
>> + ret = (pgd_t *)kzalloc(1 << PGDIR_ORDER, GFP_KERNEL);
>> return ret;
>> }
> We may want to consider using a slab cache. Maybe an area where we want
> to merge 32 and 64 bit code, though it doesn't have to be right now.
> Do we know the impact of using kzalloc instead of gfp for when it's
> really just a single page though ? Does it have overhead or will kzalloc
> just fallback to gfp ? If it has overhead, then we probably want to
> ifdef and keep using gfp for the 1-page case.
This depends on allocator: SLUB looks calling __get_free_pages() if
size > PAGE_SIZE [note, not >= !], but SLAB doesn't. So, we'll add
ifdef here.
>> __init_refok pte_t *pte_alloc_one_kernel(struct mm_struct *mm, unsigned
>> long address)
>> @@ -400,7 +395,7 @@ void kernel_map_pages(struct page *page, int numpages,
>> int enable)
>> #endif /* CONFIG_DEBUG_PAGEALLOC */
>>
>> static int fixmaps;
>> -unsigned long FIXADDR_TOP = 0xfffff000;
>> +unsigned long FIXADDR_TOP = (-PAGE_SIZE);
>> EXPORT_SYMBOL(FIXADDR_TOP);
>>
>> void __set_fixmap (enum fixed_addresses idx, phys_addr_t phys, pgprot_t
>> flags)
>> diff --git a/arch/powerpc/platforms/Kconfig.cputype
>> b/arch/powerpc/platforms/Kconfig.cputype
>> index 548efa5..73a5aa9 100644
>> --- a/arch/powerpc/platforms/Kconfig.cputype
>> +++ b/arch/powerpc/platforms/Kconfig.cputype
>> @@ -204,7 +204,7 @@ config PPC_STD_MMU_32
>>
>> config PPC_MM_SLICES
>> bool
>> - default y if HUGETLB_PAGE || PPC_64K_PAGES
>> + default y if HUGETLB_PAGE || (PPC64 && PPC_64K_PAGES)
>> default n
> I would make it PPC_64 && (HUGETLB_PAGE || PPC_64K_PAGES) for now,
> I don't think we want to use the existing slice code on anything else.
> Make it even PPC_STD_MMU_64
> Cheers,
> Ben.
Regards, Yuri
--
Yuri Tikhonov, Senior Software Engineer
Emcraft Systems, www.emcraft.com
_______________________________________________
Linuxppc-dev mailing list
Linuxppc-dev@ozlabs.org
https://ozlabs.org/mailman/listinfo/linuxppc-dev
