Re: [PATCH v2 03/13] KVM: arm64: Support host MMIO trap handlers for unmapped devices

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Host MMIO trap handler fails to properly mask, sign-extend, and truncate register values during emulation.
- [Medium] Unconditional 4-byte instruction skip breaks AArch32 EL0 Thumb execution.
--

--- Patch [3]: [PATCH v2 03/13] KVM: arm64: Support host MMIO trap handlers for unmapped devices ---
commit 4f82f551a35a497988ae3208761d26c15c28de61
Author: Sebastian Ene <[email protected]>

KVM: arm64: Support host MMIO trap handlers for unmapped devices

This commit introduces a stage-2 fault handler in the hypervisor to intercept
and emulate host memory accesses to unmapped or protected MMIO regions. It
hooks into handle_host_mem_abort() to check if a registered callback can handle
the access.

> diff --git a/arch/arm64/kvm/hyp/nvhe/mem_protect.c b/arch/arm64/kvm/hyp/nvhe/mem_protect.c
> index 500c18c2fd48b..7e978e0c44b9e 100644
> --- a/arch/arm64/kvm/hyp/nvhe/mem_protect.c
> +++ b/arch/arm64/kvm/hyp/nvhe/mem_protect.c
[ ... ]
> @@ -752,6 +753,50 @@ static void host_inject_mem_abort(struct kvm_cpu_context *host_ctxt)
>  	inject_host_exception(esr);
>  }
>  
> +static bool handle_host_mmio_trap(struct kvm_cpu_context *host_ctxt, u64 esr, u64 addr)
> +{
> +	u64 offset, reg_value = 0, start, end;
> +	u8 reg_size, reg_index;
> +	bool write;
> +	int i;
> +
> +	for (i = 0; i < num_protected_reg; i++) {
> +		if (!pkvm_protected_regs[i].pfn || !pkvm_protected_regs[i].nr_pages ||
> +		    !pkvm_protected_regs[i].cb)
> +			continue;
> +
> +		start = PFN_PHYS(pkvm_protected_regs[i].pfn);
> +		end = start + PFN_PHYS(pkvm_protected_regs[i].nr_pages);
> +		reg_size = BIT((esr & ESR_ELx_SAS) >> ESR_ELx_SAS_SHIFT);
> +
> +		if (start > addr || addr + reg_size > end)
> +			continue;
> +
> +		reg_index = (esr & ESR_ELx_SRT_MASK) >> ESR_ELx_SRT_SHIFT;
> +		write = (esr & ESR_ELx_WNR) == ESR_ELx_WNR;
> +		offset = addr - start;
> +
> +		if (write && reg_index != 31)
> +			reg_value = host_ctxt->regs.regs[reg_index];
> +
> +		pkvm_protected_regs[i].cb(&pkvm_protected_regs[i], offset, write,
> +					  &reg_value, reg_size);
> +
> +		if (!write && reg_index != 31)
> +			host_ctxt->regs.regs[reg_index] = reg_value;

[Severity: High]
Does this emulation handle sign extension and register width properly?

When the host executes an instruction like LDRSB, shouldn't the handler inspect
the ESR_ELx_SSE (Sign Extend) bit to properly sign-extend the value? Also, for
32-bit register destinations (ESR_ELx_SF == 0), the architecture requires the
upper 32 bits to be zeroed, but it looks like this assigns the value directly.

> +
> +		kvm_skip_host_instr();

[Severity: Medium]
Can this break AArch32 EL0 Thumb execution?

kvm_skip_host_instr() unconditionally adds 4 to the ELR. If the stage-2 data
abort originates from a host userspace process executing a 2-byte Thumb
instruction, shouldn't it inspect the ESR_ELx_IL bit to advance the PC by 2
bytes instead?

> +		return true;
> +	}
> +
> +	return false;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=3