From: Bruno Piazera Larsen <bruno.larsen@eldorado.org.br>
To: Richard Henderson <richard.henderson@linaro.org>, 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 16:58:00 -0300 [thread overview]
Message-ID: <39c92ce9-46b8-4847-974c-647c7a5ca2ae@eldorado.org.br> (raw)
In-Reply-To: <d7139129-428a-f6c9-c6e2-e540208d62aa@linaro.org>
[-- Attachment #1: Type: text/plain, Size: 3775 bytes --]
On 02/06/2021 16:26, Richard Henderson wrote:
> On 6/2/21 12:18 PM, Bruno Larsen (billionai) wrote:
>> Based-on: <20210518201146.794854-1-richard.henderson@linaro.org>
>>
>> This commit attempts to implement a first draft of a solution to the
>> first bug mentioned by Richard Henderson in this e-mail
>> https://lists.nongnu.org/archive/html/qemu-devel/2021-05/msg06247.html
>> The second bug was not touched, which is basically implementing the
>> solution C
>>
>> To sumarize the first bug here, from my understanding, when an address
>> translation is asked of a 64bit mmu that uses hashtables, the code
>> attempts to check some permission bits, but checks them from the wrong
>> location.
>>
>> The solution implemented here is more complex than necessary on
>> purpose, to make it more readable (and make sure I understand what is
>> going on). If that would really fix the problem, I'll move to
>> implementing an actual solution, and to all affected functions.
>>
>> Signed-off-by: Bruno Larsen (billionai) <bruno.larsen@eldorado.org.br>
>> ---
>> target/ppc/mmu-hash64.c | 12 ++++++++++--
>> 1 file changed, 10 insertions(+), 2 deletions(-)
>>
>> diff --git a/target/ppc/mmu-hash64.c b/target/ppc/mmu-hash64.c
>> index c1b98a97e9..63f10f1be7 100644
>> --- a/target/ppc/mmu-hash64.c
>> +++ b/target/ppc/mmu-hash64.c
>> @@ -887,6 +887,14 @@ bool ppc_hash64_xlate(PowerPCCPU *cpu, vaddr
>> eaddr, MMUAccessType access_type,
>> int exec_prot, pp_prot, amr_prot, prot;
>> int need_prot;
>> hwaddr raddr;
>> + unsigned immu_idx, dmmu_idx;
>> + immu_idx = (env->hflags >> HFLAGS_IMMU_IDX) & 7;
>> + dmmu_idx = (env->hflags >> HFLAGS_DMMU_IDX) & 7;
>
> This doesn't help at all with the reported bug. You're still reading
> from env. You need the mmu_idx that was passed to ppc_cpu_tlb_fill.
Ah, I saw a macro for MMU_IDX and assumed they pointed to different
locations. Ok, the fix for ppc_cpu_tlb_fill should be easy, then
>
> 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?
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?
>
>
>> + 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.
>
>> - if (access_type == MMU_INST_FETCH ? !msr_ir : !msr_dr) {
>> + if (access_type == MMU_INST_FETCH ? !MSR[IR] : !MSR[DR]) {
>
> Which simplifies this condition to just a single test.
>
>
> r~
--
Bruno Piazera Larsen
Instituto de Pesquisas ELDORADO
<https://www.eldorado.org.br/?utm_campaign=assinatura_de_e-mail&utm_medium=email&utm_source=RD+Station>
Departamento Computação Embarcada
Analista de Software Trainee
Aviso Legal - Disclaimer <https://www.eldorado.org.br/disclaimer.html>
[-- Attachment #2: Type: text/html, Size: 6106 bytes --]
next prev parent reply other threads:[~2021-06-02 19:59 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 [this message]
2021-06-02 22:19 ` Richard Henderson
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=39c92ce9-46b8-4847-974c-647c7a5ca2ae@eldorado.org.br \
--to=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 \
--cc=richard.henderson@linaro.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.