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
