Re: [PATCH 1/2] target/arm: add support for FEAT_DIT, Data Independent Timing
On Mon, 14 Dec 2020 at 18:11, Rebecca Cran wrote: > > On 12/11/20 2:37 PM, Peter Maydell wrote: > > On Fri, 11 Dec 2020 at 19:51, Richard Henderson > > wrote: > >> I'll let Peter weigh in, but I think it makes sense to move the SS bit > >> somewhere else (e.g. env->pstate) and merge it into SPSR_ELx upon > >> interrupt. > >> While what we're doing here is convenient, it's not architectural, and it > >> would > >> be better to follow GetPSRFromPSTATE pseudocode. > > > > Yes, I think that's the best thing to do. We've been skating > > by on CPSR and SPSR being the same thing and it's just > > stopped working. > > Would you like me to work on this, or is it something the arm > maintainers would fix? You're the person that wants to add the change that requires this refactoring, so if you could, I think the best thing would be for you to do this bit of work and send it as part of this patchseries. thanks -- PMM
Re: [PATCH 1/2] target/arm: add support for FEAT_DIT, Data Independent Timing
On 12/11/20 2:37 PM, Peter Maydell wrote: On Fri, 11 Dec 2020 at 19:51, Richard Henderson wrote: I'll let Peter weigh in, but I think it makes sense to move the SS bit somewhere else (e.g. env->pstate) and merge it into SPSR_ELx upon interrupt. While what we're doing here is convenient, it's not architectural, and it would be better to follow GetPSRFromPSTATE pseudocode. Yes, I think that's the best thing to do. We've been skating by on CPSR and SPSR being the same thing and it's just stopped working. Would you like me to work on this, or is it something the arm maintainers would fix? -- Rebecca Cran
Re: [PATCH 1/2] target/arm: add support for FEAT_DIT, Data Independent Timing
On Fri, 11 Dec 2020 at 19:51, Richard Henderson
wrote:
>
> On 12/11/20 1:33 PM, Rebecca Cran wrote:
> > Is the comment in target/arm/op_helper.c:397 still relevant?
> >
> > uint32_t HELPER(cpsr_read)(CPUARMState *env)
> > {
> > /*
> > * We store the ARMv8 PSTATE.SS bit in env->uncached_cpsr.
> > * This is convenient for populating SPSR_ELx, but must be
> > * hidden from aarch32 mode, where it is not visible.
> > *
> > * TODO: ARMv8.4-DIT -- need to move SS somewhere else.
> > */
> > return cpsr_read(env) & ~(CPSR_EXEC | PSTATE_SS);
> > }
>
> I forgot about this. So we can't "just" store DIT in uncached_cpsr.
>
> I'll let Peter weigh in, but I think it makes sense to move the SS bit
> somewhere else (e.g. env->pstate) and merge it into SPSR_ELx upon interrupt.
> While what we're doing here is convenient, it's not architectural, and it
> would
> be better to follow GetPSRFromPSTATE pseudocode.
Yes, I think that's the best thing to do. We've been skating
by on CPSR and SPSR being the same thing and it's just
stopped working.
-- PMM
Re: [PATCH 1/2] target/arm: add support for FEAT_DIT, Data Independent Timing
On 12/11/20 7:08 AM, Richard Henderson wrote:
Alternately, or additionally, TCG is outside of the security domain (only
hardware accelerators like KVM are inside), and so we may implement this as a
NOP.
Thanks for the feedback, I'll send a v2 patch next week.
Is the comment in target/arm/op_helper.c:397 still relevant?
uint32_t HELPER(cpsr_read)(CPUARMState *env)
{
/*
* We store the ARMv8 PSTATE.SS bit in env->uncached_cpsr.
* This is convenient for populating SPSR_ELx, but must be
* hidden from aarch32 mode, where it is not visible.
*
* TODO: ARMv8.4-DIT -- need to move SS somewhere else.
*/
return cpsr_read(env) & ~(CPSR_EXEC | PSTATE_SS);
}
--
Rebecca Cran
Re: [PATCH 1/2] target/arm: add support for FEAT_DIT, Data Independent Timing
On 12/11/20 1:33 PM, Rebecca Cran wrote:
> Is the comment in target/arm/op_helper.c:397 still relevant?
>
> uint32_t HELPER(cpsr_read)(CPUARMState *env)
> {
> /*
> * We store the ARMv8 PSTATE.SS bit in env->uncached_cpsr.
> * This is convenient for populating SPSR_ELx, but must be
> * hidden from aarch32 mode, where it is not visible.
> *
> * TODO: ARMv8.4-DIT -- need to move SS somewhere else.
> */
> return cpsr_read(env) & ~(CPSR_EXEC | PSTATE_SS);
> }
I forgot about this. So we can't "just" store DIT in uncached_cpsr.
I'll let Peter weigh in, but I think it makes sense to move the SS bit
somewhere else (e.g. env->pstate) and merge it into SPSR_ELx upon interrupt.
While what we're doing here is convenient, it's not architectural, and it would
be better to follow GetPSRFromPSTATE pseudocode.
r~
Re: [PATCH 1/2] target/arm: add support for FEAT_DIT, Data Independent Timing
On 12/10/20 11:13 PM, Rebecca Cran wrote:
> Add support for FEAT_DIT. DIT (Data Independent Timing) is a required
> feature for ARMv8.4. Since virtual machine execution is largely
> nondeterministic, it's implemented as a NOP.
Alternately, or additionally, TCG is outside of the security domain (only
hardware accelerators like KVM are inside), and so we may implement this as a
NOP.
>
> Signed-off-by: Rebecca Cran
> ---
> target/arm/cpu.h | 20 +-
> target/arm/helper.c| 28 +++-
> target/arm/internals.h | 6 +
> target/arm/translate-a64.c | 14 ++
> 4 files changed, 66 insertions(+), 2 deletions(-)
>
> diff --git a/target/arm/cpu.h b/target/arm/cpu.h
> index 4c9cbfbd9975..862be662cef7 100644
> --- a/target/arm/cpu.h
> +++ b/target/arm/cpu.h
> @@ -269,6 +269,7 @@ typedef struct CPUARMState {
> uint32_t NF; /* N is bit 31. All other bits are undefined. */
> uint32_t ZF; /* Z set if zero. */
> uint32_t QF; /* 0 or 1 */
> +uint32_t DIT; /* 0 or 1 */
You don't need to add this. Leave the DIT bit in uncached_cpsr.
> +++ b/target/arm/translate-a64.c
> @@ -1696,6 +1696,20 @@ static void handle_msr_i(DisasContext *s, uint32_t
> insn,
> tcg_temp_free_i32(t1);
> break;
>
> +case 0x1a: /* DIT */
> +if (!dc_isar_feature(aa64_dit, s)) {
> +goto do_unallocated;
> +}
> +if (crm & 1) {
> +set_pstate_bits(PSTATE_DIT);
> +} else {
> +clear_pstate_bits(PSTATE_DIT);
> +}
> +t1 = tcg_const_i32(s->current_el);
> +gen_helper_rebuild_hflags_a64(cpu_env, t1);
> +tcg_temp_free_i32(t1);
> +break;
You don't need to rebuild hflags, because the implementation of DIT is a nop.
All you need is to record the pstate change. You may wish to add a comment
here about that, reminding the reader.
r~
