Re: [PATCH v1 04/17] xen/riscv: introduce device-agnostic MMIO emulation dispatch

[Jan Beulich <jbeulich@xxxxxxxx>]

On 20.07.2026 18:02, Oleksii Kurochko wrote:
> RISC-V guests can expose several virtual interrupt controllers at
> distinct GPA ranges: vPLIC (hasn't been introduced yet) for legacy machines,
> vAPLIC and vIMSIC for AIA-compliant ones (is being introduced in the follow
> up patches). Routing MMIO faults via a per-device is_access() check in the
> trap handler would couple it to every device it must serve, requiring a
> new conditional branch in the fault path each time a new emulated device is
> added.
> 
> Introduce a per-domain MMIO handler registration table, modeled
> after the equivalent ARM framework, so that virtual devices
> self-register their GPA ranges and read/write callbacks at domain
> creation time. The MMIO fault path delegates to a single
> try_handle_mmio() entry point and remains agnostic of which device
> owns a particular address.
> 
> Subsequent patches wire this into arch_domain_create() and the MMIO fault
> path in traps.c.
> 
> Signed-off-by: Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>
> Reviewed-by: Baptiste Le Duc <baptiste.le-duc@xxxxxxxxxx>
> ---
> Note that find_mmio_handler() and try_handle_mmio() is handling found
> handler differently for now in comparison to Arm. But this behaviour will
> be aligned at the end. Look at discussion:
>   
> https://lore.kernel.org/xen-devel/cd78972e-88d5-471d-a201-5f9cd1392c73@xxxxxxxxx/T/#t
> ---
> ---
>  xen/arch/riscv/Makefile             |   1 +
>  xen/arch/riscv/domain.c             |   4 +
>  xen/arch/riscv/include/asm/domain.h |   3 +
>  xen/arch/riscv/include/asm/mmio.h   |  63 ++++++++++++
>  xen/arch/riscv/mmio.c               | 145 ++++++++++++++++++++++++++++
>  5 files changed, 216 insertions(+)
>  create mode 100644 xen/arch/riscv/include/asm/mmio.h
>  create mode 100644 xen/arch/riscv/mmio.c
> 
> diff --git a/xen/arch/riscv/Makefile b/xen/arch/riscv/Makefile
> index 046f73f4d87c..c452ebc3cf61 100644
> --- a/xen/arch/riscv/Makefile
> +++ b/xen/arch/riscv/Makefile
> @@ -14,6 +14,7 @@ obj-y += intc.o
>  obj-y += irq.o
>  obj-y += kernel.init.o
>  obj-y += mm.o
> +obj-y += mmio.o
>  obj-y += p2m.o
>  obj-y += paging.o
>  obj-y += pt.o
> diff --git a/xen/arch/riscv/domain.c b/xen/arch/riscv/domain.c
> index 4db9c28662c7..1e6f0ef66c2f 100644
> --- a/xen/arch/riscv/domain.c
> +++ b/xen/arch/riscv/domain.c
> @@ -12,6 +12,7 @@
>  #include <asm/cpufeature.h>
>  #include <asm/csr.h>
>  #include <asm/intc.h>
> +#include <asm/mmio.h>
>  #include <asm/riscv_encoding.h>
>  #include <asm/vtimer.h>
>  
> @@ -308,6 +309,9 @@ int arch_domain_create(struct domain *d,
>      if ( (rc = p2m_init(d, config)) != 0)
>          goto fail;
>  
> +    if ( (rc = domain_io_init(d, MAX_IO_HANDLER)) != 0 )
> +        goto fail;

Why does MAX_IO_HANDLER need passing into the function? Isn't that a global
boundary?

> --- /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>
> +
> +#define MAX_IO_HANDLER  16
> +
> +typedef struct {
> +    paddr_t gpa;
> +    unsigned int len;  /* access width in bytes (1, 2, 4, 8) */
> +    bool is_write;
> +    register_t data;   /* store: value to write; load: value read (set by 
> handler) */
> +} 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,
> +                                     register_t *r);
> +typedef enum io_state (*mmio_write_t)(struct vcpu *v, mmio_info_t *info,
> +                                      register_t r);

Can't info be pointer-to-const in the write case? In both cases, why is there
both "r" passed into the function as well as the info->data field, supposedly
(as per the comment) serving the same purpose?

Furthermore I think it helps if ...

> +struct mmio_handler_ops {
> +    mmio_read_t read;
> +    mmio_write_t write;

... pointer-ness is easily seen at use sites. I.e.

typedef enum io_state mmio_read_t(struct vcpu *v, mmio_info_t *info,
                                  register_t *r);
typedef enum io_state mmio_write_t(struct vcpu *v, const mmio_info_t *info,
                                   register_t r);

struct mmio_handler_ops {
    mmio_read_t *read;
    mmio_write_t *write;
};

> +};
> +
> +struct mmio_handler {
> +    paddr_t addr;
> +    paddr_t size;
> +    const struct mmio_handler_ops *ops;
> +};
> +
> +struct vmmio {
> +    unsigned int num_entries;
> +    unsigned int max_num_entries;
> +    rwlock_t lock;
> +    struct mmio_handler *handlers;

There shouldn't be any writes through this pointer, should there? In which
case it (once again) wants to be pointer-to-const.

> --- /dev/null
> +++ b/xen/arch/riscv/mmio.c
> @@ -0,0 +1,145 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +/*
> + * Copyright (C) Vates
> + */
> +
> +#include <xen/bsearch.h>
> +#include <xen/lib.h>
> +#include <xen/rwlock.h>
> +#include <xen/sched.h>
> +#include <xen/sort.h>
> +#include <xen/xvmalloc.h>
> +
> +#include <asm/current.h>
> +#include <asm/mmio.h>
> +
> +static enum io_state handle_read(const struct mmio_handler *handler,
> +                                 struct vcpu *v,
> +                                 mmio_info_t *info)
> +{
> +    register_t r = 0;
> +    enum io_state rc;
> +
> +    rc = handler->ops->read(v, info, &r);
> +    if ( rc == IO_HANDLED )
> +        info->data = r;

Extending my earlier comment: Why could ->read() not put the value directly
into info->data? And why ...

> +static enum io_state handle_write(const struct mmio_handler *handler,
> +                                  struct vcpu *v,
> +                                  mmio_info_t *info)
> +{
> +    return handler->ops->write(v, info, info->data);

... can't write take the value directly from info->data?

> +}
> +
> +/* Assumes mmio regions are not overlapping. */

Are you guaranteeing this anywhere?

> +static int cmp_mmio_handler(const void *key, const void *elem)
> +{
> +    const struct mmio_handler *handler0 = key;
> +    const struct mmio_handler *handler1 = elem;
> +
> +    if ( handler0->addr < handler1->addr )
> +        return -1;
> +
> +    if ( handler0->addr >= (handler1->addr + handler1->size) )
> +        return 1;
> +
> +    return 0;
> +}
> +
> +static void swap_mmio_handler(void *a, void *b)
> +{
> +    struct mmio_handler *t1 = a, *t2 = b;
> +
> +    SWAP(*t1, *t2);
> +}
> +
> +/*
> + * Return a copy of the matching handler rather than a pointer into
> + * vmmio->handlers: a concurrent register_mmio_handler() re-sorts the
> + * array, so an escaped pointer could refer to a different (or torn)
> + * entry once the lock is dropped.  The copy stays valid as the ops
> + * structures are never freed.
> + */
> +static bool find_mmio_handler(struct domain *d, paddr_t gpa,
> +                              struct mmio_handler *out)
> +{
> +    struct vmmio *vmmio = &d->arch.vmmio;
> +    struct mmio_handler key = { .addr = gpa };
> +    const struct mmio_handler *handler;
> +
> +    read_lock(&vmmio->lock);
> +    handler = bsearch(&key, vmmio->handlers, vmmio->num_entries,
> +                      sizeof(*handler), cmp_mmio_handler);

So beyond the assumption stated further up you also assume the array to
be sorted. Which you ...

> +void 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 *handler;
> +
> +    write_lock(&vmmio->lock);
> +
> +    BUG_ON(vmmio->num_entries >= vmmio->max_num_entries);

(Do we really need to crash in such a case? Can't we just fail domain
creation?)

> +    handler = &vmmio->handlers[vmmio->num_entries];
> +    handler->ops = ops;
> +    handler->addr = addr;
> +    handler->size = size;
> +    vmmio->num_entries++;
> +
> +    /* Sort mmio handlers in ascending order based on base address */
> +    sort(vmmio->handlers, vmmio->num_entries, sizeof(struct mmio_handler),
> +         cmp_mmio_handler, swap_mmio_handler);

... arrange for here, yet in a pretty inefficient way: Inserting in an
already sorted list can be had without recurring calls to sort().

> +int domain_io_init(struct domain *d, unsigned int max_count)
> +{
> +    rwlock_init(&d->arch.vmmio.lock);
> +    d->arch.vmmio.num_entries = 0;
> +    d->arch.vmmio.max_num_entries = max_count;
> +    d->arch.vmmio.handlers = xvzalloc_array(struct mmio_handler, max_count);

If already an allocation is needed in all cases, why not allocate struct
vmmio, defined like this:

struct vmmio {
    unsigned int num_entries;
    unsigned int max_num_entries;
    rwlock_t lock;
    struct mmio_handler handlers[];
};

and then using xvzalloc_flex_struct(). Or yet simpler if (as mentioned
elsewhere) max_count doesn't need passing into here:

struct vmmio {
    unsigned int num_entries;
    unsigned int max_num_entries;
    rwlock_t lock;
    struct mmio_handler handlers[MAX_IO_HANDLER];
};

Jan