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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 30.09.2026 18:18, Oleksii Kurochko wrote:
> --- a/xen/arch/riscv/include/asm/domain.h
> +++ b/xen/arch/riscv/include/asm/domain.h
> @@ -9,6 +9,7 @@
>  
>  #include <asm/cpufeature.h>
>  #include <asm/guest-layout.h>
> +#include <asm/mmio.h>
>  #include <asm/p2m.h>
>  #include <asm/vtimer.h>
>  
> @@ -101,6 +102,8 @@ struct arch_domain {
>      const unsigned long *isa;
>  
>      struct vintc *vintc;
> +
> +    struct vmmio vmmio;

This is a decent size structure, so I wonder whether this really wants
embedding in struct domain. If it was a pointer here (with the struct
dynamically allocated), the struct definition itself may not even need
exposing in a header.

> --- /dev/null
> +++ b/xen/arch/riscv/mmio.c
> @@ -0,0 +1,175 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +
> +#include <xen/bsearch.h>
> +#include <xen/lib.h>
> +#include <xen/rwlock.h>
> +#include <xen/sched.h>
> +#include <xen/string.h>
> +
> +#include <asm/current.h>
> +#include <asm/mmio.h>
> +
> +/*
> + * bsearch() comparator: @key holds the address to look up in its addr field,
> + * @elem is an entry of vmmio->handlers. Relies on the regions not
> + * overlapping, which register_mmio_handler() enforces.
> + */
> +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;
> +}
> +
> +/*
> + * Return a copy of the matching handler rather than a pointer into
> + * vmmio->handlers: a concurrent register_mmio_handler() shifts entries
> + * up to keep the array sorted, 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,

The first parameter here apparently wants to be named currd, much like ...

> +                              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);
> +    if ( handler )
> +        *out = *handler;
> +    read_unlock(&vmmio->lock);
> +
> +    return handler != NULL;
> +}
> +
> +static enum io_state try_handle_mmio(mmio_info_t *info)
> +{
> +    struct vcpu *v = current;

... this variable wants to be named curr. Unless of course there are
plans to not unconditionally use current here.

> +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 || end <= addr )
> +        return -EINVAL;

I think this checking goes a little too far: It's absurd for some call site
to pass NULL into this function. The other three checks are sufficiently
plausible, albeit I wonder whether this wouldn't want to be an assertion
then.

Jan