From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-12.9 required=3.0 tests=BAYES_00, HEADER_FROM_DIFFERENT_DOMAINS,HTML_MESSAGE,INCLUDES_CR_TRAILER,INCLUDES_PATCH, MAILING_LIST_MULTI,NICE_REPLY_A,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED, USER_AGENT_SANE_1 autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 3F915C47083 for ; Wed, 2 Jun 2021 19:59:47 +0000 (UTC) Received: from lists.gnu.org (lists.gnu.org [209.51.188.17]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 7AE216135D for ; Wed, 2 Jun 2021 19:59:46 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 7AE216135D Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=eldorado.org.br Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=qemu-devel-bounces+qemu-devel=archiver.kernel.org@nongnu.org Received: from localhost ([::1]:53470 helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1loX1l-0006ka-Al for qemu-devel@archiver.kernel.org; Wed, 02 Jun 2021 15:59:45 -0400 Received: from eggs.gnu.org ([2001:470:142:3::10]:43566) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1loX0A-0005iI-Rh; Wed, 02 Jun 2021 15:58:06 -0400 Received: from [201.28.113.2] (port=14585 helo=outlook.eldorado.org.br) by eggs.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1loX08-000660-Hv; Wed, 02 Jun 2021 15:58:06 -0400 Received: from power9a ([10.10.71.235]) by outlook.eldorado.org.br with Microsoft SMTPSVC(8.5.9600.16384); Wed, 2 Jun 2021 16:58:01 -0300 Received: from [127.0.0.1] (unknown [10.10.71.235]) by power9a (Postfix) with ESMTPS id AEF1C80148C; Wed, 2 Jun 2021 16:58:00 -0300 (-03) Subject: Re: [RFC PATCH] target/ppc: fix address translation bug for hash table mmus To: Richard Henderson , qemu-devel@nongnu.org References: <20210602191822.90182-1-bruno.larsen@eldorado.org.br> From: Bruno Piazera Larsen Message-ID: <39c92ce9-46b8-4847-974c-647c7a5ca2ae@eldorado.org.br> Date: Wed, 2 Jun 2021 16:58:00 -0300 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.8.1 MIME-Version: 1.0 In-Reply-To: Content-Type: multipart/alternative; boundary="------------F203444E559899C6A3A29510" Content-Language: en-US X-OriginalArrivalTime: 02 Jun 2021 19:58:01.0051 (UTC) FILETIME=[977172B0:01D757E9] X-Host-Lookup-Failed: Reverse DNS lookup failed for 201.28.113.2 (failed) Received-SPF: pass client-ip=201.28.113.2; envelope-from=bruno.larsen@eldorado.org.br; helo=outlook.eldorado.org.br X-Spam_score_int: -16 X-Spam_score: -1.7 X-Spam_bar: - X-Spam_report: (-1.7 / 5.0 requ) BAYES_00=-1.9, HTML_MESSAGE=0.001, NICE_REPLY_A=-0.613, RDNS_NONE=0.793, SPF_HELO_NONE=0.001, SPF_PASS=-0.001 autolearn=no autolearn_force=no X-Spam_action: no action X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.23 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: farosas@linux.ibm.com, luis.pires@eldorado.org.br, Greg Kurz , lucas.araujo@eldorado.org.br, fernando.valle@eldorado.org.br, qemu-ppc@nongnu.org, matheus.ferst@eldorado.org.br, david@gibson.dropbear.id.au Errors-To: qemu-devel-bounces+qemu-devel=archiver.kernel.org@nongnu.org Sender: "Qemu-devel" This is a multi-part message in MIME format. --------------F203444E559899C6A3A29510 Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit 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) >> --- >>   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 Departamento Computação Embarcada Analista de Software Trainee Aviso Legal - Disclaimer --------------F203444E559899C6A3A29510 Content-Type: text/html; charset=utf-8 Content-Transfer-Encoding: 8bit


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
Departamento Computação Embarcada
Analista de Software Trainee
Aviso Legal - Disclaimer
--------------F203444E559899C6A3A29510--