Re: [PATCH v6 2/3] x86/mm: add API for marking only part of a MMIO page read only

[Jan Beulich <jbeulich@xxxxxxxx>]

On 23.07.2024 05:24, Marek Marczykowski-Górecki wrote:
> +static int __init subpage_mmio_ro_add_page(
> +    mfn_t mfn,
> +    unsigned int offset_s,
> +    unsigned int offset_e)
> +{
> +    struct subpage_ro_range *entry = NULL;
> +    bool new_entry = false;
> +    unsigned int i;
> +
> +    entry = subpage_mmio_find_page(mfn);
> +    if ( !entry )
> +    {
> +        entry = xzalloc(struct subpage_ro_range);
> +        if ( !entry )
> +            return -ENOMEM;
> +        entry->mfn = mfn;
> +        list_add(&entry->list, &subpage_ro_ranges);
> +        new_entry = true;
> +    }
> +
> +    for ( i = offset_s; i <= offset_e; i += MMIO_RO_SUBPAGE_GRAN )
> +    {
> +        bool oldbit = __test_and_set_bit(i / MMIO_RO_SUBPAGE_GRAN,
> +                                         entry->ro_elems);
> +        ASSERT(!oldbit);
> +    }
> +
> +    return new_entry ? 0 : 1;

Imo simply !new_entry would be more concise.

> +int __init subpage_mmio_ro_add(
> +    paddr_t start,
> +    size_t size)
> +{
> +    mfn_t mfn_start = maddr_to_mfn(start);
> +    paddr_t end = start + size - 1;
> +    mfn_t mfn_end = maddr_to_mfn(end);
> +    unsigned int offset_end = 0;
> +    int rc;
> +    bool subpage_start, subpage_end;
> +
> +    /*
> +     * In release, build unaligned start will protect larger area,

I think the first comma wants to move past "build".

> +     * so tolerate it.
> +     * But unaligned size would result in smaller area, so deny it.
> +     */
> +    ASSERT(IS_ALIGNED(start, MMIO_RO_SUBPAGE_GRAN));
> +    ASSERT(IS_ALIGNED(size, MMIO_RO_SUBPAGE_GRAN));
> +    if ( !IS_ALIGNED(size, MMIO_RO_SUBPAGE_GRAN) )
> +        return -EINVAL;

I hoped you would, when adding the comment, recall an earlier comment of
mine: If you want to tolerate mis-aligned start in release builds, you
need to make further adjustments to the subsequent logic (at which
point the respective assertion may become pointless); see below. While
things may work okay without (I didn't fully convince myself either way),
the main point here is that you want to make sure we test in debug builds
what's actually used in release one. Hence subtleties like this would
better be dealt with uniformly between release and debug builds.

> +    if ( !size )
> +        return 0;
> +
> +    if ( mfn_eq(mfn_start, mfn_end) )
> +    {
> +        /* Both starting and ending parts handled at once */
> +        subpage_start = PAGE_OFFSET(start) || PAGE_OFFSET(end) != PAGE_SIZE 
> - 1;
> +        subpage_end = false;
> +    }
> +    else
> +    {
> +        subpage_start = PAGE_OFFSET(start);
> +        subpage_end = PAGE_OFFSET(end) != PAGE_SIZE - 1;
> +    }

Neither of the two subpage_start calculations is correct when start is
misaligned, but within the first 8 bytes of a page.

Jan