Re: [PATCH v2 06/16] KVM: arm64: gic-v5: Add ITS MMIO state emulation

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] KVM: arm64: gic-v5: GICV5_ITS_SYNCR registered as 32-bit instead of 64-bit
- [Medium] KVM: arm64: gic-v5: vgic_v5_its_reset() sets idr1.l2sz=0 for 2-level tables
--

commit 26a03544ad8ae134679e971108dc975221b2093a
Author: Sascha Bischoff <sascha.bischoff@arm.com>

KVM: arm64: gic-v5: Add ITS MMIO state emulation

This commit replaces the placeholder GICv5 ITS IODEV region with the
architected register layout and adds MMIO read/write handlers and reset
state for the ITS configuration and translation-control registers.

> diff --git a/arch/arm64/kvm/vgic/vgic-its-v5.c b/arch/arm64/kvm/vgic/vgic-its-v5.c
> index 5790b029f09b6..f04161c315b78 100644
> --- a/arch/arm64/kvm/vgic/vgic-its-v5.c
> +++ b/arch/arm64/kvm/vgic/vgic-its-v5.c

[ ... ]

> +static void vgic_v5_mmio_write_its_dt_itt(struct kvm *kvm, void *dev,
> +					  gpa_t addr, unsigned int len,
> +					  unsigned long val)
> +{
> +	struct vgic_v5_its *its = dev;
> +	size_t offset = addr & (SZ_64K - 1);
> +
> +	guard(mutex)(&its->its_lock);
> +
> +	switch (offset) {

[ ... ]

> +	case GICV5_ITS_INV_EVENTR:
> +		its->inv_eventr.l1 =
> +			FIELD_GET(GICV5_ITS_INV_EVENTR_L1, val);
> +		its->inv_eventr.itt_l2sz =
> +			FIELD_GET(GICV5_ITS_INV_EVENTR_ITT_L2SZ, val);
> +		its->inv_eventr.i = FIELD_GET(GICV5_ITS_INV_EVENTR_I, val);
> +		return;
> +	case GICV5_ITS_SYNCR:
> +	case GICV5_ITS_READ_EVENTR:
> +		return;

[ ... ]

> @@ -31,11 +298,153 @@ static void vgic_v5_mmio_write_its_wi(struct kvm *kvm, void *dev, gpa_t addr,
>  	}
>  
>  static const struct vgic_register_region vgic_v5_its_registers[] = {

[ ... ]

> +	REGISTER_ITS_V5_DESC(GICV5_ITS_STATUSR, vgic_v5_mmio_read_its_dt_itt,
> +			     vgic_v5_mmio_write_its_wi, 4, VGIC_ACCESS_32bit),
> +	REGISTER_ITS_V5_DESC(GICV5_ITS_SYNCR, vgic_v5_mmio_read_its_raz,
> +			     vgic_v5_mmio_write_its_dt_itt, 4,
> +			     VGIC_ACCESS_32bit),

[Severity: Medium]
Should GICV5_ITS_SYNCR be registered as an 8-byte register with
VGIC_ACCESS_64bit | VGIC_ACCESS_32bit (and handle GICV5_ITS_SYNCR + 4 in
vgic_v5_mmio_write_its_dt_itt())?

In include/linux/irqchip/arm-gic-v5.h, GICV5_ITS_SYNCR is a 64-bit register
with GICV5_ITS_SYNCR_SYNC at BIT_ULL(63) and GICV5_ITS_SYNCR_SYNCALL at
BIT_ULL(32):

#define GICV5_ITS_SYNCR_SYNC			BIT_ULL(63)
#define GICV5_ITS_SYNCR_SYNCALL			BIT_ULL(32)
#define GICV5_ITS_SYNCR_DEVICEID		GENMASK_ULL(31, 0)

In drivers/irqchip/irq-gic-v5-its.c, gicv5_its_syncr() performs a 64-bit MMIO
write to this register:

	syncr = FIELD_PREP(GICV5_ITS_SYNCR_SYNC, 1) |
		FIELD_PREP(GICV5_ITS_SYNCR_DEVICEID, its_dev->device_id);

	its_writeq_relaxed(its, syncr, GICV5_ITS_SYNCR);

With length 4 and VGIC_ACCESS_32bit in vgic_v5_its_registers[],
vgic_get_mmio_region() -> check_region() rejects 64-bit guest accesses to
GICV5_ITS_SYNCR, and vgic_find_mmio_region() fails to match 32-bit accesses to
the upper word at GICV5_ITS_SYNCR + 4. This also causes the
KVM_DEV_ARM_VGIC_GRP_ITS_REGS userspace attribute access added later in the
series to treat GICV5_ITS_SYNCR as a 32-bit register rather than a 64-bit
register.

[ ... ]

> +static void vgic_v5_its_reset(struct vgic_v5_its *its)
> +{
> +	u64 mmfr0;
> +
> +	its->enabled = false;
> +
> +	mmfr0 = read_sanitised_ftr_reg(SYS_ID_AA64MMFR0_EL1);
> +	its->idr0.pa_range = cpuid_feature_extract_unsigned_field(
> +		mmfr0, ID_AA64MMFR0_EL1_PARANGE_SHIFT);
> +	its->idr0.domain = GICV5_ITS_IDR0_DOMAIN_NON_SECURE;
> +	its->idr0.mec = 0;
> +	its->idr0.mpam = 0;
> +	its->idr0.swe = 0;
> +	its->idr0.its_id = 0;
> +
> +	its->idr1.dt_levels = 1;
> +	its->idr1.itt_levels = 1;
> +	its->idr1.l2sz = 0;

[Severity: Medium]
In vgic_v5_its_reset(), dt_levels and itt_levels are set to 1 to advertise
2-level Device Tables and Interrupt Translation Tables, while its->idr1.l2sz
is initialized to 0.

In include/linux/irqchip/arm-gic-v5.h, GICV5_ITS_IDR1_L2SZ (bits [10:8]) is a
3-bit support bitmap rather than the 2-bit size encoding used in
GICV5_ITS_DT_CFGR_L2SZ:

#define GICV5_ITS_IDR1_L2SZ		GENMASK(10, 8)
#define GICV5_ITS_IDR1_ITT_LEVELS	BIT(7)
#define GICV5_ITS_IDR1_DT_LEVELS	BIT(6)
#define GICV5_ITS_IDR1_DEVICEID_BITS	GENMASK(5, 0)

#define GICV5_ITS_IDR1_L2SZ_SUPPORT_4KB(r)	FIELD_GET(BIT(8), (r))
#define GICV5_ITS_IDR1_L2SZ_SUPPORT_16KB(r)	FIELD_GET(BIT(9), (r))
#define GICV5_ITS_IDR1_L2SZ_SUPPORT_64KB(r)	FIELD_GET(BIT(10), (r))

When a guest reads GICV5_ITS_IDR1 through vgic_v5_mmio_read_its_misc(),
FIELD_PREP(GICV5_ITS_IDR1_L2SZ, its->idr1.l2sz) reports 0b000, indicating that
none of the 4KB, 16KB, or 64KB L2 granule sizes are supported. In
drivers/irqchip/irq-gic-v5-its.c, gicv5_its_l2sz_two_level() checks those
support bits and fails all of them (falling back to 4KB even on 16KB or 64KB
page size guests, despite parse_two_level_dt() and parse_two_level_itt()
supporting all three sizes).

Should its->idr1.l2sz be initialized with the supported L2 table size bits set
instead of 0?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261102194454.4086911-1-sascha.bischoff@arm.com?part=6