Re: [PATCH v2 06/13] KVM: arm64: Shadow the ITS command queue and setup emulation

[Fuad Tabba]

Hi Seb,

The approach looks right to me. A few things I'd fix, though:

On Fri, Aug 07, 2026 at 04:43:16PM +0000, Sebastian Ene wrote:

[...]

> Co-authored-by: Bartłomiej Grzesik <bgrzesik@xxxxxxxxxx>
> Signed-off-by: Sebastian Ene <sebastianene@xxxxxxxxxx>

Could you switch this to Co-developed-by: and add Bartłomiej's
Signed-off-by after it?

[...]

> +void pkvm_its_emulate_handler(struct pkvm_protected_reg *region, u64 offset, bool write, u64 *reg,
> +                              u8 reg_size)
> +{
[...]
> +        for (reg_handler = its_handlers; reg_handler->access_size; reg_handler++) {
> +                if (reg_handler->offset > offset ||
> +                    reg_handler->offset + reg_handler->access_size <= offset)
> +                        continue;
> +
> +                if (reg_handler->access_size < reg_size)
> +                        return;
> +
> +                if (write && reg_handler->write) {
> +                        hyp_spin_lock(&priv->its_lock);
> +                        reg_handler->write(region, offset, *reg);

The match could require the offset and size to be exact. The handlers
ignore offset, so a 4-byte access at GITS_CWRITER + 4 matches this
entry and runs as a full 64-bit one. (Sashiko)

> +static int pkvm_setup_its_shadow_cmdq(struct its_host_state *host_state)
> +{
> +        u64 start_pfn, num_pages, i;
[...]
> +        start_pfn = hyp_virt_to_pfn(host_state->cmd_host_copy);
> +        num_pages = host_state->cmdq_len >> PAGE_SHIFT;
[...]
> +        ret = __pkvm_host_donate_hyp(hyp_virt_to_pfn(host_state->cmd_original), num_pages);

Should setup take the base and size from GITS_CBASER and reject a
mismatch? Nothing ties cmd_original or cmdq_len to it, so a wrong base
leaves the ITS on the real queue the host still writes, and a short
cmdq_len runs the cwriter_write() memcpy past the donated pages.
(Sashiko)

> +unshare_cmd_host:
> +        if (i == 0)
> +                return ret;
> +
> +        for (i = i - 1; i >= 0; i--)
> +                __pkvm_host_unshare_hyp(start_pfn + i);
> +        return ret;

while (i--) would do it. i is a u64, so i >= 0 always holds and the
loop wraps at zero instead of ending. (Sashiko)

> +        priv_state->base = (void __iomem *)__hyp_va(dev_addr);
> +        priv_state->cmd_original = host_state->cmd_original;
> +        priv_state->cmd_host_copy = host_state->cmd_host_copy;
[...]
> +        its_reg->priv = priv_state;

An smp_store_release() here would order those writes and the memset
above against the region->priv read pkvm_its_emulate_handler() does
locklessly on other CPUs.

> +        ret = __pkvm_host_donate_hyp(hyp_virt_to_pfn(priv), priv_num_pages);
[...]
> +err_with_priv:
> +        WARN_ON(__pkvm_hyp_donate_host(hyp_virt_to_pfn(priv_state), 1));

priv_num_pages should come back here, not one, or the rest stay
hyp-owned. (Sashiko)

Cheers,
/fuad