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
