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

[Oleksii Kurochko <oleksii.kurochko@gmail.com>]

On 7/28/26 2:23 PM, Jan Beulich wrote:
> 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@gmail.com>
>> Reviewed-by: Baptiste Le Duc <baptiste.le-duc@vates.tech>
>> ---
>> 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@gmail.com/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?

Good question. Considering that all domains are initialized with 
MAX_IO_HANDLER I think we could drop an argument for domain_io_init() 
and just use MAX_IO_HANDLER inside it for init. of handlers array.

> 
>> --- /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?

With the current implementaion it could be done for both mmio_read_t and 
mmio_write_t as value is return through r argument.

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?

Agree, we don't need both "r" and info->data as they are serving the 
same purpose.

But I don't know which one option is actually better to drop "r" 
argument or drop ->data member in mmio_info_t.


> 
> 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;
> };
> 

I will apply that.

>> +};
>> +
>> +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.

Agree, it should be 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?

I totally agree, it can. Do you think it is better to keep ->data and 
drop an argument 'r' or vice versa?

> 
>> +}
>> +
>> +/* Assumes mmio regions are not overlapping. */
> 
> Are you guaranteeing this anywhere?

There is no such guarantee. register_mmio_handler() simply adds the 
handler to the handlers array without performing any checks. I can add 
such a check. The only question is whether it should be enabled only in 
debug builds or in all builds.

I assume this is a rare case, and overlapping regions would indicate 
that something is wrong with the guest's memory layout configuration so 
it seems like it would be enough to add only for debug builds.

> 
>> +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?)

Generally, no. However, the approach used by Arm's dom0less solution is 
to crash as soon as any issue occurs instead of trying to continue 
running other domains, so I follow the same approach for RISC-V.

Even if I return an error here, the common dom0less code will panic anyway.

> 
>> +    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().

Good point. I will rework that.
> 
>> +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];
> };
> 
Agree, both option are good to me. Considering that we are going to use 
MAX_IO_HANDLER then second option is really better for now.

Thanks!

~ Oleksii