Re: [PATCH 10/43] KVM: arm64: gic-v5: Implement VPE IRS MMIO Ops

Marc Zyngier
2026-04-30T08:46:28Z

On Mon, 27 Apr 2026 17:09:27 +0100,
Sascha Bischoff <Sascha.Bischoff@arm.com> wrote:
>
> Introduce interfaces to make VPEs valid, and to configure them, via
> the host's IRS. As with the other valid bits in the GICv5 VM tables,
> VPEs cannot be made valid directly, and instead are made valid via an
> IRS MMIO Op.
>
> Additionally, some of the VPE configuration takes place via the IRS
> MMIO interface too (via the IRS_VPE_CR0, IRS_VPE_DBR). VPE doorbells
> are, for example, configured via this interface.
>
> The existing VPE-doorbell-based commands are extended with:
>
> VPE_MAKE_VALID - Make the VPE valid in the VPET
> VPE_CR0_READ - Handle a guest read from IRS_PE_CR0
> VPE_CR0_WRITE - Handle a guest write to IRS_PE_CR0
>
> Note: There is no VPE_MAKE_INVALID as VPEs are only made invalid on
> teardown, at which point the whole VMTE is marked as invalid. Hence,
> it is not required.
>
> Signed-off-by: Sascha Bischoff <sascha.bischoff@arm.com>
> ---
> arch/arm64/kvm/vgic/vgic-v5.c | 164 +++++++++++++++++++++++++++++
> include/linux/irqchip/arm-gic-v5.h | 27 +++++
> 2 files changed, 191 insertions(+)
>
> diff --git a/arch/arm64/kvm/vgic/vgic-v5.c b/arch/arm64/kvm/vgic/vgic-v5.c
> index 49eb01ca07961..0649729f6b834 100644
> --- a/arch/arm64/kvm/vgic/vgic-v5.c
> +++ b/arch/arm64/kvm/vgic/vgic-v5.c
> @@ -253,6 +253,25 @@ static int vgic_v5_irs_wait_for_vm_op(void)
> return 0;
> }
>
> +/* Wait for completion of an VPE_STATUSR change */
> +static int vgic_v5_irs_wait_for_vpe_op(void)
> +{
> + int ret;
> + u32 statusr;
> +
> + ret = readl_relaxed_poll_timeout_atomic(
> + irs_base + GICV5_IRS_VPE_STATUSR, statusr,
> + FIELD_GET(GICV5_IRS_VPE_STATUSR_IDLE, statusr), 1,
> + USEC_PER_SEC);

Formatting.

> +
> + if (ret == -ETIMEDOUT) {
> + pr_err_ratelimited("Time out waiting for IRS VPE Op\n");
> + return ret;
> + }
> +
> + return 0;

You seem to have a number of these primitives. Consider having a
generic helper that takes the required parameters, including a partial
string in case of error.

> +}
> +
> static int vgic_v5_irs_assign_vmt(bool two_level, u8 vm_id_bits, phys_addr_t vmt_base)
> {
> u64 vmt_baser;
> @@ -369,10 +388,142 @@ static int vgic_v5_irs_set_vist_invalid(int vm_id, bool spi_ist)
> return __vgic_v5_irs_update_vist_validity(vm_id, spi_ist, true);
> }
>
> +static int vgic_v5_irs_set_up_vpe(int vm_id, int vpe_id, irq_hw_number_t db_hwirq)
> +{
> + u64 vmap_vper, dbr, selr;
> + u32 statusr, cr0;
> + int ret;
> +
> + guard(raw_spinlock)(&vm_config_lock);
> +
> + /* Make sure that we are idle to begin with */
> + ret = vgic_v5_irs_wait_for_vm_op();
> + if (ret)
> + return ret;
> +
> + /* Mark the VPE as valid */
> + vmap_vper = FIELD_PREP(GICV5_IRS_VMAP_VPER_VPE_ID, vpe_id) |
> + FIELD_PREP(GICV5_IRS_VMAP_VPER_VM_ID, vm_id) |
> + FIELD_PREP(GICV5_IRS_VMAP_VPER_M, true);

That's another of these single bit mask used with FIELD_PREP. Consider
rewriting it as:

vmap_vper = FIELD_PREP(GICV5_IRS_VMAP_VPER_VPE_ID, vpe_id) |
FIELD_PREP(GICV5_IRS_VMAP_VPER_VM_ID, vm_id) |
GICV5_IRS_VMAP_VPER_M;

> + irs_writeq_relaxed(vmap_vper, GICV5_IRS_VMAP_VPER);
> +
> + /* Wait for the VPE to be marked valid in the VPET */
> + ret = vgic_v5_irs_wait_for_vm_op();
> + if (ret)
> + return ret;
> +
> + selr = FIELD_PREP(GICV5_IRS_VPE_SELR_VPE_ID, vpe_id) |
> + FIELD_PREP(GICV5_IRS_VPE_SELR_VM_ID, vm_id) |
> + FIELD_PREP(GICV5_IRS_VPE_SELR_S, true);
> + irs_writeq_relaxed(selr, GICV5_IRS_VPE_SELR);
> +
> + ret = vgic_v5_irs_wait_for_vpe_op();
> + if (ret)
> + return ret;
> +
> + statusr = irs_readl_relaxed(GICV5_IRS_VPE_STATUSR);
> + if (!FIELD_GET(GICV5_IRS_VPE_STATUSR_V, statusr))
> + return -EINVAL;
> +
> + /* Set targeted only routing (disable 1ofN vPE selection) */
> + cr0 = FIELD_PREP(GICV5_IRS_VPE_CR0_DPS, true);
> + irs_writel_relaxed(cr0, GICV5_IRS_VPE_CR0);
> +
> + ret = vgic_v5_irs_wait_for_vpe_op();
> + if (ret)
> + return ret;
> +
> + statusr = irs_readl_relaxed(GICV5_IRS_VPE_STATUSR);
> + if (FIELD_GET(GICV5_IRS_VPE_STATUSR_F, statusr))
> + ret = -EINVAL;
> +
> + /*
> + * The VPE has not yet run. Therefore, make sure that all interrupts
> + * will generate a doorbell.
> + */
> + dbr = FIELD_PREP(GICV5_IRS_VPE_DBR_LPI_ID, db_hwirq) |
> + FIELD_PREP(GICV5_IRS_VPE_DBR_DBPM, 0b11111) |
> + FIELD_PREP(GICV5_IRS_VPE_DBR_REQ_DB, false) |

And anything that set to false can be removed altogether.

> + FIELD_PREP(GICV5_IRS_VPE_DBR_DBV, true);
> + irs_writeq_relaxed(dbr, GICV5_IRS_VPE_DBR);
> +
> + ret = vgic_v5_irs_wait_for_vpe_op();
> + if (ret)
> + return ret;
> +
> + statusr = irs_readl_relaxed(GICV5_IRS_VPE_STATUSR);
> + if (FIELD_GET(GICV5_IRS_VPE_STATUSR_F, statusr))
> + return -EINVAL;
> +
> + return 0;
> +}
> +
> +static int vgic_v5_irs_vpe_cr0_read(int vm_id, int vpe_id, u64 *cr0)
> +{
> + u32 statusr;
> + u64 selr;
> + int ret;
> +
> + guard(raw_spinlock)(&vm_config_lock);
> +
> + selr = FIELD_PREP(GICV5_IRS_VPE_SELR_VPE_ID, vpe_id) |
> + FIELD_PREP(GICV5_IRS_VPE_SELR_VM_ID, vm_id) |
> + FIELD_PREP(GICV5_IRS_VPE_SELR_S, true);
> + irs_writeq_relaxed(selr, GICV5_IRS_VPE_SELR);
> +
> + ret = vgic_v5_irs_wait_for_vpe_op();
> + if (ret)
> + return ret;
> +
> + statusr = irs_readl_relaxed(GICV5_IRS_VPE_STATUSR);
> + if (!FIELD_GET(GICV5_IRS_VPE_STATUSR_V, statusr))
> + return -EINVAL;
> +
> + *cr0 = irs_readl_relaxed(GICV5_IRS_VPE_CR0);
> +
> + return 0;

I'd rather this function returned the CR0 value directly, even if the
IDLE bit isn't set. You can have a WARN_ONCE() if you want.

> +}
> +
> +static int vgic_v5_irs_vpe_cr0_update(int vm_id, int vpe_id, u32 cr0)

