Re: [PATCH v2 08/39] xen/riscv: introduce device-agnostic MMIO emulation dispatch

[Jan Beulich <jbeulich@xxxxxxxx>]

On 27.08.2026 17:20, Oleksii Kurochko wrote:
> --- /dev/null
> +++ b/xen/arch/riscv/include/asm/mmio.h
> @@ -0,0 +1,63 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +#ifndef RISCV_MMIO_H
> +#define RISCV_MMIO_H
> +
> +#include <xen/lib.h>
> +#include <xen/rwlock.h>
> +
> +struct domain;
> +struct vcpu;
> +
> +#define MAX_IO_HANDLER  16
> +
> +typedef struct {
> +    paddr_t gpa;
> +    unsigned int len;  /* access width in bytes (1, 2, 4, 8) */
> +    bool is_write;
> +    /* store: value to write; load: value read (set by handler) */

Nit: Comment style (twice). I'd also like to mention that enumerating
access widths is prone to go stale once the V or Q extensions are
supported.

> +    register_t data;
> +} mmio_info_t;
> +
> +enum io_state
> +{
> +    IO_ABORT,       /* The IO was handled and led to an abort. */
> +    IO_HANDLED,     /* The IO was successfully handled. */
> +    IO_UNHANDLED,   /* No handler found for the IO. */
> +};
> +
> +typedef enum io_state (mmio_read_t)(struct vcpu *v, mmio_info_t *info);
> +typedef enum io_state (mmio_write_t)(struct vcpu *v, const mmio_info_t 
> *info);

I don't quite understand the (need for) parentheses around the typedef
names.

> +/*
> + * Check alignment and dispatch a decoded MMIO access to a registered
> + * handler. On success (0), info->data holds the read value for loads.
> + *
> + * There is no "retry" outcome to handle: find_mmio_handler() returns a
> + * copy of the matching handler taken under vmmio->lock and the ops
> + * structures are never freed, so the lookup result cannot go stale
> + * between finding the handler and invoking it.
> + */
> +int do_mmio(mmio_info_t *info, paddr_t fault_addr, unsigned int len)
> +{
> +    /* Fault address should be aligned to length of MMIO */
> +    if ( fault_addr & (len - 1) )
> +        return -EIO;

Better first check (or at least assert) that len is a power of 2?

> +int register_mmio_handler(struct domain *d,
> +                          const struct mmio_handler_ops *ops,
> +                          paddr_t addr, paddr_t size)
> +{
> +    struct vmmio *vmmio = &d->arch.vmmio;
> +    struct mmio_handler *handlers = vmmio->handlers;
> +    paddr_t end = addr + size;
> +    unsigned int i;
> +    int rc = 0;
> +    bool overlap;
> +
> +    if ( !ops || !ops->read || !ops->write || !size || end < addr )
> +        return -EINVAL;

"!size || end < addr" can be had shorter as "end <= addr".

Whether it's worth checking ops to be non-NULL I question, bit I wouldn't
insist on dropping the check.

> +    write_lock(&vmmio->lock);
> +
> +    if ( vmmio->num_entries >= ARRAY_SIZE(vmmio->handlers) )
> +    {
> +        rc = -ENOSPC;
> +        goto out;
> +    }
> +
> +    /*
> +     * The array is kept sorted by base address, so rather than appending and
> +     * re-sorting, find the slot the new region belongs to and shift the tail
> +     * up by one.
> +     */
> +    for ( i = vmmio->num_entries;
> +          i > 0 && handlers[i - 1].addr > addr;
> +          i-- )
> +        /* Nothing */;

    for ( i = vmmio->num_entries; i-- > 0 && handlers[i].addr > addr; )
        /* Nothing */;

?

> +    /*
> +     * Regions are required not to overlap; check both neighbours. Their
> +     * addr + size cannot overflow, as such regions are rejected above when
> +     * they get registered.
> +     */
> +    overlap = (i > 0 && handlers[i - 1].addr + handlers[i - 1].size > addr) 
> ||
> +              (i < vmmio->num_entries && end > handlers[i].addr);
> +
> +    if ( overlap )
> +    {
> +        rc = -EEXIST;

I fear -EEXIST can be misleading; it generally means _this_ range is
already covered, not some sub-range thereof.

> +void domain_io_init(struct domain *d)
> +{
> +    rwlock_init(&d->arch.vmmio.lock);
> +    d->arch.vmmio.num_entries = 0;

The latter shouldn't be necessary, as struct domain-s start out zero
filled.

Jan