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

[Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>]

On 8/3/26 12:41 PM, Jan Beulich wrote:
> On 31.07.2026 17:24, Oleksii Kurochko wrote:
> > 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.
> Didn't you say you'd get rid of the use of sort()?
Yes, I will. I just wrote that for the case if sort() will still present.

~ Oleksii