*_write() would be better than *_update() when you already have
*_read(). Specially as a consequence of VPE_CR0_WRITE.

> +{
> + u32 statusr;
> + u64 selr;
> + int ret;
> +
> + guard(raw_spinlock)(&vm_config_lock);
> +
> + selr = FIELD_PREP(GICV5_IRS_VPE_SELR_VPE_ID, vpe_id) |
> + FIELD_PREP(GICV5_IRS_VPE_SELR_VM_ID, vm_id) |
> + FIELD_PREP(GICV5_IRS_VPE_SELR_S, true);
> + irs_writeq_relaxed(selr, GICV5_IRS_VPE_SELR);
> +
> + ret = vgic_v5_irs_wait_for_vpe_op();
> + if (ret)
> + return ret;
> +
> + statusr = irs_readl_relaxed(GICV5_IRS_VPE_STATUSR);
> + if (!FIELD_GET(GICV5_IRS_VPE_STATUSR_V, statusr))
> + return ret;

return 0? But you have set SELR to something. Surely reading V==0
here is an indication of a bug. So should you report the error? Warn?

> +
> + irs_writel_relaxed(cr0, GICV5_IRS_VPE_CR0);
> +
> + ret = vgic_v5_irs_wait_for_vpe_op();
> + if (ret)
> + return ret;
> +
> + statusr = irs_readl_relaxed(GICV5_IRS_VPE_STATUSR);
> + if (FIELD_GET(GICV5_IRS_VPE_STATUSR_F, statusr))
> + return -EINVAL;
> +
> + return 0;
> +}
> +
> static int vgic_v5_db_set_vcpu_affinity(struct irq_data *data, void *vcpu_info)
> {
> struct vgic_v5_vm *vm = data->domain->host_data;
> struct gicv5_cmd_info *cmd_info = vcpu_info;
> + /* Our VPE ID is the index within the doorbell domain */
> + u16 vpe_id = data->hwirq;
>
> switch (cmd_info->cmd_type) {
> case VMT_L2_MAP:
> @@ -381,6 +532,19 @@ static int vgic_v5_db_set_vcpu_affinity(struct irq_data *data, void *vcpu_info)
> return vgic_v5_irs_set_vm_valid(vm->vm_id);
> case VMTE_MAKE_INVALID:
> return vgic_v5_irs_set_vm_invalid(vm->vm_id);
> + case VPE_MAKE_VALID:
> + /*
> + * We need the actual LPI ID which lives in the top-most parent
> + * domain. This hwirq won't include the type (LPI) but that's
> + * not required for the IRS_VPE_DBR.
> + */
> + while (data->parent_data != NULL)
> + data = data->parent_data;
> + return vgic_v5_irs_set_up_vpe(vm->vm_id, vpe_id, data->hwirq);
> + case VPE_CR0_READ:
> + return vgic_v5_irs_vpe_cr0_read(vm->vm_id, vpe_id, &cmd_info->data);
> + case VPE_CR0_WRITE:
> + return vgic_v5_irs_vpe_cr0_update(vm->vm_id, vpe_id, cmd_info->data);
> case SPI_VIST_MAKE_VALID:
> return vgic_v5_irs_set_vist_valid(vm->vm_id, true);
> case LPI_VIST_MAKE_VALID:
> diff --git a/include/linux/irqchip/arm-gic-v5.h b/include/linux/irqchip/arm-gic-v5.h
> index ff5ad653252d2..54b573783cd75 100644
> --- a/include/linux/irqchip/arm-gic-v5.h
> +++ b/include/linux/irqchip/arm-gic-v5.h
> @@ -90,9 +90,14 @@
> #define GICV5_IRS_VMT_BASER 0x0200
> #define GICV5_IRS_VMT_CFGR 0x0210
> #define GICV5_IRS_VMT_STATUSR 0x0214
> +#define GICV5_IRS_VPE_SELR 0x0240
> +#define GICV5_IRS_VPE_DBR 0x0248
> +#define GICV5_IRS_VPE_CR0 0x0258
> +#define GICV5_IRS_VPE_STATUSR 0x025c
> #define GICV5_IRS_VMAP_L2_VMTR 0x02c0
> #define GICV5_IRS_VMAP_VMR 0x02c8
> #define GICV5_IRS_VMAP_VISTR 0x02d0
> +#define GICV5_IRS_VMAP_VPER 0x02e0
>
> #define GICV5_IRS_IDR0_VIRT BIT(6)
>
> @@ -199,6 +204,21 @@
>
> #define GICV5_IRS_VMT_STATUSR_IDLE BIT(0)
>
> +#define GICV5_IRS_VPE_SELR_S BIT_ULL(63)
> +#define GICV5_IRS_VPE_SELR_VPE_ID GENMASK_ULL(47, 32)
> +#define GICV5_IRS_VPE_SELR_VM_ID GENMASK_ULL(15, 0)
> +
> +#define GICV5_IRS_VPE_DBR_DBV BIT_ULL(63)
> +#define GICV5_IRS_VPE_DBR_REQ_DB BIT_ULL(62)
> +#define GICV5_IRS_VPE_DBR_DBPM GENMASK_ULL(36, 32)
> +#define GICV5_IRS_VPE_DBR_LPI_ID GENMASK_ULL(23, 0)
> +
> +#define GICV5_IRS_VPE_CR0_DPS BIT(0)
> +
> +#define GICV5_IRS_VPE_STATUSR_F BIT(2)
> +#define GICV5_IRS_VPE_STATUSR_V BIT(1)
> +#define GICV5_IRS_VPE_STATUSR_IDLE BIT(0)
> +
> #define GICV5_IRS_VMAP_L2_VMTR_M BIT_ULL(63)
> #define GICV5_IRS_VMAP_L2_VMTR_VM_ID GENMASK_ULL(15, 0)
>
> @@ -211,6 +231,10 @@
> #define GICV5_IRS_VMAP_VISTR_VM_ID GENMASK_ULL(47, 32)
> #define GICV5_IRS_VMAP_VISTR_TYPE GENMASK_ULL(31, 29)
>
> +#define GICV5_IRS_VMAP_VPER_M BIT_ULL(63)
> +#define GICV5_IRS_VMAP_VPER_VM_ID GENMASK_ULL(47, 32)
> +#define GICV5_IRS_VMAP_VPER_VPE_ID GENMASK_ULL(15, 0)
> +
> #define GICV5_ISTL1E_VALID BIT_ULL(0)
> #define GICV5_IRS_ISTL1E_SIZE 8UL
>
> @@ -480,6 +504,9 @@ enum gicv5_vcpu_info_cmd_type {
> VMT_L2_MAP, /* Map in a L2 VMT - *may* happen on VM init */
> VMTE_MAKE_VALID, /* Make the VMTE valid */
> VMTE_MAKE_INVALID, /* Make the VMTE (et al.) invalid */
> + VPE_MAKE_VALID, /* No corresponding invalid */
> + VPE_CR0_READ, /* Read of VPE_CR0 (guest read from PE_CR0) */
> + VPE_CR0_WRITE, /* Write to VPE_CR0 (guest write to PE_CR0) */
> SPI_VIST_MAKE_VALID, /* No corresponding invalid */
> LPI_VIST_MAKE_VALID, /* Triggered by a guest */
> LPI_VIST_MAKE_INVALID, /* Triggered by a guest */

Thanks,

M.

--
Without deviation from the norm, progress is not possible.