[Qemu-devel] [PATCH v3 4/4] target-arm: Comments added to identify cases in a switch

[Thomas Hanson]

3 cases in a switch in disas_exc() require reference to the
ARM ARM spec in order to determine what case they're handling.

Signed-off-by: Thomas Hanson <thomas.han...@linaro.org>
---
 target-arm/translate-a64.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/target-arm/translate-a64.c b/target-arm/translate-a64.c
index b4a4b72..eb63e2f 100644
--- a/target-arm/translate-a64.c
+++ b/target-arm/translate-a64.c
@@ -1688,12 +1688,12 @@ static void disas_exc(DisasContext *s, uint32_t insn)
  * instruction works properly.
  */
 switch (op2_ll) {
-case 1:
+case 1: /* SVC */
 gen_ss_advance(s);
 gen_exception_insn(s, 0, EXCP_SWI, syn_aa64_svc(imm16),
default_exception_el(s));
 break;
-case 2:
+case 2: /* HVC */
 if (s->current_el == 0) {
 unallocated_encoding(s);
 break;
@@ -1706,7 +1706,7 @@ static void disas_exc(DisasContext *s, uint32_t insn)
 gen_ss_advance(s);
 gen_exception_insn(s, 0, EXCP_HVC, syn_aa64_hvc(imm16), 2);
 break;
-case 3:
+case 3: /* SMC */
 if (s->current_el == 0) {
 unallocated_encoding(s);
 break;
-- 
1.9.1

[Qemu-devel] [PATCH v3 2/4] target-arm: Code changes to implement overwrite of tag field on PC load

[Thomas Hanson]

For BR, BLR and RET instructions, if tagged addresses are enabled, the
tag field in the address must be cleared out prior to loading the
address into the PC.  Depending on the current EL, it will be set to
either all 0's or all 1's.

Signed-off-by: Thomas Hanson <thomas.han...@linaro.org>
---
 target-arm/translate-a64.c | 91 +++---
 target-arm/translate.h |  1 +
 2 files changed, 87 insertions(+), 5 deletions(-)

diff --git a/target-arm/translate-a64.c b/target-arm/translate-a64.c
index 3b15d2c..8321218 100644
--- a/target-arm/translate-a64.c
+++ b/target-arm/translate-a64.c
@@ -41,6 +41,7 @@ static TCGv_i64 cpu_pc;
 
 /* Load/store exclusive handling */
 static TCGv_i64 cpu_exclusive_high;
+static TCGv_i64 cpu_reg(DisasContext *s, int reg);
 
 static const char *regnames[] = {
 "x0", "x1", "x2", "x3", "x4", "x5", "x6", "x7",
@@ -176,6 +177,85 @@ void gen_a64_set_pc_im(uint64_t val)
 tcg_gen_movi_i64(cpu_pc, val);
 }
 
+/* Load the PC from a generic TCG variable.
+ *
+ * If address tagging is enabled via the TCR TBI bits, then loading
+ * an address into the PC will clear out any tag in the it:
+ *  + for EL2 and EL3 there is only one TBI bit, and if it is set
+ *then the address is zero-extended, clearing bits [63:56]
+ *  + for EL0 and EL1, TBI0 controls addresses with bit 55 == 0
+ *and TBI1 controls addressses with bit 55 == 1.
+ *If the appropriate TBI bit is set for the address then
+ *the address is sign-extended from bit 55 into bits [63:56]
+ *
+ * We can avoid doing this for relative-branches, because the
+ * PC + offset can never overflow into the tag bits (assuming
+ * that virtual addresses are less than 56 bits wide, as they
+ * are currently), but we must handle it for branch-to-register.
+ */
+static void gen_a64_set_pc_var(DisasContext *s, TCGv_i64 src)
+{
+
+if (s->current_el <= 1) {
+/* Test if NEITHER or BOTH TBI values are set.  If so, no need to
+ * examine bit 55 of address, can just generate code.
+ * If mixed, then test via generated code
+ */
+if (s->tbi0 && s->tbi1) {
+TCGv_i64 tmp_reg = tcg_temp_new_i64();
+/* Both bits set, sign extension from bit 55 into [63:56] will
+ * cover both cases
+ */
+tcg_gen_shli_i64(tmp_reg, src, 8);
+tcg_gen_sari_i64(cpu_pc, tmp_reg, 8);
+tcg_temp_free_i64(tmp_reg);
+} else if (!s->tbi0 && !s->tbi1) {
+/* Neither bit set, just load it as-is */
+tcg_gen_mov_i64(cpu_pc, src);
+} else {
+TCGv_i64 tcg_tmpval = tcg_temp_new_i64();
+TCGv_i64 tcg_bit55  = tcg_temp_new_i64();
+TCGv_i64 tcg_zero   = tcg_const_i64(0);
+
+tcg_gen_andi_i64(tcg_bit55, src, (1ull << 55));
+
+if (s->tbi0) {
+/* tbi0==1, tbi1==0, so 0-fill upper byte if bit 55 = 0 */
+tcg_gen_andi_i64(tcg_tmpval, src,
+ 0x00FFull);
+tcg_gen_movcond_i64(TCG_COND_EQ, cpu_pc, tcg_bit55, tcg_zero,
+tcg_tmpval, src);
+} else {
+/* tbi0==0, tbi1==1, so 1-fill upper byte if bit 55 = 1 */
+tcg_gen_ori_i64(tcg_tmpval, src,
+0xFF00ull);
+tcg_gen_movcond_i64(TCG_COND_NE, cpu_pc, tcg_bit55, tcg_zero,
+tcg_tmpval, src);
+}
+tcg_temp_free_i64(tcg_zero);
+tcg_temp_free_i64(tcg_bit55);
+tcg_temp_free_i64(tcg_tmpval);
+}
+} else {  /* EL > 1 */
+if (s->tbi0) {
+/* Force tag byte to all zero */
+tcg_gen_andi_i64(cpu_pc, src, 0x00FFull);
+} else {
+/* Load unmodified address */
+tcg_gen_mov_i64(cpu_pc, src);
+}
+ }
+}
+
+/* Load the PC from a register.
+ *
+ * Convert register into a TCG variable and call gen_a64_set_pc_var()
+ */
+void gen_a64_set_pc_reg(DisasContext *s, unsigned int rn)
+{
+gen_a64_set_pc_var(s, cpu_reg(s, rn));
+}
+
 typedef struct DisasCompare64 {
 TCGCond cond;
 TCGv_i64 value;
@@ -1704,12 +1784,13 @@ static void disas_uncond_b_reg(DisasContext *s, 
uint32_t insn)
 
 switch (opc) {
 case 0: /* BR */
-case 2: /* RET */
-tcg_gen_mov_i64(cpu_pc, cpu_reg(s, rn));
-break;
 case 1: /* BLR */
-tcg_gen_mov_i64(cpu_pc, cpu_reg(s, rn));
-tcg_gen_movi_i64(cpu_reg(s, 30), s->pc);
+case 2: /* RET */
+gen_a64_set_pc_reg(s, rn);
+/* BLR also needs to load return address */
+if (opc == 1) {
+tcg_gen_movi_i64(cpu_reg(s, 30), s->pc);
+}
 break;
 cas

[Qemu-devel] [PATCH v3 3/4] target-arm: Comments to mark location of pending work for 56 bit addresses

[Thomas Hanson]

Certain instructions which can not directly load a tagged address value
may trigger a corner case when the address size is 56 bits.  This is
because incrementing or offsetting from the current PC can cause an
arithetic roll-over into the tag bits.  Per the ARM ARM spec, these cases
should also be addressed by cleaning up the tag field.

Signed-off-by: Thomas Hanson <thomas.han...@linaro.org>
---
 target-arm/translate-a64.c | 12 
 1 file changed, 12 insertions(+)

diff --git a/target-arm/translate-a64.c b/target-arm/translate-a64.c
index 8321218..b4a4b72 100644
--- a/target-arm/translate-a64.c
+++ b/target-arm/translate-a64.c
@@ -1232,6 +1232,9 @@ static inline AArch64DecodeFn *lookup_disas_fn(const 
AArch64DecodeTable *table,
  */
 static void disas_uncond_b_imm(DisasContext *s, uint32_t insn)
 {
+/*If/when address size is 56 bits, this could overflow into address tag
+ * byte, and that byte should be fixed per ARM ARM spec.
+ */
 uint64_t addr = s->pc + sextract32(insn, 0, 26) * 4 - 4;
 
 if (insn & (1U << 31)) {
@@ -1259,6 +1262,9 @@ static void disas_comp_b_imm(DisasContext *s, uint32_t 
insn)
 sf = extract32(insn, 31, 1);
 op = extract32(insn, 24, 1); /* 0: CBZ; 1: CBNZ */
 rt = extract32(insn, 0, 5);
+/*If/when address size is 56 bits, this could overflow into address tag
+ * byte, and that byte should be fixed per ARM ARM spec.
+ */
 addr = s->pc + sextract32(insn, 5, 19) * 4 - 4;
 
 tcg_cmp = read_cpu_reg(s, rt, sf);
@@ -1287,6 +1293,9 @@ static void disas_test_b_imm(DisasContext *s, uint32_t 
insn)
 
 bit_pos = (extract32(insn, 31, 1) << 5) | extract32(insn, 19, 5);
 op = extract32(insn, 24, 1); /* 0: TBZ; 1: TBNZ */
+/*If/when address size is 56 bits, this could overflow into address tag
+ * byte, and that byte should be fixed per ARM ARM spec.
+ */
 addr = s->pc + sextract32(insn, 5, 14) * 4 - 4;
 rt = extract32(insn, 0, 5);
 
@@ -1316,6 +1325,9 @@ static void disas_cond_b_imm(DisasContext *s, uint32_t 
insn)
 unallocated_encoding(s);
 return;
 }
+/*If/when address size is 56 bits, this could overflow into address tag
+ * byte, and that byte should be fixed per ARM ARM spec.
+ */
 addr = s->pc + sextract32(insn, 5, 19) * 4 - 4;
 cond = extract32(insn, 0, 4);
 
-- 
1.9.1

[Qemu-devel] [PATCH v3 0/4] target-arm: Handle tagged addresses when loading PC

[Thomas Hanson]

If tagged addresses are enabled, then addresses being loaded into the 
PC must be cleaned up by overwriting the tag bits with either all 0's 
or all 1's as specified in the ARM ARM spec.  The decision process is 
dependent on whether the code will be running in EL0/1 or in EL2/3 and 
is controlled by a combination of Top Byte Ignored (TBI) bits in the 
TCR and the value of bit 55 in the address being loaded. 

TBI values are extracted from the appropriate TCR and made available 
to TCG code generation routines by inserting them into the TB flags 
field and then transferring them to DisasContext structure in 
gen_intermediate_code_a64().

New function gen_a64_set_pc_var() encapsulates the logic required to 
determine whether clean up of the tag byte is required and then 
generating the code to correctly load the PC. New function 
gen_a64_set_pc_reg() accepts a register number and then calls
gen_a64_set_pc_var().
  
In addition to those instruction which can directly load a tagged 
address into the PC, there are others which increment or add a value to
the PC.  If 56 bit addressing is used, these instructions can cause an 
arithmetic roll-over into the tag bits.  The ARM ARM specification for 
handling tagged addresses requires that these cases also be addressed
by cleaning up the tag field.  This work has been deferred because 
there is currently no CPU model available for testing with 56 bit 
addresses.

v1->v2:
  - Updated patch descriptions per Peter's commments
  - Added function header and other comments as recommended
  - Change return type from long to unit32_t for arm_regime_tbi0() &
arm_regime_tbi1()
  - Moved prototype of gen_a64_set_pc_reg() from patch 1 to patch 2
  - Moved assignment of dc->tbi0 & dc->tbi1 from patch 2 to patch 1
  - Split out documentation comments into separate patch.

v2->v3
  - Split gen_a64_set_pc_reg() into 2 functions:
* gen_a64_set_pc_var() which takes a TCGv_i64 argument and 
* gen_a64_set_pc_reg() which takes a register number and calls 
  gen_a64_set_pc_var() after mapping the register to a variable

  Still looking into handling of tagged addresses for exceptions and
  exception returns.  Will handle that as a separate patch set.


Thomas Hanson (4):
  target-arm: Infrastucture changes to enable handling of tagged address
loading into PC
  target-arm: Code changes to implement overwrite of tag field on PC
load
  target-arm: Comments to mark location of pending work for 56 bit
addresses
  target-arm: Comments added to identify cases in a switch

[Qemu-devel] [PATCH v3 1/4] target-arm: Infrastucture changes to enable handling of tagged address loading into PC

[Thomas Hanson]

When capturing the current CPU state for the TB, extract the TBI0 and TBI1
values from the correct TCR for the current EL and then add them to the TB
flags field.

Then, at the start of code generation for the block, copy the TBI fields
into the DisasContext structure.

Signed-off-by: Thomas Hanson <thomas.han...@linaro.org>
---
 target-arm/cpu.h   | 39 +--
 target-arm/helper.c| 46 ++
 target-arm/translate-a64.c |  2 ++
 target-arm/translate.h |  2 ++
 4 files changed, 87 insertions(+), 2 deletions(-)

diff --git a/target-arm/cpu.h b/target-arm/cpu.h
index 76d824d..699e6e5 100644
--- a/target-arm/cpu.h
+++ b/target-arm/cpu.h
@@ -2191,7 +2191,11 @@ static inline bool 
arm_cpu_data_is_big_endian(CPUARMState *env)
 #define ARM_TBFLAG_BE_DATA_SHIFT20
 #define ARM_TBFLAG_BE_DATA_MASK (1 << ARM_TBFLAG_BE_DATA_SHIFT)
 
-/* Bit usage when in AArch64 state: currently we have no A64 specific bits */
+/* Bit usage when in AArch64 state */
+#define ARM_TBFLAG_TBI0_SHIFT 0/* TBI0 for EL0/1 or TBI for EL2/3 */
+#define ARM_TBFLAG_TBI0_MASK (0x1ull << ARM_TBFLAG_TBI0_SHIFT)
+#define ARM_TBFLAG_TBI1_SHIFT 1/* TBI1 for EL0/1  */
+#define ARM_TBFLAG_TBI1_MASK (0x1ull << ARM_TBFLAG_TBI1_SHIFT)
 
 /* some convenience accessor macros */
 #define ARM_TBFLAG_AARCH64_STATE(F) \
@@ -,6 +2226,10 @@ static inline bool 
arm_cpu_data_is_big_endian(CPUARMState *env)
 (((F) & ARM_TBFLAG_NS_MASK) >> ARM_TBFLAG_NS_SHIFT)
 #define ARM_TBFLAG_BE_DATA(F) \
 (((F) & ARM_TBFLAG_BE_DATA_MASK) >> ARM_TBFLAG_BE_DATA_SHIFT)
+#define ARM_TBFLAG_TBI0(F) \
+(((F) & ARM_TBFLAG_TBI0_MASK) >> ARM_TBFLAG_TBI0_SHIFT)
+#define ARM_TBFLAG_TBI1(F) \
+(((F) & ARM_TBFLAG_TBI1_MASK) >> ARM_TBFLAG_TBI1_SHIFT)
 
 static inline bool bswap_code(bool sctlr_b)
 {
@@ -2319,12 +2327,38 @@ static inline bool arm_cpu_bswap_data(CPUARMState *env)
 }
 #endif
 
+/**
+ * arm_regime_tbi0:
+ * @env: CPUARMState
+ * @mmu_idx: MMU index indicating required translation regime
+ *
+ * Extracts the TBI0 value from the appropriate TCR for the current EL
+ *
+ * Returns: the TBI0 value.
+ */
+extern uint32_t arm_regime_tbi0(CPUARMState *env, ARMMMUIdx mmu_idx);
+
+/**
+ * arm_regime_tbi1:
+ * @env: CPUARMState
+ * @mmu_idx: MMU index indicating required translation regime
+ *
+ * Extracts the TBI1 value from the appropriate TCR for the current EL
+ *
+ * Returns: the TBI1 value.
+ */
+extern uint32_t arm_regime_tbi1(CPUARMState *env, ARMMMUIdx mmu_idx);
+
 static inline void cpu_get_tb_cpu_state(CPUARMState *env, target_ulong *pc,
 target_ulong *cs_base, uint32_t *flags)
 {
+ARMMMUIdx mmu_idx = cpu_mmu_index(env, false);
 if (is_a64(env)) {
 *pc = env->pc;
 *flags = ARM_TBFLAG_AARCH64_STATE_MASK;
+/* Get control bits for tagged addresses */
+*flags |= (arm_regime_tbi0(env, mmu_idx) << ARM_TBFLAG_TBI0_SHIFT);
+*flags |= (arm_regime_tbi1(env, mmu_idx) << ARM_TBFLAG_TBI1_SHIFT);
 } else {
 *pc = env->regs[15];
 *flags = (env->thumb << ARM_TBFLAG_THUMB_SHIFT)
@@ -2343,7 +2377,8 @@ static inline void cpu_get_tb_cpu_state(CPUARMState *env, 
target_ulong *pc,
<< ARM_TBFLAG_XSCALE_CPAR_SHIFT);
 }
 
-*flags |= (cpu_mmu_index(env, false) << ARM_TBFLAG_MMUIDX_SHIFT);
+*flags |= (mmu_idx << ARM_TBFLAG_MMUIDX_SHIFT);
+
 /* The SS_ACTIVE and PSTATE_SS bits correspond to the state machine
  * states defined in the ARM ARM for software singlestep:
  *  SS_ACTIVE   PSTATE.SS   State
diff --git a/target-arm/helper.c b/target-arm/helper.c
index 25f612d..70e2742 100644
--- a/target-arm/helper.c
+++ b/target-arm/helper.c
@@ -6720,6 +6720,52 @@ static inline TCR *regime_tcr(CPUARMState *env, 
ARMMMUIdx mmu_idx)
 return >cp15.tcr_el[regime_el(env, mmu_idx)];
 }
 
+/* Returns TBI0 value for current regime el */
+uint32_t arm_regime_tbi0(CPUARMState *env, ARMMMUIdx mmu_idx)
+{
+TCR *tcr;
+uint32_t el;
+
+/* For EL0 and EL1, TBI is controlled by stage 1's TCR, so convert
+   * a stage 1+2 mmu index into the appropriate stage 1 mmu index.
+   */
+if (mmu_idx == ARMMMUIdx_S12NSE0 || mmu_idx == ARMMMUIdx_S12NSE1) {
+mmu_idx += ARMMMUIdx_S1NSE0;
+}
+
+tcr = regime_tcr(env, mmu_idx);
+el = regime_el(env, mmu_idx);
+
+if (el > 1) {
+return extract64(tcr->raw_tcr, 20, 1);
+} else {
+return extract64(tcr->raw_tcr, 37, 1);
+}
+}
+
+/* Returns TBI1 value for current regime el */
+uint32_t arm_regime_tbi1(CPUARMState *env, ARMMMUIdx mmu_idx)
+{
+TCR *tcr;
+uint32_t el;
+
+/* For EL0 and EL1, TBI is controlled by stage 1's TCR, so convert
+   * a stage 1+2 mmu index into the appropriate stage 1 mmu index.

Re: [Qemu-devel] [PATCH 2/3] target-arm: Code changes to implement overwrite of tag field on PC load

[Thomas Hanson]

On 5 October 2016 at 16:01, Peter Maydell <peter.mayd...@linaro.org> wrote:

> On 5 October 2016 at 14:53, Tom Hanson <thomas.han...@linaro.org> wrote:
> > On 09/29/2016 07:24 PM, Peter Maydell wrote:
> >> On 16 September 2016 at 10:34, Thomas Hanson <thomas.han...@linaro.org>
> wrote:
> >>> +void gen_a64_set_pc_reg(DisasContext *s, unsigned int rn)
> >>
> >> I think it would be better to take a TCGv_i64 here rather than
> >> unsigned int rn (ie have the caller do the cpu_reg(s, rn)).
> >> (You probably don't need that prototype of cpu_reg() above if
> >> you do this, though that's not why it's better.)
> >>
> > Why would this be better?
> >
> > To me, the caller has a register number and wants that register used to
> load
> > the PC.  So, it passes in the register number.
> >
> > The fact that gen_a64_set_pc_reg() needs to convert that into a TCGv_i64
> is
> > an implementation detail that should be encapsulated/hidden from the
> caller.
> >
> > If the desire is to eliminate the multiple cpu_reg() calls inside of
> > gen_a64_set_pc_reg() then that mapping could be done at the top of the
> > function before the outer if().
>
> It matches the style of the rest of the code which generally
> prefers to convert register numbers into TCGv earlier rather
> than later (at the level which is doing decode of instruction
> bits, rather than inside utility functions), and gives you a
> more flexible utility function, which can do a "write value to PC"
> for any value, not just something that happens to be in a CPU
> register. And as you say it avoids calling cpu_reg() multiple times
> as a side benefit.
>
> thanks
> -- PMM
>

Sorry,  didn't see this before I submitted v2.  Even now I can't get
Thunderbird to display it.  Sigh.  (Apologies if this comes through with
bars on the side, I have to use the web mail interface in order to be able
to respond.)

This approach seems counter to both structured and OO design principles
which would push common code (like type conversion) down into the lower
level function in order to increase re-use and minimize code duplication.
Those principles suggest that if we need a gen_a64_set_pc_value() function
that can load the PC from something other than a register or an immediate,
then it should be a lower level function than, and be called by,
gen_a64_set_pc_reg().  This also has the benefit of reducing clutter in the
caller, making it more readable and more maintainable.

As a separate issue, we now have functions to load the PC from an immediate
value and from a register.  Where else could we legitimately load the PC
from?

[Qemu-devel] [PATCH v2 1/4] target-arm: Infrastucture changes to enable handling of tagged address loading into PC

[Thomas Hanson]

When capturing the current CPU state for the TB, extract the TBI0 and TBI1
values from the correct TCR for the current EL and then add them to the TB
flags field.

Then, at the start of code generation for the block, copy the TBI fields
into the DisasContext structure.

Signed-off-by: Thomas Hanson <thomas.han...@linaro.org>
---
 target-arm/cpu.h   | 39 +--
 target-arm/helper.c| 46 ++
 target-arm/translate-a64.c |  2 ++
 target-arm/translate.h |  2 ++
 4 files changed, 87 insertions(+), 2 deletions(-)

diff --git a/target-arm/cpu.h b/target-arm/cpu.h
index 76d824d..699e6e5 100644
--- a/target-arm/cpu.h
+++ b/target-arm/cpu.h
@@ -2191,7 +2191,11 @@ static inline bool 
arm_cpu_data_is_big_endian(CPUARMState *env)
 #define ARM_TBFLAG_BE_DATA_SHIFT20
 #define ARM_TBFLAG_BE_DATA_MASK (1 << ARM_TBFLAG_BE_DATA_SHIFT)
 
-/* Bit usage when in AArch64 state: currently we have no A64 specific bits */
+/* Bit usage when in AArch64 state */
+#define ARM_TBFLAG_TBI0_SHIFT 0/* TBI0 for EL0/1 or TBI for EL2/3 */
+#define ARM_TBFLAG_TBI0_MASK (0x1ull << ARM_TBFLAG_TBI0_SHIFT)
+#define ARM_TBFLAG_TBI1_SHIFT 1/* TBI1 for EL0/1  */
+#define ARM_TBFLAG_TBI1_MASK (0x1ull << ARM_TBFLAG_TBI1_SHIFT)
 
 /* some convenience accessor macros */
 #define ARM_TBFLAG_AARCH64_STATE(F) \
@@ -,6 +2226,10 @@ static inline bool 
arm_cpu_data_is_big_endian(CPUARMState *env)
 (((F) & ARM_TBFLAG_NS_MASK) >> ARM_TBFLAG_NS_SHIFT)
 #define ARM_TBFLAG_BE_DATA(F) \
 (((F) & ARM_TBFLAG_BE_DATA_MASK) >> ARM_TBFLAG_BE_DATA_SHIFT)
+#define ARM_TBFLAG_TBI0(F) \
+(((F) & ARM_TBFLAG_TBI0_MASK) >> ARM_TBFLAG_TBI0_SHIFT)
+#define ARM_TBFLAG_TBI1(F) \
+(((F) & ARM_TBFLAG_TBI1_MASK) >> ARM_TBFLAG_TBI1_SHIFT)
 
 static inline bool bswap_code(bool sctlr_b)
 {
@@ -2319,12 +2327,38 @@ static inline bool arm_cpu_bswap_data(CPUARMState *env)
 }
 #endif
 
+/**
+ * arm_regime_tbi0:
+ * @env: CPUARMState
+ * @mmu_idx: MMU index indicating required translation regime
+ *
+ * Extracts the TBI0 value from the appropriate TCR for the current EL
+ *
+ * Returns: the TBI0 value.
+ */
+extern uint32_t arm_regime_tbi0(CPUARMState *env, ARMMMUIdx mmu_idx);
+
+/**
+ * arm_regime_tbi1:
+ * @env: CPUARMState
+ * @mmu_idx: MMU index indicating required translation regime
+ *
+ * Extracts the TBI1 value from the appropriate TCR for the current EL
+ *
+ * Returns: the TBI1 value.
+ */
+extern uint32_t arm_regime_tbi1(CPUARMState *env, ARMMMUIdx mmu_idx);
+
 static inline void cpu_get_tb_cpu_state(CPUARMState *env, target_ulong *pc,
 target_ulong *cs_base, uint32_t *flags)
 {
+ARMMMUIdx mmu_idx = cpu_mmu_index(env, false);
 if (is_a64(env)) {
 *pc = env->pc;
 *flags = ARM_TBFLAG_AARCH64_STATE_MASK;
+/* Get control bits for tagged addresses */
+*flags |= (arm_regime_tbi0(env, mmu_idx) << ARM_TBFLAG_TBI0_SHIFT);
+*flags |= (arm_regime_tbi1(env, mmu_idx) << ARM_TBFLAG_TBI1_SHIFT);
 } else {
 *pc = env->regs[15];
 *flags = (env->thumb << ARM_TBFLAG_THUMB_SHIFT)
@@ -2343,7 +2377,8 @@ static inline void cpu_get_tb_cpu_state(CPUARMState *env, 
target_ulong *pc,
<< ARM_TBFLAG_XSCALE_CPAR_SHIFT);
 }
 
-*flags |= (cpu_mmu_index(env, false) << ARM_TBFLAG_MMUIDX_SHIFT);
+*flags |= (mmu_idx << ARM_TBFLAG_MMUIDX_SHIFT);
+
 /* The SS_ACTIVE and PSTATE_SS bits correspond to the state machine
  * states defined in the ARM ARM for software singlestep:
  *  SS_ACTIVE   PSTATE.SS   State
diff --git a/target-arm/helper.c b/target-arm/helper.c
index 25f612d..70e2742 100644
--- a/target-arm/helper.c
+++ b/target-arm/helper.c
@@ -6720,6 +6720,52 @@ static inline TCR *regime_tcr(CPUARMState *env, 
ARMMMUIdx mmu_idx)
 return >cp15.tcr_el[regime_el(env, mmu_idx)];
 }
 
+/* Returns TBI0 value for current regime el */
+uint32_t arm_regime_tbi0(CPUARMState *env, ARMMMUIdx mmu_idx)
+{
+TCR *tcr;
+uint32_t el;
+
+/* For EL0 and EL1, TBI is controlled by stage 1's TCR, so convert
+   * a stage 1+2 mmu index into the appropriate stage 1 mmu index.
+   */
+if (mmu_idx == ARMMMUIdx_S12NSE0 || mmu_idx == ARMMMUIdx_S12NSE1) {
+mmu_idx += ARMMMUIdx_S1NSE0;
+}
+
+tcr = regime_tcr(env, mmu_idx);
+el = regime_el(env, mmu_idx);
+
+if (el > 1) {
+return extract64(tcr->raw_tcr, 20, 1);
+} else {
+return extract64(tcr->raw_tcr, 37, 1);
+}
+}
+
+/* Returns TBI1 value for current regime el */
+uint32_t arm_regime_tbi1(CPUARMState *env, ARMMMUIdx mmu_idx)
+{
+TCR *tcr;
+uint32_t el;
+
+/* For EL0 and EL1, TBI is controlled by stage 1's TCR, so convert
+   * a stage 1+2 mmu index into the appropriate stage 1 mmu index.

[Qemu-devel] [PATCH v2 0/4] target-arm: Handle tagged addresses when loading PC

[Thomas Hanson]

If tagged addresses are enabled, then addresses being loaded into the 
PC must be cleaned up by overwriting the tag bits with either all 0's 
or all 1's as specified in the ARM ARM spec.  The decision process is 
dependent on whether the code will be running in EL0/1 or in EL2/3 and 
is controlled by a combination of Top Byte Ignored (TBI) bits in the 
TCR and the value of bit 55 in the address being loaded. 

TBI values are extracted from the appropriate TCR and made available 
to TCG code generation routines by inserting them into the TB flags 
field and then transferring them to DisasContext structure in 
gen_intermediate_code_a64().

New function gen_a64_set_pc_reg() encapsulates the logic required to 
determine whether clean up of the tag byte is required and then 
generating the code to correctly load the PC.
  
In addition to those instruction which can directly load a tagged 
address into the PC, there are others which increment or add a value to
the PC.  If 56 bit addressing is used, these instructions can cause an 
arithmetic roll-over into the tag bits.  The ARM ARM specification for 
handling tagged addresses requires that these cases also be addressed
by cleaning up the tag field.  This work has been deferred because 
there is currently no CPU model available for testing with 56 bit 
addresses.

v1->v2:
  - Updated patch descriptions per Peter's commments
  - Added function header and other comments as recommended
  - Change return type from long to unit32_t for arm_regime_tbi0() &
arm_regime_tbi1()
  - Moved prototype of gen_a64_set_pc_reg() from patch 1 to patch 2
  - Moved assignment of dc->tbi0 & dc->tbi1 from patch 2 to patch 1
  - Split out documentation comments into separate patch.

  Still looking into handling of tagged addresses for exceptions and
  exception returns.  Will handle that as a separate patch set.


Thomas Hanson (4):
  target-arm: Infrastucture changes to enable handling of tagged address
loading into PC
  target-arm: Code changes to implement overwrite of tag field on PC
load
  target-arm: Comments to mark location of pending work for 56 bit
addresses
  target-arm: Comments added to identify cases in a switch

 target-arm/cpu.h   |  39 -
 target-arm/helper.c|  46 +
 target-arm/translate-a64.c | 101 +
 target-arm/translate.h |   3 ++
 4 files changed, 179 insertions(+), 10 deletions(-)

-- 
1.9.1

[Qemu-devel] [PATCH v2 4/4] target-arm: Comments added to identify cases in a switch

[Thomas Hanson]

3 cases in a switch in disas_exc() require reference to the
ARM ARM spec in order to determine what case they're handling.

Signed-off-by: Thomas Hanson <thomas.han...@linaro.org>
---
 target-arm/translate-a64.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/target-arm/translate-a64.c b/target-arm/translate-a64.c
index a1e5f2c..728e929 100644
--- a/target-arm/translate-a64.c
+++ b/target-arm/translate-a64.c
@@ -1678,12 +1678,12 @@ static void disas_exc(DisasContext *s, uint32_t insn)
  * instruction works properly.
  */
 switch (op2_ll) {
-case 1:
+case 1: /* SVC */
 gen_ss_advance(s);
 gen_exception_insn(s, 0, EXCP_SWI, syn_aa64_svc(imm16),
default_exception_el(s));
 break;
-case 2:
+case 2: /* HVC */
 if (s->current_el == 0) {
 unallocated_encoding(s);
 break;
@@ -1696,7 +1696,7 @@ static void disas_exc(DisasContext *s, uint32_t insn)
 gen_ss_advance(s);
 gen_exception_insn(s, 0, EXCP_HVC, syn_aa64_hvc(imm16), 2);
 break;
-case 3:
+case 3: /* SMC */
 if (s->current_el == 0) {
 unallocated_encoding(s);
 break;
-- 
1.9.1

[Qemu-devel] [PATCH v2 3/4] target-arm: Comments to mark location of pending work for 56 bit addresses

[Thomas Hanson]

Certain instructions which can not directly load a tagged address value
may trigger a corner case when the address size is 56 bits.  This is
because incrementing or offsetting from the current PC can cause an
arithetic roll-over into the tag bits.  Per the ARM ARM spec, these cases
should also be addressed by cleaning up the tag field.

Signed-off-by: Thomas Hanson <thomas.han...@linaro.org>
---
 target-arm/translate-a64.c | 12 
 1 file changed, 12 insertions(+)

diff --git a/target-arm/translate-a64.c b/target-arm/translate-a64.c
index 14e91fb..a1e5f2c 100644
--- a/target-arm/translate-a64.c
+++ b/target-arm/translate-a64.c
@@ -1222,6 +1222,9 @@ static inline AArch64DecodeFn *lookup_disas_fn(const 
AArch64DecodeTable *table,
  */
 static void disas_uncond_b_imm(DisasContext *s, uint32_t insn)
 {
+/*If/when address size is 56 bits, this could overflow into address tag
+ * byte, and that byte should be fixed per ARM ARM spec.
+ */
 uint64_t addr = s->pc + sextract32(insn, 0, 26) * 4 - 4;
 
 if (insn & (1U << 31)) {
@@ -1249,6 +1252,9 @@ static void disas_comp_b_imm(DisasContext *s, uint32_t 
insn)
 sf = extract32(insn, 31, 1);
 op = extract32(insn, 24, 1); /* 0: CBZ; 1: CBNZ */
 rt = extract32(insn, 0, 5);
+/*If/when address size is 56 bits, this could overflow into address tag
+ * byte, and that byte should be fixed per ARM ARM spec.
+ */
 addr = s->pc + sextract32(insn, 5, 19) * 4 - 4;
 
 tcg_cmp = read_cpu_reg(s, rt, sf);
@@ -1277,6 +1283,9 @@ static void disas_test_b_imm(DisasContext *s, uint32_t 
insn)
 
 bit_pos = (extract32(insn, 31, 1) << 5) | extract32(insn, 19, 5);
 op = extract32(insn, 24, 1); /* 0: TBZ; 1: TBNZ */
+/*If/when address size is 56 bits, this could overflow into address tag
+ * byte, and that byte should be fixed per ARM ARM spec.
+ */
 addr = s->pc + sextract32(insn, 5, 14) * 4 - 4;
 rt = extract32(insn, 0, 5);
 
@@ -1306,6 +1315,9 @@ static void disas_cond_b_imm(DisasContext *s, uint32_t 
insn)
 unallocated_encoding(s);
 return;
 }
+/*If/when address size is 56 bits, this could overflow into address tag
+ * byte, and that byte should be fixed per ARM ARM spec.
+ */
 addr = s->pc + sextract32(insn, 5, 19) * 4 - 4;
 cond = extract32(insn, 0, 4);
 
-- 
1.9.1

[Qemu-devel] [PATCH v2 2/4] target-arm: Code changes to implement overwrite of tag field on PC load

[Thomas Hanson]

For BR, BLR and RET instructions, if tagged addresses are enabled, the
tag field in the address must be cleared out prior to loading the
address into the PC.  Depending on the current EL, it will be set to
either all 0's or all 1's.

Signed-off-by: Thomas Hanson <thomas.han...@linaro.org>
---
 target-arm/translate-a64.c | 81 +++---
 target-arm/translate.h |  1 +
 2 files changed, 77 insertions(+), 5 deletions(-)

diff --git a/target-arm/translate-a64.c b/target-arm/translate-a64.c
index 3b15d2c..14e91fb 100644
--- a/target-arm/translate-a64.c
+++ b/target-arm/translate-a64.c
@@ -41,6 +41,7 @@ static TCGv_i64 cpu_pc;
 
 /* Load/store exclusive handling */
 static TCGv_i64 cpu_exclusive_high;
+static TCGv_i64 cpu_reg(DisasContext *s, int reg);
 
 static const char *regnames[] = {
 "x0", "x1", "x2", "x3", "x4", "x5", "x6", "x7",
@@ -176,6 +177,75 @@ void gen_a64_set_pc_im(uint64_t val)
 tcg_gen_movi_i64(cpu_pc, val);
 }
 
+/* Load the PC from a register.
+ *
+ * If address tagging is enabled via the TCR TBI bits, then loading
+ * an address into the PC will clear out any tag in the it:
+ *  + for EL2 and EL3 there is only one TBI bit, and if it is set
+ *then the address is zero-extended, clearing bits [63:56]
+ *  + for EL0 and EL1, TBI0 controls addresses with bit 55 == 0
+ *and TBI1 controls addressses with bit 55 == 1.
+ *If the appropriate TBI bit is set for the address then
+ *the address is sign-extended from bit 55 into bits [63:56]
+ *
+ * We can avoid doing this for relative-branches, because the
+ * PC + offset can never overflow into the tag bits (assuming
+ * that virtual addresses are less than 56 bits wide, as they
+ * are currently), but we must handle it for branch-to-register.
+ */
+void gen_a64_set_pc_reg(DisasContext *s, unsigned int rn)
+{
+if (s->current_el <= 1) {
+/* Test if NEITHER or BOTH TBI values are set.  If so, no need to
+ * examine bit 55 of address, can just generate code.
+ * If mixed, then test via generated code
+ */
+if (s->tbi0 && s->tbi1) {
+TCGv_i64 tmp_reg = tcg_temp_new_i64();
+/* Both bits set, sign extension from bit 55 into [63:56] will
+ * cover both cases
+ */
+tcg_gen_shli_i64(tmp_reg, cpu_reg(s, rn), 8);
+tcg_gen_sari_i64(cpu_pc, tmp_reg, 8);
+tcg_temp_free_i64(tmp_reg);
+} else if (!s->tbi0 && !s->tbi1) {
+/* Neither bit set, just load it as-is */
+tcg_gen_mov_i64(cpu_pc, cpu_reg(s, rn));
+} else {
+TCGv_i64 tcg_tmpval = tcg_temp_new_i64();
+TCGv_i64 tcg_bit55  = tcg_temp_new_i64();
+TCGv_i64 tcg_zero   = tcg_const_i64(0);
+
+tcg_gen_andi_i64(tcg_bit55, cpu_reg(s, rn), (1ull << 55));
+
+if (s->tbi0) {
+/* tbi0==1, tbi1==0, so 0-fill upper byte if bit 55 = 0 */
+tcg_gen_andi_i64(tcg_tmpval, cpu_reg(s, rn),
+ 0x00FFull);
+tcg_gen_movcond_i64(TCG_COND_EQ, cpu_pc, tcg_bit55, tcg_zero,
+tcg_tmpval, cpu_reg(s, rn));
+} else {
+/* tbi0==0, tbi1==1, so 1-fill upper byte if bit 55 = 1 */
+tcg_gen_ori_i64(tcg_tmpval, cpu_reg(s, rn),
+0xFF00ull);
+tcg_gen_movcond_i64(TCG_COND_NE, cpu_pc, tcg_bit55, tcg_zero,
+tcg_tmpval, cpu_reg(s, rn));
+}
+tcg_temp_free_i64(tcg_zero);
+tcg_temp_free_i64(tcg_bit55);
+tcg_temp_free_i64(tcg_tmpval);
+}
+} else {  /* EL > 1 */
+if (s->tbi0) {
+/* Force tag byte to all zero */
+tcg_gen_andi_i64(cpu_pc, cpu_reg(s, rn), 0x00FFull);
+} else {
+/* Load unmodified address */
+tcg_gen_mov_i64(cpu_pc, cpu_reg(s, rn));
+}
+ }
+}
+
 typedef struct DisasCompare64 {
 TCGCond cond;
 TCGv_i64 value;
@@ -1704,12 +1774,13 @@ static void disas_uncond_b_reg(DisasContext *s, 
uint32_t insn)
 
 switch (opc) {
 case 0: /* BR */
-case 2: /* RET */
-tcg_gen_mov_i64(cpu_pc, cpu_reg(s, rn));
-break;
 case 1: /* BLR */
-tcg_gen_mov_i64(cpu_pc, cpu_reg(s, rn));
-tcg_gen_movi_i64(cpu_reg(s, 30), s->pc);
+case 2: /* RET */
+gen_a64_set_pc_reg(s, rn);
+/* BLR also needs to load return address */
+if (opc == 1) {
+tcg_gen_movi_i64(cpu_reg(s, 30), s->pc);
+}
 break;
 case 4: /* ERET */
 if (s->current_el == 0) {
diff --git a/target-arm/translate.h b/target-arm/translate.h
index a53f25a..49c042e 10

[Qemu-devel] [PATCH 1/3] target-arm: Infrastucture changes to enable handling of tagged address loading into PC

[Thomas Hanson]

New arm_regime_tbi0() and arm_regime_tbi0() to extract the TBI values from
the correct TCR for the current EL.

New shift, mask and accessor macro definitions needed to add TBI flag bits
to the TB flags field.

cpu_get_tb_cpu_state() inserst the TBI values into 'flags' parameter
so that they show up in the TB flags field.

tbi0, tbi1 fields added to definition of DisasContext structure.

Signed-off-by: Thomas Hanson <thomas.han...@linaro.org>
---
 target-arm/cpu.h   | 20 ++--
 target-arm/helper.c| 42 ++
 target-arm/translate.h |  3 +++
 3 files changed, 63 insertions(+), 2 deletions(-)

diff --git a/target-arm/cpu.h b/target-arm/cpu.h
index 76d824d..c2d6e75 100644
--- a/target-arm/cpu.h
+++ b/target-arm/cpu.h
@@ -2191,7 +2191,11 @@ static inline bool 
arm_cpu_data_is_big_endian(CPUARMState *env)
 #define ARM_TBFLAG_BE_DATA_SHIFT20
 #define ARM_TBFLAG_BE_DATA_MASK (1 << ARM_TBFLAG_BE_DATA_SHIFT)
 
-/* Bit usage when in AArch64 state: currently we have no A64 specific bits */
+/* Bit usage when in AArch64 state */
+#define ARM_TBFLAG_TBI0_SHIFT 0/* TBI0 for EL0/1 or TBI for EL2/3 */
+#define ARM_TBFLAG_TBI0_MASK (0x1ull << ARM_TBFLAG_TBI0_SHIFT)
+#define ARM_TBFLAG_TBI1_SHIFT 1/* TBI1 for EL0/1  */
+#define ARM_TBFLAG_TBI1_MASK (0x1ull << ARM_TBFLAG_TBI1_SHIFT)
 
 /* some convenience accessor macros */
 #define ARM_TBFLAG_AARCH64_STATE(F) \
@@ -,6 +2226,10 @@ static inline bool 
arm_cpu_data_is_big_endian(CPUARMState *env)
 (((F) & ARM_TBFLAG_NS_MASK) >> ARM_TBFLAG_NS_SHIFT)
 #define ARM_TBFLAG_BE_DATA(F) \
 (((F) & ARM_TBFLAG_BE_DATA_MASK) >> ARM_TBFLAG_BE_DATA_SHIFT)
+#define ARM_TBFLAG_TBI0(F) \
+(((F) & ARM_TBFLAG_TBI0_MASK) >> ARM_TBFLAG_TBI0_SHIFT)
+#define ARM_TBFLAG_TBI1(F) \
+(((F) & ARM_TBFLAG_TBI1_MASK) >> ARM_TBFLAG_TBI1_SHIFT)
 
 static inline bool bswap_code(bool sctlr_b)
 {
@@ -2319,12 +2327,19 @@ static inline bool arm_cpu_bswap_data(CPUARMState *env)
 }
 #endif
 
+long arm_regime_tbi0(CPUARMState *env, ARMMMUIdx mmu_idx);
+long arm_regime_tbi1(CPUARMState *env, ARMMMUIdx mmu_idx);
+
 static inline void cpu_get_tb_cpu_state(CPUARMState *env, target_ulong *pc,
 target_ulong *cs_base, uint32_t *flags)
 {
+ARMMMUIdx mmu_idx = cpu_mmu_index(env, false);
 if (is_a64(env)) {
 *pc = env->pc;
 *flags = ARM_TBFLAG_AARCH64_STATE_MASK;
+/* Get control bits for tagged addresses */
+*flags |= (arm_regime_tbi0(env, mmu_idx) << ARM_TBFLAG_TBI0_SHIFT);
+*flags |= (arm_regime_tbi1(env, mmu_idx) << ARM_TBFLAG_TBI1_SHIFT);
 } else {
 *pc = env->regs[15];
 *flags = (env->thumb << ARM_TBFLAG_THUMB_SHIFT)
@@ -2343,7 +2358,8 @@ static inline void cpu_get_tb_cpu_state(CPUARMState *env, 
target_ulong *pc,
<< ARM_TBFLAG_XSCALE_CPAR_SHIFT);
 }
 
-*flags |= (cpu_mmu_index(env, false) << ARM_TBFLAG_MMUIDX_SHIFT);
+*flags |= (mmu_idx << ARM_TBFLAG_MMUIDX_SHIFT);
+
 /* The SS_ACTIVE and PSTATE_SS bits correspond to the state machine
  * states defined in the ARM ARM for software singlestep:
  *  SS_ACTIVE   PSTATE.SS   State
diff --git a/target-arm/helper.c b/target-arm/helper.c
index bdb842c..526306e 100644
--- a/target-arm/helper.c
+++ b/target-arm/helper.c
@@ -6720,6 +6720,48 @@ static inline TCR *regime_tcr(CPUARMState *env, 
ARMMMUIdx mmu_idx)
 return >cp15.tcr_el[regime_el(env, mmu_idx)];
 }
 
+/* Returns TBI0 value for current regime el */
+long arm_regime_tbi0(CPUARMState *env, ARMMMUIdx mmu_idx)
+{
+TCR*tcr;
+uint32_t el;
+
+/* regime_el fails for mmu_idx = ARMMMUIdx_S12NSE0 or ARMMMUIdx_S12NSE1 */
+if (mmu_idx == ARMMMUIdx_S12NSE0 || mmu_idx == ARMMMUIdx_S12NSE1) {
+mmu_idx += ARMMMUIdx_S1NSE0;
+}
+
+tcr = regime_tcr(env, mmu_idx);
+el = regime_el(env, mmu_idx);
+
+if (el > 1) {
+return extract64(tcr->raw_tcr, 20, 1);
+} else {
+return extract64(tcr->raw_tcr, 37, 1);
+}
+}
+
+/* Returns TBI0 value for current regime el */
+long arm_regime_tbi1(CPUARMState *env, ARMMMUIdx mmu_idx)
+{
+TCR*tcr;
+uint32_t el;
+
+/* regime_el fails for mmu_idx = ARMMMUIdx_S12NSE0 or ARMMMUIdx_S12NSE1 */
+if (mmu_idx == ARMMMUIdx_S12NSE0 || mmu_idx == ARMMMUIdx_S12NSE1) {
+mmu_idx += ARMMMUIdx_S1NSE0;
+}
+
+tcr = regime_tcr(env, mmu_idx);
+el = regime_el(env, mmu_idx);
+
+if (el > 1) {
+return 0;
+} else {
+return extract64(tcr->raw_tcr, 38, 1);
+}
+}
+
 /* Return the TTBR associated with this translation regime */
 static inline uint64_t regime_ttbr(CPUARMState *env, ARMMMUIdx mmu_idx,
int ttbrn)
diff --git a/target-arm/translate.h b/target-arm/translate.h
index dbd7ac8..5dfd394

[Qemu-devel] [PATCH 0/3] tareget-arm: Handle tagged addresses when loading PC

[Thomas Hanson]

If tagged addresses are enabled, then addresses being loaded into the 
PC must be cleaned up by overwriting the tag bits with either all 0's 
or all 1's as specified in the ARM ARM spec.  The decision process is 
dependent on whether the code will be running in EL0/1 or in EL2/3 and 
is controlled by a combination of Top Byte Ignored (TBI) bits in the 
TCR and the value of bit 55 in the address being loaded. 

TBI values are extracted from the appropriate TCR and made available 
to TCG code generation routines by inserting them into the TB flags 
field and then transferring them to DisasContext structure in 
gen_intermediate_code_a64().

New function gen_a64_set_pc_reg() encapsulates the logic required to 
determine whether clean up of the tag byte is required and then 
generating the code to correctly load the PC.
  
In addition to those instruction which can directly load a tagged 
address into the PC, there are others which increment or add a value to
the PC.  If 56 bit addressing is used, these instructions can cause an 
arithmetic roll-over into the tag bits.  The ARM ARM specification for 
handling tagged addresses requires that these cases also be addressed
by cleaning up the tag field.  This work has been deferred because 
there is currently no CPU model available for testing with 56 bit 
addresses.


Thomas Hanson (3):
  target-arm: Infrastucture changes to enable handling of tagged address
loading into PC
  target-arm: Code changes to implement overwrite of tag field on PC
load
  target-arm: Comments to mark location of pending work for 56 bit
addresses

 target-arm/cpu.h   | 20 +--
 target-arm/helper.c| 42 +++
 target-arm/translate-a64.c | 85 +-
 target-arm/translate.h |  3 ++
 4 files changed, 140 insertions(+), 10 deletions(-)

-- 
1.9.1

[Qemu-devel] [PATCH 2/3] target-arm: Code changes to implement overwrite of tag field on PC load

[Thomas Hanson]

gen_intermediate_code_a64() transfers TBI values from TB->flags to
DisasContext structure.

disas_uncond_b_reg() calls new function gen_a64_set_pc_reg() to handle BR,
BLR and RET instructions.

gen_a64_set_pc_reg() implements all of the required checks and overwiting
logic to clean up the tag field of an address before loading the PC.
Currently only called in one place, but will be used in the future to
handle arithmetic overflow cases with 56-bit addresses.  (See following
patch.)

Signed-off-by: Thomas Hanson <thomas.han...@linaro.org>
---
 target-arm/translate-a64.c | 67 ++
 1 file changed, 62 insertions(+), 5 deletions(-)

diff --git a/target-arm/translate-a64.c b/target-arm/translate-a64.c
index f5e29d2..4d6f951 100644
--- a/target-arm/translate-a64.c
+++ b/target-arm/translate-a64.c
@@ -41,6 +41,7 @@ static TCGv_i64 cpu_pc;
 
 /* Load/store exclusive handling */
 static TCGv_i64 cpu_exclusive_high;
+static TCGv_i64 cpu_reg(DisasContext *s, int reg);
 
 static const char *regnames[] = {
 "x0", "x1", "x2", "x3", "x4", "x5", "x6", "x7",
@@ -176,6 +177,58 @@ void gen_a64_set_pc_im(uint64_t val)
 tcg_gen_movi_i64(cpu_pc, val);
 }
 
+void gen_a64_set_pc_reg(DisasContext *s, unsigned int rn)
+{
+if (s->current_el <= 1) {
+/* Test if NEITHER or BOTH TBI values are set.  If so, no need to
+ * examine bit 55 of address, can just generate code.
+ * If mixed, then test via generated code
+ */
+if (s->tbi0 && s->tbi1) {
+TCGv_i64 tmp_reg = tcg_temp_new_i64();
+/* Both bits set, just fix it */
+tcg_gen_shli_i64(tmp_reg, cpu_reg(s, rn), 8);
+tcg_gen_sari_i64(cpu_pc, tmp_reg, 8);
+tcg_temp_free_i64(tmp_reg);
+} else if (!s->tbi0 && !s->tbi1) {
+/* Neither bit set, just load it as-is */
+tcg_gen_mov_i64(cpu_pc, cpu_reg(s, rn));
+} else {
+TCGv_i64 tcg_tmpval = tcg_temp_new_i64();
+TCGv_i64 tcg_bit55  = tcg_temp_new_i64();
+TCGv_i64 tcg_zero   = tcg_const_i64(0);
+
+tcg_gen_andi_i64(tcg_bit55, cpu_reg(s, rn), (1ull << 55));
+
+if (s->tbi0) {
+/* tbi0==1, tbi1==0, so 0-fill upper byte if bit 55 = 0 */
+tcg_gen_andi_i64(tcg_tmpval, cpu_reg(s, rn),
+ 0x00FFull);
+tcg_gen_movcond_i64(TCG_COND_EQ, cpu_pc, tcg_bit55, tcg_zero,
+tcg_tmpval, cpu_reg(s, rn));
+} else {
+/* tbi0==0, tbi1==1, so 1-fill upper byte if bit 55 = 1 */
+tcg_gen_ori_i64(tcg_tmpval, cpu_reg(s, rn),
+0xFF00ull);
+tcg_gen_movcond_i64(TCG_COND_NE, cpu_pc, tcg_bit55, tcg_zero,
+tcg_tmpval, cpu_reg(s, rn));
+}
+tcg_temp_free_i64(tcg_zero);
+tcg_temp_free_i64(tcg_bit55);
+tcg_temp_free_i64(tcg_tmpval);
+}
+} else {  /* EL > 1 */
+if (s->tbi0) {
+/* Force tag byte to all zero */
+tcg_gen_andi_i64(cpu_pc, cpu_reg(s, rn), 0x00FFull);
+} else {
+/* Load unmodified address */
+tcg_gen_mov_i64(cpu_pc, cpu_reg(s, rn));
+}
+ }
+
+}
+
 typedef struct DisasCompare64 {
 TCGCond cond;
 TCGv_i64 value;
@@ -1691,12 +1744,14 @@ static void disas_uncond_b_reg(DisasContext *s, 
uint32_t insn)
 
 switch (opc) {
 case 0: /* BR */
-case 2: /* RET */
-tcg_gen_mov_i64(cpu_pc, cpu_reg(s, rn));
-break;
 case 1: /* BLR */
-tcg_gen_mov_i64(cpu_pc, cpu_reg(s, rn));
-tcg_gen_movi_i64(cpu_reg(s, 30), s->pc);
+case 2: /* RET */
+/* Check for tagged addresses and generate appropriate code */
+gen_a64_set_pc_reg(s, rn);
+/* BLR also needs to load return address */
+if (opc == 1) {
+tcg_gen_movi_i64(cpu_reg(s, 30), s->pc);
+}
 break;
 case 4: /* ERET */
 if (s->current_el == 0) {
@@ -11150,6 +11205,8 @@ void gen_intermediate_code_a64(ARMCPU *cpu, 
TranslationBlock *tb)
 dc->condexec_mask = 0;
 dc->condexec_cond = 0;
 dc->mmu_idx = ARM_TBFLAG_MMUIDX(tb->flags);
+dc->tbi0 = ARM_TBFLAG_TBI0(tb->flags);
+dc->tbi1 = ARM_TBFLAG_TBI1(tb->flags);
 dc->current_el = arm_mmu_idx_to_el(dc->mmu_idx);
 #if !defined(CONFIG_USER_ONLY)
 dc->user = (dc->current_el == 0);
-- 
1.9.1

[Qemu-devel] [PATCH 3/3] target-arm: Comments to mark location of pending work for 56 bit addresses

[Thomas Hanson]

Certain instructions which can not directly load a tagged address value
may trigger a corner case when the address size is 56 bits.  This is
because incrementing or offsetting from the current PC can cause an
arithetic roll-over into the tag bits.  Per the ARM ARM spec, these cases
should also be addressed by cleaning up the tag field.

This work was not done at this time since the changes could not be tested
with current CPU models.  Comments have been added to flag the locations
where this will need to be fixed once a model is available.

3 comments added in same file to identify cases in a switch.

Signed-off-by: Thomas Hanson <thomas.han...@linaro.org>
---
 target-arm/translate-a64.c | 18 +++---
 1 file changed, 15 insertions(+), 3 deletions(-)

diff --git a/target-arm/translate-a64.c b/target-arm/translate-a64.c
index 4d6f951..8810180 100644
--- a/target-arm/translate-a64.c
+++ b/target-arm/translate-a64.c
@@ -1205,6 +1205,9 @@ static inline AArch64DecodeFn *lookup_disas_fn(const 
AArch64DecodeTable *table,
  */
 static void disas_uncond_b_imm(DisasContext *s, uint32_t insn)
 {
+/*If/when address size is 56 bits, this could overflow into address tag
+ * byte, and that byte should be fixed per ARM ARM spec.
+ */
 uint64_t addr = s->pc + sextract32(insn, 0, 26) * 4 - 4;
 
 if (insn & (1U << 31)) {
@@ -1232,6 +1235,9 @@ static void disas_comp_b_imm(DisasContext *s, uint32_t 
insn)
 sf = extract32(insn, 31, 1);
 op = extract32(insn, 24, 1); /* 0: CBZ; 1: CBNZ */
 rt = extract32(insn, 0, 5);
+/*If/when address size is 56 bits, this could overflow into address tag
+ * byte, and that byte should be fixed per ARM ARM spec.
+ */
 addr = s->pc + sextract32(insn, 5, 19) * 4 - 4;
 
 tcg_cmp = read_cpu_reg(s, rt, sf);
@@ -1260,6 +1266,9 @@ static void disas_test_b_imm(DisasContext *s, uint32_t 
insn)
 
 bit_pos = (extract32(insn, 31, 1) << 5) | extract32(insn, 19, 5);
 op = extract32(insn, 24, 1); /* 0: TBZ; 1: TBNZ */
+/*If/when address size is 56 bits, this could overflow into address tag
+ * byte, and that byte should be fixed per ARM ARM spec.
+ */
 addr = s->pc + sextract32(insn, 5, 14) * 4 - 4;
 rt = extract32(insn, 0, 5);
 
@@ -1289,6 +1298,9 @@ static void disas_cond_b_imm(DisasContext *s, uint32_t 
insn)
 unallocated_encoding(s);
 return;
 }
+/*If/when address size is 56 bits, this could overflow into address tag
+ * byte, and that byte should be fixed per ARM ARM spec.
+ */
 addr = s->pc + sextract32(insn, 5, 19) * 4 - 4;
 cond = extract32(insn, 0, 4);
 
@@ -1636,12 +1648,12 @@ static void disas_exc(DisasContext *s, uint32_t insn)
  * instruction works properly.
  */
 switch (op2_ll) {
-case 1:
+case 1: /* SVC */
 gen_ss_advance(s);
 gen_exception_insn(s, 0, EXCP_SWI, syn_aa64_svc(imm16),
default_exception_el(s));
 break;
-case 2:
+case 2: /* HVC */
 if (s->current_el == 0) {
 unallocated_encoding(s);
 break;
@@ -1654,7 +1666,7 @@ static void disas_exc(DisasContext *s, uint32_t insn)
 gen_ss_advance(s);
 gen_exception_insn(s, 0, EXCP_HVC, syn_aa64_hvc(imm16), 2);
 break;
-case 3:
+case 3: /* SMC */
 if (s->current_el == 0) {
 unallocated_encoding(s);
 break;
-- 
1.9.1

Re: [Qemu-devel] QEMU make: ROM is too large

[Thomas Hanson]

On 22 August 2016 at 08:23, Peter Maydell  wrote:

> PS: just passing --enable-debug to configure should
> be sufficient to do a no-optimization debug-symbols
> build; do you really need to manually specify
> CFLAGS? I wonder whether you're ending up with two
> -Osomething options in your CFLAGS which is then
> confusing the "override with -O2" logic in the
> pc-bios/optionrom/Makefile.
>

Peter,

Looks like you're onto something.

With:
CFLAGS="-g3 -O0" ./configure --target-list=aarch64-softmmu,arm-softmmu
--enable-vhost-net --enable-virtfs

cc -I/home/tom/QEMU/SrcTree/tcg -I/home/tom/QEMU/SrcTree/tcg/i386
-I/home/tom/QEMU/SrcTree/linux-headers
-I/home/tom/QEMU/SrcTree/linux-headers -I. -I/home/tom/QEMU/SrcTree
-I/home/tom/QEMU/SrcTree/include -Iqga -Iqga -I/usr/include/pixman-1
 -Werror -fPIE -DPIE -m64 -D_GNU_SOURCE -D_FILE_OFFSET_BITS=64
-D_LARGEFILE_SOURCE -Wstrict-prototypes -Wredundant-decls -Wall -Wundef
-Wwrite-strings -Wmissing-prototypes -fno-strict-aliasing -fno-common
 -Wendif-labels -Wmissing-include-dirs -Wempty-body -Wnested-externs
-Wformat-security -Wformat-y2k -Winit-self -Wignored-qualifiers
-Wold-style-declaration -Wold-style-definition -Wtype-limits
-fstack-protector-strong -I/home/tom/QEMU/SrcTree/tests -I
qga/qapi-generated -MMD -MP -MT qga/main.o -MF qga/main.d *-O2*
-U_FORTIFY_SOURCE
-D_FORTIFY_SOURCE=2 -pthread -I/usr/include/glib-2.0
-I/usr/lib/x86_64-linux-gnu/glib-2.0/include   *-g -g3 -O0  -c* -o
qga/main.o qga/main.c


With
./configure --enable-debug --target-list=aarch64-softmmu,arm-softmmu
--enable-vhost-net --enable-virtfs

cc -I/home/tom/QEMU/SrcTree/tcg -I/home/tom/QEMU/SrcTree/tcg/i386
-I/home/tom/QEMU/SrcTree/linux-headers
-I/home/tom/QEMU/SrcTree/linux-headers -I. -I/home/tom/QEMU/SrcTree
-I/home/tom/QEMU/SrcTree/include -Iqga -Iqga -I/usr/include/pixman-1
 -Werror -fPIE -DPIE -m64 -D_GNU_SOURCE -D_FILE_OFFSET_BITS=64
-D_LARGEFILE_SOURCE -Wstrict-prototypes -Wredundant-decls -Wall -Wundef
-Wwrite-strings -Wmissing-prototypes -fno-strict-aliasing -fno-common
 -Wendif-labels -Wmissing-include-dirs -Wempty-body -Wnested-externs
-Wformat-security -Wformat-y2k -Winit-self -Wignored-qualifiers
-Wold-style-declaration -Wold-style-definition -Wtype-limits
-fstack-protector-strong -I/home/tom/QEMU/SrcTree/tests -I
qga/qapi-generated -MMD -MP -MT qga/main.o -MF qga/main.d -pthread
-I/usr/include/glib-2.0 -I/usr/lib/x86_64-linux-gnu/glib-2.0/include   *-g
  -c -o* qga/main.o qga/main.c

AND with the 2nd (--enable-debug) command line, the problem goes away.

Thanks!

Re: [Qemu-devel] QEMU make: ROM is too large

[Thomas Hanson]

Update: After several rounds, git bisect says:
7f2569246c81d5f88e74c142b8fbdc0ee601bffe is the first bad commit
commit 7f2569246c81d5f88e74c142b8fbdc0ee601bffe
Author: Paolo Bonzini <pbonz...@redhat.com>
Date:   Fri Aug 5 10:51:37 2016 +0200

linuxboot_dma: avoid guest ABI breakage on gcc vs. clang compilation

It's late on a Friday, I'll dig more on Monday.  Any help would be greatly
appreciated.

On 19 August 2016 at 11:58, Thomas Hanson <thomas.han...@linaro.org> wrote:

> All,
>
> Just pulled top of tree, make clean and make as follows:
> CFLAGS="-g3 -O0" ./configure  --enable-vhost-net --enable-virtfs
> make -j8
>
> Build fails with
>   ASoptionrom/kvmvapic.o
>   Building optionrom/multiboot.img
>   Building optionrom/linuxboot.img
>   Building optionrom/linuxboot_dma.img
>   Building optionrom/kvmvapic.img
>   Building optionrom/multiboot.raw
>   Building optionrom/linuxboot.raw
>   Building optionrom/linuxboot_dma.raw
>   Building optionrom/kvmvapic.raw
>   Signing optionrom/multiboot.bin
>   Signing optionrom/linuxboot.bin
>   CCqga/channel-posix.o
>   Signing optionrom/linuxboot_dma.bin
>   CCqga/qapi-generated/qga-qapi-types.o
> error: ROM is too large (2072 > 1536)
> make[1]: *** [linuxboot_dma.bin] Error 1
>
> Last commit in repo is:
>   commit 02b1ad881cbb1795029737a9077db60267dc0c6f
>   Merge: 5844365 156af3a
>   Author: Peter Maydell <peter.mayd...@linaro.org>
>   Date:   Thu Aug 18 14:42:51 2016 +0100
>
> Merge remote-tracking branch 'remotes/stefanha/tags/block-pull-request'
> into staging
>
> Nothing jumped out at me in a quick review of Makefiles, scripts, configs,
> etc.
>
> Any suggestions as to the source of the issue or how to fix it?
>
> Thanks,
> Tom
>

[Qemu-devel] QEMU make: ROM is too large

[Thomas Hanson]

All,

Just pulled top of tree, make clean and make as follows:
CFLAGS="-g3 -O0" ./configure  --enable-vhost-net --enable-virtfs
make -j8

Build fails with
  ASoptionrom/kvmvapic.o
  Building optionrom/multiboot.img
  Building optionrom/linuxboot.img
  Building optionrom/linuxboot_dma.img
  Building optionrom/kvmvapic.img
  Building optionrom/multiboot.raw
  Building optionrom/linuxboot.raw
  Building optionrom/linuxboot_dma.raw
  Building optionrom/kvmvapic.raw
  Signing optionrom/multiboot.bin
  Signing optionrom/linuxboot.bin
  CCqga/channel-posix.o
  Signing optionrom/linuxboot_dma.bin
  CCqga/qapi-generated/qga-qapi-types.o
error: ROM is too large (2072 > 1536)
make[1]: *** [linuxboot_dma.bin] Error 1

Last commit in repo is:
  commit 02b1ad881cbb1795029737a9077db60267dc0c6f
  Merge: 5844365 156af3a
  Author: Peter Maydell 
  Date:   Thu Aug 18 14:42:51 2016 +0100

Merge remote-tracking branch 'remotes/stefanha/tags/block-pull-request'
into staging

Nothing jumped out at me in a quick review of Makefiles, scripts, configs,
etc.

Any suggestions as to the source of the issue or how to fix it?

Thanks,
Tom

Re: [Qemu-devel] best way to implement emulation of AArch64 tagged addresses

[Thomas Hanson]

Ah, true.

On 9 April 2016 at 09:57, Richard Henderson <r...@twiddle.net> wrote:

> On 04/08/2016 05:29 PM, Thomas Hanson wrote:
>
>> Looking at tcg_out_tlb_load():
>> If I'm reading the pseudo-assembler of the function names correctly, it
>> looks
>> like in the i386 code we're already masking the address being checked:
>>  tgen_arithi(s, ARITH_AND + trexw, r1, TARGET_PAGE_MASK | (aligned ?
>> s_mask
>> : 0), 0);
>> where  TARGET_PAGE_MASK is a simple all-1's mask in the appropriate upper
>> bits.
>>
>> Can we just poke some 0's into that mask in the tag locations?
>>
>
> No, because we'd no longer have a sign-extended 32-bit value, as fits in
> that immediate operand field.  To load the constant you're asking for, we'd
> need a 64-bit move insn and another register.
>
>
> r~
>

Re: [Qemu-devel] best way to implement emulation of AArch64 tagged addresses

[Thomas Hanson]

Looking at tcg_out_tlb_load():
If I'm reading the pseudo-assembler of the function names correctly, it
looks like in the i386 code we're already masking the address being
checked:
tgen_arithi(s, ARITH_AND + trexw, r1, TARGET_PAGE_MASK | (aligned ?
s_mask : 0), 0);
where  TARGET_PAGE_MASK is a simple all-1's mask in the appropriate upper
bits.

Can we just poke some 0's into that mask in the tag locations?  And, of
course, do the same when creating the TLB entry.

Unless of course we're in the case of (TARGET_LONG_BITS >
TCG_TARGET_REG_BITS) (that would be 64 bit on 32 bit right?) when addrhi
gets tested separately. Then we'd have to do the shift as above.

MIPS logic appears similar on a quick read.  In the sparc code I'm not
seeing a pre-existing mask  but it's getting late and my eyes are giving
out.  Those are the only tcg_out_tlb_load() versions I can find.


As to frequency I'm assuming that there are far fewer tagged pointers than
untagged.  But then again I haven't seen a good use case for tagged
pointers.  Would love to hear one.

On 8 April 2016 at 12:10, Richard Henderson  wrote:

> On 04/08/2016 10:20 AM, Tom Hanson wrote:
> > Is it an option to mask off the tag bits in all cases? Is there any case
> > it which those bits are valid address bits?
>
> It's not impossible to mask off bits in the address -- we do that for
> running
> 32-bit on 64-bit all of the time.  It's all a question of how well the
> average
> program will perform, I suppose.
>
> For instance.  Are there more tagged addresses than non-tagged addresses?
> If
> we mask off bits, that will affect *every* memory operation.  If tagged
> addresses are rare, then that is a waste.  If tagged addresses are common,
> however, then we may well spend too much time ping-ponging in the TLB.
>
> The fastest method I can think of to ignore high order bits is to shift the
> address comparator left.  The TLB comparator would be stored pre-shifted,
> so
> this would add only one insn on the fast path.  Or perhaps zero in the
> case of
> an arm/aarch64 host, where the compare insn itself can perform the shift.
>
> Of course, a double-word shift would be completely out of the question when
> doing 64-bit on 32-bit emulation.  But we don't need that -- just shift the
> high part of the address left to discard bits, leaving a funny looking
> hole in
> the middle of the comparator.
>
> This is simple enough that it should be relatively easy to patch up all of
> the
> tcg backends to match, if we decide to go with it.
>
>
> r~
>
>

Re: [Qemu-devel] [Qemu-arm] [PATCH] sd: Fix "info qtree" on boards with SD cards

[Thomas Hanson]

Sounds like a good idea.  Much easier to fix a problem with an
explicit error than to chase a seg fault.

On 15 March 2016 at 14:41, Peter Maydell <peter.mayd...@linaro.org> wrote:
> On 15 March 2016 at 20:33, Peter Maydell <peter.mayd...@linaro.org> wrote:
>> On 15 March 2016 at 20:28, Thomas Hanson <thomas.han...@linaro.org> wrote:
>>> The patch looks good.
>>>
>>> Would it also be good to update bus_add_child() so that it NULL-checks
>>> its "bus" parameter before dereferencing it?
>>
>> No, I think it's just a programming error to call qdev_set_parent_bus()
>> with a NULL bus parameter, so crashing is fine.
>
> ...but it might be helpful to assert in qdev_try_create() that
> if we're using the default bus then the object is a sysbus
> device object. At least then the problem will be immediately
> clear rather than only showing up if you run a monitor command
> later.
>
> thanks
> -- PMM

Re: [Qemu-devel] [Qemu-arm] [PATCH] hw/arm/bcm2836: Wire up CPU timer interrupts correctly

[Thomas Hanson]

On 17 March 2016 at 07:46, Peter Maydell <peter.mayd...@linaro.org> wrote:
> On 17 March 2016 at 13:37, Thomas Hanson <thomas.han...@linaro.org> wrote:
>>
>> On Mar 17, 2016 4:33 AM, "Peter Maydell" <peter.mayd...@linaro.org> wrote:
>>>
>>> Wire up the CPU timer interrupts in the right order, with the
>>> nonsecure physical timer on cntpnsirq, the hyp timer on cnthpirq,
>>> and the secure physical timer on cntpsirq. (We did get the
>>> virt timer right, at least.)
>
>> What drives the order?
>
> This is modelling the hardware:
> https://www.raspberrypi.org/documentation/hardware/raspberrypi/bcm2836/README.md
>
> The interrupt controller has 4 lines named CNTPSIRQ, CNTPNSIRQ, etc,
> and we need to hook the lines from the CPU up to the interrupt
> controller appropriately. The names we ended up with in QEMU's
> CPU model (GTIMER_PHYS, etc) don't match the signal names that
> a hardware A7 uses, which is slightly irritating, but not a big deal.
>
> The A7 TRM lists which timer is which signal:
> http://infocenter.arm.com/help/topic/com.arm.doc.ddi0464f/BABBABDJ.html
> (though you can pretty much guess from the signal names).
>
> thanks
> -- PMM

Ah, thanks.  I was thinking time-order. It's still early...

Re: [Qemu-devel] [Qemu-arm] [PATCH] hw/arm/bcm2836: Wire up CPU timer interrupts correctly

[Thomas Hanson]

On Mar 17, 2016 4:33 AM, "Peter Maydell"  wrote:
>
> Wire up the CPU timer interrupts in the right order, with the
> nonsecure physical timer on cntpnsirq, the hyp timer on cnthpirq,
> and the secure physical timer on cntpsirq. (We did get the
> virt timer right, at least.)
>
> Reported-by: Antonio Huete Jiménez 
> Signed-off-by: Peter Maydell 
> ---
>  hw/arm/bcm2836.c | 6 +-
>  1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/hw/arm/bcm2836.c b/hw/arm/bcm2836.c
> index 89a6b35..1cefe41 100644
> --- a/hw/arm/bcm2836.c
> +++ b/hw/arm/bcm2836.c
> @@ -136,9 +136,13 @@ static void bcm2836_realize(DeviceState *dev, Error
**errp)
>
>  /* Connect timers from the CPU to the interrupt controller */
>  qdev_connect_gpio_out(DEVICE(>cpus[n]), GTIMER_PHYS,
> -qdev_get_gpio_in_named(DEVICE(>control), "cntpsirq",
n));
> +qdev_get_gpio_in_named(DEVICE(>control), "cntpnsirq",
n));
>  qdev_connect_gpio_out(DEVICE(>cpus[n]), GTIMER_VIRT,
>  qdev_get_gpio_in_named(DEVICE(>control), "cntvirq",
n));
> +qdev_connect_gpio_out(DEVICE(>cpus[n]), GTIMER_HYP,
> +qdev_get_gpio_in_named(DEVICE(>control), "cnthpirq",
n));
> +qdev_connect_gpio_out(DEVICE(>cpus[n]), GTIMER_SEC,
> +qdev_get_gpio_in_named(DEVICE(>control), "cntpsirq",
n));
>  }
>  }
>
> --
> 1.9.1
>
>

What drives the order?

Re: [Qemu-devel] [Qemu-arm] [PATCH] sd: Fix "info qtree" on boards with SD cards

[Thomas Hanson]

The patch looks good.

Would it also be good to update bus_add_child() so that it NULL-checks
its "bus" parameter before dereferencing it?

-Tom

On 15 March 2016 at 10:56, Peter Maydell  wrote:
> The SD card object is not a SysBusDevice, so don't create it with
> qdev_create() if we're not assigning it to a specific bus; use
> object_new() instead.
>
> This was causing 'info qtree' to segfault on boards with SD cards,
> because qdev_create(NULL, TYPE_FOO) puts the created object on the
> system bus, and then we may try to run functions like sysbus_dev_print()
> on it, which fail when casting the object to SysBusDevice.
>
> (This is the same mistake that we made with the NAND device
> and fixed in commit 6749695eaaf346c1.)
>
> Reported-by: hitmoon 
> Signed-off-by: Peter Maydell 
> ---
> I assume that using qdev_create() for non-SysBus devices is
> OK if we are passing in a specific bus pointer, because we do
> this already for various things including PCI devices. The
> various "properly QOMified" uses of TYPE_SD_CARD do that; only
> this sd_init() function for the legacy uses doesn't.
> ---
>  hw/sd/sd.c | 6 --
>  1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/hw/sd/sd.c b/hw/sd/sd.c
> index 00c320d..1568057 100644
> --- a/hw/sd/sd.c
> +++ b/hw/sd/sd.c
> @@ -563,17 +563,19 @@ static const VMStateDescription sd_vmstate = {
>  /* Legacy initialization function for use by non-qdevified callers */
>  SDState *sd_init(BlockBackend *blk, bool is_spi)
>  {
> +Object *obj;
>  DeviceState *dev;
>  Error *err = NULL;
>
> -dev = qdev_create(NULL, TYPE_SD_CARD);
> +obj = object_new(TYPE_SD_CARD);
> +dev = DEVICE(obj);
>  qdev_prop_set_drive(dev, "drive", blk, );
>  if (err) {
>  error_report("sd_init failed: %s", error_get_pretty(err));
>  return NULL;
>  }
>  qdev_prop_set_bit(dev, "spi", is_spi);
> -object_property_set_bool(OBJECT(dev), true, "realized", );
> +object_property_set_bool(obj, true, "realized", );
>  if (err) {
>  error_report("sd_init failed: %s", error_get_pretty(err));
>  return NULL;
> --
> 1.9.1
>
>