Re: [PATCH v7 11/20] xen/riscv: introduce per-vCPU IMSIC state

[Oleksii Kurochko <oleksii.kurochko@gmail.com>]

On 8/4/26 5:48 PM, Oleksii Kurochko wrote:
> Each vCPU interacting with the IMSIC requires state to track the
> associated guest interrupt file and its backing context.
> 
> Introduce a per-vCPU structure to hold IMSIC-related state, including
> the guest interrupt file identifier and the CPU providing the backing
> VS-file. Access to the guest file identifier is protected by a lock.
> 
> Initialize this structure during vCPU setup and store it in arch_vcpu.
> The initial state marks the VS-file as software-backed until it becomes
> associated with a physical CPU.
> 
> Add helper to retrieve the guest interrupt file identifier:
> - vcpu_guest_file_id() is going to be used during update of APLIC's
>    target register with the pair of information <guest_file_id, cpu_id>
>    (to have MSI delivery mode work properly) when guest is trying to
>    access vAPLIC's target register.
> It will be used in the follow up patches.
> 
> Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
> Acked-by: Jan Beulich <jbeulich@suse.com>
> ---
> Changes in v6-7:
>   - Nothing changed. Only rebase.
> ---
> Changes in v5:
>   - Move v->arch.vimsic_state = imsic_state; after full initialization of
>     the struct, so the pointer only becomes globally visible once all
>     fields are set up.
>   - Add Acked-by: Jan Beulich <jbeulich@suse.com>.
> ---
> Changes in v4:
> -  s/w vs h/w IMSIC VS-file commentary for struct vimsic_state:
>     - fix the vsfile_pcpu h/w condition:
>       "vsfile_pcpu >= 0" -> "vsfile_pcpu < NR_CPUS"
>       (the old wording conflicted with the s/w "== NR_CPUS" case).
>     - reorder both comment blocks to the "s/w ... / h/w ..." form for readability.
>   - drop IMPOSSIBLE_GUEST_FILE_ID: the s/w IMSIC VS-file is always available
>     and corresponds to guest_file_id == 0, which xvzalloc() already provides,
>     so the explicit initializer in vcpu_imsic_init() and the macro itself
>     are unneeded.
> ---
> Changes in v3:
>   - Drop const from imsic_set_guest_file_id() and vcpu_imsic_deinit() as
>     it only works due to vimsic_state being a pointer member.
>   - Use XVFREE() in vcpu_imsic_deinit() to make it idempotent.
>   - Fix SW-file typo in struct vimsic_state comments; should be VS-file.
>   - Drop imsic_set_guest_file_id() here, it will be added later when it
>     will be nessary to initialise guest file id as the correspondendt code
>     in this patch series was reworked and there is no need to use this
>     function in arch_vcpu_create().
>   - Introduce IMPOSSIBLE_GUEST_FILE_ID and init with it ->guest_file_id.
> ---
> Changes in v2:
>   - Rename imsic_state to vimsic_state.
>   - Use 'unsigned int' for vsfile_pcpu.
>   - Drop initialzation of ->guest_file_id as it will be by default zero.
>   - Add the comment about ->guest_file_id field.
>   - Drop __init for vcpu_imsic_init() as it could be used during post-boot
>     vCPU creation.
>   - Update the commit message.
>   - Drop locks around ->guest_file_id() in  vcpu_guest_file_id() and imsic_set_guest_file_id().
> ---
> ---
>   xen/arch/riscv/imsic.c              | 35 +++++++++++++++++++++++++++++
>   xen/arch/riscv/include/asm/domain.h |  2 ++
>   xen/arch/riscv/include/asm/imsic.h  | 22 ++++++++++++++++++
>   3 files changed, 59 insertions(+)
> 
> diff --git a/xen/arch/riscv/imsic.c b/xen/arch/riscv/imsic.c
> index f7b70a8da09e..5a5758e45dc2 100644
> --- a/xen/arch/riscv/imsic.c
> +++ b/xen/arch/riscv/imsic.c
> @@ -16,6 +16,7 @@
>   #include <xen/errno.h>
>   #include <xen/init.h>
>   #include <xen/macros.h>
> +#include <xen/sched.h>
>   #include <xen/smp.h>
>   #include <xen/spinlock.h>
>   #include <xen/xvmalloc.h>
> @@ -56,6 +57,11 @@ do {                            \
>       csr_clear(CSR_SIREG, v);    \
>   } while (0)
>   
> +unsigned int vcpu_guest_file_id(const struct vcpu *v)
> +{
> +    return ACCESS_ONCE(v->arch.vimsic_state->guest_file_id);
> +}
> +
>   void __init imsic_ids_local_delivery(bool enable)
>   {
>       if ( enable )
> @@ -312,6 +318,35 @@ static int imsic_parse_node(const struct dt_device_node *node,
>       return 0;
>   }
>   
> +int vcpu_imsic_init(struct vcpu *v)
> +{
> +    struct vimsic_state *imsic_state;
> +
> +    /* Allocate IMSIC context */
> +    imsic_state = xvzalloc(struct vimsic_state);
> +    if ( !imsic_state )
> +        return -ENOMEM;
> +
> +    /* Setup IMSIC context  */
> +    rwlock_init(&imsic_state->vsfile_lock);
> +
> +    /*
> +     * xvzalloc() already cleared the context, so guest_file_id == 0, i.e. the
> +     * always-available s/w IMSIC VS-file. Only vsfile_pcpu needs an explicit
> +     * initializer as its s/w VS-file value is NR_CPUS rather than 0.
> +     */
> +    imsic_state->vsfile_pcpu = NR_CPUS;
> +
Considering our conversation in another patch series vsfile_cpu would be 
better name. Don't you mind if I will change vsfile_pcpu -> vsfile_cpu 
and everywhere it is needed in this patch with saving of your Acked-by?

~ Oleksii