Re: [PATCH v1 07/17] xen/riscv: introduce vCPU AIA initialization

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

> Introduce vcpu_aia_init() to initialize the AIA-related state needed
> for a vCPU to have a working guest interrupt file.
> 
> A guest (VS) interrupt file must be mapped to one of a pCPU's
> hardware interrupt files (if they exist), so the pCPU a vCPU will actually
> run on needs to be known first. arch_vcpu_create() is therefore not a
> suitable place to call vcpu_aia_init(), since the pCPU assigned to a
> vCPU can still change before it is first scheduled. To avoid
> reassigning the VS interrupt file id and remapping it to a different
> pCPU's hardware interrupt file, vcpu_aia_init() will instead be
> called from a later point in the scheduling path (e.g.
> continue_to_new_vcpu()), to be introduced in a follow-up patch. Since
> it will end up being called from a non-__init context, it is not
> itself marked __init.


> 
> Introduce imsic_update_state() to update a vCPU's guest IMSIC state
> (the guest interrupt file id and the pCPU whose hardware interrupt
> file it is mapped to) as a single consistent unit. This state can be
> read concurrently, e.g. by a future helper that checks whether a
> vCPU has a pending IMSIC interrupt, though no such consumer exists
> yet at this stage - so it is protected by a lock.
> 
> Signed-off-by: Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>
>
> diff --git a/xen/arch/riscv/aia.c b/xen/arch/riscv/aia.c
> index 4f7f46f58f..ed19600d46 100644
> --- a/xen/arch/riscv/aia.c
> +++ b/xen/arch/riscv/aia.c
> @@ -14,6 +14,7 @@
>  #include <asm/cpufeature.h>
>  #include <asm/csr.h>
>  #include <asm/current.h>
> +#include <asm/imsic.h>
>  
>  struct vgein_ctrl {
>      unsigned long bmp;
> @@ -36,6 +37,35 @@ bool aia_usable(void)
>      return _aia_usable;
>  }
>  
> +void vcpu_aia_init(struct vcpu *v)
> +{
> +    unsigned int new_vsfile_id;
> +    int rc;
> +
> +    if ( !aia_usable() )
> +        return;
> +
> +    new_vsfile_id = vgein_assign(v);
> +
> +    /*
> +     * vgein_assign() returns 0 when no free h/w guest interrupt file is
> +     * available (including GEILEN == 0); imsic_map_guest_file() maps nothing
> +     * in that case.
> +     */
> +    rc = imsic_map_guest_file(v, new_vsfile_id);
> +    if ( rc )
> +    {
> +        /* Can't continue w/o correctly mapped IMSIC interrupt file */
> +        domain_crash(v->domain);

The vgein id assigned a few lines up is not released here. The domain is dying
anyway, but the guest interrupt file stays marked in use on that pCPU forever,
since nothing else ever calls vgein_release(). A vgein_release(v,
new_vsfile_id) before the domain_crash() would fix it.

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