On 10.01.2018 22:53, David Hildenbrand wrote:
> To use virtual addresses, we have to
> - build page tables with identity mapping
> - setup the primary ASCE in cr1
> - enable DAT in the PSW
>
> Not using the Linux definititons/implementation as they contain too much
s/definititons/definitions/
> software defined stuff / things we don't need.
>
> Written from scratch. Tried to stick to the general Linux naming
> schemes.
>
> As we currently don't invalidate anything except page table entries, it
> is suficient to only use ipte for now.
s/suficient/sufficient/
> Signed-off-by: David Hildenbrand <david@xxxxxxxxxx>
> ---
> lib/s390x/asm/arch_def.h | 45 ++++++++++
> lib/s390x/asm/page.h | 24 +++++
> lib/s390x/asm/pgtable.h | 222 +++++++++++++++++++++++++++++++++++++++++++++++
> lib/s390x/mmu.c | 90 +++++++++++++++++++
> lib/s390x/sclp.c | 1 +
> s390x/Makefile | 2 +
> s390x/cstart64.S | 3 +-
> 7 files changed, 386 insertions(+), 1 deletion(-)
> create mode 100644 lib/s390x/asm/pgtable.h
> create mode 100644 lib/s390x/mmu.c
>
> diff --git a/lib/s390x/asm/arch_def.h b/lib/s390x/asm/arch_def.h
> index ae27ab2..416dc75 100644
> --- a/lib/s390x/asm/arch_def.h
> +++ b/lib/s390x/asm/arch_def.h
> @@ -163,4 +163,49 @@ static inline int tprot(unsigned long addr)
> return cc;
> }
>
> +static inline void lctlg(int cr, uint64_t value)
> +{
> + asm volatile(
> + " lctlg %1,%1,%0\n"
> + : : "Q" (value), "i" (cr));
Doesn't the compiler complain here? I though that "i" is only possible
with constants? I guess the compiler is smart enough to replace it with
the right constants, since this is an inline function. But maybe better
play safe and turn this into a macro instead.
> +}
> +
> +static inline uint64_t stctg(int cr)
> +{
> + uint64_t value;
> +
> + asm volatile(
> + " stctg %1,%1,%0\n"
> + : "=Q" (value) : "i" (cr) : "memory");
dito
> + return value;
> +}
> +
> +static inline void configure_dat(int enable)
> +{
> + struct psw psw = {
> + .mask = 0,
> + .addr = 0,
> + };
> +
> + asm volatile(
> + /* fetch the old PSW masks */
> + " epsw %1,%2\n"
> + /* remove the DAT flag first */
> + " nilh %1,0xfbff\n"
> + " or %3,%3\n"
> + " jz disable\n"
> + /* set DAT flag if enable == true */
> + " oilh %1,0x0400\n"
> + " st %1,0(%0)\n"
> + "disable:\n"
Shouldn't the "disable" label be placed before the "st" ?
> + " st %2,4(%0)\n"
> + /* load the target address for our new PSW */
> + " larl %1,cont\n"
> + " stg %1,8(%0)\n"
> + " lpswe 0(%0)\n"
> + "cont:\n"
> + : : "a" (&psw), "r" (0), "r" (1), "r" (enable)
The usage of "r" (0), "r" (1) looks wrong. I think you should rather put
them into the output list instead, since they are modified within the
assembler code. Or maybe even better, use fixed registers in the
assembler code and mark the corresponding registers in the clobber list.
> + : "memory", "cc");
> +}
Why is this an inline function in this header? It seems to only be used
in mmu.c, so I'd suggest to move it there (or even in a separate
assembler file).
> #endif
> diff --git a/lib/s390x/asm/page.h b/lib/s390x/asm/page.h
> index 141a456..e7cc16d 100644
> --- a/lib/s390x/asm/page.h
> +++ b/lib/s390x/asm/page.h
[...]
I'll review this part later...
> diff --git a/lib/s390x/sclp.c b/lib/s390x/sclp.c
> index c0492b8..522c387 100644
> --- a/lib/s390x/sclp.c
> +++ b/lib/s390x/sclp.c
> @@ -26,6 +26,7 @@ static uint64_t ram_size;
> static void mem_init(phys_addr_t freemem_start, phys_addr_t mem_end)
> {
> phys_alloc_init(freemem_start, ram_size - freemem_start);
> + setup_vm();
Do we really want to enable VM uncoditionally for all tests? x86 also
calls setup_vm() only for some specific test. I think we should keep the
possibility to run some tests in real mode, too, shouldn't we?
> }
>
Thomas
