Re: [PATCH v2 04/13] KVM: Parse the device tree and register the ITS region with pKVM

[Fuad Tabba]

Hi Seb,

On Fri, 7 Aug 2026 at 17:43, Sebastian Ene <sebastianene@xxxxxxxxxx> wrote:
...
> +static int __init register_its_emulated_region(void)
> +{
> +       struct device_node *np;
> +       struct resource res;
> +       int i = 0;
> +       int ret;
> +
> +       for_each_compatible_node(np, NULL, "arm,gic-v3-its") {
> +               ret = of_address_to_resource(np, 0, &res);
> +               if (ret)
> +                       goto out_fail;

its_of_probe() also requires of_device_is_available() and
msi-controller, and its_init() goes to its_acpi_probe() when the
fwnode isn't an OF node.

So a disabled node has its region taken from the host here, and on
ACPI nothing matches at all: num_protected_reg stays 0, this returns
0, and pKVM finalises with the ITS unmediated. The two DT checks are
plain property queries, so they work this early. What's the intent for
ACPI?

> +
> +               if (i >= PKVM_PROTECTED_REGS_NUM) {
> +                       kvm_err("Out of protected region slots\n");
> +                       ret = -ENOSPC;
> +                       goto out_fail;
> +               }

Tested after of_address_to_resource(), so a bad resource on the ninth
node comes back as that error rather than -ENOSPC. Could the bound
come first?

> +               /*
> +                * Note: don't unmap the entire animal from the host because devices need
> +                * to be able to access GITS_TRANSLATER to raise MSIs. If the
> +                * page where GITS_TRANSLATER is given to HYP, devices won't be
> +                * able to map it in their IOMMU when the IOMMU is managed by
> +                * pKVM.
> +                */
> +               kvm_nvhe_sym(pkvm_protected_regs)[i].pfn = PHYS_PFN(res.start);
> +               kvm_nvhe_sym(pkvm_protected_regs)[i].cb =
> +                       lm_alias(&kvm_nvhe_sym(its_emulate_forward_req));
> +               kvm_nvhe_sym(pkvm_protected_regs)[i].nr_pages =
> +                       PFN_DOWN(min_t(u64, resource_size(&res), PAGE_ALIGN_DOWN(GITS_TRANSLATER)));

I don't think the ITS likes to be called an animal :P, and the next
sentence is missing a word after "If the page where GITS_TRANSLATER
is".

nr_pages is 0 for a resource smaller than a page. Could that be rejected?

> +
> +               i++;
> +       }
> +
> +       kvm_nvhe_sym(num_protected_reg) = i;
> +       return 0;
> +out_fail:

num_protected_reg is set on the success path only, so out_fail leaves
it disagreeing with the entries already written. Inert today, but
could the failure path zero it?

Cheers,
/fuad

> +       of_node_put(np);
> +       return ret;
> +}
> +
>  void __init kvm_hyp_reserve(void)
>  {
>         u64 hyp_mem_pages = 0;
> @@ -57,6 +103,12 @@ void __init kvm_hyp_reserve(void)
>                 return;
>         }
>
> +       ret = register_its_emulated_region();
> +       if (ret) {
> +               kvm_err("Failed to register ITS region %d\n", ret);
> +               return;
> +       }
> +
>         hyp_mem_pages += hyp_s1_pgtable_pages();
>         hyp_mem_pages += host_s2_pgtable_pages();
>         hyp_mem_pages += hyp_vm_table_pages();
> --
> 2.55.0.654.g21b8a5bc05-goog
>