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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 09.09.2026 16:04, Oleksii Kurochko wrote:
> 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) )

With more parentheses added, I suppose.

>>> +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.

I questioned merely the checking of ops itself, not that of the ->read
and ->write hooks.

>>> +    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.

Oh, I'm sorry for the bad suggestion then.

Jan