| Message ID | 20180215150248.28922-13-julien.grall@arm.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | xen/arm: PSCI 1.1 and SMCCC-1.1 support and XSA-254 variant 2 update | expand |
On Thu, 15 Feb 2018, Julien Grall wrote: > Currently, the behavior of do_common_cpu will slightly change depending > on the PSCI version passed in parameter. Looking at the code, more the > specific 0.2 behavior could move out of the function or adapted for 0.1: > > - x0/r0 can be updated on PSCI 0.1 because general purpose registers > are undefined upon CPU on. > - PSCI 0.1 does not defined PSCI_ALREADY_ON. However, it would be > safer to bail out if the CPU is already on. > > Based on this, the parameter 'ver' is removed and do_psci_cpu_on > (implementation for PSCI 0.1) is adapted to avoid returning > PSCI_ALREADY_ON. > > Signed-off-by: Julien Grall <julien.grall@arm.com> > Reviewed-by: Volodymyr Babchuk <volodymyr.babchuk@epam.com> Acked-by: Stefano Stabellini <sstabellini@kernel.org> > --- > The reviewed-by was kept despite move this patch towards the end > of the series because there was no clash with the rest of the series. > > Changes in v2: > - Move the patch towards the end of the series as not strictly > necessary for SP2. > - Add Volodymyr's reviewed-by > --- > xen/arch/arm/vpsci.c | 28 ++++++++++++++++++---------- > 1 file changed, 18 insertions(+), 10 deletions(-) > > diff --git a/xen/arch/arm/vpsci.c b/xen/arch/arm/vpsci.c > index 19ee7caeb4..7ea3ea58e3 100644 > --- a/xen/arch/arm/vpsci.c > +++ b/xen/arch/arm/vpsci.c > @@ -22,7 +22,7 @@ > #include <public/sched.h> > > static int do_common_cpu_on(register_t target_cpu, register_t entry_point, > - register_t context_id,int ver) > + register_t context_id) > { > struct vcpu *v; > struct domain *d = current->domain; > @@ -40,8 +40,7 @@ static int do_common_cpu_on(register_t target_cpu, register_t entry_point, > if ( is_64bit_domain(d) && is_thumb ) > return PSCI_INVALID_PARAMETERS; > > - if ( (ver == PSCI_VERSION(0, 2)) && > - !test_bit(_VPF_down, &v->pause_flags) ) > + if ( !test_bit(_VPF_down, &v->pause_flags) ) > return PSCI_ALREADY_ON; > > if ( (ctxt = alloc_vcpu_guest_context()) == NULL ) > @@ -55,18 +54,21 @@ static int do_common_cpu_on(register_t target_cpu, register_t entry_point, > ctxt->ttbr0 = 0; > ctxt->ttbr1 = 0; > ctxt->ttbcr = 0; /* Defined Reset Value */ > + > + /* > + * x0/r0_usr are always updated because for PSCI 0.1 the general > + * purpose registers are undefined upon CPU_on. > + */ > if ( is_32bit_domain(d) ) > { > ctxt->user_regs.cpsr = PSR_GUEST32_INIT; > - if ( ver == PSCI_VERSION(0, 2) ) > - ctxt->user_regs.r0_usr = context_id; > + ctxt->user_regs.r0_usr = context_id; > } > #ifdef CONFIG_ARM_64 > else > { > ctxt->user_regs.cpsr = PSR_GUEST64_INIT; > - if ( ver == PSCI_VERSION(0, 2) ) > - ctxt->user_regs.x0 = context_id; > + ctxt->user_regs.x0 = context_id; > } > #endif > > @@ -93,7 +95,14 @@ static int do_common_cpu_on(register_t target_cpu, register_t entry_point, > > static int32_t do_psci_cpu_on(uint32_t vcpuid, register_t entry_point) > { > - return do_common_cpu_on(vcpuid, entry_point, 0 , PSCI_VERSION(0, 1)); > + int32_t ret; > + > + ret = do_common_cpu_on(vcpuid, entry_point, 0); > + /* > + * PSCI 0.1 does not define the return code PSCI_ALREADY_ON. > + * Instead, return PSCI_INVALID_PARAMETERS. > + */ > + return (ret == PSCI_ALREADY_ON) ? PSCI_INVALID_PARAMETERS : ret; > } > > static int32_t do_psci_cpu_off(uint32_t power_state) > @@ -137,8 +146,7 @@ static int32_t do_psci_0_2_cpu_on(register_t target_cpu, > register_t entry_point, > register_t context_id) > { > - return do_common_cpu_on(target_cpu, entry_point, context_id, > - PSCI_VERSION(0, 2)); > + return do_common_cpu_on(target_cpu, entry_point, context_id); > } > > static const unsigned long target_affinity_mask[] = { > -- > 2.11.0 >
Hi, On 15/02/18 15:02, Julien Grall wrote: > Currently, the behavior of do_common_cpu will slightly change depending > on the PSCI version passed in parameter. Looking at the code, more the > specific 0.2 behavior could move out of the function or adapted for 0.1: > > - x0/r0 can be updated on PSCI 0.1 because general purpose registers > are undefined upon CPU on. Is that explicitly mentioned somewhere in the spec? I couldn't find anything in the original DEN0022A. Or is that because it does *not* mention anything about the GPR state that we are safe to put anything in there? > - PSCI 0.1 does not defined PSCI_ALREADY_ON. However, it would be > safer to bail out if the CPU is already on. > > Based on this, the parameter 'ver' is removed and do_psci_cpu_on > (implementation for PSCI 0.1) is adapted to avoid returning > PSCI_ALREADY_ON. > > Signed-off-by: Julien Grall <julien.grall@arm.com> > Reviewed-by: Volodymyr Babchuk <volodymyr.babchuk@epam.com> Given that it's safe to clobber x0/r0 on CPU_ON in PSCI 0.1, the rest looks correct to me: Reviewed-by: Andre Przywara <andre.przywara@arm.com> Cheers, Andre. > --- > The reviewed-by was kept despite move this patch towards the end > of the series because there was no clash with the rest of the series. > > Changes in v2: > - Move the patch towards the end of the series as not strictly > necessary for SP2. > - Add Volodymyr's reviewed-by > --- > xen/arch/arm/vpsci.c | 28 ++++++++++++++++++---------- > 1 file changed, 18 insertions(+), 10 deletions(-) > > diff --git a/xen/arch/arm/vpsci.c b/xen/arch/arm/vpsci.c > index 19ee7caeb4..7ea3ea58e3 100644 > --- a/xen/arch/arm/vpsci.c > +++ b/xen/arch/arm/vpsci.c > @@ -22,7 +22,7 @@ > #include <public/sched.h> > > static int do_common_cpu_on(register_t target_cpu, register_t entry_point, > - register_t context_id,int ver) > + register_t context_id) > { > struct vcpu *v; > struct domain *d = current->domain; > @@ -40,8 +40,7 @@ static int do_common_cpu_on(register_t target_cpu, register_t entry_point, > if ( is_64bit_domain(d) && is_thumb ) > return PSCI_INVALID_PARAMETERS; > > - if ( (ver == PSCI_VERSION(0, 2)) && > - !test_bit(_VPF_down, &v->pause_flags) ) > + if ( !test_bit(_VPF_down, &v->pause_flags) ) > return PSCI_ALREADY_ON; > > if ( (ctxt = alloc_vcpu_guest_context()) == NULL ) > @@ -55,18 +54,21 @@ static int do_common_cpu_on(register_t target_cpu, register_t entry_point, > ctxt->ttbr0 = 0; > ctxt->ttbr1 = 0; > ctxt->ttbcr = 0; /* Defined Reset Value */ > + > + /* > + * x0/r0_usr are always updated because for PSCI 0.1 the general > + * purpose registers are undefined upon CPU_on. > + */ > if ( is_32bit_domain(d) ) > { > ctxt->user_regs.cpsr = PSR_GUEST32_INIT; > - if ( ver == PSCI_VERSION(0, 2) ) > - ctxt->user_regs.r0_usr = context_id; > + ctxt->user_regs.r0_usr = context_id; > } > #ifdef CONFIG_ARM_64 > else > { > ctxt->user_regs.cpsr = PSR_GUEST64_INIT; > - if ( ver == PSCI_VERSION(0, 2) ) > - ctxt->user_regs.x0 = context_id; > + ctxt->user_regs.x0 = context_id; > } > #endif > > @@ -93,7 +95,14 @@ static int do_common_cpu_on(register_t target_cpu, register_t entry_point, > > static int32_t do_psci_cpu_on(uint32_t vcpuid, register_t entry_point) > { > - return do_common_cpu_on(vcpuid, entry_point, 0 , PSCI_VERSION(0, 1)); > + int32_t ret; > + > + ret = do_common_cpu_on(vcpuid, entry_point, 0); > + /* > + * PSCI 0.1 does not define the return code PSCI_ALREADY_ON. > + * Instead, return PSCI_INVALID_PARAMETERS. > + */ > + return (ret == PSCI_ALREADY_ON) ? PSCI_INVALID_PARAMETERS : ret; > } > > static int32_t do_psci_cpu_off(uint32_t power_state) > @@ -137,8 +146,7 @@ static int32_t do_psci_0_2_cpu_on(register_t target_cpu, > register_t entry_point, > register_t context_id) > { > - return do_common_cpu_on(target_cpu, entry_point, context_id, > - PSCI_VERSION(0, 2)); > + return do_common_cpu_on(target_cpu, entry_point, context_id); > } > > static const unsigned long target_affinity_mask[] = { >
On 21/02/18 16:27, Andre Przywara wrote: > Hi, Hi, > On 15/02/18 15:02, Julien Grall wrote: >> Currently, the behavior of do_common_cpu will slightly change depending >> on the PSCI version passed in parameter. Looking at the code, more the >> specific 0.2 behavior could move out of the function or adapted for 0.1: >> >> - x0/r0 can be updated on PSCI 0.1 because general purpose registers >> are undefined upon CPU on. > > Is that explicitly mentioned somewhere in the spec? I couldn't find > anything in the original DEN0022A. Or is that because it does *not* > mention anything about the GPR state that we are safe to put anything in > there? Because nothing tells you what is the GPR state when booting a secondary CPUs in the ARM ARM. This is done to the specification to decide what would be the value. Today Xen will zero them, but it is not because of specific requirements just to avoid leak hypervisor content in those registers. Cheers, > >> - PSCI 0.1 does not defined PSCI_ALREADY_ON. However, it would be >> safer to bail out if the CPU is already on. >> >> Based on this, the parameter 'ver' is removed and do_psci_cpu_on >> (implementation for PSCI 0.1) is adapted to avoid returning >> PSCI_ALREADY_ON. >> >> Signed-off-by: Julien Grall <julien.grall@arm.com> >> Reviewed-by: Volodymyr Babchuk <volodymyr.babchuk@epam.com> > > Given that it's safe to clobber x0/r0 on CPU_ON in PSCI 0.1, the rest > looks correct to me: > > Reviewed-by: Andre Przywara <andre.przywara@arm.com> > > Cheers, > Andre. > >> --- >> The reviewed-by was kept despite move this patch towards the end >> of the series because there was no clash with the rest of the series. >> >> Changes in v2: >> - Move the patch towards the end of the series as not strictly >> necessary for SP2. >> - Add Volodymyr's reviewed-by >> --- >> xen/arch/arm/vpsci.c | 28 ++++++++++++++++++---------- >> 1 file changed, 18 insertions(+), 10 deletions(-) >> >> diff --git a/xen/arch/arm/vpsci.c b/xen/arch/arm/vpsci.c >> index 19ee7caeb4..7ea3ea58e3 100644 >> --- a/xen/arch/arm/vpsci.c >> +++ b/xen/arch/arm/vpsci.c >> @@ -22,7 +22,7 @@ >> #include <public/sched.h> >> >> static int do_common_cpu_on(register_t target_cpu, register_t entry_point, >> - register_t context_id,int ver) >> + register_t context_id) >> { >> struct vcpu *v; >> struct domain *d = current->domain; >> @@ -40,8 +40,7 @@ static int do_common_cpu_on(register_t target_cpu, register_t entry_point, >> if ( is_64bit_domain(d) && is_thumb ) >> return PSCI_INVALID_PARAMETERS; >> >> - if ( (ver == PSCI_VERSION(0, 2)) && >> - !test_bit(_VPF_down, &v->pause_flags) ) >> + if ( !test_bit(_VPF_down, &v->pause_flags) ) >> return PSCI_ALREADY_ON; >> >> if ( (ctxt = alloc_vcpu_guest_context()) == NULL ) >> @@ -55,18 +54,21 @@ static int do_common_cpu_on(register_t target_cpu, register_t entry_point, >> ctxt->ttbr0 = 0; >> ctxt->ttbr1 = 0; >> ctxt->ttbcr = 0; /* Defined Reset Value */ >> + >> + /* >> + * x0/r0_usr are always updated because for PSCI 0.1 the general >> + * purpose registers are undefined upon CPU_on. >> + */ >> if ( is_32bit_domain(d) ) >> { >> ctxt->user_regs.cpsr = PSR_GUEST32_INIT; >> - if ( ver == PSCI_VERSION(0, 2) ) >> - ctxt->user_regs.r0_usr = context_id; >> + ctxt->user_regs.r0_usr = context_id; >> } >> #ifdef CONFIG_ARM_64 >> else >> { >> ctxt->user_regs.cpsr = PSR_GUEST64_INIT; >> - if ( ver == PSCI_VERSION(0, 2) ) >> - ctxt->user_regs.x0 = context_id; >> + ctxt->user_regs.x0 = context_id; >> } >> #endif >> >> @@ -93,7 +95,14 @@ static int do_common_cpu_on(register_t target_cpu, register_t entry_point, >> >> static int32_t do_psci_cpu_on(uint32_t vcpuid, register_t entry_point) >> { >> - return do_common_cpu_on(vcpuid, entry_point, 0 , PSCI_VERSION(0, 1)); >> + int32_t ret; >> + >> + ret = do_common_cpu_on(vcpuid, entry_point, 0); >> + /* >> + * PSCI 0.1 does not define the return code PSCI_ALREADY_ON. >> + * Instead, return PSCI_INVALID_PARAMETERS. >> + */ >> + return (ret == PSCI_ALREADY_ON) ? PSCI_INVALID_PARAMETERS : ret; >> } >> >> static int32_t do_psci_cpu_off(uint32_t power_state) >> @@ -137,8 +146,7 @@ static int32_t do_psci_0_2_cpu_on(register_t target_cpu, >> register_t entry_point, >> register_t context_id) >> { >> - return do_common_cpu_on(target_cpu, entry_point, context_id, >> - PSCI_VERSION(0, 2)); >> + return do_common_cpu_on(target_cpu, entry_point, context_id); >> } >> >> static const unsigned long target_affinity_mask[] = { >>
On 21/02/18 16:37, Julien Grall wrote: > > > On 21/02/18 16:27, Andre Przywara wrote: >> Hi, > > Hi, > >> On 15/02/18 15:02, Julien Grall wrote: >>> Currently, the behavior of do_common_cpu will slightly change depending >>> on the PSCI version passed in parameter. Looking at the code, more the >>> specific 0.2 behavior could move out of the function or adapted for 0.1: >>> >>> - x0/r0 can be updated on PSCI 0.1 because general purpose >>> registers >>> are undefined upon CPU on. >> >> Is that explicitly mentioned somewhere in the spec? I couldn't find >> anything in the original DEN0022A. Or is that because it does *not* >> mention anything about the GPR state that we are safe to put anything in >> there? > > Because nothing tells you what is the GPR state when booting a secondary > CPUs in the ARM ARM. This is done to the specification to decide what > would be the value. > > Today Xen will zero them, but it is not because of specific requirements > just to avoid leak hypervisor content in those registers. I have added in the commit message: "This was deduced from the spec not mentioning the state of general purpose registers on CPU on." Cheers,
diff --git a/xen/arch/arm/vpsci.c b/xen/arch/arm/vpsci.c index 19ee7caeb4..7ea3ea58e3 100644 --- a/xen/arch/arm/vpsci.c +++ b/xen/arch/arm/vpsci.c @@ -22,7 +22,7 @@ #include <public/sched.h> static int do_common_cpu_on(register_t target_cpu, register_t entry_point, - register_t context_id,int ver) + register_t context_id) { struct vcpu *v; struct domain *d = current->domain; @@ -40,8 +40,7 @@ static int do_common_cpu_on(register_t target_cpu, register_t entry_point, if ( is_64bit_domain(d) && is_thumb ) return PSCI_INVALID_PARAMETERS; - if ( (ver == PSCI_VERSION(0, 2)) && - !test_bit(_VPF_down, &v->pause_flags) ) + if ( !test_bit(_VPF_down, &v->pause_flags) ) return PSCI_ALREADY_ON; if ( (ctxt = alloc_vcpu_guest_context()) == NULL ) @@ -55,18 +54,21 @@ static int do_common_cpu_on(register_t target_cpu, register_t entry_point, ctxt->ttbr0 = 0; ctxt->ttbr1 = 0; ctxt->ttbcr = 0; /* Defined Reset Value */ + + /* + * x0/r0_usr are always updated because for PSCI 0.1 the general + * purpose registers are undefined upon CPU_on. + */ if ( is_32bit_domain(d) ) { ctxt->user_regs.cpsr = PSR_GUEST32_INIT; - if ( ver == PSCI_VERSION(0, 2) ) - ctxt->user_regs.r0_usr = context_id; + ctxt->user_regs.r0_usr = context_id; } #ifdef CONFIG_ARM_64 else { ctxt->user_regs.cpsr = PSR_GUEST64_INIT; - if ( ver == PSCI_VERSION(0, 2) ) - ctxt->user_regs.x0 = context_id; + ctxt->user_regs.x0 = context_id; } #endif @@ -93,7 +95,14 @@ static int do_common_cpu_on(register_t target_cpu, register_t entry_point, static int32_t do_psci_cpu_on(uint32_t vcpuid, register_t entry_point) { - return do_common_cpu_on(vcpuid, entry_point, 0 , PSCI_VERSION(0, 1)); + int32_t ret; + + ret = do_common_cpu_on(vcpuid, entry_point, 0); + /* + * PSCI 0.1 does not define the return code PSCI_ALREADY_ON. + * Instead, return PSCI_INVALID_PARAMETERS. + */ + return (ret == PSCI_ALREADY_ON) ? PSCI_INVALID_PARAMETERS : ret; } static int32_t do_psci_cpu_off(uint32_t power_state) @@ -137,8 +146,7 @@ static int32_t do_psci_0_2_cpu_on(register_t target_cpu, register_t entry_point, register_t context_id) { - return do_common_cpu_on(target_cpu, entry_point, context_id, - PSCI_VERSION(0, 2)); + return do_common_cpu_on(target_cpu, entry_point, context_id); } static const unsigned long target_affinity_mask[] = {
Currently, the behavior of do_common_cpu will slightly change depending on the PSCI version passed in parameter. Looking at the code, more the specific 0.2 behavior could move out of the function or adapted for 0.1: - x0/r0 can be updated on PSCI 0.1 because general purpose registers are undefined upon CPU on. - PSCI 0.1 does not defined PSCI_ALREADY_ON. However, it would be safer to bail out if the CPU is already on. Based on this, the parameter 'ver' is removed and do_psci_cpu_on (implementation for PSCI 0.1) is adapted to avoid returning PSCI_ALREADY_ON. Signed-off-by: Julien Grall <julien.grall@arm.com> Reviewed-by: Volodymyr Babchuk <volodymyr.babchuk@epam.com> --- The reviewed-by was kept despite move this patch towards the end of the series because there was no clash with the rest of the series. Changes in v2: - Move the patch towards the end of the series as not strictly necessary for SP2. - Add Volodymyr's reviewed-by --- xen/arch/arm/vpsci.c | 28 ++++++++++++++++++---------- 1 file changed, 18 insertions(+), 10 deletions(-)