Re: [PATCH v2 04/13] KVM: Parse the device tree and register the ITS region with pKVM

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [Critical] The function pointer for the MMIO emulator passed to EL2 points to unmapped/NX memory, causing an Instruction Abort.
- [High] MMIO emulation in `its_emulate_forward_req` double-swaps on Big Endian kernels by using `readX_relaxed` and `writeX_relaxed`.
- [High] pKVM incorrectly maps and accesses MMIO devices using the hypervisor's linear map (`__hyp_va()`) rather than a private VA range.
- [High] handle_host_mmio_trap ignores the ESR_ELx_SSE (Sign Extend) bit for MMIO reads, returning zero-extended values for signed loads.
--

--- Patch [4]: [PATCH v2 04/13] KVM: Parse the device tree and register the ITS region with pKVM ---
commit 6f98f20a817e227941fab67881459ac609810480
Author: Sebastian Ene <[email protected]>

KVM: Parse the device tree and register the ITS region with pKVM

Identify the ITS base address from the device tree and store it in the
pkvm_protected_regs array so that it will be unmapped from the host
address space. Register a callback to forward all the MMIO requests to
the device to prevent breaking ITS functionality.

> diff --git a/arch/arm64/kvm/hyp/nvhe/its_emulate.c b/arch/arm64/kvm/hyp/nvhe/its_emulate.c
> new file mode 100644
> index 0000000000000..63a42f520ed25
> --- /dev/null
> +++ b/arch/arm64/kvm/hyp/nvhe/its_emulate.c
> @@ -0,0 +1,37 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +
> +#include <asm/kvm_pkvm.h>
> +#include <nvhe/mem_protect.h>
> +
> +void its_emulate_forward_req(struct pkvm_protected_reg *region, u64 offset, bool write, u64 *reg,
> +			     u8 reg_size)
> +{
> +	void __iomem *addr = __hyp_va(PFN_PHYS(region->pfn) + offset);

[Severity: High]
Does using __hyp_va() here work for MMIO addresses? The __hyp_va() macro
subtracts hyp_physvirt_offset, which is based on RAM physical addresses.
Since MMIO regions often reside at lower physical addresses than RAM,
wouldn't this underflow and produce a virtual address with the top bits set,
leading to a Translation Fault? This same issue seems to be present in
__pkvm_host_donate_hyp_mmio() where it populates the page tables.

> +
> +	switch (reg_size) {
> +	case 1:
> +		if (!write)
> +			*reg = readb_relaxed(addr);
> +		else
> +			writeb_relaxed(*reg, addr);

[Severity: High]
Will these relaxed accessors cause a double-swap on Big Endian kernels?
When the host traps to EL2 on an MMIO access, the CPU register already
contains the exact data the host intended to write (swapped to Little
Endian by the host driver's writel). Using writeb_relaxed and similar
helpers will apply cpu_to_le* again. Should this use the __raw_read*
and __raw_write* accessors to preserve the trapped register's exact bit
representation?

> +		break;

[ ... ]

> diff --git a/arch/arm64/kvm/pkvm.c b/arch/arm64/kvm/pkvm.c
> index 053e4f733e4be..5e80da18f3120 100644
> --- a/arch/arm64/kvm/pkvm.c
> +++ b/arch/arm64/kvm/pkvm.c

[ ... ]

> @@ -39,6 +44,47 @@ static int __init register_memblock_regions(void)
>  
> +static int __init register_its_emulated_region(void)
> +{

[ ... ]

> +		kvm_nvhe_sym(pkvm_protected_regs)[i].pfn = PHYS_PFN(res.start);
> +		kvm_nvhe_sym(pkvm_protected_regs)[i].cb =
> +			lm_alias(&kvm_nvhe_sym(its_emulate_forward_req));

[Severity: Critical]
Can this function pointer be safely executed at EL2? The host initializes
the callback using lm_alias, producing a host linear-map address. When
the hypervisor later applies kern_hyp_va() to it in
donate_protected_mmio_regions(), it converts it to a hyp linear-map
address. However, .hyp.text physical pages are not part of the
dynamically allocated hyp memory pool, so they are unmapped in the hyp
linear map. Does jumping to this pointer in handle_host_mmio_trap()
trigger an Instruction Abort?

[Severity: High]
Regarding how this callback's result is handled in handle_host_mmio_trap(),
does the trap handler process sign extension for MMIO reads? When the host
traps to EL2 on a stage-2 data abort caused by a signed read access (like
LDRSB), the ESR_ELx_SSE bit indicates the destination register should be
sign-extended. If handle_host_mmio_trap() writes the zero-extended value
directly into the host context, could this alter functional behavior for
signed loads?

> +		kvm_nvhe_sym(pkvm_protected_regs)[i].nr_pages =
> +			PFN_DOWN(min_t(u64, resource_size(&res), PAGE_ALIGN_DOWN(GITS_TRANSLATER)));

[ ... ]

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