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

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.

>>> +/*
>>> + * Return a copy of the matching handler rather than a pointer into
>>> + * vmmio->handlers: a concurrent register_mmio_handler() re-sorts the
>>> + * array, so an escaped pointer could refer to a different (or torn)
>>> + * entry once the lock is dropped.  The copy stays valid as the ops
>>> + * structures are never freed.
>>> + */
>>> +static bool find_mmio_handler(struct domain *d, paddr_t gpa,
>>> +                              struct mmio_handler *out)
>>> +{
>>> +    struct vmmio *vmmio = &d->arch.vmmio;
>>> +    struct mmio_handler key = { .addr = gpa };
>>> +    const struct mmio_handler *handler;
>>> +
>>> +    read_lock(&vmmio->lock);
>>> +    handler = bsearch(&key, vmmio->handlers, vmmio->num_entries,
>>> +                      sizeof(*handler), cmp_mmio_handler);
>>
>> So beyond the assumption stated further up you also assume the array to
>> be sorted. Which you ...
>>
>>> +void 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 *handler;
>>> +
>>> +    write_lock(&vmmio->lock);
>>> +
>>> +    BUG_ON(vmmio->num_entries >= vmmio->max_num_entries);
>>
>> (Do we really need to crash in such a case? Can't we just fail domain
>> creation?)
> 
> Generally, no. However, the approach used by Arm's dom0less solution is 
> to crash as soon as any issue occurs instead of trying to continue 
> running other domains, so I follow the same approach for RISC-V.
> 
> Even if I return an error here, the common dom0less code will panic anyway.

That's the policy there, but you're writing code here also for the case where
Dom0 creates domains.

Jan