Re: [PATCH v1 11/17] xen/riscv: add vAPLIC state save/restore hooks

On 8/12/26 4:19 PM, Jan Beulich wrote:
> On 20.07.2026 18:02, Oleksii Kurochko wrote:
>> --- a/xen/arch/riscv/include/asm/vaplic.h
>> +++ b/xen/arch/riscv/include/asm/vaplic.h
>> @@ -34,4 +34,7 @@ struct vaplic {
>>   int domain_vaplic_init(struct domain *d);
>>   void domain_vaplic_deinit(struct domain *d);
>>   
>> +void vaplic_state_save(struct vcpu *v);
>> +void vaplic_state_restore(struct vcpu *v);
> 
> Why would these be needed? Can't ...
> 
>> --- a/xen/arch/riscv/vaplic.c
>> +++ b/xen/arch/riscv/vaplic.c
>> @@ -400,9 +400,27 @@ static const struct mmio_handler_ops vaplic_mmio_ops = {
>>       .write = vaplic_mmio_write,
>>   };
>>   
>> +void vaplic_state_save(struct vcpu *v)
> 
> ... both be static? 

They are only needed to cover potentially two cases (w/ MSI and w/o MSI 
support) but I see a sense two follow your suggestion below ...

> And don't they want to be cf_check?

Agree, cf_check should be used here.

> 
>> +{
>> +    if ( has_msi_support() )
>> +        imsic_state_save(v);
>> +    else
>> +        BUG_ON("unimplemented");
>> +}
>> +
>> +void vaplic_state_restore(struct vcpu *v)
>> +{
>> +    if ( has_msi_support() )
>> +        imsic_state_restore(v);
>> +    else
>> +        BUG_ON("unimplemented");
>> +}
> 
> If you're merely forwarding the calls, why can't ...
> 
>>   static const struct vintc_ops vintc_ops = {
>>       .vcpu_init = vaplic_init,
>>       .vcpu_deinit = vaplic_deinit,
>> +    .store_state = vaplic_state_save,
>> +    .restore_state = vaplic_state_restore,
> 
> ... imsic_state_{save,restore}() be used directly here? And whatever other
> pair of handlers for the case when it's not IMSIC?

... It could be done in that way. I will follow it.

Thanks.

~ Oleksii