Re: [PATCH v2 15/39] xen/riscv: add IMSIC vCPU context switch handlers

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

On 9/4/26 1:33 PM, Baptiste Le Duc wrote:
>> IMSIC state currently needs to track only which physical CPU owns a vCPU's
>> IMSIC guest interrupt file, as the CPU id is part of the physical address
>> the file is mapped at.
>>
>> Add imsic_ctxt_switch_from() to record that CPU when a vCPU is switched
>> out. A vCPU running on the s/w VS-file has no h/w file bound to a CPU, so
>> there is nothing to record for it. The recorded value stays unused until
>> vCPU migration support, which needs it to find the file to move away from,
>> is added later.
>>
>> imsic_ctxt_switch_to() has nothing to do: by the time a vCPU is switched
>> in, VGEIN is already assigned to it and its guest interrupt file is already
>> mapped. Work is only required once a vCPU can move to a different CPU,
>> which means recalculating VGEIN and remapping the file; that is handled
>> separately by the vCPU migration patches.
>>
>> Install both as the ctxt_switch_{from,to} hooks of struct vintc_ops. MSI
>> delivery is the only mode Xen supports ( aplic_init() panics on an APLIC
>> without an "msi-parent" property, and a guest's domaincfg.DM reads back as
>> a fixed one) so the vAPLIC state to save and restore is always the IMSIC
>> one and no vAPLIC-level forwarder is needed. Being indirect call targets,
>> both handlers get cf_check.
>>
>> Co-developed-by: Romain Caritey <Romain.Caritey@microchip.com>
>> Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
>>
>> diff --git a/xen/arch/riscv/imsic.c b/xen/arch/riscv/imsic.c
>> index ad0a220eda..3787f270d8 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>
>> @@ -342,6 +343,28 @@ static int __init imsic_parse_node(const struct dt_device_node *node,
>>       return 0;
>>   }
>>   
>> +void cf_check imsic_ctxt_switch_from(struct vcpu *v)
>> +{
>> +    struct vimsic_state *imsic_state = v->arch.vimsic_state;
>> +    unsigned long flags;
>> +
>> +    /*
>> +     * A vCPU using the s/w IMSIC VS-file (guest_file_id == 0) has no h/w
>> +     * VS-file bound to a physical CPU, so there is no location to record.
>> +     */
>> +    if ( !vcpu_guest_file_id(v) )
>> +        return;
>> +
>> +    write_lock_irqsave(&imsic_state->vsfile_lock, flags);
>> +    imsic_state->vsfile_cpu = v->processor;
>> +    write_unlock_irqrestore(&imsic_state->vsfile_lock, flags);
>> +}
>> +
>> +void cf_check imsic_ctxt_switch_to(struct vcpu *v)
>> +{
>> +    /* Nothing to do */
>> +}
>> +
>>   int cf_check vcpu_imsic_init(struct vcpu *v)
>>   {
>>       struct vimsic_state *imsic_state;
>> diff --git a/xen/arch/riscv/include/asm/imsic.h b/xen/arch/riscv/include/asm/imsic.h
>> index 93f9e44c7d..73129c3c9e 100644
>> --- a/xen/arch/riscv/include/asm/imsic.h
>> +++ b/xen/arch/riscv/include/asm/imsic.h
>> @@ -109,4 +109,7 @@ unsigned int vcpu_guest_file_id(const struct vcpu *v);
>>   
>>   int vimsic_make_domu_dt_node(struct kernel_info *kinfo, unsigned int *phandle);
>>   
>> +void imsic_ctxt_switch_from(struct vcpu *v);
>> +void imsic_ctxt_switch_to(struct vcpu *v);
>> +
>>   #endif /* ASM_RISCV_IMSIC_H */
>> diff --git a/xen/arch/riscv/vaplic.c b/xen/arch/riscv/vaplic.c
>> index 8726f7203d..6c60fe2baf 100644
>> --- a/xen/arch/riscv/vaplic.c
>> +++ b/xen/arch/riscv/vaplic.c
>> @@ -422,6 +422,13 @@ static const struct mmio_handler_ops vaplic_mmio_ops = {
>>   static const struct vintc_ops vintc_ops = {
>>       .vcpu_init = vcpu_imsic_init,
>>       .vcpu_deinit = vcpu_imsic_deinit,
>> +    /*
>> +     * MSI delivery is the only supported mode: aplic_init() panics on an
>> +     * APLIC without an "msi-parent", so the vAPLIC state to save and restore
>> +     * is always the IMSIC one.
>> +     */
>> +    .ctxt_switch_from = imsic_ctxt_switch_from,
>> +    .ctxt_switch_to = imsic_ctxt_switch_to,
> 
> Maybe this patch could be merged with previous one: patch 50fe9554e1c0 ("xen/riscv: introduce vintc_ctxt_switch_{from,to}()")

At some point I agree but I think I prefer a little bit to separate 
them. It is safe to have them separately as context switch isn't 
happening between these patches so any problem will occur. What I 
intented to do is that to separately introduce abstraction and 
separately introduce users of this abstraction.

~ Oleksii