Re: [PULL v2 13/15] hw/rx: Add RX GDB simulator

[Philippe Mathieu-Daudé]

On 9/9/20 7:56 PM, Philippe Mathieu-Daudé wrote:
> Hi Yoshinori,
> 
> On 9/7/20 3:13 PM, Peter Maydell wrote:
>> On Mon, 22 Jun 2020 at 20:20, Philippe Mathieu-Daudé  wrote:
>>>
>>> From: Yoshinori Sato 
>>>
>>> Add the RX machine internally simulated in GDB.
>>
>> Hi; Coverity points out a memory leak (CID 1432307) in this function:
>>
>>> +static void rx_gdbsim_init(MachineState *machine)
>>> +{
>>
>>> +if (dtb_filename) {
>>> +ram_addr_t dtb_offset;
>>> +int dtb_size;
>>> +void *dtb;
>>> +
>>> +dtb = load_device_tree(dtb_filename, _size);
>>
>> This allocates memory...
>>
>>> +if (dtb == NULL) {
>>> +error_report("Couldn't open dtb file %s", dtb_filename);
>>> +exit(1);
>>> +}
>>> +if (machine->kernel_cmdline &&
>>> +qemu_fdt_setprop_string(dtb, "/chosen", "bootargs",
>>> +machine->kernel_cmdline) < 0) {
>>> +error_report("Couldn't set /chosen/bootargs");
>>> +exit(1);
>>> +}
>>> +/* DTB is located at the end of SDRAM space. */
>>> +dtb_offset = machine->ram_size - dtb_size;
>>> +rom_add_blob_fixed("dtb", dtb, dtb_size,
>>> +   SDRAM_BASE + dtb_offset);
>>
>> ...and rom_add_blob_fixed() copies that memory, it doesn't take
>> ownership of it, so after we've called it we need to
>> g_free(fdt);
> 
> Can you send a patch to fix this please?

ping?

> 
>>
>>> +/* Set dtb address to R1 */
>>> +RXCPU(first_cpu)->env.regs[1] = SDRAM_BASE + dtb_offset;
>>> +}
>>> +}
>>
>> thanks
>> -- PMM
>>
> 

Re: [PULL v2 13/15] hw/rx: Add RX GDB simulator

[Philippe Mathieu-Daudé]

Hi Yoshinori,

On 9/7/20 3:13 PM, Peter Maydell wrote:
> On Mon, 22 Jun 2020 at 20:20, Philippe Mathieu-Daudé  wrote:
>>
>> From: Yoshinori Sato 
>>
>> Add the RX machine internally simulated in GDB.
> 
> Hi; Coverity points out a memory leak (CID 1432307) in this function:
> 
>> +static void rx_gdbsim_init(MachineState *machine)
>> +{
> 
>> +if (dtb_filename) {
>> +ram_addr_t dtb_offset;
>> +int dtb_size;
>> +void *dtb;
>> +
>> +dtb = load_device_tree(dtb_filename, _size);
> 
> This allocates memory...
> 
>> +if (dtb == NULL) {
>> +error_report("Couldn't open dtb file %s", dtb_filename);
>> +exit(1);
>> +}
>> +if (machine->kernel_cmdline &&
>> +qemu_fdt_setprop_string(dtb, "/chosen", "bootargs",
>> +machine->kernel_cmdline) < 0) {
>> +error_report("Couldn't set /chosen/bootargs");
>> +exit(1);
>> +}
>> +/* DTB is located at the end of SDRAM space. */
>> +dtb_offset = machine->ram_size - dtb_size;
>> +rom_add_blob_fixed("dtb", dtb, dtb_size,
>> +   SDRAM_BASE + dtb_offset);
> 
> ...and rom_add_blob_fixed() copies that memory, it doesn't take
> ownership of it, so after we've called it we need to
> g_free(fdt);

Can you send a patch to fix this please?

> 
>> +/* Set dtb address to R1 */
>> +RXCPU(first_cpu)->env.regs[1] = SDRAM_BASE + dtb_offset;
>> +}
>> +}
> 
> thanks
> -- PMM
> 

