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

[Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>]

On 7/30/26 6:09 PM, Jan Beulich wrote:
> On 30.07.2026 18:03, Oleksii Kurochko wrote:
> > On 7/28/26 2:23 PM, Jan Beulich wrote:
> > > On 20.07.2026 18:02, Oleksii Kurochko wrote:
> > > > --- /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?
> How can I know? You know future plans you have.
>
> > > > +}
> > > > +
> > > > +/* 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.
> Depends on what other badness can happen when this is violated. My gut
> feeling is that checking in debug builds may be enough.
Overlapping regions would be a Xen bug rather than something a guest can 
trigger — register_mmio_handler() is only called from Xen's own emulated 
device code, so the layout isn't under guest control.
The badness is worse than just mis-emulating one device though: 
cmp_mmio_handler() is used both by bsearch() and by sort(). With 
overlapping regions it's no longer a consistent ordering, so sort() may 
produce an arbitrary order and lookups can then fail (or match the wrong 
handler) even for regions which don't overlap themselves. That would 
show up as a spurious fault injected into the guest, which is quite hard 
to debug.
So I agree a check is worthwhile; I'll add one under CONFIG_DEBUG in 
register_mmio_handler().
> > > > +/*
> > > > + * 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.
> That's the policy there, but you're writing code here also for the case where
> Dom0 creates domains.
Missed that. In this case I agree that it would be nice to return something.

Thanks.

~ Oleksii