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

[Baptiste Le Duc <baptiste.le-duc@xxxxxxxxxx>]

> RISC-V guests can expose several virtual interrupt controllers at
> distinct GPA ranges: vPLIC (hasn't been introduced yet) for legacy machines,
> vAPLIC and vIMSIC for AIA-compliant ones (are being introduced in the follow
> up patches). 
As Jan said here [1], we shouldn't use "as later in this series" in
commit message...

[1]: 
https://lore.kernel.org/xen-devel/cover.1787838835.git.oleksii.kurochko@xxxxxxxxx/T/#m56fbac1ceb0642d5d868e9dcb2b5ed93ecbe5058
> Routing MMIO faults via a per-device is_access() check in the
> trap handler would couple it to every device it must serve, requiring a
> new conditional branch in the fault path each time a new emulated device is
> added.
> 
> Introduce a per-domain MMIO handler registration table, modeled
> after the equivalent ARM framework, so that virtual devices
> self-register their GPA ranges and read/write callbacks at domain
> creation time. The MMIO fault path delegates to a single
> try_handle_mmio() entry point and remains agnostic of which device
> owns a particular address.
> 
> A subsequent patch wires this into the MMIO fault path in traps.c.
...same here
> 
> Signed-off-by: Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>
>
> diff --git a/xen/arch/riscv/Makefile b/xen/arch/riscv/Makefile
> index 3b948c11dd..ce6410a299 100644
> --- a/xen/arch/riscv/Makefile
> +++ b/xen/arch/riscv/Makefile
> @@ -14,6 +14,7 @@ obj-y += intc.o
>  obj-y += irq.o
>  obj-y += kernel.init.o
>  obj-y += mm.o
> +obj-y += mmio.o
>  obj-y += p2m.o
>  obj-y += paging.o
>  obj-y += pt.o
> diff --git a/xen/arch/riscv/domain.c b/xen/arch/riscv/domain.c
> index 57c37cb2df..ec327a5e8a 100644
> --- a/xen/arch/riscv/domain.c
> +++ b/xen/arch/riscv/domain.c
> @@ -12,6 +12,7 @@
>  #include <asm/cpufeature.h>
>  #include <asm/csr.h>
>  #include <asm/intc.h>
> +#include <asm/mmio.h>
>  #include <asm/riscv_encoding.h>
>  #include <asm/vtimer.h>
>  
> @@ -316,6 +317,8 @@ int arch_domain_create(struct domain *d,
>      if ( (rc = p2m_init(d, config)) != 0)
>          goto fail;
>  
> +    domain_io_init(d);
> +
>      if ( (rc = domain_vintc_init(d)) )
>          goto fail;
>  
> diff --git a/xen/arch/riscv/include/asm/domain.h 
> b/xen/arch/riscv/include/asm/domain.h
> index e035b33ddf..15e8fa1968 100644
> --- a/xen/arch/riscv/include/asm/domain.h
> +++ b/xen/arch/riscv/include/asm/domain.h
> @@ -9,6 +9,7 @@
>  
>  #include <asm/cpufeature.h>
>  #include <asm/guest-layout.h>
> +#include <asm/mmio.h>
>  #include <asm/p2m.h>
>  #include <asm/vtimer.h>
>  
> @@ -101,6 +102,8 @@ struct arch_domain {
>      const unsigned long *isa;
>  
>      struct vintc *vintc;
> +
> +    struct vmmio vmmio;
>  };
>  
>  #include <xen/sched.h>
> diff --git a/xen/arch/riscv/include/asm/mmio.h 
> b/xen/arch/riscv/include/asm/mmio.h
> new file mode 100644
> index 0000000000..582969e535
> --- /dev/null
> +++ b/xen/arch/riscv/include/asm/mmio.h
> @@ -0,0 +1,63 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +#ifndef RISCV_MMIO_H
> +#define RISCV_MMIO_H
> +
> +#include <xen/lib.h>
> +#include <xen/rwlock.h>
> +
> +struct domain;
> +struct vcpu;
> +
> +#define MAX_IO_HANDLER  16
> +
> +typedef struct {
> +    paddr_t gpa;
> +    unsigned int len;  /* access width in bytes (1, 2, 4, 8) */
> +    bool is_write;
> +    /* store: value to write; load: value read (set by handler) */
> +    register_t data;
> +} mmio_info_t;
> +
> +enum io_state
> +{
> +    IO_ABORT,       /* The IO was handled and led to an abort. */
> +    IO_HANDLED,     /* The IO was successfully handled. */
> +    IO_UNHANDLED,   /* No handler found for the IO. */
> +};
> +
> +typedef enum io_state (mmio_read_t)(struct vcpu *v, mmio_info_t *info);
> +typedef enum io_state (mmio_write_t)(struct vcpu *v, const mmio_info_t 
> *info);
> +
> +struct mmio_handler_ops {
> +    mmio_read_t *read;
> +    mmio_write_t *write;
> +};
> +
> +struct mmio_handler {
> +    paddr_t addr;
> +    paddr_t size;
> +    const struct mmio_handler_ops *ops;
> +};
> +
> +struct vmmio {
> +    unsigned int num_entries;
> +    rwlock_t lock;
> +    struct mmio_handler handlers[MAX_IO_HANDLER];
> +};
> +
> +int do_mmio(mmio_info_t *info, paddr_t fault_addr, unsigned int len);
> +int register_mmio_handler(struct domain *d,
> +                          const struct mmio_handler_ops *ops,
> +                          paddr_t addr, paddr_t size);
> +void domain_io_init(struct domain *d);
> +
> +#endif /* RISCV_MMIO_H */
> +
> +/*
> + * Local variables:
> + * mode: C
> + * c-file-style: "BSD"
> + * c-basic-offset: 4
> + * indent-tabs-mode: nil
> + * End:
> + */
> diff --git a/xen/arch/riscv/mmio.c b/xen/arch/riscv/mmio.c
> new file mode 100644
> index 0000000000..d241ab5ea1
> --- /dev/null
> +++ b/xen/arch/riscv/mmio.c
> @@ -0,0 +1,176 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +
> +#include <xen/bsearch.h>
> +#include <xen/lib.h>
> +#include <xen/rwlock.h>
> +#include <xen/sched.h>
> +#include <xen/string.h>
> +
> +#include <asm/current.h>
> +#include <asm/mmio.h>
> +
> +/*
> + * bsearch() comparator: @key holds the address to look up in its addr field,
> + * @elem is an entry of vmmio->handlers. Relies on the regions not
> + * overlapping, which register_mmio_handler() enforces.
> + */
> +static int cmp_mmio_handler(const void *key, const void *elem)
> +{
> +    const struct mmio_handler *handler0 = key;
> +    const struct mmio_handler *handler1 = elem;
> +
> +    if ( handler0->addr < handler1->addr )
> +        return -1;
> +
> +    if ( handler0->addr >= (handler1->addr + handler1->size) )
> +        return 1;
> +
> +    return 0;
> +}
> +
> +/*
> + * Return a copy of the matching handler rather than a pointer into
> + * vmmio->handlers: a concurrent register_mmio_handler() shifts entries
> + * up to keep the array sorted, 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);
> +    if ( handler )
> +        *out = *handler;
> +    read_unlock(&vmmio->lock);
> +
> +    return handler != NULL;
> +}
> +
> +static enum io_state try_handle_mmio(mmio_info_t *info)
> +{
> +    struct vcpu *v = current;
> +    struct mmio_handler handler = {};
> +
> +    if ( !find_mmio_handler(v->domain, info->gpa, &handler) )
> +        return IO_UNHANDLED;
> +
> +    if ( info->is_write )
> +        return handler.ops->write(v, info);
> +    else
> +        return handler.ops->read(v, info);
> +}
> +
> +/*
> + * 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;
> +
> +    info->gpa = fault_addr;
> +    info->len = len;
> +
> +    switch ( try_handle_mmio(info) )
> +    {
> +    case IO_HANDLED:
> +        return 0;
> +
> +    case IO_ABORT:
> +        return -EIO;
> +
> +    default:
> +        return -EOPNOTSUPP;
> +    }
> +}
> +
> +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;
Just a question: is the aim of end < addr check to handle possible overflow of 
end?

-- 
Baptiste Le Duc <baptiste.le-duc@xxxxxxxxxx>