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

[Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>]

On 9/9/26 3:24 PM, Jan Beulich wrote:
> On 27.08.2026 17:20, Oleksii Kurochko wrote:
> > +/*
> > + * Check alignment and dispatch a decoded MMIO access to a registered
> > + * handler. On success (0), info->data holds the read value for loads.
> > + *
> > + * There is no "retry" outcome to handle: find_mmio_handler() returns a
> > + * copy of the matching handler taken under vmmio->lock and the ops
> > + * structures are never freed, so the lookup result cannot go stale
> > + * between finding the handler and invoking it.
> > + */
> > +int do_mmio(mmio_info_t *info, paddr_t fault_addr, unsigned int len)
> > +{
> > +    /* Fault address should be aligned to length of MMIO */
> > +    if ( fault_addr & (len - 1) )
> > +        return -EIO;
> Better first check (or at least assert) that len is a power of 2?
It make sense. I will do then:

    if ( len & (len - 1) || fault_addr & (len - 1) )

> > +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 || !size || end < addr )
> > +        return -EINVAL;
> "!size || end < addr" can be had shorter as "end <= addr".
I will apply this.

> Whether it's worth checking ops to be non-NULL I question, bit I wouldn't
> insist on dropping the check.
Probably it isn't really needed but just extra check that someone miss 
to provide implementation of ->read, ->write still could be useful. Also 
it is executed only at boot time so not big perfomance impact.

> > +    write_lock(&vmmio->lock);
> > +
> > +    if ( vmmio->num_entries >= ARRAY_SIZE(vmmio->handlers) )
> > +    {
> > +        rc = -ENOSPC;
> > +        goto out;
> > +    }
> > +
> > +    /*
> > +     * The array is kept sorted by base address, so rather than appending and
> > +     * re-sorting, find the slot the new region belongs to and shift the tail
> > +     * up by one.
> > +     */
> > +    for ( i = vmmio->num_entries;
> > +          i > 0 && handlers[i - 1].addr > addr;
> > +          i-- )
> > +        /* Nothing */;
>      for ( i = vmmio->num_entries; i-- > 0 && handlers[i].addr > addr; )
>          /* Nothing */;
>
> ?
It seems like it will break the code after it.

This breaks cases:
1. On a normal exit (handlers[i].addr <= addr), i is the index of the 
entry that was found, whereas the insertion slot ought to be i + 1. The 
code below, however, uses i as the insertion slot and handlers[i - 1] as 
the left neighbour - an off-by-one.
2.If every entry has a bigger addr than the new one, the loop exits when 
i == 0: 0 > 0 is false, yet i-- has already taken effect, so i == 
UINT_MAX. Then i > 0 is true, leading to a read of handlers[UINT_MAX - 
1] and to memmove() with a size of (num_entries - UINT_MAX).
Case 1 pretty easy to fix, just use proper indexing but case 2 will 
require extra check at least. Thereby I think we could keep here 
original for loop.
> > +    /*
> > +     * Regions are required not to overlap; check both neighbours. Their
> > +     * addr + size cannot overflow, as such regions are rejected above when
> > +     * they get registered.
> > +     */
> > +    overlap = (i > 0 && handlers[i - 1].addr + handlers[i - 1].size > addr) ||
> > +              (i < vmmio->num_entries && end > handlers[i].addr);
> > +
> > +    if ( overlap )
> > +    {
> > +        rc = -EEXIST;
> I fear -EEXIST can be misleading; it generally means _this_ range is
> already covered, not some sub-range thereof.
Then probably EADDRINUSE() would be better.

Thanks.

~ Oleksii