[Differential] D2535: New, experimental PMAP implementation for MIPS64

[jmallett (Juli Mallett)]

jmallett added inline comments.

INLINE COMMENTS

> kvm_mips.h:52
>  #define	MIPS32_PFN_MASK		(0x1FFFFFC0)
> -#define	MIPS64_PFN_MASK		0x3FFFFFFC0
> +#define	MIPS64_PFN_MASK		0xFFFFFFC0
>  #define	MIPS_PFN_SHIFT		(6)

In this and the PFN mask changes in the kernel headers, I really wonder whether we can safely make these changes.  I understand they give us more upper software bits, and that on CHERI these are safe because the software and hardware are harmonized here, but are we quite sure that there's no real hardware where the physical address layout might require being able to decode these bits of the PFN?

> options.mips:103
> +#
> +MIPS64_NEW_PMAP			opt_global.h
> +

Can we get a timeline on deprecation of the old pmap, and note that the option is temporary?

> options.mips:115
> +# Use one large page (currently 16K) for the kernel thread stack
> +KSTACK_LARGE_PAGE   		opt_global.h
> +

I would really rather just see us only use large kernel stack pages.  It just makes more sense.  I don't think the option here is healthy for us.  Do we really support hardware which doesn't use PageMask?

> pmap.h:184
>  void *pmap_mapdev(vm_paddr_t, vm_size_t);
> +#ifdef MIPS64_NEW_PMAP
> +#define	pmap_page_is_mapped(m)	(!TAILQ_EMPTY(&(m)->md.pv_list))

Are these ifdefs correct?  I am having a hard time making sense of what changed here and why.

> pte.h:82
> + * prevent loading and storing of capabilities, so we have reduced the 55-bit
> + * shift to 53 bits.
>   */

Again, really not sure we can safely do this on all systems.  Robert is right to be cautious.

> vmparam.h:109
> + * Currently mips64 only supports one size or level (VM_LEVEL_0_ORDER) of
> + * superpages (2MB)
> + */

I'm surprised by this, since there seems to half-be infrastructure for other levels.  What's the plan here?

> pmap.c:812
>  		pte = *ptep;
> -		if (pte_test(&pte, PTE_V) && (!pte_test(&pte, PTE_RO) ||
> +		if (pte_test(&pte, PTE_VALID) && (!pte_test(&pte, PTE_RO) ||
>  		    (prot & VM_PROT_WRITE) == 0)) {

pte_is_valid or whatever here, as was done elsewhere?  Consistency would help get over all the PTE bit renaming.

> pmap.c:3459
> + *
> + * Reference bit emulation is not supported in this pmap implementation.
> + */

So why all the new PTE fields?  Again, confused.

> tlb.c:43
>  
> +#include "opt_vm.h"
> +

Necessary?

> trap.c:646
> +				 * Attempt to recover by flushing the user TLB
> +				 * and resetting the status bit.
> +				 */

Can you explain to me why this happens with the new pmap?  Is it related to superpages somehow?  It seems worrying.  Again, I must be missing something?

> vm_glue.c:315
>  
> +#if defined(__mips__)
> +

This is a good change to make stack allocation a bit more semantic, flexible, clear, etc.  It's also very self-contained, and I'd suggest it be reviewed and committed separately.

REPOSITORY
  rS FreeBSD src repository

REVISION DETAIL
  https://reviews.freebsd.org/D2535

EMAIL PREFERENCES
  https://reviews.freebsd.org/settings/panel/emailpreferences/

To: sbruno, sson
Cc: brooks, jmallett, markj, alc, sbruno, rwatson, emaste, imp, freebsd-mips-list, dnelson_1901_yahoo.com, mizhka_gmail.com