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

[Jürgen Groß <jgross@xxxxxxxx>]

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.

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

Attachment: OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key

Attachment: OpenPGP_signature.asc
Description: OpenPGP digital signature