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

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