[PATCH v2 05/13] irqchip/gic-v3-its: Add support for the ITS emulation setup
Fuad Tabba
fuad.tabba at linux.dev
Tue Sep 15 06:12:45 PDT 2026
Hi Seb,
On Fri, 7 Aug 2026 at 17:43, Sebastian Ene <sebastianene at google.com> wrote:
>
> Introduce two new helper functions to allow locking the ITS and setting
> up a copy of the host ITS state that will be given to the pKVM
> emulation. The caller of these functions is responsible to implement a
> callback which will be used to setup the emulation layer. The calling
> flow is expected to do the following:
>
> pkvm_its_emulate_setup(its_phys, host)
> // allocate memory for the priv state of the ITS emulation
> // call the its emulation setup(its_phys, host, priv_state);
>
> pkvm_drop_host_privileges()
> its_emulate_acquire_locks(&flags);
> on_each_cpu(_kvm_host_prot_finalize, &ret, 1);
> its_emulate_release_locks(ret, &flags, pkvm_its_emulate_setup);
>
> Augment the its_baser structure with a new fiels that will hold a
> pointer to the base table copy. The gic ITS driver will use the pointer
> to the base table copy when emulation is enabled, as this allows us to
> hide away the original first level of an indirect table to prevent the
> following:
>
> // assumming an indirect Device Table layout
> 1. malicious host patches an entry in the 1st level table with an
> address that it wants to write to.
> 2. malicious host issues MAPD to install a DTE in the table pointed by
> the address from (1).
>
> As the driver only manipulates a copy of the table, the emulation is
> responsible for looking at the updates from the copy table, sanitizing
> them and updating the original table before talking to the hardware.
>
> In a simillar fashion, when emulation is in place we no longer let the
> gic ITS driver use the original command queue but we present the driver
> a copy of it and we hide away the original command queue from the driver
> as this will be used entirely by the emulation layer.
>
> Co-authored-by: Bartłomiej Grzesik <bgrzesik at google.com>
> Signed-off-by: Sebastian Ene <sebastianene at google.com>
A few typos, "fiels" and "simillar" here, "innaccessible" and
"assumming" further down.
Missing SoB.
> ---
> drivers/irqchip/irq-gic-v3-its.c | 157 +++++++++++++++++++++++++++--
> include/linux/irqchip/arm-gic-v3.h | 39 +++++++
> 2 files changed, 185 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c
> index 6f5811aae59c..e74ae9220af5 100644
> --- a/drivers/irqchip/irq-gic-v3-its.c
> +++ b/drivers/irqchip/irq-gic-v3-its.c
> @@ -78,17 +78,6 @@ struct its_collection {
> u16 col_id;
> };
>
> -/*
> - * The ITS_BASER structure - contains memory information, cached
> - * value of BASER register configuration and ITS page size.
> - */
> -struct its_baser {
> - void *base;
> - u64 val;
> - u32 order;
> - u32 psz;
> -};
> -
> struct its_device;
>
> /*
> @@ -5226,6 +5215,152 @@ static int __init its_compute_its_list_map(struct its_node *its)
> return its_number;
> }
>
> +static void its_free_snapshot(struct its_host_state *snapshot)
> +{
> + int i;
> +
> + if (snapshot->cmd_host_copy)
> + its_free_pages(snapshot->cmd_host_copy, get_order(ITS_CMD_QUEUE_SZ));
> +
> + for (i = 0; i < GITS_BASER_NR_REGS; i++) {
> + if (!snapshot->tables[i].base_snapshot)
> + continue;
> +
> + its_free_pages(snapshot->tables[i].base_snapshot, snapshot->tables[i].order);
> + }
> +
> + its_free_pages(snapshot, 0);
> +}
> +
> +static struct its_host_state *its_snapshot_host_state(struct its_node *its)
> +{
> + void *page;
> + struct its_host_state *snapshot;
> + int i;
> +
> + page = its_alloc_pages_node(its->numa_node, GFP_ATOMIC | __GFP_ZERO, 0);
> + if (!page)
> + return NULL;
> +
> + snapshot = (void *)page_address(page);
> + page = its_alloc_pages_node(its->numa_node, GFP_ATOMIC | __GFP_ZERO,
> + get_order(ITS_CMD_QUEUE_SZ));
Once its_emulate_release_locks() has a caller, all of these run under
its_lock and every its->lock with interrupts off. Could the buffers be
allocated before the locks are taken, leaving only the copies under
them? (Sashiko is trying to say the same thing I think)
...
> +static int its_emulate_switch_queues_locked(struct its_node *its, its_emulate_setup cb)
> +{
...
> + baser_phys = virt_to_phys(host.tables[i].base_snapshot);
> + if (IS_ENABLED(CONFIG_ARM64_64K_PAGES) && (baser_phys >> 48))
> + baser_phys = GITS_BASER_PHYS_52_to_48(baser_phys);
its_setup_baser() has a third condition here: it fails unless psz is
SZ_64K, since 52-bit PA needs a 64K page size. This converts at any
page size, and psz is in host.tables[i].psz. Could that check come
too?
Also "host_snaphsot" in this function.
> +
> + its->tables[i].val &= ~GENMASK(47, 12);
> + its->tables[i].val |= baser_phys;
> + its->tables[i].base = host.tables[i].base_snapshot;
> + }
> +
> + return 0;
> +}
> +
> +void its_emulate_acquire_locks(unsigned long *flags)
> +{
> + struct its_node *its;
> +
> + if (WARN_ON(!flags))
> + return;
> +
> + raw_spin_lock_irqsave(&its_lock, *flags);
> +
> + list_for_each_entry(its, &its_nodes, entry)
> + raw_spin_lock(&its->lock);
pkvm_drop_host_privileges() runs on_each_cpu() inside this section, so
the IPI goes out with interrupts off and lockdep_assert_irqs_enabled()
in smp_call_function_many_cond() fires on a PROVE_LOCKING build.
Moving the locks isn't free, though: priv is only set by the setup
hypercall after the IPI, and the handler drops every ITS access until
then. Would running the setup before the stage-2 install work, or
forwarding on a NULL priv?
> +}
> +
> +int its_emulate_release_locks(int ret_pkvm_finalize, unsigned long *flags, its_emulate_setup cb)
> +{
> + struct its_node *its;
> + int ret = 0;
> +
> + if (WARN_ON(!flags || !cb))
> + ret = -EINVAL;
The acquire returns with no locks held, and the release then unlocks
them anyway and reads *flags. The only caller passes &its_flags, so
could both go?
> +
> + list_for_each_entry(its, &its_nodes, entry) {
> + if (!ret_pkvm_finalize && !ret)
> + ret = its_emulate_switch_queues_locked(its, cb);
> +
> + raw_spin_unlock(&its->lock);
> + }
> +
> + raw_spin_unlock_irqrestore(&its_lock, *flags);
> +
> + return ret;
> +}
> +
> static int __init its_probe_one(struct its_node *its)
> {
> u64 baser, tmp;
> diff --git a/include/linux/irqchip/arm-gic-v3.h b/include/linux/irqchip/arm-gic-v3.h
> index ea5fd2374ebe..b75f82cef4bf 100644
> --- a/include/linux/irqchip/arm-gic-v3.h
> +++ b/include/linux/irqchip/arm-gic-v3.h
> @@ -657,6 +657,45 @@ static inline bool gic_enable_sre(void)
> return !!(val & ICC_SRE_EL1_SRE);
> }
>
> +/*
> + * The ITS_BASER structure - contains memory information, cached
> + * value of BASER register configuration and ITS page size.
> + */
> +struct its_baser {
> + void *base;
> +
> + /*
> + * The table used when emulation is in place and indirect layout is
> + * configured.
> + */
> + void *base_snapshot;
> + u64 val;
> + u32 order;
> + u32 psz;
> +};
> +
> +struct its_host_state {
> + struct its_baser tables[GITS_BASER_NR_REGS];
> +
> + /* The command queue used after the emulation is in place */
> + void *cmd_host_copy;
> +
> + /* The command queue configured by the ITS driver at boot */
> + void *cmd_original;
> + void *cmd_write;
> + size_t cmdq_len;
> +};
struct its_baser was private to irq-gic-v3-its.c. Would it be better
for these to have their own header?
Cheers,
/fuad
> +/*
> + * Callback used to initialize the emulation. It is expected to allocate memory for the private
> + * state of the emulation and receive as arguments copy of the host ITS driver state along
> + * with the address of the ITS.
> + */
> +typedef int (*its_emulate_setup)(phys_addr_t its_phys_base, struct its_host_state *host);
> +
> +void its_emulate_acquire_locks(unsigned long *flags);
> +int its_emulate_release_locks(int ret_pkvm_finalize, unsigned long *flags, its_emulate_setup cb);
> +
> #endif
>
> #endif
> --
> 2.55.0.654.g21b8a5bc05-goog
>
More information about the linux-arm-kernel
mailing list