On Mon, Nov 02, 2026 at 07:44:45PM +0000, Sascha Bischoff wrote:
quoted hunk ↗ jump to hunk
+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>
quoted hunk ↗ jump to hunk
+ 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.
quoted hunk ↗ jump to hunk
+ 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.
quoted hunk ↗ jump to hunk
+ 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.
quoted hunk ↗ jump to hunk
+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()?
quoted hunk ↗ jump to hunk
+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