Re: [PULL v2 13/15] hw/rx: Add RX GDB simulator

[Peter Maydell]

On Mon, 22 Jun 2020 at 20:20, Philippe Mathieu-Daudé  wrote:
>
> From: Yoshinori Sato 
>
> Add the RX machine internally simulated in GDB.

Hi; Coverity points out a memory leak (CID 1432307) in this function:

> +static void rx_gdbsim_init(MachineState *machine)
> +{

> +if (dtb_filename) {
> +ram_addr_t dtb_offset;
> +int dtb_size;
> +void *dtb;
> +
> +dtb = load_device_tree(dtb_filename, _size);

This allocates memory...

> +if (dtb == NULL) {
> +error_report("Couldn't open dtb file %s", dtb_filename);
> +exit(1);
> +}
> +if (machine->kernel_cmdline &&
> +qemu_fdt_setprop_string(dtb, "/chosen", "bootargs",
> +machine->kernel_cmdline) < 0) {
> +error_report("Couldn't set /chosen/bootargs");
> +exit(1);
> +}
> +/* DTB is located at the end of SDRAM space. */
> +dtb_offset = machine->ram_size - dtb_size;
> +rom_add_blob_fixed("dtb", dtb, dtb_size,
> +   SDRAM_BASE + dtb_offset);

...and rom_add_blob_fixed() copies that memory, it doesn't take
ownership of it, so after we've called it we need to
g_free(fdt);

> +/* Set dtb address to R1 */
> +RXCPU(first_cpu)->env.regs[1] = SDRAM_BASE + dtb_offset;
> +}
> +}

thanks
-- PMM

[PULL v2 13/15] hw/rx: Add RX GDB simulator

[Philippe Mathieu-Daudé]

From: Yoshinori Sato 

Add the RX machine internally simulated in GDB.

Signed-off-by: Yoshinori Sato 
Tested-by: Philippe Mathieu-Daudé 
Reviewed-by: Philippe Mathieu-Daudé 
Signed-off-by: Richard Henderson 
[PMD: Use TYPE_RX62N_CPU, use #define for RX62N_NR_TMR/CMT/SCI,
 renamed CPU -> MCU, device -> microcontroller]
Signed-off-by: Philippe Mathieu-Daudé 
Message-Id: <20200224141923.82118-18-ys...@users.sourceforge.jp>
[PMD: Split of MCU, rename gdbsim, Add gdbsim-r5f562n7/r5f562n8]
Signed-off-by: Philippe Mathieu-Daudé 
---
 default-configs/rx-softmmu.mak |   1 +
 include/hw/rx/rx62n.h  |   4 +
 hw/rx/rx-gdbsim.c  | 198 +
 MAINTAINERS|   7 ++
 hw/rx/Kconfig  |   4 +
 hw/rx/Makefile.objs|   1 +
 6 files changed, 215 insertions(+)
 create mode 100644 hw/rx/rx-gdbsim.c

diff --git a/default-configs/rx-softmmu.mak b/default-configs/rx-softmmu.mak
index 7c4eb2c1a0..df2b4e4f42 100644
--- a/default-configs/rx-softmmu.mak
+++ b/default-configs/rx-softmmu.mak
@@ -1,2 +1,3 @@
 # Default configuration for rx-softmmu
 
+CONFIG_RX_GDBSIM=y
diff --git a/include/hw/rx/rx62n.h b/include/hw/rx/rx62n.h
index 1d3e6a5cad..aa94758c27 100644
--- a/include/hw/rx/rx62n.h
+++ b/include/hw/rx/rx62n.h
@@ -37,6 +37,10 @@
 #define TYPE_R5F562N7_MCU "r5f562n7-mcu"
 #define TYPE_R5F562N8_MCU "r5f562n8-mcu"
 
+#define EXT_CS_BASE 0x0100
+#define VECTOR_TABLE_BASE   0xff80
+#define RX62N_CFLASH_BASE   0xfff8
+
 #define RX62N_NR_TMR2
 #define RX62N_NR_CMT2
 #define RX62N_NR_SCI6
