| Message ID | 20230630122551.21766-6-anjo@rev.ng (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Collapse CPUNegativeOffsetState into CPUState | expand |
On 6/30/23 14:25, Anton Johansson wrote: > @@ -448,6 +448,13 @@ struct CPUState { > > /* track IOMMUs whose translations we've cached in the TCG TLB */ > GArray *iommu_notifiers; > + > + /* > + * The following fields needs to be within CPU_MAX_NEGATIVE_ENV_OFFSET of > + * CPUArchState. As CPUArchState is assumed to follow CPUState in the > + * ArchCPU struct these are placed last. This is checked statically. > + */ > + CPUTLB tlb; > }; This is what we had before CPUNegativeOffsetState, comment and all, and over the course of time the comment was ignored and the CPUTLB crept toward the middle of the structure. I really don't see how this merge helps. There's nothing target-specific about CPUTLBDescFast or CPUNegativeOffsetState, and keeping them separate enforces their importance. r~
On 6/30/23 16:16, Richard Henderson wrote: > On 6/30/23 14:25, Anton Johansson wrote: >> @@ -448,6 +448,13 @@ struct CPUState { >> /* track IOMMUs whose translations we've cached in the TCG >> TLB */ >> GArray *iommu_notifiers; >> + >> + /* >> + * The following fields needs to be within >> CPU_MAX_NEGATIVE_ENV_OFFSET of >> + * CPUArchState. As CPUArchState is assumed to follow CPUState >> in the >> + * ArchCPU struct these are placed last. This is checked >> statically. >> + */ >> + CPUTLB tlb; >> }; > > This is what we had before CPUNegativeOffsetState, comment and all, > and over the course of time the comment was ignored and the CPUTLB > crept toward the middle of the structure. I recall you talking about this earlier. Is there any reason the drifting of CPUTLB/IcountDecr from env wouldn't be caught by the static asserts we've added? > > I really don't see how this merge helps. There's nothing > target-specific about CPUTLBDescFast or CPUNegativeOffsetState, and > keeping them separate enforces their importance. > There isn't anything target specific about CPUTLBDescFast but its offset in ArchCPU does depend on the target. This is due to the TARGET_PAGE_ENTRY_EXTRA macro in CPUTLBEntryFull which is replaced with a union in the first patch of this patchset. On second thought, I think you're right and keeping CPUTLB and IcountDecr together to emphasize their importance makes sense. There would still be an implicit assumption on the target to keep CPUArchState and CPUState close together, at least the intent is signaled better by CPUNegativeOffsetState. Even so, statically asserting the offset to CPUTLB and IcountDecr would be a good idea. The main concern of this patchset is being able to access the tlb and icount_decr fields in a target-independent way only having access to CPUState. Merging CPUNegativeOffsetState simply made the accessing part simpler, but let's have a cpu_tlb(CPUState *) function instead. I'll pull out the patch for making CPUTLB target independent and include it in the patchset replacing CPUArchState with CPUState in cputlb.c and user-exec.c. The static assert patches will be resubmitted separately. Thanks,
diff --git a/include/exec/cpu-all.h b/include/exec/cpu-all.h index 8018ce783e..706daa49ec 100644 --- a/include/exec/cpu-all.h +++ b/include/exec/cpu-all.h @@ -453,7 +453,7 @@ static inline CPUNegativeOffsetState *cpu_neg(CPUState *cpu) */ static inline CPUTLB *env_tlb(CPUArchState *env) { - return &env_neg(env)->tlb; + return &env_cpu(env)->tlb; } #endif /* CPU_ALL_H */ diff --git a/include/exec/cpu-defs.h b/include/exec/cpu-defs.h index dff6c37f6b..add0f3c541 100644 --- a/include/exec/cpu-defs.h +++ b/include/exec/cpu-defs.h @@ -25,7 +25,6 @@ #include "qemu/host-utils.h" #include "qemu/thread.h" -#include "exec/tlb-common.h" #include "hw/core/cpu.h" #include "cpu-param.h" @@ -83,7 +82,6 @@ * before CPUArchState, as a field named "neg". */ typedef struct CPUNegativeOffsetState { - CPUTLB tlb; IcountDecr icount_decr; } CPUNegativeOffsetState; diff --git a/include/exec/tlb-common.h b/include/exec/tlb-common.h index 838a1f0d2a..450c0156bf 100644 --- a/include/exec/tlb-common.h +++ b/include/exec/tlb-common.h @@ -190,8 +190,8 @@ typedef struct CPUTLBCommon { /* * The entire softmmu tlb, for all MMU modes. * The meaning of each of the MMU modes is defined in the target code. - * Since this is placed within CPUNegativeOffsetState, the smallest - * negative offsets are at the end of the struct. + * Since this is placed within CPUState, the smallest negative offsets + * are at the end of the struct. */ typedef struct CPUTLB { diff --git a/include/hw/core/cpu.h b/include/hw/core/cpu.h index 0377f74d48..adf6158899 100644 --- a/include/hw/core/cpu.h +++ b/include/hw/core/cpu.h @@ -448,6 +448,13 @@ struct CPUState { /* track IOMMUs whose translations we've cached in the TCG TLB */ GArray *iommu_notifiers; + + /* + * The following fields needs to be within CPU_MAX_NEGATIVE_ENV_OFFSET of + * CPUArchState. As CPUArchState is assumed to follow CPUState in the + * ArchCPU struct these are placed last. This is checked statically. + */ + CPUTLB tlb; }; typedef QTAILQ_HEAD(CPUTailQ, CPUState) CPUTailQ; diff --git a/accel/tcg/translate-all.c b/accel/tcg/translate-all.c index d3d4fbc1a4..5582aaf653 100644 --- a/accel/tcg/translate-all.c +++ b/accel/tcg/translate-all.c @@ -339,8 +339,17 @@ TranslationBlock *tb_gen_code(CPUState *cpu, tcg_ctx->page_bits = TARGET_PAGE_BITS; tcg_ctx->page_mask = TARGET_PAGE_MASK; tcg_ctx->tlb_dyn_max_bits = CPU_TLB_DYN_MAX_BITS; - tcg_ctx->tlb_fast_offset = - (int)offsetof(ArchCPU, neg.tlb.f) - (int)offsetof(ArchCPU, env); + +#define TLB_FAST_OFFSET \ + ((int)offsetof(ArchCPU, parent_obj.tlb.f) - (int)offsetof(ArchCPU, env)) + + QEMU_BUILD_BUG_ON(TLB_FAST_OFFSET < CPU_MAX_NEGATIVE_ENV_OFFSET || + TLB_FAST_OFFSET > 0); + + tcg_ctx->tlb_fast_offset = TLB_FAST_OFFSET; + +#undef TLB_FAST_OFFSET + #endif tcg_ctx->insn_start_words = TARGET_INSN_START_WORDS; #ifdef TCG_GUEST_DEFAULT_MO
As CPUTLB is now target-agnostic it can be moved from CPUNegativeOffsetState to CPUState, and the negative offset from CPUArchState can instead be statically asserted to be greater than CPU_MAX_NEGATIVE_ENV_OFFSET. This also opens up the door for reducing the dependency of common code on CPUArchState. Signed-off-by: Anton Johansson <anjo@rev.ng> --- include/exec/cpu-all.h | 2 +- include/exec/cpu-defs.h | 2 -- include/exec/tlb-common.h | 4 ++-- include/hw/core/cpu.h | 7 +++++++ accel/tcg/translate-all.c | 13 +++++++++++-- 5 files changed, 21 insertions(+), 7 deletions(-)