mbox series

[00/31] Add Loongarch softmmu support.

Message ID 1634628917-10031-1-git-send-email-yangxiaojuan@loongson.cn (mailing list archive)
Headers show
Series Add Loongarch softmmu support. | expand

Message

Xiaojuan Yang Oct. 19, 2021, 7:34 a.m. UTC
This series patch add softmmu support for LoongArch.
Base on the linux-user emulation support V7 patch.

The latest kernel:
  * https://github.com/loongson/linux/tree/loongarch-next
The manual:
  * https://github.com/loongson/LoongArch-Documentation/releases/tag/2021.10.11

Patch 1 Add a readme for code download and supply binary for test.
Patch 2-3 define CSR registers and set the init/reset value.
Patch 4-5 Add cpu related functions.
Patch 6-12 Add Privileged instruction simulation used by LoongArch.
Patch 13 Emulate a virt pci host based on the LoongArch 7A1000.
Patch 14 Emulate a virt 3a5000 board different from host.
Patch 15-20 Emulate the interrupt controller used by the virt board.
Patch 21-22 Add devices used by the board.
Patch 23-25 Add bios and smbios support needed for the start.
Patch 26 Add a simple acpi model.
Patch 27 Enable mttcg mode.
Patch 28-29 Support for numa more than 4 cpus.
Patch 30-31 Add some functions for debug.

Xiaojuan Yang (31):
  target/loongarch: Upate the README for the softmmu.
  target/loongarch: Add CSR registers definition
  target/loongarch: Set default csr values.
  target/loongarch: Add basic vmstate description of CPU.
  target/loongarch: Implement qmp_query_cpu_definitions()
  target/loongarch: Add mmu support for Loongarch CPU.
  target/loongarch: Add loongarch csr/iocsr instruction support
  target/loongarch: Add tlb instruction support
  target/loongarch: Add other core instructions support
  target/loongarch: Add loongarch interrupt and exception handle
  target/loongarch: Add stabletimer support
  target/loongarch: Add timer related instructions support.
  hw/pci-host: Add ls7a1000 PCIe Host bridge support for Loongson
    Platform
  hw/loongarch: Add a virt loongarch 3A5000 board support
  hw/loongarch: Add loongarch cpu interrupt support(CPUINTC)
  hw/loongarch: Add loongarch ipi interrupt support(IPI)
  hw/intc: Add loongarch ls7a interrupt controller support(PCH-PIC)
  hw/intc: Add loongarch ls7a msi interrupt controller support(PCH-MSI)
  hw/intc: Add loongarch extioi interrupt controller(EIOINTC)
  hw/loongarch: Add irq hierarchy for the system
  hw/loongarch: Add some devices support for 3A5000.
  hw/loongarch: Add loongarch ls7a rtc device support
  hw/loongarch: Add default bios startup support.
  hw/loongarch: Add -kernel and -initrd options support
  hw/loongarch: Add loongarch smbios support
  hw/loongarch: Add loongarch acpi support
  hw/loongarch: Add mttcg mode support
  hw/loongarch: use machine->possible_cpus for storing possible topology
    info
  hw/loongarch: Add numa support.
  target/loongarch: Add gdb support.
  target/loongarch: Add privilege instructions diassembly

 .../devices/loongarch64-softmmu/default.mak   |   3 +
 configs/targets/loongarch64-softmmu.mak       |   4 +
 disas/loongarch.c                             | 173 ++++
 gdb-xml/loongarch-base64.xml                  |  43 +
 gdb-xml/loongarch-fpu64.xml                   |  57 ++
 hw/Kconfig                                    |   1 +
 hw/acpi/Kconfig                               |   4 +
 hw/acpi/ls7a.c                                | 351 +++++++
 hw/acpi/meson.build                           |   1 +
 hw/intc/Kconfig                               |  12 +
 hw/intc/loongarch_extioi.c                    | 589 +++++++++++
 hw/intc/loongarch_pch_msi.c                   |  74 ++
 hw/intc/loongarch_pch_pic.c                   | 284 ++++++
 hw/intc/meson.build                           |   3 +
 hw/loongarch/Kconfig                          |  22 +
 hw/loongarch/acpi-build.c                     | 661 +++++++++++++
 hw/loongarch/fw_cfg.c                         |  34 +
 hw/loongarch/fw_cfg.h                         |  16 +
 hw/loongarch/ipi.c                            | 147 +++
 hw/loongarch/loongarch_int.c                  |  62 ++
 hw/loongarch/ls3a5000_virt.c                  | 638 ++++++++++++
 hw/loongarch/meson.build                      |   7 +
 hw/meson.build                                |   1 +
 hw/pci-host/Kconfig                           |   4 +
 hw/pci-host/ls7a.c                            | 224 +++++
 hw/pci-host/meson.build                       |   1 +
 hw/rtc/Kconfig                                |   3 +
 hw/rtc/ls7a_rtc.c                             | 327 +++++++
 hw/rtc/meson.build                            |   1 +
 include/exec/poison.h                         |   2 +
 include/hw/acpi/ls7a.h                        |  54 ++
 include/hw/intc/loongarch_extioi.h            | 102 ++
 include/hw/intc/loongarch_pch_msi.h           |  17 +
 include/hw/intc/loongarch_pch_pic.h           |  50 +
 include/hw/loongarch/gipi.h                   |  38 +
 include/hw/loongarch/loongarch.h              |  79 ++
 include/hw/pci-host/ls7a.h                    |  67 ++
 include/sysemu/arch_init.h                    |   1 +
 linux-user/loongarch64/cpu_loop.c             |   3 +-
 pc-bios/loongarch_bios.bin                    | Bin 0 -> 4128768 bytes
 qapi/machine-target.json                      |   6 +-
 qapi/machine.json                             |   2 +-
 softmmu/qdev-monitor.c                        |   3 +-
 target/Kconfig                                |   1 +
 target/loongarch/Kconfig                      |   2 +
 target/loongarch/README                       | 134 +++
 target/loongarch/cpu-csr.h                    | 493 ++++++++++
 target/loongarch/cpu.c                        | 448 ++++++++-
 target/loongarch/cpu.h                        |  56 +-
 target/loongarch/csr_helper.c                 | 765 +++++++++++++++
 target/loongarch/gdbstub.c                    |  95 ++
 target/loongarch/helper.h                     |  24 +
 target/loongarch/insn_trans/trans_core.c      | 302 ++++++
 target/loongarch/insn_trans/trans_extra.c     |   2 +
 target/loongarch/insns.decode                 |  45 +
 target/loongarch/internals.h                  |  93 ++
 target/loongarch/machine.c                    | 253 +++++
 target/loongarch/meson.build                  |  10 +
 target/loongarch/op_helper.c                  |  77 +-
 target/loongarch/ramdisk                      | Bin 0 -> 3077952 bytes
 target/loongarch/stabletimer.c                |  71 ++
 target/loongarch/tlb_helper.c                 | 918 ++++++++++++++++++
 target/loongarch/translate.c                  |   8 +-
 target/loongarch/vmlinux                      | Bin 0 -> 24565536 bytes
 64 files changed, 7955 insertions(+), 13 deletions(-)
 create mode 100644 configs/devices/loongarch64-softmmu/default.mak
 create mode 100644 configs/targets/loongarch64-softmmu.mak
 create mode 100644 gdb-xml/loongarch-base64.xml
 create mode 100644 gdb-xml/loongarch-fpu64.xml
 create mode 100644 hw/acpi/ls7a.c
 create mode 100644 hw/intc/loongarch_extioi.c
 create mode 100644 hw/intc/loongarch_pch_msi.c
 create mode 100644 hw/intc/loongarch_pch_pic.c
 create mode 100644 hw/loongarch/Kconfig
 create mode 100644 hw/loongarch/acpi-build.c
 create mode 100644 hw/loongarch/fw_cfg.c
 create mode 100644 hw/loongarch/fw_cfg.h
 create mode 100644 hw/loongarch/ipi.c
 create mode 100644 hw/loongarch/loongarch_int.c
 create mode 100644 hw/loongarch/ls3a5000_virt.c
 create mode 100644 hw/loongarch/meson.build
 create mode 100644 hw/pci-host/ls7a.c
 create mode 100644 hw/rtc/ls7a_rtc.c
 create mode 100644 include/hw/acpi/ls7a.h
 create mode 100644 include/hw/intc/loongarch_extioi.h
 create mode 100644 include/hw/intc/loongarch_pch_msi.h
 create mode 100644 include/hw/intc/loongarch_pch_pic.h
 create mode 100644 include/hw/loongarch/gipi.h
 create mode 100644 include/hw/loongarch/loongarch.h
 create mode 100644 include/hw/pci-host/ls7a.h
 create mode 100644 pc-bios/loongarch_bios.bin
 create mode 100644 target/loongarch/Kconfig
 create mode 100644 target/loongarch/cpu-csr.h
 create mode 100644 target/loongarch/csr_helper.c
 create mode 100644 target/loongarch/gdbstub.c
 create mode 100644 target/loongarch/insn_trans/trans_core.c
 create mode 100644 target/loongarch/machine.c
 create mode 100644 target/loongarch/ramdisk
 create mode 100644 target/loongarch/stabletimer.c
 create mode 100644 target/loongarch/tlb_helper.c
 create mode 100755 target/loongarch/vmlinux

