GICv3 register context is not saved during system suspend

[soby-mathew]

Today neither the kernel nor the firmware saves and restores GICv3 distributor context during system suspend. This was not a problem on Juno because it was GICv2 and the kernel caters for the save and restore of Distributor because the assumptions of GICv2 systems are different from those with GICv3. A firmware is expected to be always present on GICv3 systems.

This leaves us with 2 choices , save/restore GICv3 Distributor context in Kernel or firmware. The problems of implementing this in kernel are:

1. The kernel is not aware of power domain of GIC and cannot determine when the GIC will lose its context. So, if implemented in kernel, it has to save register context for every suspend.
2. The GIC ITS configuration is a one-way operation and there is no generic way to save and restore this.

If the implementation is in firmware, since the firmware is aware of power domains, it can save and restore GICv3 when required. If system suspend might result in loss of ITS information, then firmware can choose to downgrade SYSTEM domain power down to a shallower power down state which does not result in loss of power to GIC or even deny SYSTEM suspend.

These problems tilt the argument in favour of implementation in firmware. The primary concern of doing this in firmware was the memory cost for save and restoring several GIC Distributor registers. This can be overcome by allowing the platform to specify the memory for the save and restore (Possibly in a secure memory carve-out in DDR).

[nmenon]

@soby-mathew storing in secure memory carve out opens us up for a physical attack? Should'nt we put in hooks such that OPTEE or some other OS will provide encryption services prior to store into DDR?

[soby-mathew]

Hi Nishanth,
Yes, that is a possibility. The memory region to save and restore GIC context is a choice left to the platform. So if there is a platform with enough Secure SRAM to save GIC context, it can do so in the same.

Perhaps, within ARM Trusted Firmware, we can provide GICv3 driver helpers to save and restore secure and non-secure context separately. Thus a platform can choose to save secure GICv3 context in Secure SRAM and the non-secure context in secure DDR carve-out. This reduces the security risk in case of a physical attack.

[nmenon]

@soby-mathew instead of a piecemeal approach, maybe a generic api needs to be present for allocating out of context save and restore region and gic code allocate, save and restore from the same perhaps?

[soby-mathew]

OK, I may not have understood your suggestion completely. As we don't support memory allocation at run-time, an API to allocate memory at runtime may not be possible.

What is possible though is to define a platform API to get the region statically allocated for this purpose
Or,
Provide more framework support for platforms to easily allocate this region (like, have linker symbols defined by platforms and then referred to by the GICv3 driver to save and restore).
Need more thoughts on this.

If you have any ideas for the same, we are happy to hear them.

[nmenon]

Hmm.. I was thinking of a simpler requestor mechanism at boot similar to linux kernel's genalloc framework - something that linux kernel's sram driver already uses -> it just needs a section to be defined, and allocation happens on a need basis from the section, it does'nt need to continue providing functionality after boot is done (but that is something that is easily supported).
Benefit:
a) we dont need to break our heads for various memory allocation schemes needed for multiple subsystems that may need context save and restore
b) we might be able to plug in hooks for the allocator framework to add centralized obfuscator or encryption plug instead of having to do it per driver hook.. the region needs to be saved and restored, contents are equally risky for physical attack..

[soby-mathew]

OK, now I understand your piecemeal comment :) . Although I don't agree with the dynamic nature of allocation, I like the concept. If we have the need to save and restore several devices then a more generic way to allocate memory for the same would make sense. The current understanding is that kernel will save the context of devices it cares about and the same applies to Secure OS as well. Currently we think GICv3 is a one-off case and we don't think this is going to be the norm.

we might be able to plug in hooks for the allocator framework to add centralized obfuscator or encryption plug instead of having to do it per driver hook.. the region needs to be saved and restored, contents are equally risky for physical attack.

The support we are planning to add will also ensure that this can be done by platform if it desires. But this is a piecemeal solution as you said. If we can justify other usecases with the same requirement, perhaps we can think of a framework along these lines.

[nmenon]

@soby-mathew how do we manage tlbs for system mmu for example? arm side, we can rebuild, but with components in SoC that ATF is supposed to manage, dont we need to save and restore those configs?

[soby-mathew]

From my understanding, the SMMU, unlike GICv3, does not have any registers that are one-way operation. Hence the kernel can save and restore setting it cares about and similarly any secure side settings can be restored/re-initialized by Secure world. All of the save and restore need not be in firmware.

Perhaps, a framework to use pre-allocated region in firmware would be useful I think. Need more thoughts on this.

[soby-mathew]

Internal ref : https://jira.arm.com/browse/GENFW-1800

[dbasehore]

Is there any progress on this? I'm looking to add the general functionality to restore the gicv3 state on resume. I'm looking to put this in ATF. I was going to have platforms that lose state for the gicv3 recall the init functions for the gicd and gicr. Then any registers that can be changed would be restored after that.

For the allocated memory problem, I was going to rely on --gc-sections to remove the functions/data from the ATF binary for the platforms that don't use it. I would also leave the memory allocation up to the platform (so it could decide on whether to put it in Secure SRAM or somewhere else).

How is the GIC ITS unable to be restored in a general manner? Is there something that needs to be done besides storing the writable registers in the GIC Distributor space for it?

[soby-mathew]

@dbasehore, Although we had some initial design discussions regarding this, I did not have time to progress the implementation due to other priorities. This is planned for the next month.

Yes, your suggestion for doing init and then restoring the register context sounds correct to me.

The ITS tables are configured via ITS commands. My knowledge of ITS is also very weak, but as I understand, the specification says that "details of power management of ITS are implementation defined". For example, when GITS_BASER is restored, if the address pointed to by the register is non - zero (because the tables are already present in memory), the behaviour is unpredictable. There are other nuances in the spec which prevents the generic save/restore of ITS.

