From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by sourceware.org (Postfix) with ESMTPS id 8348E3858D35 for ; Mon, 22 Apr 2024 15:24:02 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 8348E3858D35 Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=redhat.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=redhat.com ARC-Filter: OpenARC Filter v1.0.0 sourceware.org 8348E3858D35 Authentication-Results: server2.sourceware.org; arc=none smtp.remote-ip=170.10.133.124 ARC-Seal: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1713799450; cv=none; b=O8etoTpbHoxLWVj7grIuwNO7LUlTWvQIGZdnI8/X3ixeT4z7C16t7U3saDu/SvUnvJJMt5UxBJ0ETxY+NssPfZ7IjbdpfsCw0+mH92ag1nR/qo+OfGVyIww9gIk9rS6FPLZDIHGPaSGUIrRDfwlbhWrrKS85Mbp2xun7dFzQa88= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1713799450; c=relaxed/simple; bh=cT7+W+mXwbdYwfENHAzDKHeAUQH467db0b061uxGZ0U=; h=DKIM-Signature:From:To:Subject:Date:Message-ID:MIME-Version; b=DIvwHH83j5GbSXCkzvS+lCnMr/bTHA4qauSM9d4nffky9OpCZZ6U0FVFZW3QPsm0qLnRHJKphFsRNJ/iOAi5QPgYcZM2uRr8aRrrLaBA2kOlRpBEXDdOl0+NSUy6xXS0vxBa1Mw9r3D6fSn8s2oIbeNQSw0FHauounorhnzAiu8= ARC-Authentication-Results: i=1; server2.sourceware.org DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1713799441; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=VokthJBtHez3sZwQAxRSEkuUTxEQFR0qDupf0BwFuqU=; b=XDqyo5K3t+G5+HSdMPJrVbRW+JHex0dMU+wn9lIzjv5gHvWuvAxABK9Ymo29msMisyRrE/ QDSyCFRhVfmPNPX7BBTPaGOYV5lK55hNSU1pIFaEuzl2Ig7Rot/zij2UCeBH3pOYTXEq4a blkkKcTNnlWBCUjZLKGjw8/i9VF5Zo4= Received: from mail-wr1-f70.google.com (mail-wr1-f70.google.com [209.85.221.70]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-460-8781Aig2O8mkW13F9idteg-1; Mon, 22 Apr 2024 11:01:36 -0400 X-MC-Unique: 8781Aig2O8mkW13F9idteg-1 Received: by mail-wr1-f70.google.com with SMTP id ffacd0b85a97d-346c08df987so2466698f8f.0 for ; Mon, 22 Apr 2024 08:01:36 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1713798095; x=1714402895; h=mime-version:message-id:date:references:in-reply-to:subject:to:from :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=VokthJBtHez3sZwQAxRSEkuUTxEQFR0qDupf0BwFuqU=; b=Oh/YylyRr6HJSNDT1xcSZ1K7yzzFgSVbgrLl8W0T4/O01lhER0Mak3drkDj/bfLaML nIa1adThcb9uzWlzB0XcdR0EJQxeWc1rhoKzcCeCcHROQDPRX7OyHgE1/+0PnJ8XeuRs SM2SPeL3ppNFHSs1F4JG9tWNlZbYdZ0IoKjx5NnXMv7MPSaYEe9A3mJfemAVAjFz1N2o RhYV1CxT7HHvkIQj0x+/EY2SYUL0ToS67OPuBP0CbgTiv9olJ7R3PfSGRwgYGp69e5mc wxVk+kSgw4DB5eq8x/u/We1daYKqj20Uy1cZ2fNXz3WgbJk5COh/kk1lSJoSxUUgqbPt KaxQ== X-Forwarded-Encrypted: i=1; AJvYcCV8iZSqjnLqL8MoYID0pVLfokFbIalBSq1H3f316aCE/bCbqSTbnroMGhITS9VDeLDZjGDKh2yBxk5BV5w8q0BbsY18FG0lNjbbBg== X-Gm-Message-State: AOJu0YxtJ2w23Pt486IRDit5RAz1as4yf8/XVI6JR8109ZiX7ShSW9M+ ToBLPSqzD2iLXUa/QgZmQH7h2POC7QhZFmB0nUKMWiY9CvWwai0SZRcDMKjIXatn/zFM6nfNE3X 481PVexbnVTfXEklJsO+08SBaEngDIBW+LdR22otthgzfwPxU7dV80d+5l50ltnhyVyg= X-Received: by 2002:a05:6000:12d0:b0:343:b748:9af2 with SMTP id l16-20020a05600012d000b00343b7489af2mr7300160wrx.19.1713798094387; Mon, 22 Apr 2024 08:01:34 -0700 (PDT) X-Google-Smtp-Source: AGHT+IFygYvVSJ2dCfv+eDkkooRsYoMhwYF8f1ToYedgbd+m7xkASvSlWcKQE2nULMx5qxeqdtPc6g== X-Received: by 2002:a05:6000:12d0:b0:343:b748:9af2 with SMTP id l16-20020a05600012d000b00343b7489af2mr7300122wrx.19.1713798093695; Mon, 22 Apr 2024 08:01:33 -0700 (PDT) Received: from localhost (185.223.159.143.dyn.plus.net. [143.159.223.185]) by smtp.gmail.com with ESMTPSA id q30-20020adfb19e000000b0034a9addd4e6sm7149985wra.81.2024.04.22.08.01.33 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 22 Apr 2024 08:01:33 -0700 (PDT) From: Andrew Burgess To: Bernd Edlinger , "gdb-patches@sourceware.org" Subject: Re: [PATCH] sim: riscv: Fix some issues with class-a instructions In-Reply-To: References: Date: Mon, 22 Apr 2024 16:01:32 +0100 Message-ID: <87ttjtfnlf.fsf@redhat.com> MIME-Version: 1.0 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain X-Spam-Status: No, score=-12.2 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,GIT_PATCH_0,RCVD_IN_DNSWL_NONE,RCVD_IN_MSPIKE_H4,RCVD_IN_MSPIKE_WL,SPF_HELO_NONE,SPF_NONE,TXREP autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: Bernd Edlinger writes: > This fixes some issues with atomic instruction handling. First the > instructions may have AQ and/or RL bits set, but the emulator has no > such concept, so we have to ignore those. > > According to the spec the memory must be naturally aligned, otherwise > an exception shall be thrown, so do the sim_core read/write aligned. > In the case of riscv64 target, there were the LR_D and SC_D > 64bit load and store instructions missing, so add those. > > Also the AMOMIN/AMOMAX[U]_W instructions were not correct for riscv64 > because the upper half word of the input registers were not ignored > as they should, so use explicit type-casts to uint32_t and int32_t > for those. > > And finally make the class-a instruction set only executable if a > riscv cpu model with A extension is selected. > --- > sim/riscv/sim-main.c | 72 ++++++++++++++++++++++++++++++++------------ > 1 file changed, 53 insertions(+), 19 deletions(-) > > diff --git a/sim/riscv/sim-main.c b/sim/riscv/sim-main.c > index e4b15b533ba..be38baa2f93 100644 > --- a/sim/riscv/sim-main.c > +++ b/sim/riscv/sim-main.c > @@ -841,6 +841,9 @@ execute_m (SIM_CPU *cpu, unsigned_word iw, const struct riscv_opcode *op) > static sim_cia > execute_a (SIM_CPU *cpu, unsigned_word iw, const struct riscv_opcode *op) > { > + unsigned_word mask_aq = OP_MASK_AQ << OP_SH_AQ; > + unsigned_word mask_rl = OP_MASK_RL << OP_SH_RL; > + unsigned_word mask_aqrl = mask_aq | mask_rl; > struct riscv_sim_cpu *riscv_cpu = RISCV_SIM_CPU (cpu); > SIM_DESC sd = CPU_STATE (cpu); > struct riscv_sim_state *state = RISCV_SIM_STATE (sd); > @@ -855,13 +858,18 @@ execute_a (SIM_CPU *cpu, unsigned_word iw, const struct riscv_opcode *op) > sim_cia pc = riscv_cpu->pc + 4; > > /* Handle these two load/store operations specifically. */ > - switch (op->match) > + switch (op->match & ~mask_aqrl) Given the aq/rl bits are not handled, maybe we should be issuing a 1 time warning if we encounter an instruction with these bits set? Or maybe it's not relevant for the simulator? I think more detail about what the thinking is here would help: Is this safe to ignore, or is this something we're putting off for the future? > { > + case MATCH_LR_D: > case MATCH_LR_W: > TRACE_INSN (cpu, "%s %s, (%s);", op->name, rd_name, rs1_name); > - store_rd (cpu, rd, > - sim_core_read_unaligned_4 (cpu, riscv_cpu->pc, read_map, > - riscv_cpu->regs[rs1])); > + if (op->xlen_requirement == 64) > + tmp = sim_core_read_aligned_8 (cpu, riscv_cpu->pc, read_map, > + riscv_cpu->regs[rs1]); > + else > + tmp = EXTEND32 (sim_core_read_aligned_4 (cpu, riscv_cpu->pc, read_map, > + riscv_cpu->regs[rs1])); I might be completely off the mark here, but I'm unsure about this change from using 'unaligned' to 'aligned' here. >From my reading of common/sim-n-core.h, I believe the aligned/unaligned refers to the callers knowledge of the address. So calling sim_core_read_aligned_* means that the common code should assume that the address is correctly aligned. While sim_core_read_unaligned_* handles the case that the address might not be aligned, and raises the exception. Please remember: I might be completely wrong here, and you just need to explain to me what's going on better. > + store_rd (cpu, rd, tmp); > > /* Walk the reservation list to find an existing match. */ > amo_curr = state->amo_reserved_list; > @@ -878,6 +886,7 @@ execute_a (SIM_CPU *cpu, unsigned_word iw, const struct riscv_opcode *op) > amo_curr->next = state->amo_reserved_list; > state->amo_reserved_list = amo_curr; > goto done; > + case MATCH_SC_D: > case MATCH_SC_W: > TRACE_INSN (cpu, "%s %s, %s, (%s);", op->name, rd_name, rs2_name, > rs1_name); > @@ -889,7 +898,12 @@ execute_a (SIM_CPU *cpu, unsigned_word iw, const struct riscv_opcode *op) > if (amo_curr->addr == riscv_cpu->regs[rs1]) > { > /* We found a reservation, so operate it. */ > - sim_core_write_unaligned_4 (cpu, riscv_cpu->pc, write_map, > + if (op->xlen_requirement == 64) > + sim_core_write_aligned_8 (cpu, riscv_cpu->pc, write_map, > + riscv_cpu->regs[rs1], > + riscv_cpu->regs[rs2]); > + else > + sim_core_write_aligned_4 (cpu, riscv_cpu->pc, write_map, > riscv_cpu->regs[rs1], > riscv_cpu->regs[rs2]); > store_rd (cpu, rd, 0); > @@ -913,14 +927,14 @@ execute_a (SIM_CPU *cpu, unsigned_word iw, const struct riscv_opcode *op) > TRACE_INSN (cpu, "%s %s, %s, (%s);", > op->name, rd_name, rs2_name, rs1_name); > if (op->xlen_requirement == 64) > - tmp = sim_core_read_unaligned_8 (cpu, riscv_cpu->pc, read_map, > - riscv_cpu->regs[rs1]); > + tmp = sim_core_read_aligned_8 (cpu, riscv_cpu->pc, read_map, > + riscv_cpu->regs[rs1]); > else > - tmp = EXTEND32 (sim_core_read_unaligned_4 (cpu, riscv_cpu->pc, read_map, > - riscv_cpu->regs[rs1])); > + tmp = EXTEND32 (sim_core_read_aligned_4 (cpu, riscv_cpu->pc, read_map, > + riscv_cpu->regs[rs1])); > store_rd (cpu, rd, tmp); > > - switch (op->match) > + switch (op->match & ~mask_aqrl) > { > case MATCH_AMOADD_D: > case MATCH_AMOADD_W: > @@ -931,25 +945,37 @@ execute_a (SIM_CPU *cpu, unsigned_word iw, const struct riscv_opcode *op) > tmp = riscv_cpu->regs[rd] & riscv_cpu->regs[rs2]; > break; > case MATCH_AMOMAX_D: > - case MATCH_AMOMAX_W: > tmp = max ((signed_word) riscv_cpu->regs[rd], > (signed_word) riscv_cpu->regs[rs2]); > break; > + case MATCH_AMOMAX_W: > + tmp = max ((int32_t) riscv_cpu->regs[rd], > + (int32_t) riscv_cpu->regs[rs2]); I wonder if we should use EXTEND32 here? There seems to be a mix of styles throughout the file. > + break; > case MATCH_AMOMAXU_D: > - case MATCH_AMOMAXU_W: > tmp = max ((unsigned_word) riscv_cpu->regs[rd], > (unsigned_word) riscv_cpu->regs[rs2]); > break; > + case MATCH_AMOMAXU_W: > + tmp = max ((uint32_t) riscv_cpu->regs[rd], > + (uint32_t) riscv_cpu->regs[rs2]); And maybe EXTEND32 again here, but I guess we'd still need the cast to unsigned_word. > + break; > case MATCH_AMOMIN_D: > - case MATCH_AMOMIN_W: > tmp = min ((signed_word) riscv_cpu->regs[rd], > (signed_word) riscv_cpu->regs[rs2]); > break; > + case MATCH_AMOMIN_W: > + tmp = min ((int32_t) riscv_cpu->regs[rd], > + (int32_t) riscv_cpu->regs[rs2]); Same question as above. > + break; > case MATCH_AMOMINU_D: > - case MATCH_AMOMINU_W: > tmp = min ((unsigned_word) riscv_cpu->regs[rd], > (unsigned_word) riscv_cpu->regs[rs2]); > break; > + case MATCH_AMOMINU_W: > + tmp = min ((uint32_t) riscv_cpu->regs[rd], > + (uint32_t) riscv_cpu->regs[rs2]); Same question as above. > + break; > case MATCH_AMOOR_D: > case MATCH_AMOOR_W: > tmp = riscv_cpu->regs[rd] | riscv_cpu->regs[rs2]; > @@ -968,11 +994,11 @@ execute_a (SIM_CPU *cpu, unsigned_word iw, const struct riscv_opcode *op) > } > > if (op->xlen_requirement == 64) > - sim_core_write_unaligned_8 (cpu, riscv_cpu->pc, write_map, > - riscv_cpu->regs[rs1], tmp); > + sim_core_write_aligned_8 (cpu, riscv_cpu->pc, write_map, > + riscv_cpu->regs[rs1], tmp); > else > - sim_core_write_unaligned_4 (cpu, riscv_cpu->pc, write_map, > - riscv_cpu->regs[rs1], tmp); > + sim_core_write_aligned_4 (cpu, riscv_cpu->pc, write_map, > + riscv_cpu->regs[rs1], tmp); > > done: > return pc; > @@ -1307,7 +1333,15 @@ execute_one (SIM_CPU *cpu, unsigned_word iw, const struct riscv_opcode *op) > switch (op->insn_class) > { > case INSN_CLASS_A: > - return execute_a (cpu, iw, op); > + /* Check whether model with A extension is selected. */ > + if (riscv_cpu->csr.misa & 1) > + return execute_a (cpu, iw, op); > + else > + { > + TRACE_INSN (cpu, "UNHANDLED EXTENSION: %d", op->insn_class); > + sim_engine_halt (sd, cpu, NULL, riscv_cpu->pc, sim_signalled, > + SIM_SIGILL); > + } Thanks, Andrew > case INSN_CLASS_C: > /* Check whether model with C extension is selected. */ > if (riscv_cpu->csr.misa & 4) > -- > 2.39.2