| Message ID | 1455912292-23807-5-git-send-email-edgar.iglesias@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 19 February 2016 at 20:04, Edgar E. Iglesias <edgar.iglesias@gmail.com> wrote: > From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com> > > Add the following flags to the data abort syndrome generator: > * isv - Instruction syndrome valid > * sas - Syndrome access size > * sse - Syndrome sign extend > * srt - Syndrome register transfer > * sf - Sixty-Four bit register width > * ar - Acquire/Release > > These flags are not yet used, so this patch has no functional change. > > Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com> > --- > target-arm/internals.h | 20 ++++++++++++++++++-- > target-arm/op_helper.c | 8 ++++++-- > 2 files changed, 24 insertions(+), 4 deletions(-) > > diff --git a/target-arm/internals.h b/target-arm/internals.h > index 34e2688..4e9d9f5 100644 > --- a/target-arm/internals.h > +++ b/target-arm/internals.h > @@ -383,13 +383,29 @@ static inline uint32_t syn_insn_abort(int same_el, int ea, int s1ptw, int fsc) > | (ea << 9) | (s1ptw << 7) | fsc; > } > > -static inline uint32_t syn_data_abort(int same_el, int ea, int cm, int s1ptw, > +static inline uint32_t syn_data_abort(int same_el, int isv, > + int sas, int sse, int srt, > + int sf, int ar, > + int ea, int cm, int s1ptw, > int wnr, int fsc, > bool is_16bit) Everywhere we call this (both now and once the full patchset has been applied) isv is a constant (either 0 or 1). I think it might be cleaner to define both a syn_data_abort_with_isv() and a syn_data_abort_no_isv(). Then the no_isv version doesn't need all the arguments that are zeroes. thanks -- PMM
On 25.02.2016 20:41, Peter Maydell wrote: > On 19 February 2016 at 20:04, Edgar E. Iglesias > <edgar.iglesias@gmail.com> wrote: >> From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com> >> >> Add the following flags to the data abort syndrome generator: >> * isv - Instruction syndrome valid >> * sas - Syndrome access size >> * sse - Syndrome sign extend >> * srt - Syndrome register transfer >> * sf - Sixty-Four bit register width >> * ar - Acquire/Release >> >> These flags are not yet used, so this patch has no functional change. >> >> Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com> >> --- >> target-arm/internals.h | 20 ++++++++++++++++++-- >> target-arm/op_helper.c | 8 ++++++-- >> 2 files changed, 24 insertions(+), 4 deletions(-) >> >> diff --git a/target-arm/internals.h b/target-arm/internals.h >> index 34e2688..4e9d9f5 100644 >> --- a/target-arm/internals.h >> +++ b/target-arm/internals.h >> @@ -383,13 +383,29 @@ static inline uint32_t syn_insn_abort(int same_el, int ea, int s1ptw, int fsc) >> | (ea << 9) | (s1ptw << 7) | fsc; >> } >> >> -static inline uint32_t syn_data_abort(int same_el, int ea, int cm, int s1ptw, >> +static inline uint32_t syn_data_abort(int same_el, int isv, >> + int sas, int sse, int srt, >> + int sf, int ar, >> + int ea, int cm, int s1ptw, >> int wnr, int fsc, >> bool is_16bit) > Everywhere we call this (both now and once the full patchset has > been applied) isv is a constant (either 0 or 1). I think it might > be cleaner to define both a syn_data_abort_with_isv() and a > syn_data_abort_no_isv(). Then the no_isv version doesn't need all > the arguments that are zeroes. Alternatively, we could define a function similar to LSInstructionSyndrome from ARMv8 ARM pseudocode, but which will pack the instruction syndrome from its components into a value of ISS<24:14>. Best regards, Sergey
On Thu, Feb 25, 2016 at 05:41:00PM +0000, Peter Maydell wrote: > On 19 February 2016 at 20:04, Edgar E. Iglesias > <edgar.iglesias@gmail.com> wrote: > > From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com> > > > > Add the following flags to the data abort syndrome generator: > > * isv - Instruction syndrome valid > > * sas - Syndrome access size > > * sse - Syndrome sign extend > > * srt - Syndrome register transfer > > * sf - Sixty-Four bit register width > > * ar - Acquire/Release > > > > These flags are not yet used, so this patch has no functional change. > > > > Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com> > > --- > > target-arm/internals.h | 20 ++++++++++++++++++-- > > target-arm/op_helper.c | 8 ++++++-- > > 2 files changed, 24 insertions(+), 4 deletions(-) > > > > diff --git a/target-arm/internals.h b/target-arm/internals.h > > index 34e2688..4e9d9f5 100644 > > --- a/target-arm/internals.h > > +++ b/target-arm/internals.h > > @@ -383,13 +383,29 @@ static inline uint32_t syn_insn_abort(int same_el, int ea, int s1ptw, int fsc) > > | (ea << 9) | (s1ptw << 7) | fsc; > > } > > > > -static inline uint32_t syn_data_abort(int same_el, int ea, int cm, int s1ptw, > > +static inline uint32_t syn_data_abort(int same_el, int isv, > > + int sas, int sse, int srt, > > + int sf, int ar, > > + int ea, int cm, int s1ptw, > > int wnr, int fsc, > > bool is_16bit) > > Everywhere we call this (both now and once the full patchset has > been applied) isv is a constant (either 0 or 1). I think it might > be cleaner to define both a syn_data_abort_with_isv() and a > syn_data_abort_no_isv(). Then the no_isv version doesn't need all > the arguments that are zeroes. Hi Peter, Finally getting back to this :-) In v3, I've added two separate functions as suggested. Cheers, Edgar
diff --git a/target-arm/internals.h b/target-arm/internals.h index 34e2688..4e9d9f5 100644 --- a/target-arm/internals.h +++ b/target-arm/internals.h @@ -383,13 +383,29 @@ static inline uint32_t syn_insn_abort(int same_el, int ea, int s1ptw, int fsc) | (ea << 9) | (s1ptw << 7) | fsc; } -static inline uint32_t syn_data_abort(int same_el, int ea, int cm, int s1ptw, +static inline uint32_t syn_data_abort(int same_el, int isv, + int sas, int sse, int srt, + int sf, int ar, + int ea, int cm, int s1ptw, int wnr, int fsc, bool is_16bit) { - return (EC_DATAABORT << ARM_EL_EC_SHIFT) | (same_el << ARM_EL_EC_SHIFT) + uint32_t v; + v = (EC_DATAABORT << ARM_EL_EC_SHIFT) | (same_el << ARM_EL_EC_SHIFT) | (is_16bit ? 0 : ARM_EL_IL) | (ea << 9) | (cm << 8) | (s1ptw << 7) | (wnr << 6) | fsc; + + /* Insn Syndrome fields are RES0 if ISV is unset. */ + if (isv) { + v |= (isv << 24) | (sas << 22) | (sse << 21) | (srt << 16) + | (sf << 15) | (ar << 14); + } else { + /* If ISV is zero, the IL field should be set to one. + * See ARM ARMv8 D7.2.27 for more details. + */ + v |= ARM_EL_IL; + } + return v; } static inline uint32_t syn_swstep(int same_el, int isv, int ex) diff --git a/target-arm/op_helper.c b/target-arm/op_helper.c index 7e845d5..2522d3c 100644 --- a/target-arm/op_helper.c +++ b/target-arm/op_helper.c @@ -115,7 +115,9 @@ void tlb_fill(CPUState *cs, target_ulong addr, int is_write, int mmu_idx, syn = syn_insn_abort(same_el, 0, fi.s1ptw, syn); exc = EXCP_PREFETCH_ABORT; } else { - syn = syn_data_abort(same_el, 0, 0, fi.s1ptw, is_write == 1, syn, + syn = syn_data_abort(same_el, + 0, 0, 0, 0, 0, 0, + 0, 0, fi.s1ptw, is_write == 1, syn, 1); if (is_write == 1 && arm_feature(env, ARM_FEATURE_V6)) { fsr |= (1 << 11); @@ -162,7 +164,9 @@ void arm_cpu_do_unaligned_access(CPUState *cs, vaddr vaddr, int is_write, } raise_exception(env, EXCP_DATA_ABORT, - syn_data_abort(same_el, 0, 0, 0, is_write == 1, 0x21, + syn_data_abort(same_el, + 0, 0, 0, 0, 0, 0, + 0, 0, 0, is_write == 1, 0x21, 1), target_el); }