Re: [PATCH v2 4/7] s390x/mmu: Inject PGM_ADDRESSING on boguous table addresses

[David Hildenbrand <david@redhat.com>]

On 25.09.19 21:25, Richard Henderson wrote:
> On 9/25/19 5:52 AM, David Hildenbrand wrote:
>> +static inline int read_table_entry(hwaddr gaddr, uint64_t *entry)
>> +{
>> +    /*
>> +     * According to the PoP, these table addresses are "unpredictably real
>> +     * or absolute". Also, "it is unpredictable whether the address wraps
>> +     * or an addressing exception is recognized".
>> +     *
>> +     * We treat them as absolute addresses and don't wrap them.
>> +     */
>> +    if (unlikely(address_space_read(&address_space_memory, gaddr,
>> +                 MEMTXATTRS_UNSPECIFIED, (uint8_t *)entry, sizeof(*entry)) !=
>> +                 MEMTX_OK)) {
>> +        return -EFAULT;
>> +    }
>> +    *entry = be64_to_cpu(*entry);
>> +    return 0;
>> +}
> 
> Maybe I've been away from the kernel too long, but I don't find returning
> -EFAULT helpful.  I would return true/false for success/failure so that...
> 
> 
>> +    if (read_table_entry(origin + offs, &pt_entry)) {
>> +        return PGM_ADDRESSING;
>> +    }
> 
> ... this gets written
> 
>     if (!read_table_entry(...)) {
>         return PGM_ADDRESSING;
>     }
> 
> This statement, to me, reads "If we did not read_table_entry, return an
> addressing exception."
> 
> If you *really* want to return non-zero on failure, I would prefer returning
> PGM_ADDRESSING instead of the out-of-context -EFAULT.

I'll go for your suggestion with a bool!

> 
>> -    new_entry = ldq_phys(cs->as, origin + offs);
>> +    if (read_table_entry(origin + offs, &new_entry)) {
> 
> Do you really want to replace cs->as with address_space_memory?
> 

I guess it shouldn't make a difference (unless I am missing something),
but I can just keep using cs->as.

Thanks!

> 
> r~
> 


-- 

Thanks,

David / dhildenb