Re: [PATCH] irqchip/gic-v3-its: Reconfigure ITS from software state on resume

[Bjoern Doebel]

Hi Marc,

thanks for your response and apologies for my delay (I'm paying for this
by having to page all this back into my head).

> >   - I reproduced the original failure on *stock* v7.2-rc1. On EC2
> >     Graviton instances, hibernation resume fails 100% of the time: the
> >     ITS comes back reset, MAPD/MAPTI are never replayed, and the ENA
> >     NIC silently loses its LPIs:
> > 
> >       ena 0000:00:05.0: ... didn't receive a MSI-X interrupt (cmd 3)
> >       ena 0000:00:05.0: Failed to create IO CQ. error: -62
> >
> 
> But how did the resumed kernel get there the first place? Surely you
> had interrupts to load it, right?

Pre-hibernate we had a correctly configured guest (VM-1) on a KVM host,
including properly setup ITS etc. Then we hibernated this VM, that is
the guest wrote all its memory content into a snapshot and powered down.
While this contains some guest-side GIC state, it does not contain the
host-side virtual GIC state KVM stores.

Now we resume, that is we launch a new VM (VM-2), potentially on a
different host. This VM configures the underlying vGIC and has working
devices. At some point it then loads the hibernated memory image and
jumps into the resumed kernel. At this point, we are running a VM
configured by VM-2, but we just restored the in-guest ITS state from
VM-1. After this, the network driver no longer receives interrupts from
the device because it relies on mappings VM-1 established while the 2nd
vGIC was never told about them.

The solution to this problem is to replay VM-1's MAPD, MAPC and MAPTI
commands, which re-programs the 2nd vGIC with the mappings VM-1 had
before.

> >     The instance then has no networking after resume.
> > 
> >   - With this patch applied, the same kernel survives hibernate/resume
> >     cleanly: 9/9 cycles with zero failures, across all three Graviton
> >     generations (Graviton 2/3/4, i.e. Neoverse N1/V1/V2), networking
> >     fully restored on every resume.
> > 
> > As described in the previous message, this is the fallout from 713335b6ee29
> > ("irqchip/gic-v3-its: Implement .msi_teardown() callback"): device
> > teardown no longer happens across a suspend/resume that keeps the MSI
> > domain, so the ITS is never reprogrammed and drops interrupts after the
> > hardware has been reset.
> > 
> > Could you take a look when you get a chance?
> 
> What I don't immediately see is how this particular patch influences
> anything, given that at this point it isn't doing anything.
> 
> Beside that, how does hibernation actually influences LPIs being
> released? Can you at least describe the sequence of events?

Before 6.16 this worked "by accident". On suspend, we call ena_suspend()
-> ena_destroy_device() -> ena_disable_msix() -> pci_free_irq_vectors().
This ends up freeing all IRQ vectors and eventually calling
its_irq_domain_free(). This function releases the events from the event
map, finds the event map has run empty, and then tears the its_device
down with its_lpi_free() -> MAPD(V=0) -> its_free_device(). This happens
while devices are frozen, before the snapshot is taken. Hence, no
its_device for the NIC is stored within the hibernation snapshot.

On resume, its_msi_prepare() would then not find anything for that
DeviceID and therefore call its_create_device(). This allocates ITT and
LPI range, and issues MAPD. Eventually, we end up with VM-2's vGIC being
reconfigured from scratch.

6.16 changed the lifetime of the its_device. 713335b6ee29
("irqchip/gic-v3-its: Implement .msi_teardown() callback") moved the
its_device teardown out of its_irq_domain_free() into a new
.msi_teardown() hook, and 03c298760ed9 ("genirq/msi: Engage the
.msi_teardown() callback on domain removal") tied that hook to domain
removal. The empty event map went from being the condition that
triggered teardown to a WARN_ON_ONCE precondition. As 713335b6ee29
explains, coupling the its_device to the event count was wrong, since
drivers legitimately drop to zero MSIs and reallocate.

Our hibernation was relying on exactly that teardown. Suspend still
frees all vectors and empties the event map, but it does not remove the
MSI domain. .msi_teardown() never runs. The its_device survives with
an empty event map, and it therefore ends up in the snapshot. On resume
its_msi_prepare() finds it again via its_find_device(), marks it shared
and returns without issuing MAPD, so its_create_device() is never
reached. VM-2's vGIC is never told about the DeviceID. 

So before 6.16, the snapshot had no its_device and we got full
reconfiguration for free. Since 6.16 the snapshot has an its_device and
nothing reprograms the vGIC. All of this was never meant to support
hibernation. We should always have been restoring ITS state explicitly
on resume.

My patch reconstructs the ITS hardware state from the software state
restored with the image. That state is all present at syscore_resume()
time and does not depend on any device having resumed.
its_restore_enable() already performs the first three steps of the GICv3
ITS §5.6.1 enable sequence and stops short of the fourth, configuring
devices, collections and translations with ITS commands. So we walk
its_device_list and for each device issue MAPD(V=0), zero and flush the
ITT, then issue MAPD(V=1), since §5.3.10 makes MAPD(V=1) with a non-zero
ITT UNPREDICTABLE. Per-event MAPTI replay is deferred to
its_cpu_init_collection(), since an event's target collection has to be
MAPC'd before its MAPTI can be issued. All of it completes before
dpm_resume() runs the driver .resume callbacks, so the ITS is programmed
before any device raises an MSI.

> The other thing that worries me is that in general, (bare metal) GICv3
> is unable to deal with hibernation, given that you can't reprogram the
> property and pending table addresses. Yes, it *may* work if you are
> lucky enough that the boot kernel and the resumed one use the same
> memory regions, but this only show that you like playing Russian
> roulette.

Point taken. This patch is not trying to make hibernation work on bare
metal. It restores ITS command state only and does not touch
GICR_PROPBASER/GICR_PENDBASER, which cannot be re-pointed once
GICR_CTLR.EnableLPIs is set. If a redistributor lost power, replaying
ITS commands will not be enough. I am happy to state that limitation in
the commit message.

Best regards
Bjoern