Re: [PATCH v4 16/25] xen/riscv: introduce minimal virtual APLIC (vAPLIC) infrastructure

[Jan Beulich <jbeulich@xxxxxxxx>]

On 26.06.2026 17:46, Oleksii Kurochko wrote:
> Changes in v4:
>  - Change subject of the commit.
>  - s/APLIC_DOMAINCFG_RO80/APLIC_DOMAINCFG_RO + added a comment above 
> definition.
>  - Drop unnessary blank lines.

Did you? What about ...

> --- a/xen/arch/riscv/include/asm/aplic.h
> +++ b/xen/arch/riscv/include/asm/aplic.h
> @@ -15,6 +15,9 @@
>  
>  #include <asm/imsic.h>
>  
> +
> +/* domaincfg bits 31:24 are read-only 0x80 */
> +#define APLIC_DOMAINCFG_RO      (0x80U << 24)
>  #define APLIC_DOMAINCFG_IE      BIT(8, U)
>  #define APLIC_DOMAINCFG_DM      BIT(2, U)

... the one you pointlessly add here?

> --- a/xen/arch/riscv/include/asm/intc.h
> +++ b/xen/arch/riscv/include/asm/intc.h
> @@ -15,6 +15,7 @@ enum intc_version {
>  struct cpu_user_regs;
>  struct irq_desc;
>  struct kernel_info;
> +struct vcpu;
>  
>  struct intc_info {
>      enum intc_version hw_version;
> @@ -51,8 +52,17 @@ struct vintc_init_ops {
>      int (*make_domu_dt_node)(struct kernel_info *kinfo);
>  };
>  
> +struct vintc_ops {
> +    /* Initialize some vINTC-related stuff for a vCPU */
> +    int (*vcpu_init)(struct vcpu *v);
> +
> +    /* Deinitialize some vINTC-related stuff for a vCPU */
> +    void (*vcpu_deinit)(struct vcpu *v);
> +};
> +
>  struct vintc {
>      const struct vintc_init_ops *init_ops;
> +    const struct vintc_ops *ops;
>  };

I may have asked before: Why two distinct structures, next to each other
(and without any comment guiding what is to go where)? Plus you only set
...

> --- /dev/null
> +++ b/xen/arch/riscv/vaplic.c
> @@ -0,0 +1,63 @@
> +/* SPDX-License-Identifier: MIT */
> +/*
> + * xen/arch/riscv/vaplic.c
> + *
> + * Virtual RISC-V Advanced Platform-Level Interrupt Controller support
> + *
> + * Copyright (c) Microchip.
> + * Copyright (c) Vates
> + */
> +
> +#include <xen/errno.h>
> +#include <xen/sched.h>
> +#include <xen/xvmalloc.h>
> +
> +#include <asm/aia.h>
> +#include <asm/imsic.h>
> +#include <asm/intc.h>
> +#include <asm/vaplic.h>
> +
> +#include "aplic-priv.h"
> +
> +static int cf_check vaplic_init(struct vcpu *v)
> +{
> +    return vcpu_imsic_init(v);
> +}
> +
> +static void cf_check vaplic_deinit(struct vcpu *v)
> +{
> +    return vcpu_imsic_deinit(v);
> +}
> +
> +static const struct vintc_ops vintc_ops = {
> +    .vcpu_init = vaplic_init,
> +    .vcpu_deinit = vaplic_deinit,
> +};
> +
> +int domain_vaplic_init(struct domain *d)
> +{
> +    struct vaplic *vaplic = xvzalloc(struct vaplic);
> +
> +    if ( !vaplic )
> +        return -ENOMEM;
> +
> +    d->arch.vintc = &vaplic->vintc;
> +    d->arch.vintc->ops = &vintc_ops;
> +
> +    vaplic->regs.domaincfg = APLIC_DOMAINCFG_IE | APLIC_DOMAINCFG_DM |
> +                             APLIC_DOMAINCFG_RO;
> +
> +    return 0;
> +}

... ->ops here, leaving ->init_ops at NULL (prone to a deref that'll crash).

Jan