Re: [PATCH v1 02/17] xen/riscv: add basic VGEIN management for AIA guests

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

> It was decided to add support for IMSIC from the start instead of having APLIC
> operate in direct delivery mode, as it requires a trap-and-emulation approach,
> which is not optimal from a performance standpoint.
> 
> AIA provides a hardware-accelerated mechanism for delivering external
> interrupts to domains via "guest interrupt files" located in IMSIC.
> A single physical hart can implement multiple such files (up to GEILEN),
> allowing several virtual harts to receive interrupts directly from hardware.
> 
> Introduce per-CPU tracking of guest interrupt file identifiers (VGEIN)
> for systems implementing AIA specification. Each CPU maintains
> a bitmap describing which guest interrupt files are currently in use.
> 
> Add helpers to initialize the bitmap based on the number of available
> guest interrupt files (GEILEN), assign a VGEIN to a vCPU, and release it
> when no longer needed. When assigning a VGEIN, the corresponding value
> is written to the VGEIN field of the guest hstatus register so that
> VS-level external interrupts are delivered from the selected interrupt
> file.
> 
> Signed-off-by: Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>
>
> diff --git a/xen/arch/riscv/aia.c b/xen/arch/riscv/aia.c
> index e31c9c2d24..4f7f46f58f 100644
> --- a/xen/arch/riscv/aia.c
> +++ b/xen/arch/riscv/aia.c
> @@ -1,11 +1,33 @@
>  /* SPDX-License-Identifier: GPL-2.0-only */
>  
> +#include <xen/bitmap.h>
> +#include <xen/cpu.h>
>  #include <xen/errno.h>
>  #include <xen/init.h>

Add a #include <xen/percpu.h> here instead of in aia.h.

>  #include <xen/sections.h>
> +#include <xen/sched.h>
> +#include <xen/spinlock.h>
>  #include <xen/types.h>
> +#include <xen/xvmalloc.h>
>  
> +#include <asm/aia.h>
>  #include <asm/cpufeature.h>
> +#include <asm/csr.h>
> +#include <asm/current.h>
> +
> +struct vgein_ctrl {
> +    unsigned long bmp;
> +    spinlock_t lock;
> +    struct vcpu **owners;
> +    /* The least-significant bits are implemented first, apart from bit 0 */
> +    unsigned int geilen;
> +};
> +
> +/*
> + * VGEIN control structure for each physical CPU to track which VS (guest)
> + * interrupt file IDs are in use.
> + */
> +static DEFINE_PER_CPU(struct vgein_ctrl, vgein);
>  
>  static bool __ro_after_init _aia_usable;
>  
> @@ -14,10 +36,133 @@ bool aia_usable(void)
>      return _aia_usable;
>  }
>  
> +static int vgein_init(unsigned int cpu)

