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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 19.07.2024 04:33, Marek Marczykowski-Górecki wrote:
> @@ -4910,6 +4921,254 @@ long arch_memory_op(unsigned long cmd, 
> XEN_GUEST_HANDLE_PARAM(void) arg)
>      return rc;
>  }
>  
> +static void __iomem *subpage_mmio_find_page(mfn_t mfn)
> +{
> +    struct subpage_ro_range *entry;

With the function returning void*, my first reaction was to ask why this
isn't pointer-to-const. Yet then ...

> +    list_for_each_entry(entry, &subpage_ro_ranges, list)
> +        if ( mfn_eq(entry->mfn, mfn) )
> +            return entry;

... you're actually returning entry here, just with its type zapped for
no apparent reason. I also question the __iomem in the return type.

> +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, *iter;
> +    unsigned int i;
> +
> +    entry = subpage_mmio_find_page(mfn);
> +    if ( !entry )
> +    {
> +        /* iter == NULL marks it was a newly allocated entry */
> +        iter = NULL;

Yet you don't use "iter" for other purposes anymore. I think the variable
wants renaming and shrinking to e.g. a simple bool.

> +        entry = xzalloc(struct subpage_ro_range);
> +        if ( !entry )
> +            return -ENOMEM;
> +        entry->mfn = mfn;
> +    }
> +
> +    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);

Nit: Indentation looks to be off by 1 here.

> +        ASSERT(!oldbit);
> +    }
> +
> +    if ( !iter )
> +        list_add(&entry->list, &subpage_ro_ranges);

What's wrong with doing this right in the earlier conditional?

> +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;
> +
> +    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 think I had asked before: Why is misaligned size something that wants a
release build fallback to the assertion, but not misaligned start?

> +static void subpage_mmio_write_emulate(
> +    mfn_t mfn,
> +    unsigned int offset,
> +    const void *data,
> +    unsigned int len)
> +{
> +    struct subpage_ro_range *entry;
> +    volatile void __iomem *addr;
> +
> +    entry = subpage_mmio_find_page(mfn);
> +    if ( !entry )
> +        /* Do not print message for pages without any writable parts. */
> +        return;
> +
> +    if ( test_bit(offset / MMIO_RO_SUBPAGE_GRAN, entry->ro_elems) )
> +    {
> +write_ignored:

Nit: Like you have it further up, labels indented by at least one blank please.

Jan