Re: [PATCH] RISC-V: Fix FIXMAP area corruption on RV32 systems

From: Palmer Dabbelt
Date: Mon Aug 26 2019 - 17:17:44 EST

On Sun, 18 Aug 2019 21:49:01 PDT (-0700), anup@xxxxxxxxxxxxxx wrote:
On Sun, Aug 18, 2019 at 11:49 PM Christoph Hellwig <hch@xxxxxxxxxxxxx> wrote:


Nit: no need for the braces, the definitions below don't use it

Sure, I will update and send v2 soon.

> +#ifdef CONFIG_64BIT
> +#else
> +#endif
> +
> /*
> - * Task size is 0x4000000000 for RV64 or 0xb800000 for RV32.
> + * Task size is 0x4000000000 for RV64 or 0x9fc00000 for RV32.
> * Note that PGDIR_SIZE must evenly divide TASK_SIZE.
> */
> #ifdef CONFIG_64BIT
> #else
> #endif

Mentioning the addresses is a little weird. IMHO this would be
a much nicer place to explain the high-level memory layout, including
maybe a little ASCII art. Also we could have one #ifdef CONFIG_64BIT
for both related values. Last but not least instead of saying that
something should be dividable it would be nice to have a BUILD_BUG_ON
to enforce it.

Either way we are late in the cycle, so I guess this is ok for now:

Reviewed-by: Christoph Hellwig <hch@xxxxxx>

But I'd love to see this area improved a little further as it is full
of mine fields.

I agree with you. We also have Sparsemem and KASAN patches which
touch virtual memory layout so it's important to have virtual memory layout
documented clearly. I can add the required documentation as separate patch.

Documentation is great, but if we document something that is broken then it's still broken :)

I think this needs to just be redone -- we keep running into issues here and fixing them, but there are probably more issues and it'll probably be faster to just think through the memory map than to keep fixing bugs as they crop up. This was one of the areas of the port I didn't rewrite as part of the upstream submission process, and as a result it's pretty crusty.

I think the best place to add ASCII art would be asm/pgtable.h where all
virtual memory related defines are placed. Suggestions??