From: Richard Henderson <richard.henderson@linaro.org>
To: Bruno Piazera Larsen <bruno.larsen@eldorado.org.br>,
qemu-devel@nongnu.org
Cc: farosas@linux.ibm.com, luis.pires@eldorado.org.br,
Greg Kurz <groug@kaod.org>,
lucas.araujo@eldorado.org.br, fernando.valle@eldorado.org.br,
qemu-ppc@nongnu.org, matheus.ferst@eldorado.org.br,
david@gibson.dropbear.id.au
Subject: Re: [RFC PATCH] target/ppc: fix address translation bug for hash table mmus
Date: Wed, 2 Jun 2021 15:19:20 -0700 [thread overview]
Message-ID: <b689bdd0-4d75-7c4e-189e-922738208dc0@linaro.org> (raw)
In-Reply-To: <39c92ce9-46b8-4847-974c-647c7a5ca2ae@eldorado.org.br>
On 6/2/21 12:58 PM, Bruno Piazera Larsen wrote:
>> For the use from ppc_cpu_get_phys_page_debug, you'd pass in
>> cpu_mmu_index(env, false).
>
> ppc_cpu_get_phys_page_debug has 2 calls to ppc_xlate, one using the data MMU,
> the other using the instruction MMU. I'm guessing I should pass both, right?
Yes.
> But here we have another bit that confuses me: cpu_mmu_index returns 0 if in
> user mode, or uses the information stored in env to get it, so I don't see how
> that would be different from getting directly. Unless the point is to have
> ppc_*_xlate be generic and pc_*_debug knows the info in env is correct. Is that it?
The issue is that
(1) ppc_*_xlate should perform the lookup requested, and mmu_idx
does not *necessarily* correspond to the current contents of
env->msr et al. See (2).
(2) There is a secondary call to ppc_radix64_partition_scoped_xlate
for which the second stage page table should be read
with hypervisor permissions, and not the permissions of the
original memory access.
Note that ppc_radix64_check_prot checks msr_pr directly.
Thus the second stage lookup should use mmu_idx = 5
(HV kernel virtual mode). If I understand things correctly...
>
>>
>>
>>> + const short HV = 1, IR = 2, DR = 3;
>>> + bool MSR[3];
>>> + MSR[HV] = dmmu_idx & 2,
>>> + MSR[IR] = immu_idx & 4,
>>> + MSR[DR] = dmmu_idx & 4;
>>
>> There's no point in the array. Just use three different scalars (real_mode,
>> hv, and pr (note that pr is the major portion of the bug as reported)).
>> Additionally, you'll not be distinguishing immu_idx and dmmu_idx, but using
>> the single idx that's given.
>
> Ah, yeah, that's the "more complex than necessary, but it was easy for me to
> read" part. Scalars are a good solution. In this function in specific, PR
> doesn't actually show up anywhere, so I would actually only need 2. Anyway,
> will start working on this.
Oh, I'll note that your constants above are wrong. I think that you should
have some common routines in (mmu-)internal.h:
/*
* These correspond to the mmu_idx values computed in
* hreg_compute_hflags_value. See the tables therein.
*/
static inline bool mmuidx_pr(int idx) { return idx & 1; }
static inline bool mmuidx_real(int idx) { return idx & 2; }
static inline bool mmuidx_hv(int idx) { return idx & 4; }
because you'll want to use these past mmu-radix64.c.
Then you also have a single place to adjust if the mmu_idx are reordered at a
later date.
r~
next prev parent reply other threads:[~2021-06-02 22:20 UTC|newest]
Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-06-02 19:18 [RFC PATCH] target/ppc: fix address translation bug for hash table mmus Bruno Larsen (billionai)
2021-06-02 19:26 ` Richard Henderson
2021-06-02 19:58 ` Bruno Piazera Larsen
2021-06-02 22:19 ` Richard Henderson [this message]
2021-06-07 19:29 ` Bruno Piazera Larsen
2021-06-07 21:06 ` Richard Henderson
2021-06-08 14:39 ` Bruno Piazera Larsen
2021-06-08 15:35 ` Richard Henderson
2021-06-08 16:37 ` Bruno Piazera Larsen
2021-06-08 18:39 ` Bruno Piazera Larsen
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=b689bdd0-4d75-7c4e-189e-922738208dc0@linaro.org \
--to=richard.henderson@linaro.org \
--cc=bruno.larsen@eldorado.org.br \
--cc=david@gibson.dropbear.id.au \
--cc=farosas@linux.ibm.com \
--cc=fernando.valle@eldorado.org.br \
--cc=groug@kaod.org \
--cc=lucas.araujo@eldorado.org.br \
--cc=luis.pires@eldorado.org.br \
--cc=matheus.ferst@eldorado.org.br \
--cc=qemu-devel@nongnu.org \
--cc=qemu-ppc@nongnu.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.