Re: [Qemu-devel] [PATCH v1 2/2] s390x/tcg: low-address protection support

[David Hildenbrand <david@redhat.com>]

On 18.10.2017 20:21, Thomas Huth wrote:
> On 16.10.2017 22:23, David Hildenbrand wrote:
>> This is a neat way to implement low address protection, whereby
>> only the first 512 bytes of the first two pages (each 4096 bytes) of
>> every address space are protected.
>>
>> Store a tec of 0 for the access exception, this is what is defined by
>> Enhanced Suppression on Protection in case of a low address protection
>> (Bit 61 set to 0, rest undefined).
>>
>> We have to make sure to to pass the access address, not the masked page
>> address into mmu_translate*().
>>
>> Drop the check from testblock. So we can properly test this via
>> kvm-unit-tests.
>>
>> This will check every access going through one of the MMUs.
>>
>> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
>> Signed-off-by: David Hildenbrand <david@redhat.com>
>> ---
>>  target/s390x/excp_helper.c |  3 +-
>>  target/s390x/mem_helper.c  |  8 ----
>>  target/s390x/mmu_helper.c  | 94 +++++++++++++++++++++++++++++-----------------
>>  3 files changed, 60 insertions(+), 45 deletions(-)
> [...]
>> diff --git a/target/s390x/mmu_helper.c b/target/s390x/mmu_helper.c
>> index 9daa0fd8e2..9806685bee 100644
>> --- a/target/s390x/mmu_helper.c
>> +++ b/target/s390x/mmu_helper.c
>> @@ -106,6 +106,35 @@ static void trigger_page_fault(CPUS390XState *env, target_ulong vaddr,
>>      trigger_access_exception(env, type, ilen, tec);
>>  }
>>  
>> +/* check whether the address would be proteted by Low-Address Protection */
>> +static bool is_low_address(uint64_t addr)
>> +{
>> +    return addr < 512 || (addr >= 4096 && addr <= 4607);
>> +}
> 
> Just cosmetic, but I'd rather either use "<=" or "<" both times, so:
> 
>    return addr <= 511 || (addr >= 4096 && addr <= 4607);
> 

That one then, as it matches the wording in the PoP.

>> +    /* Check the private-space control bit */
>> +    switch (asc) {
>> +    case PSW_ASC_PRIMARY:
>> +        return !(env->cregs[1] & _ASCE_PRIVATE_SPACE);
>> +    case PSW_ASC_SECONDARY:
>> +        return !(env->cregs[7] & _ASCE_PRIVATE_SPACE);
>> +    case PSW_ASC_HOME:
>> +        return !(env->cregs[13] & _ASCE_PRIVATE_SPACE);
>> +    default:
>> +        g_assert_not_reached();
> 
> Well, this is certainly reachable - if the guest was running in access
> register mode. So it might be nicer to the user if you keep the
> error_report() here?

Right, this would be reachable via translate_pages(), but not via the
tlb. Although unlikely to hit it at that point, we can keep the error.

Conny, can you fix these two up or do you want me to resend?


-- 

Thanks,

David