Could we call this function with a different cpu arg than the current
one running? If yes, we would read hgeie of not the cpu we wanted.
> +{
> +    struct vgein_ctrl *vgein = &per_cpu(vgein, cpu);
> +
> +    csr_write(CSR_HGEIE, -1UL);
> +    vgein->geilen = flsl(csr_read(CSR_HGEIE) >> 1);
> +    csr_write(CSR_HGEIE, 0);
> +
> +    printk("cpu%u.geilen=%u\n", cpu, vgein->geilen);

> +
> +    if ( !vgein->geilen )
> +        return -EOPNOTSUPP;
> +
> +    vgein->owners = xvzalloc_array(struct vcpu *, vgein->geilen);
> +    if ( !vgein->owners )
> +        return -ENOMEM;
> +
> +    spin_lock_init(&vgein->lock);
> +
> +    return 0;
> +}
> +
> +static int cf_check cpu_callback(struct notifier_block *nfb, unsigned long 
> action,
> +                        void *hcpu)
> +{
> +    unsigned int cpu = (unsigned long)hcpu;
> +    int rc = 0;
> +
> +    switch ( action )
> +    {
> +    case CPU_STARTING:
> +        rc = vgein_init(cpu);
> +        if ( rc )
> +            printk("AIA: failed to init vgein for CPU%u\n", cpu);
> +        break;
> +    }
> +
> +    return notifier_from_errno(rc);
> +}
> +
> +static struct notifier_block cpu_nfb = {
> +    .notifier_call = cpu_callback,
> +};
> +
>  void __init aia_init(void)
>  {
> +    int rc;
> +
>      if ( !riscv_isa_extension_available(NULL, RISCV_ISA_EXT_ssaia) )
> +    {
> +        dprintk(XENLOG_WARNING, "SSAIA isn't present in riscv,isa\n");
>          return;
> +    }
> +
> +    if ( (rc = vgein_init(0)) )

Why `0` rather than smp_processor_id()? As described above vgein_init() reads 
CSR_HGEIE
of the current hart but stores the result into per_cpu(vgein, cpu), so the two
must agree.

> +    {
> +        dprintk(XENLOG_ERR, "vgein_init() failed: %d\n", rc);
> +        return;
> +    }
>  
>      _aia_usable = true;
> +
> +    register_cpu_notifier(&cpu_nfb);
> +}
> +
> +unsigned int vgein_assign(struct vcpu *v)
> +{
> +    unsigned int vgein_id;
> +    struct vgein_ctrl *vgein = &per_cpu(vgein, v->processor);

What happens if v->processor change between vgein_assign() and
vgein_release? Because it seems in such case the release will hit a
different pCPU's bitmap: the original bit will leak and an unrelated
CPU's bit will be cleared under another vCPU's feet.

> +    unsigned long *bmp = &vgein->bmp;
> +    unsigned long flags;
> +
> +    if ( !vgein->geilen )
> +        return 0;
> +
> +    spin_lock_irqsave(&vgein->lock, flags);
> +    /*
> +     * The vgein_id shouldn't be zero, as it will indicate that no guest
> +     * external interrupt source is selected for VS-level external interrupts
> +     * according to RISC-V privileged spec:
> +     *   Hypervisor Status Register (hstatus) in RISC-V privileged spec:
> +     *
> +     *   The VGEIN (Virtual Guest External Interrupt Number) field selects
> +     *   a guest external interrupt source for VS-level external interrupts.
> +     *   VGEIN is a WLRL field that must be able to hold values between zero
> +     *   and the maximum guest external interrupt number (known as GEILEN),
> +     *   inclusive.
> +     *   When VGEIN=0, no guest external interrupt source is selected for
> +     *   VS-level external interrupts.
> +     *
> +     * So start to search from bit number 1.
> +     */
> +    vgein_id = find_next_zero_bit(bmp, vgein->geilen + 1, 1);
> +
> +    if ( vgein_id > vgein->geilen )
> +        vgein_id = 0;
> +    else
> +    {

Potential index error, because above you did:

    vgein->owners = xvzalloc_array(struct vcpu*, vgein->geilen)

so valid index are 0...(vgein->geilen-1). Adopt either
one of those two options:
    1. vgein->owners[vgein_id-1] = v
    2. vgein->owners = xvzalloc_array(struct vcpu *, vgein->geilen+1) in
       vgein_init()

I think `2` could be better to have vgein->owners replicated hgeie CSR but
it would left the first entry read-only.

> +        __set_bit(vgein_id, bmp);
> +        vgein->owners[vgein_id] = v;
> +    }
> +
> +    spin_unlock_irqrestore(&vgein->lock, flags);
> +
> +#ifdef VGEIN_DEBUG

VGEIN_DEBUG is not defined anywhere in the patch, please use
gdprintk(XENLOG_DEBUG, ...) directly, or drop this branch.

> +    gprintk(XENLOG_DEBUG, "%s: %pv: vgein_id(%u), xen_cpu%u_bmp=%#lx\n",
> +           __func__, v, vgein_id, v->processor, *bmp);
> +#endif
> +
> +    return vgein_id;
> +}
> +
> +void vgein_release(struct vcpu *v, unsigned int vgein_id)
> +{
> +    unsigned long flags;
> +    struct vgein_ctrl *vgein = &per_cpu(vgein, v->processor);
> +
> +    if ( !vgein_id )
> +        return;
> +
> +    spin_lock_irqsave(&vgein->lock, flags);
> +    __clear_bit(vgein_id, &vgein->bmp);
> +    vgein->owners[vgein_id] = NULL;
> +    spin_unlock_irqrestore(&vgein->lock, flags);
> +
> +#ifdef VGEIN_DEBUG
> +    gprintk(XENLOG_DEBUG, "%s: vgein_id(%u), xen_cpu%u_bmp=%#lx\n",
> +           __func__, vgein_id, v->processor, vgein->bmp);
> +#endif
>  }
> diff --git a/xen/arch/riscv/include/asm/aia.h 
> b/xen/arch/riscv/include/asm/aia.h
> index aaa4bf91fc..c67be0069a 100644
> --- a/xen/arch/riscv/include/asm/aia.h
> +++ b/xen/arch/riscv/include/asm/aia.h
> @@ -3,8 +3,16 @@
>  #ifndef RISCV_AIA_H
>  #define RISCV_AIA_H
>  
> +#include <xen/percpu.h>

asm/aia.h needs neither <xen/percpu.h> nor <xen/spinlock.h> as struct
vgein_ctrl and the per-CPU variable both live in aia.c. Please drop them
and add <xen/percpu.h> in aia.c

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


--
Baptiste Le Duc | Vates XCP-ng Intern

XCP-ng & Xen Orchestra - Vates solutions

web: https://vates.tech