Re: [PATCH v2 05/13] irqchip/gic-v3-its: Add support for the ITS emulation setup

[Fuad Tabba]

Hi Seb,

On Fri, 7 Aug 2026 at 17:43, Sebastian Ene <sebastianene@xxxxxxxxxx> 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@xxxxxxxxxx>
> Signed-off-by: Sebastian Ene <sebastianene@xxxxxxxxxx>

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
>