Re: [PATCH v2 07/16] KVM: arm64: gic-v5: Add ITS guest table translation emulation

[Oliver Upton <oupton@kernel.org>]

On Mon, Nov 02, 2026 at 07:44:45PM +0000, Sascha Bischoff wrote:
> +static int parse_linear_dt(struct kvm *kvm, gpa_t dt_base, u32 device_id,
> +			   gpa_t *itt_base, bool *two_level_itt,
> +			   unsigned int *event_id_bits, unsigned int *itt_l2sz)
> +{
> +	__le64 dt_l2_entry;
> +	gpa_t entry_addr;
> +	u64 val;
> +	int ret;
> +

<snip>

> +	entry_addr = dt_base + device_id * sizeof(__le64);
> +	ret = kvm_read_guest_lock(kvm, entry_addr, &dt_l2_entry,
> +				  sizeof(dt_l2_entry));
> +	if (ret) {
> +		kvm_debug("Failed to read guest memory for L2 DT\n");

99% chance of this being either the guest or userspace's fault. I don't
see any reason why we need to carry a pr_debug() here.

What would be nice is some preparatory return codes or helpers for
recording the SWERR that caused an ITS operation to fail so we can add
support for ITS_IDR0.SWE later on.

The GICv3 ITS made a half-attempt at doing this. Even though it never
conveyed that information to the guest, I think it was a net-win in
terms of readability.

> +		return ret;
> +	}
> +
> +	val = le64_to_cpu(dt_l2_entry);

</snip>

Feels like "read the Nth entry from this table out of guest memory"
should be done in a helper shared with the other callsites. On top of
that, arm64 is little-endian only as of 7.4.

While I'm not saying we should go tear out every instance of byteorder
twiddling, I think that shared helper would be an excellent spot to
either do the le_to_cpu() or document the fact that both the kernel and
the ITS are LE-only.

> +	if (!FIELD_GET(GICV5_DTL2E_VALID, val)) {
> +		kvm_debug("Guest DT entry is invalid\n");
> +		return -EINVAL;
> +	}
> +
> +	*itt_base = FIELD_GET(GICV5_DTL2E_ITT_ADDR_MASK, val) << 3;
> +	*two_level_itt = FIELD_GET(GICV5_DTL2E_ITT_STRUCTURE, val);
> +	*event_id_bits = FIELD_GET(GICV5_DTL2E_EVENT_ID_BITS, val);
> +	*itt_l2sz = FIELD_GET(GICV5_DTL2E_ITT_L2SZ, val);

Same sense brewing here about the parsing, with the added gripe that
multiple out pointers can be a bit annoying to cope with. I'd rather see
the literal DTE get passed around or we use a struct containing all the
extracted fields we care about.

> +static int parse_two_level_dt(struct kvm *kvm, gpa_t dt_base,
> +			      unsigned int dt_l2sz, u32 device_id,
> +			      gpa_t *itt_base, bool *two_level_itt,
> +			      unsigned int *event_id_bits,
> +			      unsigned int *itt_l2sz)

Any reason why this can't be anchored on a pointer to the ITS? Because
you're passing down a lot of state that's just per-ITS context.

> +	if (device_id >= 1UL << device_id_bits) {

BIT_ULL()?

> +static int parse_two_level_itt(struct kvm *kvm, u32 event_id, gpa_t itt_base,
> +			       unsigned int itt_l2sz, __le64 *itt_l2_entry)
> +{
> +	unsigned int itt_split_bits, l2_event_id_bits;
> +	gpa_t itt_l2_base, entry_addr;
> +	u32 l1_event_id, l2_event_id;
> +	__le64 itt_l1_entry;
> +	u64 val;
> +	int ret;
> +
> +	switch (itt_l2sz) {
> +	case 0:
> +		itt_split_bits = 9;
> +		break;
> +	case 1:
> +		itt_split_bits = 11;
> +		break;
> +	case 2:
> +		itt_split_bits = 13;
> +		break;
> +	default:
> +		return -EINVAL;
> +	}

this should be a shared helper

Thanks,
Oliver