Re: [PATCH 01/19] xen/arm: Implement PSCI system suspend

[Julien Grall <julien@xxxxxxx>]

Hi,

On 08/10/2022 23:00, Volodymyr Babchuk wrote:
> > diff --git a/xen/arch/arm/suspend.c b/xen/arch/arm/suspend.c
> > new file mode 100644
> > index 0000000000..987ba6ac11
> > --- /dev/null
> > +++ b/xen/arch/arm/suspend.c
> > @@ -0,0 +1,182 @@
> > +/*
> > + * Copyright (C) 2022 EPAM Systems Inc.
Not related to Volodymyr's answer but I will reply here as I noticed it.

The code was written in 2018 by Aggios. So shouldn't this be a 2018 
copyright from Aggios? You can add yours on top if you want but it is 
not super clear what has been modified (a changelog after --- in the 
commit message would have been useful).
> > + *
> > + * This program is free software; you can redistribute it and/or modify
> > + * it under the terms of the GNU Lesser General Public License as published
> > + * by the Free Software Foundation; version 2.1 only. with the special
> > + * exception on linking described in file LICENSE.
> > + *
> > + * This program is distributed in the hope that it will be useful,
> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> > + * GNU Lesser General Public License for more details.
> > + */
We are not allowing LGPL* license in the hypervisor. This has to be 
GPLv2-only (this was the original contribution even if there were no 
copyright).
But it would be better to use an SPDX tag as it makes clearer which 
license is used (see [1]).
[...]

> > diff --git a/xen/common/sched/core.c b/xen/common/sched/core.c
> > index 8f4b1ca10d..4e1ea62c44 100644
> > --- a/xen/common/sched/core.c
> > +++ b/xen/common/sched/core.c
> > @@ -38,6 +38,10 @@
> >   #include <xsm/xsm.h>
> >   #include <xen/err.h>
> >   
> > +#if defined(CONFIG_ARM) || defined(CONFIG_ARM64)
> > +#include <asm/suspend.h>
> > +#endif
> > +
> >   #include "private.h"
> >   
> >   #ifdef CONFIG_XEN_GUEST
> > @@ -957,6 +961,10 @@ void vcpu_unblock(struct vcpu *v)
> >   {
> >       if ( !test_and_clear_bit(_VPF_blocked, &v->pause_flags) )
> >           return;
> > +#if defined(CONFIG_ARM) || defined(CONFIG_ARM64)
> > +    if ( test_bit(_VPF_suspended, &v->pause_flags) )
> > +        vcpu_resume(v);
> > +#endif
> This does not look good. I do understant that that was I, who suggested
> to add this flag, but I didn't expected that it will get into common code.
AFAIU, you are using the flag to indicate whether the vCPU should be 
reset. If I am not mistaken, this should only happen for vCPU0 (other 
vCPUs will be brought up using PSCI CPU ON). So you could reset the vCPU 
before blocking it.
With that, the flag should not be necessary (at least in this situation).

> Also, this is not justified in the commit message. I remeber that there was
> some discussion about why vcpu_block()/vcpu_unblock() could not be used, and
> I'd love to see its summary in the commit message.
Are you referring to the discussion in [2]?

Cheers,

[1] 20221008001544.78302-2-sstabellini@xxxxxxxxxx
[2] 1542022244-22977-3-git-send-email-mirela.simonovic@xxxxxxxxxx

Cheers,

--
Julien Grall