[dbasehore]

@soby-mathew Is there a plan to deal with restoring ITS then? I have it working on the RK3399 with just a register restore. For a general implementation, will the platform specific code have to do this, or can the general code restore the registers and the platform code can do whatever it needs to do after that to get it working?

[soby-mathew]

The current plan is not to handle ITS restore in generic driver.

I have it working on the RK3399 with just a register restore

Yes, this might just work for most platforms. But since the spec says this behaviour is implementation defined, the current plan is to leave this choice to the platform.

For a general implementation, will the platform specific code have to do this, or can the general code restore the registers and the platform code can do whatever it needs to do after that to get it working?

The idea was to introduce helper functions to save and restore GICv3 registers and these helpers will get invoked by the appropriate platform hooks during power management. The GICv3 driver by itself will not do anything unless invoked by the platform.

These are the proposed APIs for the GICv3 driver :

int gicv3_save_dist_regs(gicv3_dist_ctx_t* mem);
int gicv3_restore_dist_regs(gicv3_dist_ctx_t* mem);
int gicv3_save_redist_regs(int cpu_id, gicv3_redist_ctx_t* mem);
int gicv3_restore_redist_regs(int cpu_id, gicv3_dist_ctx_t* mem);

typedef struct gicv3_dist_ctx {
int regs[NUM_OF_DIST_REGS];
} gicv3_dist_ctx_t;

typedef struct gicv3_redist_ctx {
int regs[NUM_OF_REDIST_REGS];
} gicv3_redist_ctx_t;

So during SYSTEM suspend, the psci_plat_pm_ops->pwr_domain_suspend will invoke gicv3_save_dist_regs and gicv3_save_redist_regs. The memory for gicv3_dist_ctx_t and gicv3_redist_ctx_t needs to be allocated in a platform specific way. The ITS registers will not be saved in these API. (Perhaps if it useful for most platforms, we can have a build config to include ITS registers and this config will be turned OFF by default).

Similarly, the counterpart APIs will be invoked after GICv3 init during power up.

If you have something working, it would be very helpful if you can share your work through a branch or Pull request and that will help to expedite this task.

[dbasehore]

There's just code specific to the RK3399 right now. I'm working to make it generic, though.

[dbasehore]

Okay, it seems that the map collection ITS commands need to be resent on RK3399 after resume if the GIC loses power. I know that the preference was to implement this in ATF, but it's really simple to just have the drivers/irqchip/irq-gic-v3-its.c code call its its_cpu_init_collection function during syscore resume in just several lines of code. It seems that implementing this in the firmware would require significantly more work than that.

Is this the correct thing to do on all GICv3 platforms? Provided that there's not an issue with systems that don't cut power to the GIC, is anyone against having the GIC driver in the kernel restore this part of the GIC state?

[soby-mathew]

Yes, I agree it is not possible to replay GIC ITS commands from ARM TF for ITS restore. The ITS helpers in ARM TF are provided based on the assumption that for platforms which have ITS structures in memory, then the restore of the ITS registers is sufficient to resume ITS. If the assumption is not valid for most platforms and ITS restore requires ITS commands to be replayed, then kernel is best placed for this operation.

Is this the correct thing to do on all GICv3 platforms?

There were some concerns from the kernel team regarding this. Those concerns would need to be addressed if it is to be done generically for GICv3 platforms.

Provided that there's not an issue with systems that don't cut power to the GIC, is anyone against having the GIC driver in the kernel restore this part of the GIC state?

IMO, if this is the case, then it is good to be specified in appropriate documentation (if possible in PSCI Spec). That will ensure all the involved stake holders are having the same expectations during system suspend. But first this would need agreement that it is right way for all GICv3 platforms.

There were some internal discussions which I will try to revive. Are the corresponding linux patches posted to the lists ?

[dbasehore]

No, but I can post an RFC patch to get a discussion going if you want.

Are there any specific concerns from the kernel team at ARM regarding this?

[soby-mathew]

There were some concerns around whether the LPI pendings bits would be retained when ITS is reinitialized and it might involve implementation specifc sequences which the kernel team were not keen on adding. Not sure if there were more concerns but I guess these will have to ironed out through discussions.

[dbasehore]

Would replaying the MAPC commands affect the LPI pending bits? I'm just calling the its_cpu_init_collection function which doesn't reallocate the LPIs.

[soby-mathew]

There is some doubt whether MAPI/MAPTI are allowed to clear a pending bit which means the kernel would need to take a copy of the pending table on resume in order to inject an INT command for each pending bit. That's all my understanding about this.

[dbasehore]

Is there any update on what the kernel team thought was acceptable for resending the MAPC commands?

[soby-mathew]

Perhaps there have been discussions, but I haven't had any updates. I can try to get the status of this sometime next week.

[soby-mathew]

@dbasehore, sorry for the delay, but many people were away due to conferences. I have had a prelimanary discussion but no conclusion yet. Hope to get some clarity on this during this week.

[dbasehore]

On a related note, is there an ETA for when the Trusted Firmware patches for this will land?

[soby-mathew]

We hope to have this merged by end of this week.

[soby-mathew]

Having MAPC commands to restore the collection tables within kernel is probably alright but this would be a specific to the GIC implementation. This implementation specific ITS restore could be enabled for GIC-500 within the kernel, but kernel team would require a confirmation from the hardware guys before they accept any patches in this direction. Hope for an update soon.

I have updated the PR to leave the GITS_CTLR.Enabled to 0 within the ITS resume helper. This could be used to indicate to the kernel whether GIC ITS needs restore or not. This needs to be specified properly somewhere.