The decoder loop at line 278 relies on op->name != NULL to terminate:

for (op = ops; op->name NULL; op++) {
However, the table ends at line 267 with { "csrrd", ... } and has no sentinel { NULL, ... } entry. After processing csrrd, op++ moves past the and the next iteration reads out-of-bounds memory to check op->name. This is undefined behavior and may crash DDB or call a bogus function pointer through op->pr.

It is suggested to add a new line after this line:

	{ NULL,      0,          0,          NULL },

Based on the LoongArch manual's explanation of JIRL/LL/SC, these immediates are shifted left by 2 bits, the code here should be changed to *4.

JIRL: https://loongson.github.io/LoongArch-Documentation/LoongArch-Vol1-EN.html#_jirl
LL: https://loongson.github.io/LoongArch-Documentation/LoongArch-Vol1-EN.html#_ll_wd_sc_wd
SC: https://loongson.github.io/LoongArch-Documentation/LoongArch-Vol1-EN.html#_sc_q

static void pr_r_i14s2(struct op *op, vm_offset_t loc, uint32_t insn)
{
	db_printf("%-9s\t%s, %s, %d", op->name,
	    rname[RD(insn)], rname[RJ(insn)], SI14(insn) * 4);
}
static void pr_rr_i16s2(struct op *op, vm_offset_t loc, uint32_t insn)
{
	db_printf("%-9s\t%s, %s, %d", op->name,
	    rname[RD(insn)], rname[RJ(insn)], SI16(insn) * 4);
}

addi.w and ld.b do not have the same opcode:

addi.w = 0x02800000，bits[31:22] = 0x00a
ld.b = 0x28000000，[31:22] = 0x0a0

This code is based on an incorrect assumption, and since ld.b comes before addi.w in ops[], it would already match and return first anyway. This skip logic is dead/misleading code.

Sync the same change:

#define	OP(insn)	(((insn) >> 0) & 0x1f)	/* invtlb op immediate */

suggested to add a new line after this line:

static void pr_invtlb(struct op *op, vm_offset_t loc, uint32_t insn)
{
	db_printf("%-9s\t%u, %s, %s", op->name,
	    OP(insn), rname[RJ(insn)], rname[RK(insn)]);
}

Suggested change to:

{ "invt",  0x06490000, 0xffff801f, pr_invtlb },

addi.w and ld.b do not have the same opcode:

addi.w = 0x02800000，bits[31:22] = 0x00a
ld.b = 0x28000000，[31:22] = 0x0a0

This code is based on an incorrect assumption, and since ld.b comes before addi.w in ops[], it would already match and return first anyway. This skip logic is dead/misleading code.

It is suggested to remove this code.

%u is used to print uint64_t fields.

In db_show_mdpcpu():

db_printf("kernel    = %u\n", pc->pc_kernel_sp);
db_printf("asid_value   = %u\n", pc->pc_asid_value);

However, per machine/pcpu.h, both pc_kernel_sp and _asid_value are uint64_t. The %u format specifier expects unsigned int (32-bit), so on 64-bit builds this truncates the upper 32 bits of the value.

pcpu.h: https://reviews.freebsd.org/D59890

	uint64_t pc_kernel_sp;  /* kernel sp */ 	\
	uint64_t pc_asid_value;	/* ASID */ \

Please use %ju with auintmax_t` cast, suggested change to:

	db_printf("kernel_sp    = %ju\n", (uintmax_t)pc->pc_kernel_sp);
	db_printf("asid_value   = %ju\n", (uintmax_t)pc->pc_asid_value);

Suggested change to:

db_stack_trace_cmd(struct thread *td, struct unwind_state *frame, int count)

Sync the same change:

	for (int depth = 0; count < 0 || depth < count; depth++) {

Sync the same change:

	db_stack_trace_cmd(thr, &frame, count);

Sync the same change:

	db_stack_trace_cmd(thr, &frame, -1);

This function is called during a crash dump to temporarily map chunk pages starting at physical address pa and return the virtual address via *va.

The function is not implemented here; it looks like it only does a single line of printing. Would it be possible to add a TODO or some explanation?

For reference, the x86(dumpsys_map_chunk) implementation maps each with pmap_kenter_temporary(trunc_page(a), i).

The header is not self-contained.

eioic.h uses struct intr_irqsrc, device_t, and struct mtx, but does not include the headers that define them.

It currently compiles only because eioic.c and pch_pic.c already include <sys/interrupt.h>, <sys/types.h> (pulled in indirectly via <sys/bus.h>, etc.), and <sys/mutex.h> before including eioic.h.

#include <sys/types.h>		/* device_t */
#include <sys/cpuset.h>	/* cpuset_t */
#include <sys/mutex.h>		/* struct mtx */
#include <sys/intr.h>		/* struct intr_irqsrc */

struct eioic_softc defines struct resource *irq_res, but it is never read or written anywhere in eioic.c; it is suggested to remove it.

Should the 32-bit-related code be removed?

fix type

	/* save static register */

fix type

	 * LoongArch does.

gdb_cpu_setreg() currently accepts regnum == 0 because the regnum >= 0 check includes r0, and stores the value into gdb_frame->tf_regs[0].

LoongArch r0/$zero is architecturally hardwired to zero; any write to it is discarded by hardware. Writing through KGDB therefore not change the CPU register but does corrupt the trapframe's view of r0, which can mislead fill_regs(), get_mcontext(), or crash dumps.

	if (kdb_thread == curthread) {
		/* $zero (r0) is hardwired to zero by the architecture. */
		if (regnum == GDB_REG_ZERO)
			return;

Fix non-standard C99 anonymous unions/structs.

Modify struct pcb to remove the anonymous union, change it to a normal array member, and keep the named access macros.

	uint64_t	pcb_regs[32];	/* general purpose registers */
#define	pcb_a	pcb_regs[4]
#define	pcb_t		pcb_regs[12]
#define	pcb_s		pcb_regs[23]
#define	pcb_ra		pcb_regs[1]
#define	pcb_sp		pcb_regs[3]
	uint64_t	pcb_fregs[34];	/* floating point registers */
#define	pcb_fcsr0	pcb_fregs[32]

Sync the same change:

mcp->mc_fpregs.fp_fcsr = curpcb->pcb_fcsr0;

This header is the machine-dependent header for the ACPI subsystem to access PCI configuration space, but here it is an empty file used as a placeholder. A TODO could be added to explain this.

That's reasonable

Right

Yes, will add validation in next version

yes

will remove in next reversion

fix type

	uint64_t pc_kernel_sp;  /* Kernel stack for user exceptions */ \
	uint64_t pc_asid_value;	/* Current ASID value */ \
	uint32_t pc_asid_mask;	/* ASID bit mask */ \

will add this two constant in next reversion

I think this should be kept, it's legal 32 bit relocation in 64 bit ELF, not 32 bit ABI.

struct cpu_desc is defined in <machine/cpu.h>, and the global array is defined in identcpu.c. Putting the extern declaration in pmap.h introduces unnecessary dependencies; it should be moved into <machine/cpu.h>.

extern cpu_desc cpu_desc[MAXCPU];

Remove it here, and move extern struct cpu_desc into cpu.h.

This macro is defined in elf.h, but hardware capability bits are more appropriate in <machine/cpu.h>. Making pmap.h depend on an ELF header is not appropriate.

#define	CPU_HWCAP_PTW	(1 << 13)	/* Page Table Walker extension */

suggested to remove this include header, as the reference has already been moved into cpu.h.

Sync the same change:

if (cpu_desc[cpu].cpuinfo.hwcap & CPU_HWCAP_PTW)

that's reasonable

seems a good improvement, but loongarch is sync with other arch now, so I will ignore this for now and maybe we can submit in a separate patchset later.

currently, it mirrors arm64 and riscv and just leave it blank

will fix

will fix this

TF_FCSR0/_FCC relies on anonymous union members.

Remove the anonymous union for the FP part and change it to separate fields.

	uint64_t tf_fregs[32];	 FP registers $f0-$f31 */
	uint64_t tf_fcsr0;	/* FP control/status register 0 */
	uint64_t tf_fcc;	/* FP condition codes */

Sync the same change:

ASSYM(TF_FCSR0, offsetof(struct trapframe, tf_fcsr0));
ASSYM(TF_FCC, offsetof(struct trapframe, tf_fcc));

Sync the same change:

uint64_t fcsr = frame->tf_fcsr0;

RISC-V-specific concepts left over directly from RISC-V, RISC-V uses the CSR names mvendorid/marchid/mimpid to read CPU identification. LoongArch does not have these CSRs; it reads PRID and Arch information via CPUCFG0/CPUCFG1. Yet the LoongArch version retains the RISC-V variable names.

https://loongson.github.io/LoongArch-Documentation/LoongArch-Vol1-EN.html#_cpucfg

https://docs.riscv.org/reference/isa/v20250508/priv/sstc.html

suggested to remove it as well.

Sync the same change:

int cpu;

Change the cpuhead list traversal to CPU_FOREACH_ISSET to iterate directly over the target cpuset, avoiding scanning all online CPUs, consistent with other architecture implementations, and remove the redundant pc_pending_ipis setting.

	CPU_FOREACHSET(cpu, &cpus) {
		CTR3(KTR_SMP, "%s: cpu: %d, ipi: %x",
		    __func__, cpu, ipi);
		ipi_send(cpuid_to_pcpu[cpu (uint32_t)ipi);

An empty implementation would make callers believe the cache has been flushed when it actually has not. In scenarios such as kexec loading a new kernel, writing data to a memory disk, or loading modules, the CPU may fetch stale instructions from the I-cache or read stale data from the D-cache, causing boot failures or data corruption.

A memory barrier can be implemented as a stopgap for now:

__asm __volatile("dbar 0" ::: "memory");

can be made more concise like this:

error = uiomove(PHYS_TO_DMAP(v), cnt uio);

Sync the same change:

	ipi_cpu(cpuid IPI_WAKEUP);

Sync the same change:

ipi_write_action(coreid, IPI_WAKEUP);

boot_cpu is the hardware cpuid, and all_cpus is a logical cpuid bitmap.

When setting the CPU bitmap all_cpus, cpu_mp_start() incorrectly uses the hardware cpuid boot_cpu, whereas all_cpus should use the logical cpuid (0-based).

so when the hardware boot CPU is not 0, the bit will be set in the wrong position, causing errors in IPI sending, CPU traversal, and similar logic.

Suggested change to:

	/* CPU 0 is always the boot CPUlogical id). */
	CPU_SET(0, &all_cpus);

ofwbus is the root bus of the FDT device tree and should not appear on ACPI systems. Currently exus_attach() places ofwbus outside the if (loongarch_efi_acpi_rsdp() != 0) branch, causing ofwbus to be attached even during ACPI boot.

Note: this is only a minimal fix. The cleaner approach is to follow ARM64 and split FDT and ACPI into two separate sub-drivers, with ofwbus attached only in nexus_fdt_attach().

Suggested change to:

	if (loongarch_efi_acpi_rsdp() == 0)
		nexus_add_child(dev, 7, "ofbus", 0);

This file does not use any macros from cdefs.h, suggested to remove it.

Define named constants and cite the manual:

#define	PCH_ID		0x00	/* INT_ID, identification reg 1 */
#define	PCH_PIC_INT_NUM		0x04	/* INT_ID2[23:16]: num sources - 1 */
#define	PCH_PIC_INT_NUM_SHIFT16
#define	PCH_PIC_INT_NUM_MASK	0xff

description-of-interrupt-related-registers: https://loongson.github.io/LoongArch-Documentation/Loongson-7A1000-usermanual-EN.html#description-of-interrupt-related-registers

Use the three defined constants PCH_PIC_INT_NUM, PCH_PIC_INT_NUM_SHIFT, and PCH_PIC_INT_NUM_MASK.

	/*
	 * Determine number of interrupt inputs from the identification
	 * register 2 (LS7A1000 manual §5.2: INT_ID2[23:16] + 1+	 */
	sc->vec_count = ((pch_pic_read(sc, PCH_PIC_INT_NUM) >>
	    PCH_PIC_INT_NUM_SHIFT) & PCH_PIC_INT_NUM_MASK) + 1;

Sync the same change:

static inline pt_entry_t pmap_memattr_bits(vm_memattr_t mode);

Sync the same change:

	new_l3 = PTE_V | PTE_P | pmap_memattr_bits(pmap_page_get_memattr(m)) |
           PTE_NX | PTE_K;

Sync the same change:

PTE_V | pmap_memattr_bits(pmap_page_get_memattr(m)) | PTE_P | PTE_NX);

Sync the same change:

PTE_V | pmap_memattr_bits(pmap_page_get_memattr(m)) | PTE_P | PTE_NX;

Sync the same change:

#include <machine/watch.h>

It is suggested to add after this line:

	if (td curthread)
		la_watch_load(td);

suggested to add after this line:

dbar	0			/* release: prior accesses complete before CAS */

suggested to add after this line:

dbar	0		/* acquire: subsequent accesses after CAS */

suggested to add after this line:

dbar	0			/* release: prior accesses complete before CAS */

suggested to add after this line:

dbar	0			/* acquire: subsequent accesses after CAS */

Suggested change to:

*freq = ((uint64_t)base_freq * cfm) / cfd;