Re: [PATCH v2 24/28] target/ppc/mmu_common.c: Remove BookE handling from get_physical_address_wtlb()
On Wed May 8, 2024 at 9:40 AM AEST, BALATON Zoltan wrote:
> On Tue, 7 May 2024, Nicholas Piggin wrote:
> > On Thu May 2, 2024 at 9:43 AM AEST, BALATON Zoltan wrote:
> >> This function is no longer called for BookE MMU model so remove parts
> >> related to it. This has uncovered a few may be used uninitialised
> >> warnings that are also fixed.
> >>
> >> Signed-off-by: BALATON Zoltan
> >> ---
> >> target/ppc/mmu_common.c | 25 +
> >> 1 file changed, 5 insertions(+), 20 deletions(-)
> >>
> >> diff --git a/target/ppc/mmu_common.c b/target/ppc/mmu_common.c
> >> index a1f98f8de4..d61c41d8c9 100644
> >> --- a/target/ppc/mmu_common.c
> >> +++ b/target/ppc/mmu_common.c
> >> @@ -684,12 +684,10 @@ static int mmubooke_get_physical_address(CPUPPCState
> >> *env, mmu_ctx_t *ctx,
> >> ret = mmubooke_check_tlb(env, tlb, , >prot, address,
> >> access_type, i);
> >> if (ret != -1) {
> >> -if (ret >= 0) {
> >> -ctx->raddr = raddr;
> >> -}
> >> break;
> >> }
> >> }
> >> +ctx->raddr = raddr;
> >> qemu_log_mask(CPU_LOG_MMU,
> >>"%s: access %s " TARGET_FMT_lx " => " HWADDR_FMT_plx
> >>" %d %d\n", __func__, ret < 0 ? "refused" : "granted",
> >> @@ -897,9 +895,6 @@ static int
> >> mmubooke206_get_physical_address(CPUPPCState *env, mmu_ctx_t *ctx,
> >> ret = mmubooke206_check_tlb(env, tlb, , >prot,
> >> address,
> >> access_type, mmu_idx);
> >> if (ret != -1) {
> >> -if (ret >= 0) {
> >> -ctx->raddr = raddr;
> >> -}
> >> goto found_tlb;
> >> }
> >> }
> >> @@ -907,6 +902,7 @@ static int
> >> mmubooke206_get_physical_address(CPUPPCState *env, mmu_ctx_t *ctx,
> >>
> >> found_tlb:
> >>
> >> +ctx->raddr = raddr;
> >
> > Not sure about the uninitialized warnings here either, caller probably
> > should not be using ctx->raddr unless we returned 0...
> >
> >> qemu_log_mask(CPU_LOG_MMU, "%s: access %s " TARGET_FMT_lx " => "
> >>HWADDR_FMT_plx " %d %d\n", __func__,
> >>ret < 0 ? "refused" : "granted", address, raddr,
> >> @@ -1163,20 +1159,9 @@ static int get_physical_address_wtlb(CPUPPCState
> >> *env, mmu_ctx_t *ctx,
> >> MMUAccessType access_type, int type,
> >> int mmu_idx)
> >> {
> >> -bool real_mode;
> >> -
> >> -if (env->mmu_model == POWERPC_MMU_BOOKE) {
> >> -return mmubooke_get_physical_address(env, ctx, eaddr,
> >> access_type);
> >> -} else if (env->mmu_model == POWERPC_MMU_BOOKE206) {
> >> -return mmubooke206_get_physical_address(env, ctx, eaddr,
> >> access_type,
> >> -mmu_idx);
> >> -}
> >
> > This could just go in the previous patch when you split booke xlate?
>
> Removing this uncovers the warnings so I keep it here to separate it from
> the previous change. I gave up on trying to resolve these warnings and
> untangle the embedded functions from mmu_ctx_t which would be needed to
> move these booke functions out from this file. The other problem is that
> these booke get_physical_address functions and mmu40x_get_physical_address
> all use ppcemb_tlb_check which then needs to be in the same file and
> static to be inlined and not run too slow but 40x is still in jumbo_xlate
> so I just leave it for now and may return to it later or let somebody else
> continue from here. I think this series moves forward enough for now and I
> don't have more time now.
If you can't easily drop the path or solve the problem okay, just put
a comment or something on the zeroing and I'll take a closer look
when I merge.
>
> >> -
> >> -real_mode = (type == ACCESS_CODE) ? !FIELD_EX64(env->msr, MSR, IR)
> >> - : !FIELD_EX64(env->msr, MSR, DR);
> >> -if (real_mode && (env->mmu_model == POWERPC_MMU_SOFT_6xx ||
> >> - env->mmu_model == POWERPC_MMU_SOFT_4xx ||
> >> - env->mmu_model == POWERPC_MMU_REAL)) {
> >> +bool real_mode = (type == ACCESS_CODE) ? !FIELD_EX64(env->msr, MSR,
> >> IR)
> >> + : !FIELD_EX64(env->msr, MSR,
> >> DR);
> >> +if (real_mode) {
> >> memset(ctx, 0, sizeof(*ctx));
> >> ctx->raddr = eaddr;
> >> ctx->prot = PAGE_READ | PAGE_WRITE | PAGE_EXEC;
> >
> > This still changes beahviour of MPC8xx MMU doesn't it? It's supposed
> > to abort always.
>
> I don't think it can get here because there's still an abort case in
> ppc_tlb_invalidate_all() which is called from ppc_cpu_reset_hold() so it
> will likely crash before it could call anything here. But if you think
> it's necessary I could add a case for it
Re: [PATCH v2 24/28] target/ppc/mmu_common.c: Remove BookE handling from get_physical_address_wtlb()
On Tue, 7 May 2024, Nicholas Piggin wrote:
On Thu May 2, 2024 at 9:43 AM AEST, BALATON Zoltan wrote:
This function is no longer called for BookE MMU model so remove parts
related to it. This has uncovered a few may be used uninitialised
warnings that are also fixed.
Signed-off-by: BALATON Zoltan
---
target/ppc/mmu_common.c | 25 +
1 file changed, 5 insertions(+), 20 deletions(-)
diff --git a/target/ppc/mmu_common.c b/target/ppc/mmu_common.c
index a1f98f8de4..d61c41d8c9 100644
--- a/target/ppc/mmu_common.c
+++ b/target/ppc/mmu_common.c
@@ -684,12 +684,10 @@ static int mmubooke_get_physical_address(CPUPPCState
*env, mmu_ctx_t *ctx,
ret = mmubooke_check_tlb(env, tlb, , >prot, address,
access_type, i);
if (ret != -1) {
-if (ret >= 0) {
-ctx->raddr = raddr;
-}
break;
}
}
+ctx->raddr = raddr;
qemu_log_mask(CPU_LOG_MMU,
"%s: access %s " TARGET_FMT_lx " => " HWADDR_FMT_plx
" %d %d\n", __func__, ret < 0 ? "refused" : "granted",
@@ -897,9 +895,6 @@ static int mmubooke206_get_physical_address(CPUPPCState
*env, mmu_ctx_t *ctx,
ret = mmubooke206_check_tlb(env, tlb, , >prot, address,
access_type, mmu_idx);
if (ret != -1) {
-if (ret >= 0) {
-ctx->raddr = raddr;
-}
goto found_tlb;
}
}
@@ -907,6 +902,7 @@ static int mmubooke206_get_physical_address(CPUPPCState
*env, mmu_ctx_t *ctx,
found_tlb:
+ctx->raddr = raddr;
Not sure about the uninitialized warnings here either, caller probably
should not be using ctx->raddr unless we returned 0...
qemu_log_mask(CPU_LOG_MMU, "%s: access %s " TARGET_FMT_lx " => "
HWADDR_FMT_plx " %d %d\n", __func__,
ret < 0 ? "refused" : "granted", address, raddr,
@@ -1163,20 +1159,9 @@ static int get_physical_address_wtlb(CPUPPCState *env,
mmu_ctx_t *ctx,
MMUAccessType access_type, int type,
int mmu_idx)
{
-bool real_mode;
-
-if (env->mmu_model == POWERPC_MMU_BOOKE) {
-return mmubooke_get_physical_address(env, ctx, eaddr, access_type);
-} else if (env->mmu_model == POWERPC_MMU_BOOKE206) {
-return mmubooke206_get_physical_address(env, ctx, eaddr, access_type,
-mmu_idx);
-}
This could just go in the previous patch when you split booke xlate?
Removing this uncovers the warnings so I keep it here to separate it from
the previous change. I gave up on trying to resolve these warnings and
untangle the embedded functions from mmu_ctx_t which would be needed to
move these booke functions out from this file. The other problem is that
these booke get_physical_address functions and mmu40x_get_physical_address
all use ppcemb_tlb_check which then needs to be in the same file and
static to be inlined and not run too slow but 40x is still in jumbo_xlate
so I just leave it for now and may return to it later or let somebody else
continue from here. I think this series moves forward enough for now and I
don't have more time now.
-
-real_mode = (type == ACCESS_CODE) ? !FIELD_EX64(env->msr, MSR, IR)
- : !FIELD_EX64(env->msr, MSR, DR);
-if (real_mode && (env->mmu_model == POWERPC_MMU_SOFT_6xx ||
- env->mmu_model == POWERPC_MMU_SOFT_4xx ||
- env->mmu_model == POWERPC_MMU_REAL)) {
+bool real_mode = (type == ACCESS_CODE) ? !FIELD_EX64(env->msr, MSR, IR)
+ : !FIELD_EX64(env->msr, MSR, DR);
+if (real_mode) {
memset(ctx, 0, sizeof(*ctx));
ctx->raddr = eaddr;
ctx->prot = PAGE_READ | PAGE_WRITE | PAGE_EXEC;
This still changes beahviour of MPC8xx MMU doesn't it? It's supposed
to abort always.
I don't think it can get here because there's still an abort case in
ppc_tlb_invalidate_all() which is called from ppc_cpu_reset_hold() so it
will likely crash before it could call anything here. But if you think
it's necessary I could add a case for it in ppc_xlate() maybe.
Regards,
BALATON Zoltan
Re: [PATCH v2 24/28] target/ppc/mmu_common.c: Remove BookE handling from get_physical_address_wtlb()
On Thu May 2, 2024 at 9:43 AM AEST, BALATON Zoltan wrote:
> This function is no longer called for BookE MMU model so remove parts
> related to it. This has uncovered a few may be used uninitialised
> warnings that are also fixed.
>
> Signed-off-by: BALATON Zoltan
> ---
> target/ppc/mmu_common.c | 25 +
> 1 file changed, 5 insertions(+), 20 deletions(-)
>
> diff --git a/target/ppc/mmu_common.c b/target/ppc/mmu_common.c
> index a1f98f8de4..d61c41d8c9 100644
> --- a/target/ppc/mmu_common.c
> +++ b/target/ppc/mmu_common.c
> @@ -684,12 +684,10 @@ static int mmubooke_get_physical_address(CPUPPCState
> *env, mmu_ctx_t *ctx,
> ret = mmubooke_check_tlb(env, tlb, , >prot, address,
> access_type, i);
> if (ret != -1) {
> -if (ret >= 0) {
> -ctx->raddr = raddr;
> -}
> break;
> }
> }
> +ctx->raddr = raddr;
> qemu_log_mask(CPU_LOG_MMU,
>"%s: access %s " TARGET_FMT_lx " => " HWADDR_FMT_plx
>" %d %d\n", __func__, ret < 0 ? "refused" : "granted",
> @@ -897,9 +895,6 @@ static int mmubooke206_get_physical_address(CPUPPCState
> *env, mmu_ctx_t *ctx,
> ret = mmubooke206_check_tlb(env, tlb, , >prot,
> address,
> access_type, mmu_idx);
> if (ret != -1) {
> -if (ret >= 0) {
> -ctx->raddr = raddr;
> -}
> goto found_tlb;
> }
> }
> @@ -907,6 +902,7 @@ static int mmubooke206_get_physical_address(CPUPPCState
> *env, mmu_ctx_t *ctx,
>
> found_tlb:
>
> +ctx->raddr = raddr;
Not sure about the uninitialized warnings here either, caller probably
should not be using ctx->raddr unless we returned 0...
> qemu_log_mask(CPU_LOG_MMU, "%s: access %s " TARGET_FMT_lx " => "
>HWADDR_FMT_plx " %d %d\n", __func__,
>ret < 0 ? "refused" : "granted", address, raddr,
> @@ -1163,20 +1159,9 @@ static int get_physical_address_wtlb(CPUPPCState *env,
> mmu_ctx_t *ctx,
> MMUAccessType access_type, int type,
> int mmu_idx)
> {
> -bool real_mode;
> -
> -if (env->mmu_model == POWERPC_MMU_BOOKE) {
> -return mmubooke_get_physical_address(env, ctx, eaddr, access_type);
> -} else if (env->mmu_model == POWERPC_MMU_BOOKE206) {
> -return mmubooke206_get_physical_address(env, ctx, eaddr, access_type,
> -mmu_idx);
> -}
This could just go in the previous patch when you split booke xlate?
> -
> -real_mode = (type == ACCESS_CODE) ? !FIELD_EX64(env->msr, MSR, IR)
> - : !FIELD_EX64(env->msr, MSR, DR);
> -if (real_mode && (env->mmu_model == POWERPC_MMU_SOFT_6xx ||
> - env->mmu_model == POWERPC_MMU_SOFT_4xx ||
> - env->mmu_model == POWERPC_MMU_REAL)) {
> +bool real_mode = (type == ACCESS_CODE) ? !FIELD_EX64(env->msr, MSR, IR)
> + : !FIELD_EX64(env->msr, MSR, DR);
> +if (real_mode) {
> memset(ctx, 0, sizeof(*ctx));
> ctx->raddr = eaddr;
> ctx->prot = PAGE_READ | PAGE_WRITE | PAGE_EXEC;
This still changes beahviour of MPC8xx MMU doesn't it? It's supposed
to abort always.
Thanks,
Nick
