Re: [PATCH RFC V2 1/2] arm64: Add enable/disable d-cache support for purgatory

[Pratyush Anand <panand@redhat.com>]

Hi Akashi,

Thanks for your review.

On 12/01/2016:05:34:41 PM, AKASHI Takahiro wrote:
> >+	default:
> >+		return 32;
> 
> AA64MMFR0_PRANGE_32, instead of 'default', should explicitly be used here
> as it is defined as 0x0 in ARM ARM.

OK.

> 
> >+static void init_page_table(void)
> >+{
> >+	inval_cache_range((uint64_t)page_table,
> >+				(uint64_t)page_table + PAGE_TABLE_SIZE);
> >+	memset(page_table, 0, PAGE_TABLE_SIZE);
> 
> why invalidate first?

Humm..may be you are right. It was copied from arch/arm64/kernel/head.S.
http://lxr.free-electrons.com/source/arch/arm64/kernel/head.S#L322

> 
> >+ */
> >+static void create_identity_mapping(uint64_t start, uint64_t end,
> >+					uint64_t flags)
> >+{
> >+	uint32_t sec_shift, pgdir_shift, sec_mask;
> >+	uint64_t desc, s1, e1, s2, e2;
> >+	uint64_t *table2;
> >+
> >+	s1 = start;
> >+	e1 = end;
> >+
> >+	sec_shift = get_section_shift();
> >+	if (pgtable_level == 1) {
> >+		s1 >>= sec_shift;
> >+		e1 >>= sec_shift;
> >+		do {
> >+			desc = s1 << sec_shift;
> >+			desc |= flags;
> >+			page_table[s1] = desc;
> >+			s1++;
> >+		} while (s1 <= e1);
> 
> To be precise, this loop creates an unnecessary entry
> if 'end' is exclusive. Pls think about the case that end is
> on sector boundary. Maybe,
>     e1 = (e1 - 1) >> sec_shift

Correct.

> 
> >+	} else {
> >+		pgdir_shift = get_pgdir_shift();
> >+		sec_mask = get_section_mask();
> >+		s1 >>= pgdir_shift;
> >+		e1 >>= pgdir_shift;
> >+		do {
> >+			/*
> >+			 * If there is no table entry then write a new
> >+			 * entry else, use old entry
> >+			 */
> >+			if (!page_table[s1]) {
> 
> s1 can be larger than 0, 1 or 2 if ram is located at much higher address
> than the first (three) sector(s).

Yes, that can most likely be, but code will take care.
From page_table[0] to page_table[MAX_PAGE_SIZE / sizeof(uint64_t)] will have entries
for 1st table.

we can have at max three tables.

table1 points to &page_table[0]
table2 points to &page_table[ MAX_PAGE_SIZE / sizeof(uint64_t)]
table3 points to &page_table[ 2 * MAX_PAGE_SIZE / sizeof(uint64_t)]

each table can contain number of entries = MAX_PAGE_SIZE / sizeof(uint64_t), so
if s1  > 3, it should work fine.

> 
> >+static void enable_mmu_dcache(void)
> >+{
> >+	uint64_t tcr_flags = TCR_FLAGS | TCR_T0SZ(va_bits);
> >+
> >+	switch(page_shift) {
> >+	case 16:
> >+		tcr_flags |= TCR_TG0_64K;
> >+		break;
> >+	case 12:
> >+		tcr_flags |= TCR_TG0_4K;
> >+		break;
> >+	default:
> >+		printf("page shift not supported\n");
> >+		return;
> >+	}
> >+	/*
> >+	 * Since the page tables have been populated with non-cacheable
> >+	 * accesses (MMU disabled), invalidate the idmap page
> >+	 * tables again to remove any speculatively loaded cache lines.
> >+	 */
> >+	inval_cache_range((uint64_t)page_table,
> >+				(uint64_t)page_table + PAGE_TABLE_SIZE);
> >+
> >+	switch(get_current_el()) {
> >+	case 3:
> 
> Linux kernel should be started only in EL2 or non-secure EL1.
> why should we take care of el3?

I was not sure about it, so kept it. OK.. Will remove in next version.

> 
> >+void disable_dcache(uint64_t ram_start, uint64_t ram_end)
> >+{
> >+	switch(get_current_el()) {
> >+	case 3:
> >+		reset_sctlr_el3();
> >+		break;
> 
> ditto

OK.

> 
> >+#define ID_AA64MMFR0_PARANGE_40		0x2
> >+#define ID_AA64MMFR0_PARANGE_36		0x1
> >+#define ID_AA64MMFR0_PARANGE_32		0x0
> >+
> >+#define TCR_TG0_64K 		(1UL << 14)
> >+#define TCR_TG0_4K 		(0UL << 14)
> >+#define TCR_SHARED_NONE		(0UL << 12)
> >+#define TCR_ORGN_WBWA		(1UL << 10)
> >+#define TCR_IRGN_WBWA		(1UL << 8)
> >+#define TCR_IPS_EL1_SHIFT	32
> >+#define TCR_IPS_EL2_SHIFT	16
> >+#define TCR_IPS_EL3_SHIFT	16
> 
> TCR_PS_EL[23]_SHIFT?

OK :-) , Did not noticed it earlier.

~Pratyush

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec