Re: [PATCH v1 08/17] xen/riscv: add IMSIC state save/restore

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

> IMSIC state is currently needed only to track which physical CPU owns a
> vCPU's IMSIC interrupt file. This is required because the physical CPU
> ID is part of the physical address used to map the IMSIC file.
> 
> Add imsic_state_save() to record the current pCPU for a vCPU. When the
> vCPU is migrated to a different pCPU, the mapping will need to be updated.
> 
> When imsic_state_restore() is called, VGEIN is already assigned to the
> vCPU and the guest interrupt file is already mapped, and, as only h/w
> interrupt files are used for now, nothing specific needs to be done.
> Action is only required when the vCPU is moved to a different pCPU, which
> requires recalculating VGEIN and the mapping for the new guest interrupt
> file. That will be handled separately by vcpu_move_irqs(), which is
> introduced in a follow-up patch; until then this case is guarded by a
> BUG_ON(), which is fine.
> 
> Co-developed-by: Romain Caritey <Romain.Caritey@xxxxxxxxxxxxx>
> Signed-off-by: Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>
>
> diff --git a/xen/arch/riscv/imsic.c b/xen/arch/riscv/imsic.c
> index 2a792e756c..406bc68cbc 100644
> --- a/xen/arch/riscv/imsic.c
> +++ b/xen/arch/riscv/imsic.c
> @@ -20,6 +20,7 @@
>  #include <xen/init.h>
>  #include <xen/libfdt/libfdt.h>
>  #include <xen/macros.h>
> +#include <xen/rwlock.h>
>  #include <xen/sched.h>
>  #include <xen/smp.h>
>  #include <xen/spinlock.h>
> @@ -418,6 +419,28 @@ int imsic_map_guest_file(struct vcpu *v, unsigned int 
> vsfile_id)
>      return res;
>  }
>  
> +void imsic_state_save(struct vcpu *v)
> +{
> +    struct vimsic_state *imsic_state = v->arch.vimsic_state;
> +    unsigned long flags;
> +
> +    /*
> +     * SW interrupt file always has ->vsfile_pcpu = NR_CPUS so nothing 
> specific
> +     * should be done in this case.
> +     */
> +    if ( !vcpu_guest_file_id(v) )
> +        return;


> +
> +    write_lock_irqsave(&imsic_state->vsfile_lock, flags);
> +    imsic_state->vsfile_pcpu = cpuid_to_hartid(v->processor);

How will you detect a migration is needed? Don't you need to first know
if ->vsfile_pcpu is different to cpuid_to_hartid(v->processor)? (I
didn't take a look to other patchs for the moment, so the
explanations might be later.)

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