Comments

WANG Xuerui Oct. 19, 2021, 2:52 p.m. UTC | #1
Hi Xiaojuan,

On 10/19/21 15:34, Xiaojuan Yang wrote:
> This series patch add softmmu support for LoongArch.
> Base on the linux-user emulation support V7 patch.
>
> The latest kernel:
>    * https://github.com/loongson/linux/tree/loongarch-next
> The manual:
>    * https://github.com/loongson/LoongArch-Documentation/releases/tag/2021.10.11
>
> Patch 1 Add a readme for code download and supply binary for test.
> Patch 2-3 define CSR registers and set the init/reset value.
> Patch 4-5 Add cpu related functions.
> Patch 6-12 Add Privileged instruction simulation used by LoongArch.
> Patch 13 Emulate a virt pci host based on the LoongArch 7A1000.
> Patch 14 Emulate a virt 3a5000 board different from host.
> Patch 15-20 Emulate the interrupt controller used by the virt board.
> Patch 21-22 Add devices used by the board.
> Patch 23-25 Add bios and smbios support needed for the start.
> Patch 26 Add a simple acpi model.
> Patch 27 Enable mttcg mode.
> Patch 28-29 Support for numa more than 4 cpus.
> Patch 30-31 Add some functions for debug.
>
> Xiaojuan Yang (31):
>    target/loongarch: Upate the README for the softmmu.

Nit: "Update", also remove the trailing period -- QEMU style doesn't 
mandate full sentences for title line of commit messages AFAIK. Other 
places should be fixed as well.

>    target/loongarch: Add CSR registers definition
>    target/loongarch: Set default csr values.
>    target/loongarch: Add basic vmstate description of CPU.
>    target/loongarch: Implement qmp_query_cpu_definitions()
>    target/loongarch: Add mmu support for Loongarch CPU.
>    target/loongarch: Add loongarch csr/iocsr instruction support
And please use consistent casing for "LoongArch", "CSR", "IOCSR", "MMU", 
"TLB" etc etc -- again fix everywhere you can.
>    target/loongarch: Add tlb instruction support
>    target/loongarch: Add other core instructions support
>    target/loongarch: Add loongarch interrupt and exception handle
>    target/loongarch: Add stabletimer support
>    target/loongarch: Add timer related instructions support.
>    hw/pci-host: Add ls7a1000 PCIe Host bridge support for Loongson
>      Platform
>    hw/loongarch: Add a virt loongarch 3A5000 board support
>    hw/loongarch: Add loongarch cpu interrupt support(CPUINTC)
>    hw/loongarch: Add loongarch ipi interrupt support(IPI)
>    hw/intc: Add loongarch ls7a interrupt controller support(PCH-PIC)
>    hw/intc: Add loongarch ls7a msi interrupt controller support(PCH-MSI)
>    hw/intc: Add loongarch extioi interrupt controller(EIOINTC)
>    hw/loongarch: Add irq hierarchy for the system
>    hw/loongarch: Add some devices support for 3A5000.
>    hw/loongarch: Add loongarch ls7a rtc device support
>    hw/loongarch: Add default bios startup support.
>    hw/loongarch: Add -kernel and -initrd options support
>    hw/loongarch: Add loongarch smbios support
>    hw/loongarch: Add loongarch acpi support
>    hw/loongarch: Add mttcg mode support
>    hw/loongarch: use machine->possible_cpus for storing possible topology
>      info
>    hw/loongarch: Add numa support.
>    target/loongarch: Add gdb support.
>    target/loongarch: Add privilege instructions diassembly
>
>   .../devices/loongarch64-softmmu/default.mak   |   3 +
>   configs/targets/loongarch64-softmmu.mak       |   4 +
>   disas/loongarch.c                             | 173 ++++
>   gdb-xml/loongarch-base64.xml                  |  43 +
>   gdb-xml/loongarch-fpu64.xml                   |  57 ++
>   hw/Kconfig                                    |   1 +
>   hw/acpi/Kconfig                               |   4 +
>   hw/acpi/ls7a.c                                | 351 +++++++
>   hw/acpi/meson.build                           |   1 +
>   hw/intc/Kconfig                               |  12 +
>   hw/intc/loongarch_extioi.c                    | 589 +++++++++++
>   hw/intc/loongarch_pch_msi.c                   |  74 ++
>   hw/intc/loongarch_pch_pic.c                   | 284 ++++++
>   hw/intc/meson.build                           |   3 +
>   hw/loongarch/Kconfig                          |  22 +
>   hw/loongarch/acpi-build.c                     | 661 +++++++++++++
>   hw/loongarch/fw_cfg.c                         |  34 +
>   hw/loongarch/fw_cfg.h                         |  16 +
>   hw/loongarch/ipi.c                            | 147 +++
>   hw/loongarch/loongarch_int.c                  |  62 ++
>   hw/loongarch/ls3a5000_virt.c                  | 638 ++++++++++++
>   hw/loongarch/meson.build                      |   7 +
>   hw/meson.build                                |   1 +
>   hw/pci-host/Kconfig                           |   4 +
>   hw/pci-host/ls7a.c                            | 224 +++++
>   hw/pci-host/meson.build                       |   1 +
>   hw/rtc/Kconfig                                |   3 +
>   hw/rtc/ls7a_rtc.c                             | 327 +++++++
>   hw/rtc/meson.build                            |   1 +
>   include/exec/poison.h                         |   2 +
>   include/hw/acpi/ls7a.h                        |  54 ++
>   include/hw/intc/loongarch_extioi.h            | 102 ++
>   include/hw/intc/loongarch_pch_msi.h           |  17 +
>   include/hw/intc/loongarch_pch_pic.h           |  50 +
>   include/hw/loongarch/gipi.h                   |  38 +
>   include/hw/loongarch/loongarch.h              |  79 ++
>   include/hw/pci-host/ls7a.h                    |  67 ++
>   include/sysemu/arch_init.h                    |   1 +
>   linux-user/loongarch64/cpu_loop.c             |   3 +-
>   pc-bios/loongarch_bios.bin                    | Bin 0 -> 4128768 bytes
>   qapi/machine-target.json                      |   6 +-
>   qapi/machine.json                             |   2 +-
>   softmmu/qdev-monitor.c                        |   3 +-
>   target/Kconfig                                |   1 +
>   target/loongarch/Kconfig                      |   2 +
>   target/loongarch/README                       | 134 +++
>   target/loongarch/cpu-csr.h                    | 493 ++++++++++
>   target/loongarch/cpu.c                        | 448 ++++++++-
>   target/loongarch/cpu.h                        |  56 +-
>   target/loongarch/csr_helper.c                 | 765 +++++++++++++++
>   target/loongarch/gdbstub.c                    |  95 ++
>   target/loongarch/helper.h                     |  24 +
>   target/loongarch/insn_trans/trans_core.c      | 302 ++++++
>   target/loongarch/insn_trans/trans_extra.c     |   2 +
>   target/loongarch/insns.decode                 |  45 +
>   target/loongarch/internals.h                  |  93 ++
>   target/loongarch/machine.c                    | 253 +++++
>   target/loongarch/meson.build                  |  10 +
>   target/loongarch/op_helper.c                  |  77 +-
>   target/loongarch/ramdisk                      | Bin 0 -> 3077952 bytes
>   target/loongarch/stabletimer.c                |  71 ++
>   target/loongarch/tlb_helper.c                 | 918 ++++++++++++++++++
>   target/loongarch/translate.c                  |   8 +-
>   target/loongarch/vmlinux                      | Bin 0 -> 24565536 bytes

