>>
>> +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.
This works as it gets inlined. asm macros is frowned upon, no?
>
>> + 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" ?
yes indeed
>
>> + " 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.
I tried to use the output list way and it would not let me specify
immediates (0). So I have to introduce local variables. Or is there
another way?
>
>> + : "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).
I will move it to mmu.c but export the function, because ...
>
>> #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?
A future test framework will most likely rely on malloc to work (that's
why we are implementing it). So I enable it as default. Each test can
simply disable dat for a certain time and reenable it then via
configure_dat().
(will also use this in sieve.c to get physical memory access)
>
>> }
>>
>
> Thomas
>
Thanks!
--
Thanks,
David / dhildenb
