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

[Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>]

On 6/29/26 5:02 PM, Jan Beulich wrote:
> 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?
I think it is newly introduced... Anyway, I will drop it.

> > --- 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)? 
I will add the comments:

    /* Used during domain build only; dropped afterwards. */
    const struct vintc_init_ops *init_ops;
    /* Runtime callbacks used for the lifetime of the guest. */
    const struct vintc_ops *ops;

> 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).
It is init-ed in [PATCH v4 20/25] xen/riscv: create APLIC DT node for 
guest domains and nothing between this patches could use ->ops. I will 
update the commit message to point that.
Thanks.

~ Oleksii