Well, it might not be exactly nice to add ~30MiB of blob right here, via 
mailing list? IIUC everyone here can only see 17 of your 31 patches, 
maybe the blob-containing mail and subsequent ones all got blocked?

And perhaps it's not okay to put blobs inside target/ in the first 
place, a quick `tree` look at main branch reveals no such file. You may 
host these blobs (better yet, properly open-sourced repos) elsewhere and 
link to them in README instead.

>   64 files changed, 7955 insertions(+), 13 deletions(-)
>   create mode 100644 configs/devices/loongarch64-softmmu/default.mak
>   create mode 100644 configs/targets/loongarch64-softmmu.mak
>   create mode 100644 gdb-xml/loongarch-base64.xml
>   create mode 100644 gdb-xml/loongarch-fpu64.xml
>   create mode 100644 hw/acpi/ls7a.c
>   create mode 100644 hw/intc/loongarch_extioi.c
>   create mode 100644 hw/intc/loongarch_pch_msi.c
>   create mode 100644 hw/intc/loongarch_pch_pic.c
>   create mode 100644 hw/loongarch/Kconfig
>   create mode 100644 hw/loongarch/acpi-build.c
>   create mode 100644 hw/loongarch/fw_cfg.c
>   create mode 100644 hw/loongarch/fw_cfg.h
>   create mode 100644 hw/loongarch/ipi.c
>   create mode 100644 hw/loongarch/loongarch_int.c
>   create mode 100644 hw/loongarch/ls3a5000_virt.c
>   create mode 100644 hw/loongarch/meson.build
>   create mode 100644 hw/pci-host/ls7a.c
>   create mode 100644 hw/rtc/ls7a_rtc.c
While I haven't seen the entirety of the series, these files got created 
but MAINTAINERS is not updated. Please update the corresponding entries.
>   create mode 100644 include/hw/acpi/ls7a.h
>   create mode 100644 include/hw/intc/loongarch_extioi.h
>   create mode 100644 include/hw/intc/loongarch_pch_msi.h
>   create mode 100644 include/hw/intc/loongarch_pch_pic.h
>   create mode 100644 include/hw/loongarch/gipi.h
>   create mode 100644 include/hw/loongarch/loongarch.h
>   create mode 100644 include/hw/pci-host/ls7a.h
>   create mode 100644 pc-bios/loongarch_bios.bin
>   create mode 100644 target/loongarch/Kconfig
>   create mode 100644 target/loongarch/cpu-csr.h
>   create mode 100644 target/loongarch/csr_helper.c
>   create mode 100644 target/loongarch/gdbstub.c
>   create mode 100644 target/loongarch/insn_trans/trans_core.c
>   create mode 100644 target/loongarch/machine.c
>   create mode 100644 target/loongarch/ramdisk
>   create mode 100644 target/loongarch/stabletimer.c
>   create mode 100644 target/loongarch/tlb_helper.c
>   create mode 100755 target/loongarch/vmlinux
>
Michael S. Tsirkin Oct. 19, 2021, 4:19 p.m. UTC | #2
On Tue, Oct 19, 2021 at 03:35:09PM +0800, Xiaojuan Yang wrote:
> This patch add default bios startup.
> The bios source code will be opened int the near future.

That pretty much rules out including this patchset for now ...
Does not mean people won't review, but please tag the subject RFC
until it's all open source and can be merged.
And I would really appreciate not getting CC'd until it is.

Thanks!

