Re: [Xen-devel] [PATCH v7 10/11] arm: vsmc: remove 64 bit mode check in PSCI handler

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Fri, 6 Oct 2017, Julien Grall wrote:
> Hi Volodymyr,
> 
> On 04/10/17 22:00, Volodymyr Babchuk wrote:
> > PSCI handling code had helper routine that checked calling convention.
> > It does not needed anymore, because:
> > 
> >   - Generic handler checks that 64 bit calls can be made only by
> >     64 bit guests.
> > 
> >   - SMCCC requires that 64-bit handler should support both 32 and 64 bit
> >     calls even if they originate from 64 bit caller.
> > 
> > This patch removes that extra check.
> > 
> > Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@xxxxxxxx>
> > Reviewed-by: Stefano Stabellini <sstabellini@xxxxxxxxxx>
> 
> Sorry I just noticed this. This is odd to keep a reviewed-by from Stefano in a
> patch that changed quite a bit since he gave it (patch #2).
> 
> Usually reviewed-by means the person has reviewed the code and happy with it.
> Technically, he didn't review the new changes and if not dropped, you should
> have at least asked whether he is happy with it.
> 
> Stefano are you happy if Volodymyr keeping your tag?

No, the patch changed too much, I'll have to review it again. Drop it
for now.


> > ---
> >   xen/arch/arm/vsmc.c | 62
> > ++++++++++++++++++++++-------------------------------
> >   1 file changed, 26 insertions(+), 36 deletions(-)
> > 
> > diff --git a/xen/arch/arm/vsmc.c b/xen/arch/arm/vsmc.c
> > index 77fc915..ff84442 100644
> > --- a/xen/arch/arm/vsmc.c
> > +++ b/xen/arch/arm/vsmc.c
> > @@ -135,12 +135,6 @@ static bool handle_existing_apis(struct cpu_user_regs
> > *regs)
> >       }
> >   }
> >   -/* helper function for checking arm mode 32/64 bit */
> > -static inline int psci_mode_check(struct domain *d, uint32_t fid)
> > -{
> > -    return is_64bit_domain(d) || !smccc_is_conv_64(fid);
> > -}
> > -
> >   /* PSCI 0.2 interface and other Standard Secure Calls */
> >   static bool handle_sssc(struct cpu_user_regs *regs)
> >   {
> > @@ -165,8 +159,7 @@ static bool handle_sssc(struct cpu_user_regs *regs)
> >         case PSCI_0_2_FN_MIGRATE_INFO_UP_CPU:
> >           perfc_incr(vpsci_migrate_info_up_cpu);
> > -        if ( psci_mode_check(current->domain, fid) )
> > -            PSCI_SET_RESULT(regs, do_psci_0_2_migrate_info_up_cpu());
> > +        PSCI_SET_RESULT(regs, do_psci_0_2_migrate_info_up_cpu());
> >           return true;
> >         case PSCI_0_2_FN_SYSTEM_OFF:
> > @@ -182,48 +175,45 @@ static bool handle_sssc(struct cpu_user_regs *regs)
> >           return true;
> >         case PSCI_0_2_FN_CPU_ON:
> > -        perfc_incr(vpsci_cpu_on);
> > -        if ( psci_mode_check(current->domain, fid) )
> > -        {
> > -            register_t vcpuid = PSCI_ARG(regs, 1);
> > -            register_t epoint = PSCI_ARG(regs, 2);
> > -            register_t cid = PSCI_ARG(regs, 3);
> > +    {
> > +        register_t vcpuid = PSCI_ARG(regs, 1);
> > +        register_t epoint = PSCI_ARG(regs, 2);
> > +        register_t cid = PSCI_ARG(regs, 3);
> >   -            PSCI_SET_RESULT(regs, do_psci_0_2_cpu_on(vcpuid, epoint,
> > cid));
> > -        }
> > +        perfc_incr(vpsci_cpu_on);
> > +        PSCI_SET_RESULT(regs, do_psci_0_2_cpu_on(vcpuid, epoint, cid));
> >           return true;
> > +    }
> >         case PSCI_0_2_FN_CPU_SUSPEND:
> > -        perfc_incr(vpsci_cpu_suspend);
> > -        if ( psci_mode_check(current->domain, fid) )
> > -        {
> > -            uint32_t pstate = PSCI_ARG32(regs, 1);
> > -            register_t epoint = PSCI_ARG(regs, 2);
> > -            register_t cid = PSCI_ARG(regs, 3);
> > +    {
> > +        uint32_t pstate = PSCI_ARG32(regs, 1);
> > +        register_t epoint = PSCI_ARG(regs, 2);
> > +        register_t cid = PSCI_ARG(regs, 3);
> >   -            PSCI_SET_RESULT(regs, do_psci_0_2_cpu_suspend(pstate, epoint,
> > cid));
> > -        }
> > +        perfc_incr(vpsci_cpu_suspend);
> > +        PSCI_SET_RESULT(regs, do_psci_0_2_cpu_suspend(pstate, epoint,
> > cid));
> >           return true;
> > +    }
> >         case PSCI_0_2_FN_AFFINITY_INFO:
> > +    {
> > +        register_t taff = PSCI_ARG(regs, 1);
> > +        uint32_t laff = PSCI_ARG32(regs, 2);
> > +
> >           perfc_incr(vpsci_cpu_affinity_info);
> > -        if ( psci_mode_check(current->domain, fid) )
> > -        {
> > -            register_t taff = PSCI_ARG(regs, 1);
> > -            uint32_t laff = PSCI_ARG32(regs, 2);
> > -            PSCI_SET_RESULT(regs, do_psci_0_2_affinity_info(taff, laff));
> > -        }
> > +        PSCI_SET_RESULT(regs, do_psci_0_2_affinity_info(taff, laff));
> >           return true;
> > +    }
> >         case PSCI_0_2_FN_MIGRATE:
> > -        perfc_incr(vpsci_cpu_migrate);
> > -        if ( psci_mode_check(current->domain, fid) )
> > -        {
> > -            uint32_t tcpu = PSCI_ARG32(regs, 1);
> > +    {
> > +        uint32_t tcpu = PSCI_ARG32(regs, 1);
> >   -            PSCI_SET_RESULT(regs, do_psci_0_2_migrate(tcpu));
> > -        }
> > +        perfc_incr(vpsci_cpu_migrate);
> > +        PSCI_SET_RESULT(regs, do_psci_0_2_migrate(tcpu));
> >           return true;
> > +    }
> >         case ARM_SMCCC_FUNC_CALL_COUNT:
> >           return fill_function_call_count(regs, SSSC_SMCCC_FUNCTION_COUNT);
> > 
> 
> -- 
> Julien Grall
> 

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel