Re: [PATCH] hw/intc/arm_gicv3_kvm: Avoid reading ICC_CTLR_EL1 from kernel in cpuif reset

[Salil Mehta <salil.mehta@opnsrc.net>]

Hi Peter,

On Thu, Oct 16, 2025 at 12:46 PM Peter Maydell <peter.maydell@linaro.org> wrote:
>
> On Thu, 16 Oct 2025 at 12:13, Salil Mehta <salil.mehta@huawei.com> wrote:
> >
> > Hi Peter,
> > > > Above changes assume that the driver's configured value of the
> > > > ICC_CTLR_EL1 system register is the same as the default value. I've
> > > > verified that this currently the case. However, it safe to assume that
> > > > this will remain true in the future as well?
> > >
> > > I don't understand what you mean here. We read the kernel's ICC_CTLR_EL1
> > > at VM startup, when we know it will be the reset value, because we haven't
> > > run any VCPUs yet.
> >
> >
> > System register fetches its value from ICH_VMCR_EL2 and ICH_VTR_EL2.
> > In specific, EOIMode, PMHE and CBPR fields of ICC_CTLR_EL1 are from
> > the VMCR register. Later gets configured when driver gets loaded and again
> > re-configured in context to each CPU ON request(via in-kernel  CPU Hotplug
> > state machine; CPUHP_AP_IRQ_GIC_STARTING). This configures the VMCR
> > again and again. Although, the value as of now is same.
> >
> > You might want to check gic_cpu_sys_reg_init() in irq-gic-v3.c
>
> I'm afraid I still don't understand what you mean here. This is
> all the guest writing to the GIC registers as it starts up, right?
> That has no influence at all on what the reset value of the emulated
> hardware should be. (This is the same as on real hardware:
> it doesn't matter what the OS writes to registers when it is
> running; when the hardware resets it resets to the reset value.)

For context, the gic_cpu_init() function is invoked from two paths in
the kernel: first from gic_init_bases() when the GICv3 driver is
initially loaded on the boot CPU, and later from gic_starting_cpu()
during each CPU online transition in the hotplug state machine.

The hotplug path wires up

CPUHP_AP_IRQ_GIC_STARTING -> gic_starting_cpu

in gic_smp_init(). On every CPU online event this leads to:

gic_starting_cpu() -> gic_cpu_init() -> gic_cpu_sys_reg_init()

which reprograms the CPU-interface system registers on that CPU,
including ICC_CTLR_EL1 (fields EOIMode, PMHE, CBPR).

The following dump stack from a guest hotplug event shows this
sequence clearly:

echo 1 > /sys/devices/system/cpu/cpu1/online
[   39.287402] gic_cpu_sys_reg_init+0x4c/0x294
[   39.287406] gic_cpu_init.part.0+0xc0/0x114
[   39.287409] gic_starting_cpu+0x24/0x8c
[   39.287412] cpuhp_invoke_callback+0x104/0x20c
[   39.287419] notify_cpu_starting+0x80/0xac
[   39.287421] secondary_start_kernel+0xdc/0x15c


As a result, ICC_CTLR_EL1 is at its architectural reset value at
VM realize (before any vCPU runs), but it is guest-configured
after the driver runs and again on each later CPU online event.

Although it looks safe in practice from the way EOIMode, CBPR,
and PMHE fields are currently programmed by the guest, the
eventual value always depends on what the guest writes into
VMCR each time it configures the CPU interface during a
CPU_ON sequence. Given that, would it still be correct for
QEMU to treat the realize-time ICC_CTLR_EL1 value as a
persistent runtime value rather than just a reset template?

Since cpu_reset() will also be invoked for every CPU_ON in QEMU,
is there even a remote possibility that a guest GIC driver (Linux or
otherwise) might alter these field configurations across successive
CPU_ON/OFF cycles?

>
> We want to know what value the kernel gives to this register when
> the GIC is in a freshly reset state when no guest code has run.
> That should be the value that it has here, when we read it on realize.

But when PSCI CPU_ON is being handled entirely in the kernel
i.e. no hotplug patches are present and hence no SMCC Hypercall
exit exists then we will always use driver updated VMCR fields (I think).
In the current case even along with the hotplug patches they are the
same but in *future* will they be?

I do understand what you and Marc discussed and pointed out that,
"The reset value is indeed cast in stone once the GIC has been created."
https://lore.kernel.org/lkml/864is2x6z9.wl-maz@kernel.org/


Best regards
Salil.