diff --git a/hw/rx/rx-gdbsim.c b/hw/rx/rx-gdbsim.c
new file mode 100644
index 00..b8a56fa7af
--- /dev/null
+++ b/hw/rx/rx-gdbsim.c
@@ -0,0 +1,198 @@
+/*
+ * RX QEMU GDB simulator
+ *
+ * Copyright (c) 2019 Yoshinori Sato
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2 or later, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
+ * more details.
+ *
+ * You should have received a copy of the GNU General Public License along with
+ * this program.  If not, see .
+ */
+
+#include "qemu/osdep.h"
+#include "qemu/cutils.h"
+#include "qemu/error-report.h"
+#include "qapi/error.h"
+#include "qemu-common.h"
+#include "cpu.h"
+#include "hw/hw.h"
+#include "hw/sysbus.h"
+#include "hw/loader.h"
+#include "hw/rx/rx62n.h"
+#include "sysemu/sysemu.h"
+#include "sysemu/qtest.h"
+#include "sysemu/device_tree.h"
+#include "hw/boards.h"
+
+/* Same address of GDB integrated simulator */
+#define SDRAM_BASE  EXT_CS_BASE
+
+typedef struct RxGdbSimMachineClass {
+/*< private >*/
+MachineClass parent_class;
+/*< public >*/
+const char *mcu_name;
+uint32_t xtal_freq_hz;
+} RxGdbSimMachineClass;
+
+typedef struct RxGdbSimMachineState {
+/*< private >*/
+MachineState parent_obj;
+/*< public >*/
+RX62NState mcu;
+} RxGdbSimMachineState;
+
+#define TYPE_RX_GDBSIM_MACHINE MACHINE_TYPE_NAME("rx62n-common")
+
+#define RX_GDBSIM_MACHINE(obj) \
+OBJECT_CHECK(RxGdbSimMachineState, (obj), TYPE_RX_GDBSIM_MACHINE)
+
+#define RX_GDBSIM_MACHINE_CLASS(klass) \
+OBJECT_CLASS_CHECK(RxGdbSimMachineClass, (klass), TYPE_RX_GDBSIM_MACHINE)
+#define RX_GDBSIM_MACHINE_GET_CLASS(obj) \
+OBJECT_GET_CLASS(RxGdbSimMachineClass, (obj), TYPE_RX_GDBSIM_MACHINE)
+
+static void rx_load_image(RXCPU *cpu, const char *filename,
+  uint32_t start, uint32_t size)
+{
+static uint32_t extable[32];
+long kernel_size;
+int i;
+
+kernel_size = load_image_targphys(filename, start, size);
+if (kernel_size < 0) {
+fprintf(stderr, "qemu: could not load kernel '%s'\n", filename);
+exit(1);
+}
+cpu->env.pc = start;
+
+/* setup exception trap trampoline */
+/* linux kernel only works little-endian mode */
+for (i = 0; i < ARRAY_SIZE(extable); i++) {
+extable[i] = cpu_to_le32(0x10 + i * 4);
+}
+rom_add_blob_fixed("extable", extable, sizeof(extable), VECTOR_TABLE_BASE);
+}
+
+static void rx_gdbsim_init(MachineState *machine)
+{
+MachineClass *mc = MACHINE_GET_CLASS(machine);
+RxGdbSimMachineState *s = RX_GDBSIM_MACHINE(machine);
+RxGdbSimMachineClass *rxc = RX_GDBSIM_MACHINE_GET_CLASS(machine);
+MemoryRegion *sysmem = get_system_memory();
+const char *kernel_filename = machine->kernel_filename;
+const char *dtb_filename = machine->dtb;
+
+if (machine->ram_size < mc->default_ram_size) {
+char *sz = size_to_str(mc->default_ram_size);
+error_report("Invalid RAM size, should be more than %s", sz);
+g_free(sz);
+}
+
+/* Allocate memory