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

[Richard Henderson <richard.henderson@linaro.org>]

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.

> -    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?


r~