Re: [PATCH v1 04/17] 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 (is being introduced in the follow
> up patches). 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.
> 
> Subsequent patches wire this into arch_domain_create() and the MMIO fault
> path in traps.c.
> 
> Signed-off-by: Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>
> Reviewed-by: Baptiste Le Duc <baptiste.le-duc@xxxxxxxxxx>
>
> diff --git a/xen/arch/riscv/Makefile b/xen/arch/riscv/Makefile
> index 046f73f4d8..c452ebc3cf 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 4db9c28662..1e6f0ef66c 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>
>  
> @@ -308,6 +309,9 @@ int arch_domain_create(struct domain *d,
>      if ( (rc = p2m_init(d, config)) != 0)
>          goto fail;
>  
> +    if ( (rc = domain_io_init(d, MAX_IO_HANDLER)) != 0 )


> +        goto fail;


> +
>      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..18df1133e6
> --- /dev/null
> +++ b/xen/arch/riscv/include/asm/mmio.h
> @@ -0,0 +1,63 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */

According to coding style, it should be GPL-2.0-only.

> +#ifndef RISCV_MMIO_H
> +#define RISCV_MMIO_H
> +
> +#include <xen/lib.h>
> +#include <xen/rwlock.h>
> +
> +#define MAX_IO_HANDLER  16
> +
> +typedef struct {
> +    paddr_t gpa;
> +    unsigned int len;  /* access width in bytes (1, 2, 4, 8) */
> +    bool is_write;
> +    register_t data;   /* store: value to write; load: value read (set by 
> handler) */

Nit: line too long (85)

> +} 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,
> +                                     register_t *r);
> +typedef enum io_state (*mmio_write_t)(struct vcpu *v, mmio_info_t *info,
> +                                      register_t r);


> +
> +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;
> +    unsigned int max_num_entries;
> +    rwlock_t lock;
> +    struct mmio_handler *handlers;


> +};
> +
> +enum io_state try_handle_mmio(mmio_info_t *info);

> +void register_mmio_handler(struct domain *d,
> +                           const struct mmio_handler_ops *ops,
> +                           paddr_t addr, paddr_t size);
> +int domain_io_init(struct domain *d, unsigned int max_count);
> +void domain_io_free(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..7d56bc8b27
> --- /dev/null
> +++ b/xen/arch/riscv/mmio.c
> @@ -0,0 +1,145 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
Should be GPL-2.0-only.
> +/*
> + * Copyright (C) Vates
> + */
Why have you included a copyright notice here, but not in the other
files? I don’t know if you can keep it, but I just wanted to point out
that there are other files where this type of copyright notice includes
the year.

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


--
Baptiste Le Duc | Vates XCP-ng Intern

XCP-ng & Xen Orchestra - Vates solutions

web: https://vates.tech