> Signed-off-by: Xiaojuan Yang <yangxiaojuan@loongson.cn>
> Signed-off-by: Song Gao <gaosong@loongson.cn>
> ---
>  hw/loongarch/Kconfig             |   4 +++
>  hw/loongarch/fw_cfg.c            |  34 ++++++++++++++++++
>  hw/loongarch/fw_cfg.h            |  16 +++++++++
>  hw/loongarch/ls3a5000_virt.c     |  60 ++++++++++++++++++++++++-------
>  hw/loongarch/meson.build         |   1 +
>  include/hw/loongarch/loongarch.h |   5 +++
>  pc-bios/loongarch_bios.bin       | Bin 0 -> 4128768 bytes
>  7 files changed, 108 insertions(+), 12 deletions(-)
>  create mode 100644 hw/loongarch/fw_cfg.c
>  create mode 100644 hw/loongarch/fw_cfg.h
>  create mode 100644 pc-bios/loongarch_bios.bin
> 
> diff --git a/hw/loongarch/Kconfig b/hw/loongarch/Kconfig
> index fe100b01eb..b59cd98a7a 100644
> --- a/hw/loongarch/Kconfig
> +++ b/hw/loongarch/Kconfig
> @@ -13,3 +13,7 @@ config LOONGSON_3A5000
>      select LOONGARCH_PCH_MSI
>      select LOONGARCH_EXTIOI
>      select LS7A_RTC
> +    select FW_CFG_LOONGARCH
> +
> +config FW_CFG_LOONGARCH
> +    bool
> diff --git a/hw/loongarch/fw_cfg.c b/hw/loongarch/fw_cfg.c
> new file mode 100644
> index 0000000000..42229c5c91
> --- /dev/null
> +++ b/hw/loongarch/fw_cfg.c
> @@ -0,0 +1,34 @@
> +/*
> + * QEMU fw_cfg helpers (LOONGARCH specific)
> + *
> + * Copyright (C) 2021 Loongson Technology Corporation Limited
> + *
> + * SPDX-License-Identifier: GPL-2.0-or-later
> + */
> +
> +#include "qemu/osdep.h"
> +#include "hw/loongarch/fw_cfg.h"
> +#include "hw/loongarch/loongarch.h"
> +#include "hw/nvram/fw_cfg.h"
> +#include "sysemu/sysemu.h"
> +
> +static void fw_cfg_boot_set(void *opaque, const char *boot_device,
> +                            Error **errp)
> +{
> +    fw_cfg_modify_i16(opaque, FW_CFG_BOOT_DEVICE, boot_device[0]);
> +}
> +
> +FWCfgState *loongarch_fw_cfg_init(ram_addr_t ram_size, MachineState *ms)
> +{
> +    FWCfgState *fw_cfg;
> +    int max_cpus = ms->smp.max_cpus;
> +    int smp_cpus = ms->smp.cpus;
> +
> +    fw_cfg = fw_cfg_init_mem_wide(FW_CFG_ADDR, FW_CFG_ADDR + 8, 8, 0, NULL);
> +    fw_cfg_add_i16(fw_cfg, FW_CFG_MAX_CPUS, (uint16_t)max_cpus);
> +    fw_cfg_add_i64(fw_cfg, FW_CFG_RAM_SIZE, (uint64_t)ram_size);
> +    fw_cfg_add_i16(fw_cfg, FW_CFG_NB_CPUS, (uint16_t)smp_cpus);
> +
> +    qemu_register_boot_set(fw_cfg_boot_set, fw_cfg);
> +    return fw_cfg;
> +}
> diff --git a/hw/loongarch/fw_cfg.h b/hw/loongarch/fw_cfg.h
> new file mode 100644
> index 0000000000..d51dfb241a
> --- /dev/null
> +++ b/hw/loongarch/fw_cfg.h
> @@ -0,0 +1,16 @@
> +/*
> + * QEMU fw_cfg helpers (Loongarch specific)
> + *
> + * Copyright (C) 2021 Loongson Technology Corporation Limited
> + *
> + * SPDX-License-Identifier: MIT
> + */
> +
> +#ifndef HW_LOONGARCH_FW_CFG_H
> +#define HW_LOONGARCH_FW_CFG_H
> +
> +#include "hw/boards.h"
> +#include "hw/nvram/fw_cfg.h"
> +
> +FWCfgState *loongarch_fw_cfg_init(ram_addr_t ram_size, MachineState *ms);
> +#endif
> diff --git a/hw/loongarch/ls3a5000_virt.c b/hw/loongarch/ls3a5000_virt.c
> index 79e6605cd8..e8057f7772 100644
> --- a/hw/loongarch/ls3a5000_virt.c
> +++ b/hw/loongarch/ls3a5000_virt.c
> @@ -14,6 +14,8 @@
>  #include "hw/char/serial.h"
>  #include "sysemu/sysemu.h"
>  #include "sysemu/qtest.h"
> +#include "hw/loader.h"
> +#include "elf.h"
>  #include "hw/irq.h"
>  #include "net/net.h"
>  #include "sysemu/runstate.h"
> @@ -24,6 +26,9 @@
>  #include "hw/intc/loongarch_pch_msi.h"
>  #include "hw/pci-host/ls7a.h"
>  #include "hw/misc/unimp.h"
> +#include "hw/loongarch/fw_cfg.h"
> +
> +#define LOONGSON3_BIOSNAME "loongarch_bios.bin"
>  
>  CPULoongArchState *cpu_states[LOONGARCH_MAX_VCPUS];
>  
> @@ -181,8 +186,9 @@ static void ls3a5000_virt_init(MachineState *machine)
>      const char *cpu_model = machine->cpu_type;
>      LoongArchCPU *cpu;
>      CPULoongArchState *env;
> -    uint64_t lowram_size = 0, highram_size = 0;
> +    uint64_t highram_size = 0;
>      MemoryRegion *lowmem = g_new(MemoryRegion, 1);
> +    MemoryRegion *highmem = g_new(MemoryRegion, 1);
>      char *ramName = NULL;
>      ram_addr_t ram_size = machine->ram_size;
>      MemoryRegion *address_space_mem = get_system_memory();
> @@ -190,6 +196,10 @@ static void ls3a5000_virt_init(MachineState *machine)
>      int i;
>      MemoryRegion *iomem = NULL;
>      PCIBus *pci_bus = NULL;
> +    int bios_size;
> +    char *filename;
> +    MemoryRegion *bios = g_new(MemoryRegion, 1);
> +    ram_addr_t offset = 0;
>  
>      if (!cpu_model) {
>          cpu_model = LOONGARCH_CPU_TYPE_NAME("Loongson-3A5000");
> @@ -227,21 +237,46 @@ static void ls3a5000_virt_init(MachineState *machine)
>          qemu_register_reset(main_cpu_reset, cpu);
>      }
>  
> +    if (ram_size < 1 * GiB) {
> +        error_report("ram_size must be greater than 1G due to the bios memory layout");
> +        exit(1);
> +    }
> +
>      ramName = g_strdup_printf("loongarch.lowram");
> -    lowram_size = MIN(ram_size, 256 * 0x100000);
>      memory_region_init_alias(lowmem, NULL, ramName, machine->ram,
> -                             0, lowram_size);
> -    memory_region_add_subregion(address_space_mem, 0, lowmem);
> -
> -    highram_size = ram_size > lowram_size ? ram_size - 256 * 0x100000 : 0;
> -    if (highram_size > 0) {
> -        MemoryRegion *highmem = g_new(MemoryRegion, 1);
> -        ramName = g_strdup_printf("loongarch.highram");
> -        memory_region_init_alias(highmem, NULL, ramName, machine->ram,
> -                                 lowram_size, highram_size);
> -        memory_region_add_subregion(address_space_mem, 0x90000000, highmem);
> +                             0, 256 * MiB);
> +    memory_region_add_subregion(address_space_mem, offset, lowmem);
> +    offset += 256 * MiB;
> +
> +    highram_size = ram_size - 256 * MiB;
> +    ramName = g_strdup_printf("loongarch.highram");
> +    memory_region_init_alias(highmem, NULL, ramName, machine->ram,
> +                             offset, highram_size);
> +    memory_region_add_subregion(address_space_mem, 0x90000000, highmem);
> +    offset += highram_size;
> +
> +    /* load the BIOS image. */
> +    filename = qemu_find_file(QEMU_FILE_TYPE_BIOS,
> +                              machine->firmware ?: LOONGSON3_BIOSNAME);
> +    if (filename) {
> +        bios_size = load_image_targphys(filename, LA_BIOS_BASE, LA_BIOS_SIZE);
> +        lsms->fw_cfg = loongarch_fw_cfg_init(ram_size, machine);
> +        rom_set_fw(lsms->fw_cfg);
> +        g_free(filename);
> +    } else {
> +        bios_size = -1;
>      }
>  
> +    if ((bios_size < 0 || bios_size > LA_BIOS_SIZE) && !qtest_enabled()) {
> +        error_report("Could not load LOONGARCH bios '%s'", machine->firmware);
> +        exit(1);
> +    }
> +
> +    memory_region_init_ram(bios, NULL, "loongarch.bios",
> +                           LA_BIOS_SIZE, &error_fatal);
> +    memory_region_set_readonly(bios, true);
> +    memory_region_add_subregion(get_system_memory(), LA_BIOS_BASE, bios);
> +
>      /*Add PM mmio memory for reboot and shutdown*/
>      iomem = g_new(MemoryRegion, 1);
>      memory_region_init_io(iomem, NULL, &loongarch_pm_ops, NULL,
> @@ -293,6 +328,7 @@ static void loongarch_class_init(ObjectClass *oc, void *data)
>      mc->default_ram_id = "loongarch.ram";
>      mc->max_cpus = LOONGARCH_MAX_VCPUS;
>      mc->is_default = 1;
> +    mc->default_machine_opts = "firmware=loongarch_bios.bin";
>      mc->default_kernel_irqchip_split = false;
>      mc->block_default_type = IF_VIRTIO;
>      mc->default_boot_order = "c";
> diff --git a/hw/loongarch/meson.build b/hw/loongarch/meson.build
> index 1bd209c9eb..3fabfa72dc 100644
> --- a/hw/loongarch/meson.build
> +++ b/hw/loongarch/meson.build
> @@ -1,5 +1,6 @@
>  loongarch_ss = ss.source_set()
>  loongarch_ss.add(files('loongarch_int.c'))
>  loongarch_ss.add(when: 'CONFIG_LOONGSON_3A5000', if_true: files('ls3a5000_virt.c', 'ipi.c'))
> +loongarch_ss.add(when: 'CONFIG_FW_CFG_LOONGARCH', if_true: files('fw_cfg.c'))
>  
>  hw_arch += {'loongarch': loongarch_ss}
> diff --git a/include/hw/loongarch/loongarch.h b/include/hw/loongarch/loongarch.h
> index 210173471d..1316707f49 100644
> --- a/include/hw/loongarch/loongarch.h
> +++ b/include/hw/loongarch/loongarch.h
> @@ -35,6 +35,10 @@
>  #define CPUNAME_REG             0x1fe00020
>  #define MISC_FUNC_REG           0x1fe00420
>  #define FREQ_REG                0x1fe001d0
> +#define FW_CFG_ADDR             0x1e020000
> +
> +#define LA_BIOS_BASE            0x1c000000
> +#define LA_BIOS_SIZE            (4 * 1024 * 1024)
>  
>  typedef struct LoongarchMachineState {
>      /*< private >*/
> @@ -42,6 +46,7 @@ typedef struct LoongarchMachineState {
>  
>      gipiState   *gipi;
>      qemu_irq    *pch_irq;
> +    FWCfgState  *fw_cfg;
>  } LoongarchMachineState;
>  
>  #define TYPE_LOONGARCH_MACHINE  MACHINE_TYPE_NAME("loongson7a")

> new file mode 100644
> index 0000000000000000000000000000000000000000..ac6a691b00ac4fec9a5f4c32aad3865ac24efbb2
> GIT binary patch
> literal 4128768
> zcmeFaeSB5bmH)qTZUU$YBpL)1^`ZgKNkOsI7FumjAQU@;6`PrcY8-A%fLMccI<`Eh
> z^#F-&r#C#PE#MOuB~;r4@mXsfP6C3Rl7QoPs`iPig0{bg2ggo>59EA5YoFYk+-ssv
> 


snipped a ton of binary

> literal 0
> HcmV?d00001
> 
> -- 
> 2.27.0
Richard Henderson Oct. 19, 2021, 6:56 p.m. UTC | #3
On 10/19/21 12:34 AM, Xiaojuan Yang wrote:
> ---
>   target/loongarch/README  | 134 +++++++++++++++++++++++++++++++++++++++
>   target/loongarch/ramdisk | Bin 0 -> 3077952 bytes
>   target/loongarch/vmlinux | Bin 0 -> 24565536 bytes
>   3 files changed, 134 insertions(+)
>   create mode 100644 target/loongarch/ramdisk
>   create mode 100755 target/loongarch/vmlinux

Wang Xuerui has already mentioned this, but let's make sure not to include such large test 
binaries in future.

The proper place for this is in a stable url, which could then either be referenced in the 
documentation.  But even better than that would be an acceptance test entry -- see 
tests/acceptance/boot_linux_console.py.

> +  2.Modify the kernel code for the tcg. Modifications are listed later. I will try to
> +    commit to the kernel host.

This sounds like a bug in the qemu emulation of the device or boot environment.


r~
Richard Henderson Oct. 19, 2021, 9:11 p.m. UTC | #4
On 10/19/21 12:34 AM, Xiaojuan Yang wrote:
> @@ -272,6 +288,7 @@ static const struct SysemuCPUOps loongarch_sysemu_ops = {
>   #ifdef CONFIG_TCG
>   #include "hw/core/tcg-cpu-ops.h"
>   
> +#ifdef CONFIG_USER_ONLY
>   static bool loongarch_cpu_tlb_fill(CPUState *cs, vaddr address, int size,
>                          MMUAccessType access_type, int mmu_idx,
>                          bool probe, uintptr_t retaddr)
> @@ -280,9 +297,14 @@ static bool loongarch_cpu_tlb_fill(CPUState *cs, vaddr address, int size,
>       CPULoongArchState *env = &cpu->env;
>   
>       env->badaddr = address;
> -    cs->exception_index = EXCP_ADE;
> +    if (access_type == MMU_DATA_STORE) {
> +        cs->exception_index = EXCP_ADES;
> +    } else {
> +        cs->exception_index = EXCP_ADEL;
> +    }
>       do_raise_exception(env, cs->exception_index, retaddr);
>   }
> +#endif

It's too early to add this ifdef.  With what's upstream at the moment, you've broken 
loongarch-linux-user build by removing loongarch_cpu_tlb_fill.

There are patches out for review that would require tlb_fill be a system-only hook, but 
they have not landed yet.

> +#define LOONGARCH_HFLAG_KU     0x00003 /* kernel/user mode mask   */
> +#define LOONGARCH_HFLAG_UM     0x00003 /* user mode flag                     */
> +#define LOONGARCH_HFLAG_KM     0x00000 /* kernel mode flag                   */

I think you might as well represent all 3 priv levels: it's not a "kernel/user" mask.

> +#define EXCP_TLB_NOMATCH   0x1
> +#define EXCP_INST_NOTAVAIL 0x2 /* No valid instruction word for BadInstr */

These should be with the other EXCP values in the enum.
At the moment you're overlapping EXCP_ADES and EXCP_SYSCALL.

> @@ -130,7 +139,11 @@ void loongarch_cpu_list(void);
>   
>   static inline int cpu_mmu_index(CPULoongArchState *env, bool ifetch)
>   {
> +#ifdef CONFIG_USER_ONLY
>       return MMU_USER_IDX;
> +#else
> +    return env->CSR_CRMD & LOONGARCH_HFLAG_KU;

Better would be

     return FIELD_EX64(env->CSR_CRMD, CSR_CRMD, PLV);

since that's the field you're extracting from CRMD.

> +typedef struct ls3a5k_tlb_t ls3a5k_tlb_t;

Types should be in CamelCase, without _t suffix.

> +struct ls3a5k_tlb_t {
> +    target_ulong VPN;
> +    uint64_t PageMask;
> +    uint32_t PageSize;
> +    uint16_t ASID;
> +    unsigned int V0:1;     /* CSR_TLBLO[0] */
> +    unsigned int V1:1;
> +
> +    unsigned int D0:1;     /* CSR_TLBLO[1] */
> +    unsigned int D1:1;
> +
> +    unsigned int PLV0:2;   /* CSR_TLBLO[3:2] */
> +    unsigned int PLV1:2;
> +
> +    unsigned int MAT0:3;   /* CSR_TLBLO[5:4] */
> +    unsigned int MAT1:3;
> +
> +    unsigned int G:1;      /* CSR_TLBLO[6] */
> +
> +    uint64_t PPN0;         /* CSR_TLBLO[47:12] */
> +    uint64_t PPN1;
> +
> +    unsigned int NR0:1;    /* CSR_TLBLO[61] */
> +    unsigned int NR1:1;
> +
> +    unsigned int NX0:1;    /* CSR_TLBLO[62] */
> +    unsigned int NX1:1;
> +
> +    unsigned int NE:1;     /* CSR_TLBIDX[31] */
> +
> +    unsigned int RPLV0:1;
> +    unsigned int RPLV1:1;  /* CSR_TLBLO[63] */
> +};

It would be much better if you didn't use bitfields at all.  This was a bad idea when mips 
did it; let us not compound the error.

Just use the format defined by the architecture for the CSRs: a couple of uint64_t.  Use 
FIELD definitions to give the parts intelligible names.

> +typedef struct ls3a5k_tlb_t ls3a5k_tlb_t;
> +
> +struct CPULoongArchTLBContext {
> +    uint32_t nb_tlb;
> +    int (*map_address)(struct CPULoongArchState *env, hwaddr *physical,
> +                       int *prot, target_ulong address,
> +                       MMUAccessType access_type);
> +    struct {
> +        uint64_t     stlb_mask;
> +        uint32_t     stlb_size; /* at most : 8 * 256 = 2048 */
> +        uint32_t     mtlb_size; /* at most : 64 */
> +        ls3a5k_tlb_t tlb[LOONGARCH_TLB_MAX];
> +    } ls3a5k;
> +};

There's probably no point in using an indirect function call until you've got more than 
one mmu implementation.  You're copying too much from mips.

> +/* TLB state */
> +static int get_tlb(QEMUFile *f, void *pv, size_t size,
> +                   const VMStateField *field)
> +{
> +    ls3a5k_tlb_t *v = pv;
> +    uint32_t flags;
> +
> +    qemu_get_betls(f, &v->VPN);
> +    qemu_get_be64s(f, &v->PageMask);
> +    qemu_get_be32s(f, &v->PageSize);
> +    qemu_get_be16s(f, &v->ASID);
> +    qemu_get_be32s(f, &flags);
> +    v->RPLV1 = (flags >> 21) & 1;
> +    v->RPLV0 = (flags >> 20) & 1;
> +    v->PLV1 = (flags >> 18) & 3;
> +    v->PLV0 = (flags >> 16) & 3;
> +    v->NE = (flags >> 15) & 1;
> +    v->NR1 = (flags >> 14) & 1;
> +    v->NR0 = (flags >> 13) & 1;
> +    v->NX1 = (flags >> 12) & 1;
> +    v->NX0 = (flags >> 11) & 1;
> +    v->D1 = (flags >> 10) & 1;
> +    v->D0 = (flags >> 9) & 1;
> +    v->V1 = (flags >> 8) & 1;
> +    v->V0 = (flags >> 7) & 1;
> +    v->MAT1 = (flags >> 4) & 7;
> +    v->MAT0 = (flags >> 1) & 7;
> +    v->G = (flags >> 0) & 1;
> +    qemu_get_be64s(f, &v->PPN0);
> +    qemu_get_be64s(f, &v->PPN1);

Some of the ugly things that go away if you don't use bitfields.

> +const VMStateDescription vmstate_tlb = {
> +    .name = "cpu/tlb",
> +    .version_id = 2,
> +    .minimum_version_id = 2,

Too much copying again: version numbers do not start at 2.

> +void ls3a5k_mmu_init(CPULoongArchState *env)
> +{
> +    env->tlb = g_malloc0(sizeof(CPULoongArchTLBContext));

I think you should not make this a separate structure, and instead allocate this with 
CPULoongArchState.

> diff --git a/target/loongarch/translate.c b/target/loongarch/translate.c
> index bea290df66..0be29994f9 100644
> --- a/target/loongarch/translate.c
> +++ b/target/loongarch/translate.c
> @@ -61,9 +61,10 @@ static void loongarch_tr_init_disas_context(DisasContextBase *dcbase,
>   {
>       int64_t bound;
>       DisasContext *ctx = container_of(dcbase, DisasContext, base);
> +    CPULoongArchState *env = cs->env_ptr;
>   
>       ctx->page_start = ctx->base.pc_first & TARGET_PAGE_MASK;
> -    ctx->mem_idx = MMU_USER_IDX;
> +    ctx->mem_idx = cpu_mmu_index(env, false);

This is incorrect.  You want

     tb_flags = ctx->base.tb->flags;
     mem_idx = tb_flags & LOONGARCH_HFLAG_PRIV.

It is almost always incorrect to dereference env at this point.  Everything should have 
been encoded into tb_flags so that when we do the hashing of the TranslationBlocks we find 
the one that has been compiled for the correct privilege level, etc.


r~
Richard Henderson Oct. 20, 2021, 1:36 a.m. UTC | #5
On 10/19/21 12:34 AM, Xiaojuan Yang wrote:
> +target_ulong helper_csr_rdq(CPULoongArchState *env, uint64_t csr)
> +{
> +    int64_t v;
> +
> +#define CASE_CSR_RDQ(csr)            \
> +    case LOONGARCH_CSR_ ## csr:      \
> +    {                                \
> +        v = env->CSR_ ## csr;        \
> +        break;                       \
> +    };                               \

There's absolutely no reason to call a helper function for a simple load.

> +    case LOONGARCH_CSR_PGD:
> +
> +        if (env->CSR_TLBRERA & 0x1) {
> +            v = env->CSR_TLBRBADV;
> +        } else {
> +            v = env->CSR_BADV;
> +        }
> +
> +        if ((v >> 63) & 0x1) {
> +            v = env->CSR_PGDH;
> +        } else {
> +            v = env->CSR_PGDL;
> +        }
> +        break;

This is the only one that requires a helper on read.

> +    if (csr == LOONGARCH_CSR_ASID) {
> +        if (old_v != val) {
> +            tlb_flush(env_cpu(env));
> +        }
> +    }

And this is the only one that requires a helper on write.

> +    case LOONGARCH_CSR_ESTAT:
> +        qatomic_and(&env->CSR_ESTAT, ~mask);

Why do you believe this requires an atomic update?
What is going to race with the update to a cpu private value?

> +static bool trans_csrrd(DisasContext *ctx, unsigned rd, unsigned csr)
> +{
> +    TCGv dest = gpr_dst(ctx, rd, EXT_NONE);
> +    gen_helper_csr_rdq(dest, cpu_env, tcg_constant_i64(csr));
> +    return true;
> +}
> +
> +static bool trans_csrwr(DisasContext *ctx, unsigned rd, unsigned csr)
> +{
> +    TCGv dest = gpr_dst(ctx, rd, EXT_NONE);
> +    TCGv src1 = gpr_src(ctx, rd, EXT_NONE);
> +
> +    switch (csr) {
> +    case LOONGARCH_CSR_CRMD:
> +        tcg_gen_movi_tl(cpu_pc, ctx->base.pc_next);
> +        gen_helper_csr_wrq(dest, cpu_env, src1, tcg_constant_i64(LOONGARCH_CSR_CRMD));
> +        tcg_gen_movi_tl(cpu_pc, ctx->base.pc_next + 4);
> +        ctx->base.is_jmp = DISAS_EXIT;
> +        break;
> +    case LOONGARCH_CSR_EUEN:
> +        gen_helper_csr_wrq(dest, cpu_env, src1, tcg_constant_i64(LOONGARCH_CSR_EUEN));
> +        /* Stop translation */
> +        tcg_gen_movi_tl(cpu_pc, ctx->base.pc_next + 4);
> +        ctx->base.is_jmp = DISAS_EXIT;
> +        break;
> +    default:
> +        gen_helper_csr_wrq(dest, cpu_env, src1, tcg_constant_i64(csr));
> +        break;
> +    }
> +    return true;
> +}
> +
> +static bool trans_csrxchg(DisasContext *ctx, arg_csrxchg *a)
> +{
> +    TCGv dest = gpr_dst(ctx, a->rd, EXT_NONE);
> +    TCGv src1 = gpr_src(ctx, a->rd, EXT_NONE);
> +    TCGv src2 = gpr_src(ctx, a->rj, EXT_NONE);
> +
> +    if (a->rj == 0) {
> +        return trans_csrrd(ctx, a->rd, a->csr);
> +    } else if (a->rj == 1) {
> +        return trans_csrwr(ctx, a->rd, a->csr);
> +    }

These should have been decoded separately; see below.

You're missing the check for priv 0 here and in all other functions.

> +
> +    if (a->rd == 0) {
> +        gen_helper_csr_xchgq_r0(cpu_env, src2, tcg_constant_i64(a->csr));
> +    } else {
> +        gen_helper_csr_xchgq(dest, cpu_env, src1, src2, tcg_constant_i64(a->csr));
> +    }

Why do you believe r0 to require a special case?

> +static bool trans_iocsrrd_b(DisasContext *ctx, arg_iocsrrd_b *a)
> +{
> +    TCGv tmp = tcg_temp_new();
> +    TCGv dest = gpr_dst(ctx, a->rd, EXT_NONE);
> +    TCGv src1 = gpr_src(ctx, a->rj, EXT_NONE);
> +
> +    gen_helper_iocsr_read(tmp, cpu_env, src1);
> +    tcg_gen_qemu_ld_tl(dest, tmp, ctx->mem_idx, MO_SB);

This is wrong.  From the manual:

   All IOCSR registers use independent addressing space

therefore you cannot simply read from a modified address, you must use a completely 
different address space.

There are a couple of different solutions that are possible.

(1) Use helper functions calling address_space_ld/st*.

(2) Use a separate mmu_idx, which uses its own address space.
     This requires bouncing through MemTxAttrs, since
     cpu_asidx_from_attrs only take attrs and not mmu_idx.

The second one is may be overkill, unless there will be any cachable memory in iospace.  I 
would expect most of it to be device memory.

> +csrxchg          0000 0100 .............. ..... .....     @fmt_rdrjcsr

{
   csrrd             0000 0100 .............. 00000 .....     @fmt_rdcsr
   csrwr             0000 0100 .............. 00001 .....     @fmt_rdcsr
   csrxchg           0000 0100 .............. ..... .....     @fmt_rdrjcsr
}


r~
WANG Xuerui Oct. 20, 2021, 5:11 a.m. UTC | #6
Hi Xiaojuan,


On 2021/10/20 09:33, 杨小娟 wrote:
> Hi, Xuerui
>     Thank you for your advice, I‘ll modify the README and put the binary in the github.
>     The remaining patches are send backed, I’ll find the cause and send them again.

Okay, understood; thanks for the quick response.

You may address the several review comments then just send v2. This way
the threading is less likely to be damaged (you need to exactly specify
In-Reply-To headers and such for the re-sent patches to correctly link
to this thread, I think it's not worth the effort).

> </yangxiaojuan@loongson.cn></i.qemu@xen0n.name>
>
> 本邮件及其附件含有龙芯中科的商业秘密信息,仅限于发送给上面地址中列出的个人或群组。禁止任何其他人以任何形式使用(包括但不限于全部或部分地泄露、复制或散发)本邮件及其附件中的信息。如果您错收本邮件,请您立即电话或邮件通知发件人并删除本邮件。 
> This email and its attachments contain confidential information from Loongson Technology , which is intended only for the person or entity whose address is listed above. Any use of the information contained herein in any way (including, but not limited to, total or partial disclosure, reproduction or dissemination) by persons other than the intended recipient(s) is prohibited. If you receive this email in error, please notify the sender by phone or email immediately and delete it. 
BTW really, ask your IT department to fix this nonsense. Multiple
upstreams using mailing lists as primary means of communication
(binutils etc) find problems with the mangling of contents, added
nonsense HTML-like tags and this footer not applicable to public
communication.
Xiaojuan Yang Oct. 22, 2021, 2:25 a.m. UTC | #7
在 2021年10月20日 02:56, Richard Henderson 写道:
> On 10/19/21 12:34 AM, Xiaojuan Yang wrote:
>> ---
>>   target/loongarch/README  | 134 +++++++++++++++++++++++++++++++++++++++
>>   target/loongarch/ramdisk | Bin 0 -> 3077952 bytes
>>   target/loongarch/vmlinux | Bin 0 -> 24565536 bytes
>>   3 files changed, 134 insertions(+)
>>   create mode 100644 target/loongarch/ramdisk
>>   create mode 100755 target/loongarch/vmlinux
> 
> Wang Xuerui has already mentioned this, but let's make sure not to include such large test binaries in future.
Yes, I will put the test binaries on the github.
> 
> The proper place for this is in a stable url, which could then either be referenced in the documentation.  But even better than that would be an acceptance test entry -- see tests/acceptance/boot_linux_console.py.
> 
>> +  2.Modify the kernel code for the tcg. Modifications are listed later. I will try to
>> +    commit to the kernel host.
> 
> This sounds like a bug in the qemu emulation of the device or boot environment.
>
Yes, qemu needs some adjustments, in the next version we can use the kernel directly. Thanks
> 
> r~
Xiaojuan Yang Oct. 29, 2021, 6:26 a.m. UTC | #8
Hi, Richard:

On 10/20/2021 09:36 AM, Richard Henderson wrote:
> On 10/19/21 12:34 AM, Xiaojuan Yang wrote:
>> +target_ulong helper_csr_rdq(CPULoongArchState *env, uint64_t csr)
>> +{
>> +    int64_t v;
>> +
>> +#define CASE_CSR_RDQ(csr)            \
>> +    case LOONGARCH_CSR_ ## csr:      \
>> +    {                                \
>> +        v = env->CSR_ ## csr;        \
>> +        break;                       \
>> +    };                               \
> 
> There's absolutely no reason to call a helper function for a simple load.
> 
>> +    case LOONGARCH_CSR_PGD:
>> +
>> +        if (env->CSR_TLBRERA & 0x1) {
>> +            v = env->CSR_TLBRBADV;
>> +        } else {
>> +            v = env->CSR_BADV;
>> +        }
>> +
>> +        if ((v >> 63) & 0x1) {
>> +            v = env->CSR_PGDH;
>> +        } else {
>> +            v = env->CSR_PGDL;
>> +        }
>> +        break;
> 
> This is the only one that requires a helper on read.
> 
>> +    if (csr == LOONGARCH_CSR_ASID) {
>> +        if (old_v != val) {
>> +            tlb_flush(env_cpu(env));
>> +        }
>> +    }
> 
> And this is the only one that requires a helper on write.
> 
>> +    case LOONGARCH_CSR_ESTAT:
>> +        qatomic_and(&env->CSR_ESTAT, ~mask);
> 
> Why do you believe this requires an atomic update?
> What is going to race with the update to a cpu private value?
> 
>> +static bool trans_csrrd(DisasContext *ctx, unsigned rd, unsigned csr)
>> +{
>> +    TCGv dest = gpr_dst(ctx, rd, EXT_NONE);
>> +    gen_helper_csr_rdq(dest, cpu_env, tcg_constant_i64(csr));
>> +    return true;
>> +}
>> +
>> +static bool trans_csrwr(DisasContext *ctx, unsigned rd, unsigned csr)
>> +{
>> +    TCGv dest = gpr_dst(ctx, rd, EXT_NONE);
>> +    TCGv src1 = gpr_src(ctx, rd, EXT_NONE);
>> +
>> +    switch (csr) {
>> +    case LOONGARCH_CSR_CRMD:
>> +        tcg_gen_movi_tl(cpu_pc, ctx->base.pc_next);
>> +        gen_helper_csr_wrq(dest, cpu_env, src1, tcg_constant_i64(LOONGARCH_CSR_CRMD));
>> +        tcg_gen_movi_tl(cpu_pc, ctx->base.pc_next + 4);
>> +        ctx->base.is_jmp = DISAS_EXIT;
>> +        break;
>> +    case LOONGARCH_CSR_EUEN:
>> +        gen_helper_csr_wrq(dest, cpu_env, src1, tcg_constant_i64(LOONGARCH_CSR_EUEN));
>> +        /* Stop translation */
>> +        tcg_gen_movi_tl(cpu_pc, ctx->base.pc_next + 4);
>> +        ctx->base.is_jmp = DISAS_EXIT;
>> +        break;
>> +    default:
>> +        gen_helper_csr_wrq(dest, cpu_env, src1, tcg_constant_i64(csr));
>> +        break;
>> +    }
>> +    return true;
>> +}
>> +
>> +static bool trans_csrxchg(DisasContext *ctx, arg_csrxchg *a)
>> +{
>> +    TCGv dest = gpr_dst(ctx, a->rd, EXT_NONE);
>> +    TCGv src1 = gpr_src(ctx, a->rd, EXT_NONE);
>> +    TCGv src2 = gpr_src(ctx, a->rj, EXT_NONE);
>> +
>> +    if (a->rj == 0) {
>> +        return trans_csrrd(ctx, a->rd, a->csr);
>> +    } else if (a->rj == 1) {
>> +        return trans_csrwr(ctx, a->rd, a->csr);
>> +    }
> 
> These should have been decoded separately; see below.
> 
> You're missing the check for priv 0 here and in all other functions.
> 
>> +
>> +    if (a->rd == 0) {
>> +        gen_helper_csr_xchgq_r0(cpu_env, src2, tcg_constant_i64(a->csr));
>> +    } else {
>> +        gen_helper_csr_xchgq(dest, cpu_env, src1, src2, tcg_constant_i64(a->csr));
>> +    }
> 
> Why do you believe r0 to require a special case?
>
 
OK, I have modified  all above.

>> +static bool trans_iocsrrd_b(DisasContext *ctx, arg_iocsrrd_b *a)
>> +{
>> +    TCGv tmp = tcg_temp_new();
>> +    TCGv dest = gpr_dst(ctx, a->rd, EXT_NONE);
>> +    TCGv src1 = gpr_src(ctx, a->rj, EXT_NONE);
>> +
>> +    gen_helper_iocsr_read(tmp, cpu_env, src1);
>> +    tcg_gen_qemu_ld_tl(dest, tmp, ctx->mem_idx, MO_SB);
> 
> This is wrong.  From the manual:
> 
>   All IOCSR registers use independent addressing space
> 
> therefore you cannot simply read from a modified address, you must use a completely different address space.
> 
> There are a couple of different solutions that are possible.
> 
> (1) Use helper functions calling address_space_ld/st*.
> 
> (2) Use a separate mmu_idx, which uses its own address space.
>     This requires bouncing through MemTxAttrs, since
>     cpu_asidx_from_attrs only take attrs and not mmu_idx.
> 
> The second one is may be overkill, unless there will be any cachable memory in iospace.  I would expect most of it to be device memory.
> 
(1) For the iocsr registers, most of them act on the interrupt controller, the read and write will go to interrupt's mmio read/write.
So I modified the addr to their mmio range. The ext interrupt controller use the sysbus's function to handle the interrupt cascade and
sysbus_mmio_map to map the address which use the system memory region. So if I use a different address space, I realize a different 
sysbus_mmio_map function use a different address space, is it feasible ?

(2)Can you help me review the remaining patches? Thanks.

>> +csrxchg          0000 0100 .............. ..... .....     @fmt_rdrjcsr
> 
> {
>   csrrd             0000 0100 .............. 00000 .....     @fmt_rdcsr
>   csrwr             0000 0100 .............. 00001 .....     @fmt_rdcsr
>   csrxchg           0000 0100 .............. ..... .....     @fmt_rdrjcsr
> }
> 
> 
> r~
Richard Henderson Oct. 29, 2021, 5:38 p.m. UTC | #9
On 10/28/21 11:26 PM, yangxiaojuan wrote:
> (1) For the iocsr registers, most of them act on the interrupt controller, the read and write will go to interrupt's mmio read/write.
> So I modified the addr to their mmio range. The ext interrupt controller use the sysbus's function to handle the interrupt cascade and
> sysbus_mmio_map to map the address which use the system memory region. So if I use a different address space, I realize a different
> sysbus_mmio_map function use a different address space, is it feasible ?

You shouldn't need a different function at all.  All of the difference will be in how you 
construct the mmio region: create a new address space, and add the mmio region to that 
address space at the correct base address.

> (2)Can you help me review the remaining patches? Thanks.

The remaining patches are harder for me to review, because I'm not as familiar with the 
fine details in hw/, but I'll try.


r~