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

[Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>]

On 10/12/26 5:28 PM, Jan Beulich wrote:
> 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.
Makes sense. I'll make it a pointer allocated in domain_io_init(), with
struct vmmio (along with MAX_IO_HANDLER and struct mmio_handler) moved
into mmio.c (as they then will be used only there), so that 
<asm/domain.h> doesn't need <asm/mmio.h> anymore.
> > --- /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 ...
With having vmmio member of arch_domain struct declared as pointer then 
here it could be const struct domain *currd.
> > +                              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.
Indeed, both always refer to current. I'll rename them to currd and curr
(and make currd a pointer to const while at it).

> > +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.
Agreed, NULL ops can be dropped. As the callbacks always come from a
static ops structure, I'll turn their check into an ASSERT(). I'd keep
end <= addr as a run-time check though, as the region bounds may be
computed (e.g. from max_vcpus) rather than being constants, and the
overlap check below relies on addr + size not wrapping.

Thanks.

~ Oleksii