Re: [PATCH v5 1/4] xen/arm: Implement PSCI SYSTEM_SUSPEND call for guests

[Mykola Kvach <xakep.amatop@xxxxxxxxx>]

Hi Jürgen,

On Mon, Jul 21, 2025 at 11:08 AM Jürgen Groß <jgross@xxxxxxxx> wrote:
>
> On 28.06.25 20:17, Julien Grall wrote:
> > Hi Mykola,
> >
> > On 27/06/2025 11:51, Mykola Kvach wrote:
> >> diff --git a/xen/arch/arm/include/asm/perfc_defn.h 
> >> b/xen/arch/arm/include/asm/
> >> perfc_defn.h
> >> index effd25b69e..8dfcac7e3b 100644
> >> --- a/xen/arch/arm/include/asm/perfc_defn.h
> >> +++ b/xen/arch/arm/include/asm/perfc_defn.h
> >> @@ -33,6 +33,7 @@ PERFCOUNTER(vpsci_system_reset,        "vpsci: 
> >> system_reset")
> >>   PERFCOUNTER(vpsci_cpu_suspend,         "vpsci: cpu_suspend")
> >>   PERFCOUNTER(vpsci_cpu_affinity_info,   "vpsci: cpu_affinity_info")
> >>   PERFCOUNTER(vpsci_features,            "vpsci: features")
> >> +PERFCOUNTER(vpsci_system_suspend,      "vpsci: system_suspend")
> >>   PERFCOUNTER(vcpu_kick,                 "vcpu: notify other vcpu")
> >> diff --git a/xen/arch/arm/include/asm/psci.h 
> >> b/xen/arch/arm/include/asm/psci.h
> >> index 4780972621..48a93e6b79 100644
> >> --- a/xen/arch/arm/include/asm/psci.h
> >> +++ b/xen/arch/arm/include/asm/psci.h
> >> @@ -47,10 +47,12 @@ void call_psci_system_reset(void);
> >>   #define PSCI_0_2_FN32_SYSTEM_OFF          PSCI_0_2_FN32(8)
> >>   #define PSCI_0_2_FN32_SYSTEM_RESET        PSCI_0_2_FN32(9)
> >>   #define PSCI_1_0_FN32_PSCI_FEATURES       PSCI_0_2_FN32(10)
> >> +#define PSCI_1_0_FN32_SYSTEM_SUSPEND      PSCI_0_2_FN32(14)
> >>   #define PSCI_0_2_FN64_CPU_SUSPEND         PSCI_0_2_FN64(1)
> >>   #define PSCI_0_2_FN64_CPU_ON              PSCI_0_2_FN64(3)
> >>   #define PSCI_0_2_FN64_AFFINITY_INFO       PSCI_0_2_FN64(4)
> >> +#define PSCI_1_0_FN64_SYSTEM_SUSPEND      PSCI_0_2_FN64(14)
> >>   /* PSCI v0.2 affinity level state returned by AFFINITY_INFO */
> >>   #define PSCI_0_2_AFFINITY_LEVEL_ON      0
> >> diff --git a/xen/arch/arm/include/asm/vpsci.h 
> >> b/xen/arch/arm/include/asm/vpsci.h
> >> index 0cca5e6830..69d40f9d7f 100644
> >> --- a/xen/arch/arm/include/asm/vpsci.h
> >> +++ b/xen/arch/arm/include/asm/vpsci.h
> >> @@ -23,7 +23,7 @@
> >>   #include <asm/psci.h>
> >>   /* Number of function implemented by virtual PSCI (only 0.2 or later) */
> >> -#define VPSCI_NR_FUNCS  12
> >> +#define VPSCI_NR_FUNCS  14
> >>   /* Functions handle PSCI calls from the guests */
> >>   bool do_vpsci_0_1_call(struct cpu_user_regs *regs, uint32_t fid);
> >> diff --git a/xen/arch/arm/mmu/p2m.c b/xen/arch/arm/mmu/p2m.c
> >> index 67296dabb5..f9c09a49e2 100644
> >> --- a/xen/arch/arm/mmu/p2m.c
> >> +++ b/xen/arch/arm/mmu/p2m.c
> >> @@ -6,6 +6,8 @@
> >>   #include <xen/sched.h>
> >>   #include <xen/softirq.h>
> >> +#include <public/sched.h>
> >> +
> >>   #include <asm/alternative.h>
> >>   #include <asm/event.h>
> >>   #include <asm/flushtlb.h>
> >> @@ -198,7 +200,9 @@ void dump_p2m_lookup(struct domain *d, paddr_t addr)
> >>    */
> >>   void p2m_save_state(struct vcpu *p)
> >>   {
> >> -    p->arch.sctlr = READ_SYSREG(SCTLR_EL1);
> >> +    if ( !(p->domain->is_shutting_down &&
> >> +           p->domain->shutdown_code == SHUTDOWN_suspend) )
> >
> > This is definitely not obvious why you need to change p2m_save_state(). 
> > AFAIU,
> > you are doing this because when suspending the system, you will overwrite p-
> >  >arch.sctlr. However, this is super fragile. For instance, you still seem 
> > to
> > overwrite TTBR{0,1} and TTBCR.
> >
> > TTBR{0,1} are technically unknown at boot. So it is fine to ignore them.  
> > But
> > for TTBCR, I am not 100% whether this is supposed to be unknown.
> >
> > Anyway, adding more "if (...)" is not the right solution because in the 
> > future
> > we may decide to reset more registers.
> >
> > It would be better if we stash the value sand then update the registers. 
> > Another
> > possibility would be to entirely skip the save path for CPUs that are 
> > turned off
> > (after all this is a bit useless work...).
> >
> >> +static int do_common_cpu_on(register_t target_cpu, register_t entry_point,
> >> +                            register_t context_id)
> >> +{
> >> +    int rc;
> >> +    struct vcpu *v;
> >> +    struct domain *d = current->domain;
> >> +    register_t vcpuid;
> >> +
> >> +    vcpuid = vaffinity_to_vcpuid(target_cpu);
> >> +
> >> +    if ( (v = domain_vcpu(d, vcpuid)) == NULL )
> >> +        return PSCI_INVALID_PARAMETERS;
> >> +
> >> +    if ( !test_bit(_VPF_down, &v->pause_flags) )
> >> +        return PSCI_ALREADY_ON;
> >> +
> >> +    rc = do_setup_vcpu_ctx(v, entry_point, context_id);
> >> +    if ( rc == PSCI_SUCCESS )
> >> +        vcpu_wake(v);
> >> +
> >> +    return rc;
> >> +}
> >> +
> >>   static int32_t do_psci_cpu_on(uint32_t vcpuid, register_t entry_point)
> >>   {
> >>       int32_t ret;
> >> @@ -197,6 +208,52 @@ static void do_psci_0_2_system_reset(void)
> >>       domain_shutdown(d,SHUTDOWN_reboot);
> >>   }
> >> +static void do_resume_on_error(struct domain *d)
> >
> > This looks like an open-coding version of domain_resume() without the
> > domain_{,un}pause(). What worries me is there is a comment on top of
> > domain_pause() explaining why it is called. I understand, we can't call
> > domain_pause() here (it doesn't work on the current domain). However, it 
> > feels
> > to me there is an explanation necessary why this is fine to diverge.
> >
> > I am not a scheduler expert, so I am CCing Juergen in the hope he could 
> > provide
> > some inputs.
>
> I don't think the reason for domain_[un]pause() is directly scheduling
> related.
>
> It seems to be x86 specific for shadow page table handling.
>
> In any case I'd suggest to split domain_resume() into 2 functions, one
> of them (e.g. domain_resume_nopause()) replacing do_resume_on_error()
> and domain_resume() just pausing the domain, calling the new function,
> and then unpausing the domain again.

Got it — thank you for the review!

>
> Another option might be to move the suspend action into a tasklet, enabling
> to call domain_resume() directly if needed. I didn't check whether other
> constraints even allow this idea, though.
>
>
> Juergen

Best regards,
Mykola