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

[Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>]

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