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

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()?

> So I agree a check is worthwhile; I'll add one under CONFIG_DEBUG in 
> register_mmio_handler().

Some assertion then hopefully, rather than an open-coded use of CONFIG_DEBUG.

Jan