On 21.05.2024 04:54, Marek Marczykowski-Górecki wrote:
> --- a/xen/arch/x86/include/asm/mm.h
> +++ b/xen/arch/x86/include/asm/mm.h
> @@ -522,9 +522,27 @@ extern struct rangeset *mmio_ro_ranges;
> void memguard_guard_stack(void *p);
> void memguard_unguard_stack(void *p);
>
> +/*
> + * Add more precise r/o marking for a MMIO page. Range specified here
> + * will still be R/O, but the rest of the page (not marked as R/O via another
> + * call) will have writes passed through.
> + * The start address and the size must be aligned to SUBPAGE_MMIO_RO_ALIGN.
> + *
> + * This API cannot be used for overlapping ranges, nor for pages already
> added
> + * to mmio_ro_ranges separately.
> + *
> + * Return values:
> + * - negative: error
> + * - 0: success
> + */
> +#define SUBPAGE_MMIO_RO_ALIGN 8
This isn't just alignment, but also (and perhaps more importantly) granularity.
I think the name wants to express this.
> @@ -4910,6 +4921,260 @@ long arch_memory_op(unsigned long cmd,
> XEN_GUEST_HANDLE_PARAM(void) arg)
> return rc;
> }
>
> +/*
> + * Mark part of the page as R/O.
> + * Returns:
> + * - 0 on success - first range in the page
> + * - 1 on success - subsequent range in the page
> + * - <0 on error
> + *
> + * This needs subpage_ro_lock already taken */
Nit: Comment style (full stop and */ on its own line).
> +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;
> +
> + list_for_each_entry(iter, &subpage_ro_ranges, list)
> + {
> + if ( mfn_eq(iter->mfn, mfn) )
> + {
> + entry = iter;
> + break;
> + }
> + }
> + if ( !entry )
> + {
> + /* iter == NULL marks it was a newly allocated entry */
> + iter = NULL;
> + entry = xzalloc(struct subpage_ro_range);
> + if ( !entry )
> + return -ENOMEM;
> + entry->mfn = mfn;
> + }
> +
> + for ( i = offset_s; i <= offset_e; i += SUBPAGE_MMIO_RO_ALIGN )
> + {
> + int oldbit = __test_and_set_bit(i / SUBPAGE_MMIO_RO_ALIGN,
> + entry->ro_qwords);
Why int, not bool?
> + ASSERT(!oldbit);
> + }
> +
> + if ( !iter )
> + list_add(&entry->list, &subpage_ro_ranges);
> +
> + return iter ? 1 : 0;
> +}
> +
> +/* This needs subpage_ro_lock already taken */
> +static void __init subpage_mmio_ro_remove_page(
> + mfn_t mfn,
> + int offset_s,
> + int offset_e)
Can either of these be negative? The more that ...
> +{
> + struct subpage_ro_range *entry = NULL, *iter;
> + unsigned int i;
... this is used ...
> + list_for_each_entry(iter, &subpage_ro_ranges, list)
> + {
> + if ( mfn_eq(iter->mfn, mfn) )
> + {
> + entry = iter;
> + break;
> + }
> + }
> + if ( !entry )
> + return;
> +
> + for ( i = offset_s; i <= offset_e; i += SUBPAGE_MMIO_RO_ALIGN )
... with both of them?
> + __clear_bit(i / SUBPAGE_MMIO_RO_ALIGN, entry->ro_qwords);
> +
> + if ( !bitmap_empty(entry->ro_qwords, PAGE_SIZE / SUBPAGE_MMIO_RO_ALIGN) )
> + return;
> +
> + list_del(&entry->list);
> + if ( entry->mapped )
> + iounmap(entry->mapped);
> + xfree(entry);
> +}
> +
> +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, SUBPAGE_MMIO_RO_ALIGN));
> + ASSERT(IS_ALIGNED(size, SUBPAGE_MMIO_RO_ALIGN));
> + if ( !IS_ALIGNED(size, SUBPAGE_MMIO_RO_ALIGN) )
> + size = ROUNDUP(size, SUBPAGE_MMIO_RO_ALIGN);
> +
> + 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;
> + }
> +
> + spin_lock(&subpage_ro_lock);
> +
> + if ( subpage_start )
> + {
> + offset_end = mfn_eq(mfn_start, mfn_end) ?
> + PAGE_OFFSET(end) :
> + (PAGE_SIZE - 1);
> + rc = subpage_mmio_ro_add_page(mfn_start,
> + PAGE_OFFSET(start),
> + offset_end);
> + if ( rc < 0 )
> + goto err_unlock;
> + /* Check if not marking R/W part of a page intended to be fully R/O
> */
> + ASSERT(rc || !rangeset_contains_singleton(mmio_ro_ranges,
> + mfn_x(mfn_start)));
> + }
> +
> + if ( subpage_end )
> + {
> + rc = subpage_mmio_ro_add_page(mfn_end, 0, PAGE_OFFSET(end));
> + if ( rc < 0 )
> + goto err_unlock_remove;
> + /* Check if not marking R/W part of a page intended to be fully R/O
> */
> + ASSERT(rc || !rangeset_contains_singleton(mmio_ro_ranges,
> + mfn_x(mfn_end)));
> + }
> +
> + spin_unlock(&subpage_ro_lock);
> +
> + rc = rangeset_add_range(mmio_ro_ranges, mfn_x(mfn_start),
> mfn_x(mfn_end));
> + if ( rc )
> + goto err_remove;
> +
> + return 0;
> +
> + err_remove:
> + spin_lock(&subpage_ro_lock);
> + if ( subpage_end )
> + subpage_mmio_ro_remove_page(mfn_end, 0, PAGE_OFFSET(end));
> + err_unlock_remove:
> + if ( subpage_start )
> + subpage_mmio_ro_remove_page(mfn_start, PAGE_OFFSET(start),
> offset_end);
> + err_unlock:
> + spin_unlock(&subpage_ro_lock);
> + return rc;
> +}
> +
> +static void __iomem *subpage_mmio_get_page(struct subpage_ro_range *entry)
> +{
> + void __iomem *mapped_page;
> +
> + if ( entry->mapped )
> + return entry->mapped;
> +
> + mapped_page = ioremap(mfn_x(entry->mfn) << PAGE_SHIFT, PAGE_SIZE);
mfn_to_maddr() or some such?
> + spin_lock(&subpage_ro_lock);
> + /* Re-check under the lock */
> + if ( entry->mapped )
> + {
> + spin_unlock(&subpage_ro_lock);
> + iounmap(mapped_page);
What if ioremap() failed?
Jan
