From mboxrd@z Thu Jan 1 00:00:00 1970 From: Parth Dixit Subject: Re: [PATCH v3 2/2] xen/arm : emulation of arm's PSCI v0.2 standard Date: Thu, 10 Jul 2014 15:44:15 +0530 Message-ID: References: <1404390158-21542-1-git-send-email-parth.dixit@linaro.org> <1404390158-21542-2-git-send-email-parth.dixit@linaro.org> <1404912270.16789.52.camel@kazak.uk.xensource.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <1404912270.16789.52.camel@kazak.uk.xensource.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Ian Campbell Cc: Stefano Stabellini , tim@xen.org, Christoffer Dall , Julien Grall , xen-devel List-Id: xen-devel@lists.xenproject.org Hi, On 9 July 2014 18:54, Ian Campbell wrote: > On Thu, 2014-07-03 at 17:52 +0530, Parth Dixit wrote: >> static void do_trap_psci(struct cpu_user_regs *regs) >> { >> arm_psci_fn_t psci_call = NULL; >> >> - if ( PSCI_OP_REG(regs) >= ARRAY_SIZE(arm_psci_table) ) >> + if ( PSCI_OP_REG(regs) < PSCI_migrate ) >> { >> - domain_crash_synchronous(); >> - return; >> + if ( PSCI_OP_REG(regs) >= ARRAY_SIZE(arm_psci_table) ) >> + { >> + domain_crash_synchronous(); >> + return; >> + } >> + psci_call = arm_psci_table[PSCI_OP_REG(regs)].fn; >> + } >> + else >> + { >> + if ( ( PSCI_OP_REG(regs) & PSCI_FN_MASK ) >= ARRAY_SIZE(arm_psci_0_2_table) ) > > I think you need to also check that "PSCI_OP_REG(regs) & ~PSCI_FN_MASK" > is either the 32-bit or 64-bit base. Otherwise I could call 0xdeadfe01 > and it would work. > > Do you not also need to check that the guest is of the appropriate type? > Is an aarch64 guest allowed to call the aarch32 entry points? The spec > doesn't say so explicitly AFAICT. > > If it is allowed then I think you need to be truncating the 32-bit > arguments to 32-bits when an aarch64 guest calls the 32-bit entry > points. > > Hrm, checking through the spec I see now that only some of the functions > have both a 32- and 64-bit function id. VERSION, CPU_OFF, > MIGRATE_INFO_TYPE, SYSTEM_OFF and SYSTEM_RESET only have a single > function id (I suppose because they do not take any arguments which are > mode specific). > > I'm afraid that AFAICT you need to handle this distinction, which I > expect makes it rather hard to share the same lookup table between 32- > and 64-bit. A switch(PSCI_OP_REG(regs)) is looking increasingly like the > correct approach to me. I am bit confused, are you saying for eg. aarch64 can call "psci_0_2_suspend" function with aarch32 function id (effectively 32 bit version of function?) > >> +int do_psci_0_2_version(void) >> +{ >> + struct domain *d = current->domain; >> + >> + return ( d->arch.vpsci_ver = XEN_PSCI_V_0_2 ); > > Is this assignment really intentional? > > I see you use d->arch.vpsci_ver later in the common do_psci_0_2_cpu_on > function, so I think you did mean to do this. > > Does the spec say somewhere that after calling PSCI_0_2_VERSION an OS is > forbidden from calling PSCI v0.1 functions? > > Likewise does it mandate that an OS which supports v0.2 always calls > VERSION before any other function? > > I can't find anything in the spec which says either of those things. > Please do point out if I am wrong. > > In any case I think this approach to a common function is wrong. I think > you should implement something like: > > int do_common_cpu_on(int version, register_t target_cpu, register_t entry_point, > register_t context_id) > > similar to your current do_psci_0_2_cpu_suspend but using version > instead of d->arch.vpsci_ver. Then you have: > > int do_psci_cpu_on(uint32_t vcpuid, register_t entry_point) > { > return do_common_cpu_on(PSCI_0_1_VERSION, vcpuid, entry_point, 0); > } > int do_psci_0_2_cpu_on(register_t target_cpu, register_t entry_point, > register_t context_id) > { > return do_common_cpu_on(PSCI_0_2_VERSION, target_cpu, > entry_point, context_id) > } > > IOW the mode of operation is determined by the entry point used not some > bit of domain state. > Sure, i'll switch to entry point based approach. >> +int do_psci_0_2_cpu_on(register_t target_cpu, register_t entry_point, >> + register_t context_id) >> +{ >> struct vcpu *v; >> struct domain *d = current->domain; >> struct vcpu_guest_context *ctxt; >> int rc; >> int is_thumb = entry_point & 1; >> + uint32_t vcpuid ; > > Stray " " here. Will remove it next patchset >> + >> + if( d->arch.vpsci_ver == XEN_PSCI_V_0_2 ) >> + vcpuid = (u32)(target_cpu & MPIDR_HWID_MASK); > > Doesn't this mask off AFF3 on 64-bit? I know we don't use AFF3 today in > Xen, but this is setting a pretty subtle trap for the person who does > come to implement something which uses it. > Sure, will take care of it. >> @@ -75,12 +144,65 @@ int do_psci_cpu_on(uint32_t vcpuid, register_t entry_point) >> return PSCI_SUCCESS; >> } >> >> -int do_psci_cpu_off(uint32_t power_state) >> +static const unsigned long target_affinity_mask[] = { >> + ( MPIDR_HWID_MASK & AFFINITY_MASK( 0 ) ), >> + ( MPIDR_HWID_MASK & AFFINITY_MASK( 1 ) ), >> + ( MPIDR_HWID_MASK & AFFINITY_MASK( 2 ) ) >> +#ifdef CONFIG_ARM_64 >> + ,( MPIDR_HWID_MASK & AFFINITY_MASK( 3 ) ) >> +#endif >> +}; >> + >> +int do_psci_0_2_affinity_info(register_t target_affinity, >> + uint32_t lowest_affinity_level) >> { >> - struct vcpu *v = current; >> - if ( !test_and_set_bit(_VPF_down, &v->pause_flags) ) >> - vcpu_sleep_nosync(v); >> - return PSCI_SUCCESS; >> + struct domain *d = current->domain; >> + struct vcpu *v; >> + uint32_t vcpuid; >> + >> + if ( lowest_affinity_level < ARRAY_SIZE(target_affinity_mask) ) >> + target_affinity &= target_affinity_mask[lowest_affinity_level]; >> + else >> + return PSCI_INVALID_PARAMETERS; >> + >> + for ( vcpuid = 0; vcpuid < d->max_vcpus; vcpuid++ ) >> + { >> + v = d->vcpu[vcpuid]; >> + >> + if ( ( ( v->arch.vmpidr & target_affinity_mask[lowest_affinity_level] ) > > if you assign target_affinity_mask[lowest_affinity_level] to a local > variable you can avoid the awful state of line wrapping here. > will introduce local variable > Ian. >