| Message ID | 56995068.9050207@ezchip.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Chris, On 15/01/16 20:02, Chris Metcalf wrote: > We are using GICv2 compatibility mode in the Fast Models/Foundation > Models simulations we are running because the boot code (ATF/UEFI) > doesn't support GICv3 in our system at the moment. > > However, starting with kernel 4.2, the guest couldn't boot up because it > wasn't getting timer interrupts. I tracked this down to a kernel commit > that switched to using the "alternatives" mechanism -- rather than > seeing either a GICv2 or GICv3 and configuring appropriately, the KVM > code just configured the code that saves/restores the vgic state based > on the presence of the system register interface to the GIC CPU > interface. See the attached patch for a fix that manages this > differently and allows me to boot up the guest in this configuration. > > However, even assuming this patch can be taken into an upstream tree, I > still have a couple of additional problems: > > - I can boot up with the Foundation Models using this change, but not > with the Fast Models (again, using a v3 GIC but in v2 compatibility mode > in the device tree). The Fast Models dts looks like it has the same > configuration for the GIC and the timers so I'm not sure what's going on > here. Any suggestions appreciated. > > - Without this change, I could only boot kernels up to 4.1. With the > change, I can boot kernels up to 4.3. But 4.4 won't boot for me either; > I haven't bisected it down yet. So any suggestions on what might be > going wrong here would also be appreciated. > > We are planning to eventually use GICv3 mode in our software stack but > for the time being I assume it is interesting to resolve issues with GIC > v2 compatibility mode on GIC v3. > I'm afraid that this is the wrong approach. Whilst 4.2 was a bit too eager to use GICv3 (only checking the CPU capability and ignoring the actual state of the EL2/EL3 SRE bits), the fact that 4.4 doesn't boot is probably the sign of a broken firmware that enables the system register interface at EL3, letting the rest of the software stack to use GICv3 in native mode, and yet providing a GICv2 DT. This combination is unpredictable, and is likely to cause issues on some HW implementations. Could you please point me to the firmware you're using? Also, please check the following patches: 6d32ab2 arm64: Update booting requirements for GICv3 in GICv2 mode 76e52dd irqchip/gic: Warn if GICv3 system registers are enabled 963fcd4 arm64: cpufeatures: Check ICC_EL1_SRE.SRE before enabling ARM64_HAS_SYSREG_GIC_CPUIF 7cabd00 irqchip/gic-v3: Make gic_enable_sre an inline function d271976 arm64: el2_setup: Make sure ICC_SRE_EL2.SRE sticks before using GICv3 sysregs Can you point me to the one that prevents you from booting? Thanks, M.
On 01/18/2016 04:28 AM, Marc Zyngier wrote: > Hi Chris, > > On 15/01/16 20:02, Chris Metcalf wrote: >> We are using GICv2 compatibility mode in the Fast Models/Foundation >> Models simulations we are running because the boot code (ATF/UEFI) >> doesn't support GICv3 in our system at the moment. >> >> However, starting with kernel 4.2, the guest couldn't boot up because it >> wasn't getting timer interrupts. I tracked this down to a kernel commit >> that switched to using the "alternatives" mechanism -- rather than >> seeing either a GICv2 or GICv3 and configuring appropriately, the KVM >> code just configured the code that saves/restores the vgic state based >> on the presence of the system register interface to the GIC CPU >> interface. See the attached patch for a fix that manages this >> differently and allows me to boot up the guest in this configuration. >> >> However, even assuming this patch can be taken into an upstream tree, I >> still have a couple of additional problems: >> >> - I can boot up with the Foundation Models using this change, but not >> with the Fast Models (again, using a v3 GIC but in v2 compatibility mode >> in the device tree). The Fast Models dts looks like it has the same >> configuration for the GIC and the timers so I'm not sure what's going on >> here. Any suggestions appreciated. >> >> - Without this change, I could only boot kernels up to 4.1. With the >> change, I can boot kernels up to 4.3. But 4.4 won't boot for me either; >> I haven't bisected it down yet. So any suggestions on what might be >> going wrong here would also be appreciated. >> >> We are planning to eventually use GICv3 mode in our software stack but >> for the time being I assume it is interesting to resolve issues with GIC >> v2 compatibility mode on GIC v3. >> > I'm afraid that this is the wrong approach. Whilst 4.2 was a bit too > eager to use GICv3 (only checking the CPU capability and ignoring the > actual state of the EL2/EL3 SRE bits), the fact that 4.4 doesn't boot is > probably the sign of a broken firmware that enables the system register > interface at EL3, letting the rest of the software stack to use GICv3 in > native mode, and yet providing a GICv2 DT. > > This combination is unpredictable, and is likely to cause issues on > some HW implementations. > > Could you please point me to the firmware you're using? > > Also, please check the following patches: > > 6d32ab2 arm64: Update booting requirements for GICv3 in GICv2 mode > 76e52dd irqchip/gic: Warn if GICv3 system registers are enabled > 963fcd4 arm64: cpufeatures: Check ICC_EL1_SRE.SRE before enabling > ARM64_HAS_SYSREG_GIC_CPUIF > 7cabd00 irqchip/gic-v3: Make gic_enable_sre an inline function > d271976 arm64: el2_setup: Make sure ICC_SRE_EL2.SRE sticks before using > GICv3 sysregs > > Can you point me to the one that prevents you from booting? The problematic commit is 963fcd4, because it calls gic_enable_sre() in the host kernel even with a GICv2 DT specified, and this seems to put things in a state such that we don't receive virtual timer interrupts in the guest when we boot it up. (I'm not that familiar with the QEMU DT but it is providing a GIC v2 to the guest.) With a v4.5-rc1 host, if I "return false" before the code in gic_enable_sre() that tries to actually enable the SRE, and then hardcode the __vgic_v2_XXX_state() save/restore calls into the __vgic_XXX_state() routines, then my guest boots up OK. We are using a modified ARM version of EDK v3.0-rc0, and a modified ARM Trusted Firmware based on commit 963fcd4 (between v1.1 and 1.2). We certainly haven't touched any of the GIC code in either one. I tried to modify the host DT to enable GICv3, but then the host itself hangs on boot, so clearly more is needed. (To be fair I've only tested v4.4 in that configuration, not v4.5-rc1.) The firmware isn't yet using GICv3 so perhaps that is part of the problem.
On 26/01/16 20:43, Chris Metcalf wrote: > On 01/18/2016 04:28 AM, Marc Zyngier wrote: >> Hi Chris, >> >> On 15/01/16 20:02, Chris Metcalf wrote: >>> We are using GICv2 compatibility mode in the Fast Models/Foundation >>> Models simulations we are running because the boot code (ATF/UEFI) >>> doesn't support GICv3 in our system at the moment. >>> >>> However, starting with kernel 4.2, the guest couldn't boot up because it >>> wasn't getting timer interrupts. I tracked this down to a kernel commit >>> that switched to using the "alternatives" mechanism -- rather than >>> seeing either a GICv2 or GICv3 and configuring appropriately, the KVM >>> code just configured the code that saves/restores the vgic state based >>> on the presence of the system register interface to the GIC CPU >>> interface. See the attached patch for a fix that manages this >>> differently and allows me to boot up the guest in this configuration. >>> >>> However, even assuming this patch can be taken into an upstream tree, I >>> still have a couple of additional problems: >>> >>> - I can boot up with the Foundation Models using this change, but not >>> with the Fast Models (again, using a v3 GIC but in v2 compatibility mode >>> in the device tree). The Fast Models dts looks like it has the same >>> configuration for the GIC and the timers so I'm not sure what's going on >>> here. Any suggestions appreciated. >>> >>> - Without this change, I could only boot kernels up to 4.1. With the >>> change, I can boot kernels up to 4.3. But 4.4 won't boot for me either; >>> I haven't bisected it down yet. So any suggestions on what might be >>> going wrong here would also be appreciated. >>> >>> We are planning to eventually use GICv3 mode in our software stack but >>> for the time being I assume it is interesting to resolve issues with GIC >>> v2 compatibility mode on GIC v3. >>> >> I'm afraid that this is the wrong approach. Whilst 4.2 was a bit too >> eager to use GICv3 (only checking the CPU capability and ignoring the >> actual state of the EL2/EL3 SRE bits), the fact that 4.4 doesn't boot is >> probably the sign of a broken firmware that enables the system register >> interface at EL3, letting the rest of the software stack to use GICv3 in >> native mode, and yet providing a GICv2 DT. >> >> This combination is unpredictable, and is likely to cause issues on >> some HW implementations. >> >> Could you please point me to the firmware you're using? >> >> Also, please check the following patches: >> >> 6d32ab2 arm64: Update booting requirements for GICv3 in GICv2 mode >> 76e52dd irqchip/gic: Warn if GICv3 system registers are enabled >> 963fcd4 arm64: cpufeatures: Check ICC_EL1_SRE.SRE before enabling >> ARM64_HAS_SYSREG_GIC_CPUIF >> 7cabd00 irqchip/gic-v3: Make gic_enable_sre an inline function >> d271976 arm64: el2_setup: Make sure ICC_SRE_EL2.SRE sticks before using >> GICv3 sysregs >> >> Can you point me to the one that prevents you from booting? > > The problematic commit is 963fcd4, because it calls gic_enable_sre() > in the host kernel even with a GICv2 DT specified, and this seems to > put things in a state such that we don't receive virtual timer > interrupts in the guest when we boot it up. (I'm not that familiar with > the QEMU DT but it is providing a GIC v2 to the guest.) > > With a v4.5-rc1 host, if I "return false" before the code in gic_enable_sre() > that tries to actually enable the SRE, and then hardcode the > __vgic_v2_XXX_state() save/restore calls into the __vgic_XXX_state() > routines, then my guest boots up OK. What if you just do the "return false"? I bet that it will work as well... > We are using a modified ARM version of EDK v3.0-rc0, and a modified > ARM Trusted Firmware based on commit 963fcd4 (between v1.1 and 1.2). Are you sure of that commit? It looks suspiciously like the ID ftom the kernel tree... > We certainly haven't touched any of the GIC code in either one. > > I tried to modify the host DT to enable GICv3, but then the host itself > hangs on boot, so clearly more is needed. (To be fair I've only tested > v4.4 in that configuration, not v4.5-rc1.) The firmware isn't yet using > GICv3 so perhaps that is part of the problem. That's indeed part of the problem. The firmware running at EL3 insists on using GICv2, but still let EL2 (and EL1) use GICv3 system registers. Could you please dump the content of ICC_SRE_EL3 just before entering the kernel at EL2? If you see ICC_SRE_EL3.SRE being set, then this would indicate a firmware bug (and leave the system in an unpredictable configuration). Thanks, M.
On 01/27/2016 04:12 AM, Marc Zyngier wrote: > On 26/01/16 20:43, Chris Metcalf wrote: >> On 01/18/2016 04:28 AM, Marc Zyngier wrote: >>> Hi Chris, >>> >>> On 15/01/16 20:02, Chris Metcalf wrote: >>>> We are using GICv2 compatibility mode in the Fast Models/Foundation >>>> Models simulations we are running because the boot code (ATF/UEFI) >>>> doesn't support GICv3 in our system at the moment. >>>> >>>> However, starting with kernel 4.2, the guest couldn't boot up because it >>>> wasn't getting timer interrupts. I tracked this down to a kernel commit >>>> that switched to using the "alternatives" mechanism -- rather than >>>> seeing either a GICv2 or GICv3 and configuring appropriately, the KVM >>>> code just configured the code that saves/restores the vgic state based >>>> on the presence of the system register interface to the GIC CPU >>>> interface. See the attached patch for a fix that manages this >>>> differently and allows me to boot up the guest in this configuration. >>>> >>>> However, even assuming this patch can be taken into an upstream tree, I >>>> still have a couple of additional problems: >>>> >>>> - I can boot up with the Foundation Models using this change, but not >>>> with the Fast Models (again, using a v3 GIC but in v2 compatibility mode >>>> in the device tree). The Fast Models dts looks like it has the same >>>> configuration for the GIC and the timers so I'm not sure what's going on >>>> here. Any suggestions appreciated. >>>> >>>> - Without this change, I could only boot kernels up to 4.1. With the >>>> change, I can boot kernels up to 4.3. But 4.4 won't boot for me either; >>>> I haven't bisected it down yet. So any suggestions on what might be >>>> going wrong here would also be appreciated. >>>> >>>> We are planning to eventually use GICv3 mode in our software stack but >>>> for the time being I assume it is interesting to resolve issues with GIC >>>> v2 compatibility mode on GIC v3. >>>> >>> I'm afraid that this is the wrong approach. Whilst 4.2 was a bit too >>> eager to use GICv3 (only checking the CPU capability and ignoring the >>> actual state of the EL2/EL3 SRE bits), the fact that 4.4 doesn't boot is >>> probably the sign of a broken firmware that enables the system register >>> interface at EL3, letting the rest of the software stack to use GICv3 in >>> native mode, and yet providing a GICv2 DT. >>> >>> This combination is unpredictable, and is likely to cause issues on >>> some HW implementations. >>> >>> Could you please point me to the firmware you're using? >>> >>> Also, please check the following patches: >>> >>> 6d32ab2 arm64: Update booting requirements for GICv3 in GICv2 mode >>> 76e52dd irqchip/gic: Warn if GICv3 system registers are enabled >>> 963fcd4 arm64: cpufeatures: Check ICC_EL1_SRE.SRE before enabling >>> ARM64_HAS_SYSREG_GIC_CPUIF >>> 7cabd00 irqchip/gic-v3: Make gic_enable_sre an inline function >>> d271976 arm64: el2_setup: Make sure ICC_SRE_EL2.SRE sticks before using >>> GICv3 sysregs >>> >>> Can you point me to the one that prevents you from booting? >> The problematic commit is 963fcd4, because it calls gic_enable_sre() >> in the host kernel even with a GICv2 DT specified, and this seems to >> put things in a state such that we don't receive virtual timer >> interrupts in the guest when we boot it up. (I'm not that familiar with >> the QEMU DT but it is providing a GIC v2 to the guest.) >> >> With a v4.5-rc1 host, if I "return false" before the code in gic_enable_sre() >> that tries to actually enable the SRE, and then hardcode the >> __vgic_v2_XXX_state() save/restore calls into the __vgic_XXX_state() >> routines, then my guest boots up OK. > What if you just do the "return false"? I bet that it will work as well... Yes, that also works for my case. >> We are using a modified ARM version of EDK v3.0-rc0, and a modified >> ARM Trusted Firmware based on commit 963fcd4 (between v1.1 and 1.2). > Are you sure of that commit? It looks suspiciously like the ID ftom the > kernel tree... Hah, good catch! The double-click-to-copy behavior is kind of flakey on RHEL 6's default terminal, and I bet that bit me. It's 41099f4e. >> We certainly haven't touched any of the GIC code in either one. >> >> I tried to modify the host DT to enable GICv3, but then the host itself >> hangs on boot, so clearly more is needed. (To be fair I've only tested >> v4.4 in that configuration, not v4.5-rc1.) The firmware isn't yet using >> GICv3 so perhaps that is part of the problem. > That's indeed part of the problem. The firmware running at EL3 insists > on using GICv2, but still let EL2 (and EL1) use GICv3 system registers. > Could you please dump the content of ICC_SRE_EL3 just before entering > the kernel at EL2? If you see ICC_SRE_EL3.SRE being set, then this would > indicate a firmware bug (and leave the system in an unpredictable > configuration). Well, the firmware clearly does this intentionally. In ATF's drivers/arm/giv/arm_gic.c, the gicv3_cpuif_setup() function has a comment that reads: /******************************************************************************* * This function does some minimal GICv3 configuration. The Firmware itself does * not fully support GICv3 at this time and relies on GICv2 emulation as * provided by GICv3. This function allows software (like Linux) in later stages * to use full GICv3 features. ******************************************************************************/ and the function ends with: val = read_icc_sre_el3(); write_icc_sre_el3(val | ICC_SRE_EN | ICC_SRE_SRE); In our build environment, if I comment out those two lines, that fixes the guest boot problem (without any hacking on the Linux side), so that's good anyway. With this change it works for me in the Fast Models as well as Foundation Models, too.
On 28 January 2016 at 21:12, Chris Metcalf <cmetcalf@ezchip.com> wrote: > On 01/27/2016 04:12 AM, Marc Zyngier wrote: >> >> On 26/01/16 20:43, Chris Metcalf wrote: >>> >>> On 01/18/2016 04:28 AM, Marc Zyngier wrote: >>>> >>>> Hi Chris, >>>> >>>> On 15/01/16 20:02, Chris Metcalf wrote: >>>>> >>>>> We are using GICv2 compatibility mode in the Fast Models/Foundation >>>>> Models simulations we are running because the boot code (ATF/UEFI) >>>>> doesn't support GICv3 in our system at the moment. >>>>> >>>>> However, starting with kernel 4.2, the guest couldn't boot up because >>>>> it >>>>> wasn't getting timer interrupts. I tracked this down to a kernel >>>>> commit >>>>> that switched to using the "alternatives" mechanism -- rather than >>>>> seeing either a GICv2 or GICv3 and configuring appropriately, the KVM >>>>> code just configured the code that saves/restores the vgic state based >>>>> on the presence of the system register interface to the GIC CPU >>>>> interface. See the attached patch for a fix that manages this >>>>> differently and allows me to boot up the guest in this configuration. >>>>> >>>>> However, even assuming this patch can be taken into an upstream tree, I >>>>> still have a couple of additional problems: >>>>> >>>>> - I can boot up with the Foundation Models using this change, but not >>>>> with the Fast Models (again, using a v3 GIC but in v2 compatibility >>>>> mode >>>>> in the device tree). The Fast Models dts looks like it has the same >>>>> configuration for the GIC and the timers so I'm not sure what's going >>>>> on >>>>> here. Any suggestions appreciated. >>>>> >>>>> - Without this change, I could only boot kernels up to 4.1. With the >>>>> change, I can boot kernels up to 4.3. But 4.4 won't boot for me >>>>> either; >>>>> I haven't bisected it down yet. So any suggestions on what might be >>>>> going wrong here would also be appreciated. >>>>> >>>>> We are planning to eventually use GICv3 mode in our software stack but >>>>> for the time being I assume it is interesting to resolve issues with >>>>> GIC >>>>> v2 compatibility mode on GIC v3. >>>>> >>>> I'm afraid that this is the wrong approach. Whilst 4.2 was a bit too >>>> eager to use GICv3 (only checking the CPU capability and ignoring the >>>> actual state of the EL2/EL3 SRE bits), the fact that 4.4 doesn't boot is >>>> probably the sign of a broken firmware that enables the system register >>>> interface at EL3, letting the rest of the software stack to use GICv3 in >>>> native mode, and yet providing a GICv2 DT. >>>> >>>> This combination is unpredictable, and is likely to cause issues on >>>> some HW implementations. >>>> >>>> Could you please point me to the firmware you're using? >>>> >>>> Also, please check the following patches: >>>> >>>> 6d32ab2 arm64: Update booting requirements for GICv3 in GICv2 mode >>>> 76e52dd irqchip/gic: Warn if GICv3 system registers are enabled >>>> 963fcd4 arm64: cpufeatures: Check ICC_EL1_SRE.SRE before enabling >>>> ARM64_HAS_SYSREG_GIC_CPUIF >>>> 7cabd00 irqchip/gic-v3: Make gic_enable_sre an inline function >>>> d271976 arm64: el2_setup: Make sure ICC_SRE_EL2.SRE sticks before using >>>> GICv3 sysregs >>>> >>>> Can you point me to the one that prevents you from booting? >>> >>> The problematic commit is 963fcd4, because it calls gic_enable_sre() >>> in the host kernel even with a GICv2 DT specified, and this seems to >>> put things in a state such that we don't receive virtual timer >>> interrupts in the guest when we boot it up. (I'm not that familiar with >>> the QEMU DT but it is providing a GIC v2 to the guest.) >>> >>> With a v4.5-rc1 host, if I "return false" before the code in >>> gic_enable_sre() >>> that tries to actually enable the SRE, and then hardcode the >>> __vgic_v2_XXX_state() save/restore calls into the __vgic_XXX_state() >>> routines, then my guest boots up OK. >> >> What if you just do the "return false"? I bet that it will work as well... > > > Yes, that also works for my case. > >>> We are using a modified ARM version of EDK v3.0-rc0, and a modified >>> ARM Trusted Firmware based on commit 963fcd4 (between v1.1 and 1.2). >> What does 'EDK v3.0-rc0' mean? We don't do any versioned releases afaik, I recently fixed a GIC issue in the FVP EDK2 code, which prevented it from running the GICv3 in native mode rather than in GICv2 compatibility mode. 33ed33f ArmPkg/ArmGic: fix bug in GICv3 distributor configuration >>> We certainly haven't touched any of the GIC code in either one. >>> >>> I tried to modify the host DT to enable GICv3, but then the host itself >>> hangs on boot, so clearly more is needed. (To be fair I've only tested >>> v4.4 in that configuration, not v4.5-rc1.) The firmware isn't yet using >>> GICv3 so perhaps that is part of the problem. >> >> That's indeed part of the problem. The firmware running at EL3 insists >> on using GICv2, but still let EL2 (and EL1) use GICv3 system registers. >> Could you please dump the content of ICC_SRE_EL3 just before entering >> the kernel at EL2? If you see ICC_SRE_EL3.SRE being set, then this would >> indicate a firmware bug (and leave the system in an unpredictable >> configuration). > > > Well, the firmware clearly does this intentionally. In ATF's > drivers/arm/giv/arm_gic.c, the gicv3_cpuif_setup() function has > a comment that reads: > > /******************************************************************************* > * This function does some minimal GICv3 configuration. The Firmware itself > does > * not fully support GICv3 at this time and relies on GICv2 emulation as > * provided by GICv3. This function allows software (like Linux) in later > stages > * to use full GICv3 features. > > ******************************************************************************/ > This is deliberate, since running the GIC in v3 mode on the secure side would remove the ability on the non-secure side to use the v2 legacy mode. It does not limit the utility of the GICv3 on the non-secure side > and the function ends with: > > val = read_icc_sre_el3(); > write_icc_sre_el3(val | ICC_SRE_EN | ICC_SRE_SRE); > > In our build environment, if I comment out those two lines, that > fixes the guest boot problem (without any hacking on the Linux side), > so that's good anyway. With this change it works for me in the > Fast Models as well as Foundation Models, too. > For historical reasons, the EDK2 GIC driver infers the presence of a GICv3 from the ability to use the system register interface, and ignores the ID registers completely. Without the patch above, or the PcdArmGicV3WithV2Legacy set, the symptoms you are seeing on the firmware side are not entirely unexpected. Also note that, on the Foundation model, the GICv2 and the GICv3 live at different memory addresses.
On 01/29/2016 02:24 AM, Ard Biesheuvel wrote: > On 28 January 2016 at 21:12, Chris Metcalf <cmetcalf@ezchip.com> wrote: >> On 01/27/2016 04:12 AM, Marc Zyngier wrote: >>> On 26/01/16 20:43, Chris Metcalf wrote: >>>> On 01/18/2016 04:28 AM, Marc Zyngier wrote: >>>>> Hi Chris, >>>>> >>>>> On 15/01/16 20:02, Chris Metcalf wrote: >>>>>> We are using GICv2 compatibility mode in the Fast Models/Foundation >>>>>> Models simulations we are running because the boot code (ATF/UEFI) >>>>>> doesn't support GICv3 in our system at the moment. >>>>>> >>>>>> However, starting with kernel 4.2, the guest couldn't boot up because >>>>>> it >>>>>> wasn't getting timer interrupts. I tracked this down to a kernel >>>>>> commit >>>>>> that switched to using the "alternatives" mechanism -- rather than >>>>>> seeing either a GICv2 or GICv3 and configuring appropriately, the KVM >>>>>> code just configured the code that saves/restores the vgic state based >>>>>> on the presence of the system register interface to the GIC CPU >>>>>> interface. See the attached patch for a fix that manages this >>>>>> differently and allows me to boot up the guest in this configuration. >>>>>> >>>>>> However, even assuming this patch can be taken into an upstream tree, I >>>>>> still have a couple of additional problems: >>>>>> >>>>>> - I can boot up with the Foundation Models using this change, but not >>>>>> with the Fast Models (again, using a v3 GIC but in v2 compatibility >>>>>> mode >>>>>> in the device tree). The Fast Models dts looks like it has the same >>>>>> configuration for the GIC and the timers so I'm not sure what's going >>>>>> on >>>>>> here. Any suggestions appreciated. >>>>>> >>>>>> - Without this change, I could only boot kernels up to 4.1. With the >>>>>> change, I can boot kernels up to 4.3. But 4.4 won't boot for me >>>>>> either; >>>>>> I haven't bisected it down yet. So any suggestions on what might be >>>>>> going wrong here would also be appreciated. >>>>>> >>>>>> We are planning to eventually use GICv3 mode in our software stack but >>>>>> for the time being I assume it is interesting to resolve issues with >>>>>> GIC >>>>>> v2 compatibility mode on GIC v3. >>>>>> >>>>> I'm afraid that this is the wrong approach. Whilst 4.2 was a bit too >>>>> eager to use GICv3 (only checking the CPU capability and ignoring the >>>>> actual state of the EL2/EL3 SRE bits), the fact that 4.4 doesn't boot is >>>>> probably the sign of a broken firmware that enables the system register >>>>> interface at EL3, letting the rest of the software stack to use GICv3 in >>>>> native mode, and yet providing a GICv2 DT. >>>>> >>>>> This combination is unpredictable, and is likely to cause issues on >>>>> some HW implementations. >>>>> >>>>> Could you please point me to the firmware you're using? >>>>> >>>>> Also, please check the following patches: >>>>> >>>>> 6d32ab2 arm64: Update booting requirements for GICv3 in GICv2 mode >>>>> 76e52dd irqchip/gic: Warn if GICv3 system registers are enabled >>>>> 963fcd4 arm64: cpufeatures: Check ICC_EL1_SRE.SRE before enabling >>>>> ARM64_HAS_SYSREG_GIC_CPUIF >>>>> 7cabd00 irqchip/gic-v3: Make gic_enable_sre an inline function >>>>> d271976 arm64: el2_setup: Make sure ICC_SRE_EL2.SRE sticks before using >>>>> GICv3 sysregs >>>>> >>>>> Can you point me to the one that prevents you from booting? >>>> The problematic commit is 963fcd4, because it calls gic_enable_sre() >>>> in the host kernel even with a GICv2 DT specified, and this seems to >>>> put things in a state such that we don't receive virtual timer >>>> interrupts in the guest when we boot it up. (I'm not that familiar with >>>> the QEMU DT but it is providing a GIC v2 to the guest.) >>>> >>>> With a v4.5-rc1 host, if I "return false" before the code in >>>> gic_enable_sre() >>>> that tries to actually enable the SRE, and then hardcode the >>>> __vgic_v2_XXX_state() save/restore calls into the __vgic_XXX_state() >>>> routines, then my guest boots up OK. >>> What if you just do the "return false"? I bet that it will work as well... >> >> Yes, that also works for my case. >> >>>> We are using a modified ARM version of EDK v3.0-rc0, and a modified >>>> ARM Trusted Firmware based on commit 963fcd4 (between v1.1 and 1.2). > > What does 'EDK v3.0-rc0' mean? We don't do any versioned releases afaik, It's a git tag from the repo at git://github.com/ARM-software/edk2 . In fact I'm not quite sure we are at that exact tag, since it seems like some fixes present in v3.0-rc0 are missing from our code base. But it's an early 2015 drop in any case. > I recently fixed a GIC issue in the FVP EDK2 code, which prevented it > from running the GICv3 in native mode rather than in GICv2 > compatibility mode. > > 33ed33f ArmPkg/ArmGic: fix bug in GICv3 distributor configuration Looks like an alternate version of that fix is present in the ARM repo as commit 152ac4, and we have that fix in our repo too. >>>> We certainly haven't touched any of the GIC code in either one. >>>> >>>> I tried to modify the host DT to enable GICv3, but then the host itself >>>> hangs on boot, so clearly more is needed. (To be fair I've only tested >>>> v4.4 in that configuration, not v4.5-rc1.) The firmware isn't yet using >>>> GICv3 so perhaps that is part of the problem. >>> That's indeed part of the problem. The firmware running at EL3 insists >>> on using GICv2, but still let EL2 (and EL1) use GICv3 system registers. >>> Could you please dump the content of ICC_SRE_EL3 just before entering >>> the kernel at EL2? If you see ICC_SRE_EL3.SRE being set, then this would >>> indicate a firmware bug (and leave the system in an unpredictable >>> configuration). >> >> Well, the firmware clearly does this intentionally. In ATF's >> drivers/arm/giv/arm_gic.c, the gicv3_cpuif_setup() function has >> a comment that reads: >> >> /******************************************************************************* >> * This function does some minimal GICv3 configuration. The Firmware itself >> does >> * not fully support GICv3 at this time and relies on GICv2 emulation as >> * provided by GICv3. This function allows software (like Linux) in later >> stages >> * to use full GICv3 features. >> >> ******************************************************************************/ >> > This is deliberate, since running the GIC in v3 mode on the secure > side would remove the ability on the non-secure side to use the v2 > legacy mode. It does not limit the utility of the GICv3 on the > non-secure side It does seem that it conflicts with trying to use a GIC v2 in the DT for tip Linux, though. >> and the function ends with: >> >> val = read_icc_sre_el3(); >> write_icc_sre_el3(val | ICC_SRE_EN | ICC_SRE_SRE); >> >> In our build environment, if I comment out those two lines, that >> fixes the guest boot problem (without any hacking on the Linux side), >> so that's good anyway. With this change it works for me in the >> Fast Models as well as Foundation Models, too. >> > For historical reasons, the EDK2 GIC driver infers the presence of a > GICv3 from the ability to use the system register interface, and > ignores the ID registers completely. Without the patch above, or the > PcdArmGicV3WithV2Legacy set, the symptoms you are seeing on the > firmware side are not entirely unexpected. I believe we do set PcdArmGicV3WithV2Legacy to TRUE for our platform, but we did require my patch above in addition: [PcdsFeatureFlag.common] # Force the UEFI GIC driver to use GICv2 legacy mode. To use # GICv3 without GICv2 legacy in UEFI, the ARM Trusted Firmware needs # to configure the Non-Secure interrupts in the GIC Redistributors # which is not supported at the moment. gArmTokenSpaceGuid.PcdArmGicV3WithV2Legacy|TRUE > Also note that, on the > Foundation model, the GICv2 and the GICv3 live at different memory > addresses. We have the GIC at different addresses in any case, but I will check with our hardware folks to see if we should be using different addresses if we try to use a GIC v3. Thanks!
On 28/01/16 20:12, Chris Metcalf wrote: > On 01/27/2016 04:12 AM, Marc Zyngier wrote: >> On 26/01/16 20:43, Chris Metcalf wrote: >>> On 01/18/2016 04:28 AM, Marc Zyngier wrote: >>>> Hi Chris, >>>> >>>> On 15/01/16 20:02, Chris Metcalf wrote: >>>>> We are using GICv2 compatibility mode in the Fast Models/Foundation >>>>> Models simulations we are running because the boot code (ATF/UEFI) >>>>> doesn't support GICv3 in our system at the moment. >>>>> >>>>> However, starting with kernel 4.2, the guest couldn't boot up because it >>>>> wasn't getting timer interrupts. I tracked this down to a kernel commit >>>>> that switched to using the "alternatives" mechanism -- rather than >>>>> seeing either a GICv2 or GICv3 and configuring appropriately, the KVM >>>>> code just configured the code that saves/restores the vgic state based >>>>> on the presence of the system register interface to the GIC CPU >>>>> interface. See the attached patch for a fix that manages this >>>>> differently and allows me to boot up the guest in this configuration. >>>>> >>>>> However, even assuming this patch can be taken into an upstream tree, I >>>>> still have a couple of additional problems: >>>>> >>>>> - I can boot up with the Foundation Models using this change, but not >>>>> with the Fast Models (again, using a v3 GIC but in v2 compatibility mode >>>>> in the device tree). The Fast Models dts looks like it has the same >>>>> configuration for the GIC and the timers so I'm not sure what's going on >>>>> here. Any suggestions appreciated. >>>>> >>>>> - Without this change, I could only boot kernels up to 4.1. With the >>>>> change, I can boot kernels up to 4.3. But 4.4 won't boot for me either; >>>>> I haven't bisected it down yet. So any suggestions on what might be >>>>> going wrong here would also be appreciated. >>>>> >>>>> We are planning to eventually use GICv3 mode in our software stack but >>>>> for the time being I assume it is interesting to resolve issues with GIC >>>>> v2 compatibility mode on GIC v3. >>>>> >>>> I'm afraid that this is the wrong approach. Whilst 4.2 was a bit too >>>> eager to use GICv3 (only checking the CPU capability and ignoring the >>>> actual state of the EL2/EL3 SRE bits), the fact that 4.4 doesn't boot is >>>> probably the sign of a broken firmware that enables the system register >>>> interface at EL3, letting the rest of the software stack to use GICv3 in >>>> native mode, and yet providing a GICv2 DT. >>>> >>>> This combination is unpredictable, and is likely to cause issues on >>>> some HW implementations. >>>> >>>> Could you please point me to the firmware you're using? >>>> >>>> Also, please check the following patches: >>>> >>>> 6d32ab2 arm64: Update booting requirements for GICv3 in GICv2 mode >>>> 76e52dd irqchip/gic: Warn if GICv3 system registers are enabled >>>> 963fcd4 arm64: cpufeatures: Check ICC_EL1_SRE.SRE before enabling >>>> ARM64_HAS_SYSREG_GIC_CPUIF >>>> 7cabd00 irqchip/gic-v3: Make gic_enable_sre an inline function >>>> d271976 arm64: el2_setup: Make sure ICC_SRE_EL2.SRE sticks before using >>>> GICv3 sysregs >>>> >>>> Can you point me to the one that prevents you from booting? >>> The problematic commit is 963fcd4, because it calls gic_enable_sre() >>> in the host kernel even with a GICv2 DT specified, and this seems to >>> put things in a state such that we don't receive virtual timer >>> interrupts in the guest when we boot it up. (I'm not that familiar with >>> the QEMU DT but it is providing a GIC v2 to the guest.) >>> >>> With a v4.5-rc1 host, if I "return false" before the code in gic_enable_sre() >>> that tries to actually enable the SRE, and then hardcode the >>> __vgic_v2_XXX_state() save/restore calls into the __vgic_XXX_state() >>> routines, then my guest boots up OK. >> What if you just do the "return false"? I bet that it will work as well... > > Yes, that also works for my case. > >>> We are using a modified ARM version of EDK v3.0-rc0, and a modified >>> ARM Trusted Firmware based on commit 963fcd4 (between v1.1 and 1.2). >> Are you sure of that commit? It looks suspiciously like the ID ftom the >> kernel tree... > > Hah, good catch! The double-click-to-copy behavior is kind of flakey > on RHEL 6's default terminal, and I bet that bit me. It's 41099f4e. > >>> We certainly haven't touched any of the GIC code in either one. >>> >>> I tried to modify the host DT to enable GICv3, but then the host itself >>> hangs on boot, so clearly more is needed. (To be fair I've only tested >>> v4.4 in that configuration, not v4.5-rc1.) The firmware isn't yet using >>> GICv3 so perhaps that is part of the problem. >> That's indeed part of the problem. The firmware running at EL3 insists >> on using GICv2, but still let EL2 (and EL1) use GICv3 system registers. >> Could you please dump the content of ICC_SRE_EL3 just before entering >> the kernel at EL2? If you see ICC_SRE_EL3.SRE being set, then this would >> indicate a firmware bug (and leave the system in an unpredictable >> configuration). > > Well, the firmware clearly does this intentionally. In ATF's > drivers/arm/giv/arm_gic.c, the gicv3_cpuif_setup() function has > a comment that reads: > > /******************************************************************************* > * This function does some minimal GICv3 configuration. The Firmware itself does > * not fully support GICv3 at this time and relies on GICv2 emulation as > * provided by GICv3. This function allows software (like Linux) in later stages > * to use full GICv3 features. > ******************************************************************************/ > > and the function ends with: > > val = read_icc_sre_el3(); > write_icc_sre_el3(val | ICC_SRE_EN | ICC_SRE_SRE); > > In our build environment, if I comment out those two lines, that > fixes the guest boot problem (without any hacking on the Linux side), > so that's good anyway. With this change it works for me in the > Fast Models as well as Foundation Models, too. By the look of it, you're trying to use a GICv3 firmware, and pass a GICv2 DT to the kernel. Do not do that. Either you use a GICv2 firmware (having spoken to the ATF guys, there is a GICv2 driver in there that should work for your case) and pass a GICv2 DT, or you go GICv3 all the way. A mix of the two things is completely unsupported on the model, and solidly places you in the UNPREDICTABLE category when running that on actual HW... Thanks, M.
On 01/29/2016 12:54 PM, Marc Zyngier wrote: > By the look of it, you're trying to use a GICv3 firmware, and pass a > GICv2 DT to the kernel. Do not do that. Either you use a GICv2 firmware > (having spoken to the ATF guys, there is a GICv2 driver in there that > should work for your case) and pass a GICv2 DT, or you go GICv3 all the way. > > A mix of the two things is completely unsupported on the model, and > solidly places you in the UNPREDICTABLE category when running that on > actual HW... Once we upgrade to ATF 1.2 we will remove all of our GICv2 stuff and see if everything works smoothly; till then, at least, we seem to have a workaround in place for development that will let us keep moving forward. From the ATF docs it does seem that using a v3 GIC in v2 compatibility mode should be supported, but it's not pressing from our side to drill any deeper to try to see why it's not actually working correctly for us (though I'd be happy to try any further testing in this configuration if that's helpful). Thanks!
On 29/01/16 18:29, Chris Metcalf wrote: > On 01/29/2016 12:54 PM, Marc Zyngier wrote: >> By the look of it, you're trying to use a GICv3 firmware, and pass a >> GICv2 DT to the kernel. Do not do that. Either you use a GICv2 firmware >> (having spoken to the ATF guys, there is a GICv2 driver in there that >> should work for your case) and pass a GICv2 DT, or you go GICv3 all the way. >> >> A mix of the two things is completely unsupported on the model, and >> solidly places you in the UNPREDICTABLE category when running that on >> actual HW... > > Once we upgrade to ATF 1.2 we will remove all of our GICv2 > stuff and see if everything works smoothly; till then, at least, we > seem to have a workaround in place for development that will > let us keep moving forward. > > From the ATF docs it does seem that using a v3 GIC in v2 > compatibility mode should be supported, but it's not pressing > from our side to drill any deeper to try to see why it's not > actually working correctly for us (though I'd be happy to try > any further testing in this configuration if that's helpful). That's what I've been told as well - the GICv2 code in ATF 1.2 should do the trick out of the box. Thanks, M.
From 3dcb529de23adb918b9a4d6eca717c737f380bc3 Mon Sep 17 00:00:00 2001
From: Chris Metcalf <cmetcalf@ezchip.com>
Date: Fri, 15 Jan 2016 13:18:06 -0500
Subject: [PATCH] gic: update save/restore pointers only when gic v3 detected
The original code set up the VGIC save/restore calls in
__kvm_vcpu_run() based on whether the GIC had been detected as v2 or
v3. Commit 8a14849b4a35 ("arm64: KVM: Switch vgic save/restore to
alternative_insn") switched to making that choice based on whether
the processor feature register reports that the system register
interface to the GIC CPU interface is supported.
However, booting up with the GIC v3 in v2 compatibility mode (in
this case on the Linaro Foundation Model simulator) we find that
the v3 save/restore isn't the right thing, since we end up with no
timer interrupts being delivered to the KVM guest. Reverting to
a model where we set up the VGIC save/restore calls based on the
actual GIC type fixes this.
To do this and still keep the simplicity of the "alternatives"
model, we instead leave the v2 branch-and-link instruction in place,
but patch it dynamically to be a branch-and-link to the v3 routines
if we detect a v3 GIC.
Signed-off-by: Chris Metcalf <cmetcalf@ezchip.com>
---
arch/arm/include/asm/kvm_host.h | 5 +++++
arch/arm64/include/asm/kvm_host.h | 37 +++++++++++++++++++++++++++++++++++++
arch/arm64/kvm/hyp.S | 14 ++++----------
virt/kvm/arm/vgic.c | 3 +++
4 files changed, 49 insertions(+), 10 deletions(-)
diff --git a/arch/arm/include/asm/kvm_host.h b/arch/arm/include/asm/kvm_host.h
index c4072d9f32c7..a34ce4d73498 100644
--- a/arch/arm/include/asm/kvm_host.h
+++ b/arch/arm/include/asm/kvm_host.h
@@ -216,6 +216,11 @@ static inline int kvm_arch_dev_ioctl_check_extension(long ext)
return 0;
}
+static inline void vgic_arch_setup(const struct vgic_params *vgic)
+{
+ BUG_ON(vgic->type != VGIC_V2);
+}
+
int kvm_perf_init(void);
int kvm_perf_teardown(void);
diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
index ed039688c221..9ce98e69b5ec 100644
--- a/arch/arm64/include/asm/kvm_host.h
+++ b/arch/arm64/include/asm/kvm_host.h
@@ -27,6 +27,7 @@
#include <asm/kvm.h>
#include <asm/kvm_asm.h>
#include <asm/kvm_mmio.h>
+#include <asm/insn.h>
#define __KVM_HAVE_ARCH_INTC_INITIALIZED
@@ -244,6 +245,42 @@ static inline void __cpu_init_hyp_mode(phys_addr_t boot_pgd_ptr,
hyp_stack_ptr, vector_ptr);
}
+#ifdef CONFIG_ARM_GIC_V3
+/* Write a 'bl FUNC' instruction at address CALLSITE. */
+static inline void vgic_patch(char *callsite, char *func)
+{
+ aarch64_insn_patch_text_nosync(
+ callsite,
+ aarch64_insn_gen_branch_imm((long)callsite, (long)func,
+ AARCH64_INSN_BRANCH_LINK));
+}
+#endif
+
+static inline void vgic_arch_setup(const struct vgic_params *vgic)
+{
+ switch(vgic->type)
+ {
+ case VGIC_V2:
+ break;
+
+#ifdef CONFIG_ARM_GIC_V3
+ case VGIC_V3:
+ {
+ extern char __save_vgic_state_insn[];
+ extern char __save_vgic_v3_state[];
+ extern char __restore_vgic_state_insn[];
+ extern char __restore_vgic_v3_state[];
+ vgic_patch(__save_vgic_state_insn, __save_vgic_v3_state);
+ vgic_patch(__restore_vgic_state_insn, __restore_vgic_v3_state);
+ break;
+ }
+#endif
+
+ default:
+ BUG();
+ }
+}
+
static inline void kvm_arch_hardware_disable(void) {}
static inline void kvm_arch_hardware_unsetup(void) {}
static inline void kvm_arch_sync_events(struct kvm *kvm) {}
diff --git a/arch/arm64/kvm/hyp.S b/arch/arm64/kvm/hyp.S
index e5836138ec42..6b1eded53051 100644
--- a/arch/arm64/kvm/hyp.S
+++ b/arch/arm64/kvm/hyp.S
@@ -518,11 +518,8 @@
* Call into the vgic backend for state saving
*/
.macro save_vgic_state
-alternative_if_not ARM64_HAS_SYSREG_GIC_CPUIF
- bl __save_vgic_v2_state
-alternative_else
- bl __save_vgic_v3_state
-alternative_endif
+ENTRY(__save_vgic_state_insn)
+ bl __save_vgic_v2_state // may update to __save_vgic_v3_state
mrs x24, hcr_el2
mov x25, #HCR_INT_OVERRIDE
neg x25, x25
@@ -539,11 +536,8 @@ alternative_endif
orr x24, x24, #HCR_INT_OVERRIDE
orr x24, x24, x25
msr hcr_el2, x24
-alternative_if_not ARM64_HAS_SYSREG_GIC_CPUIF
- bl __restore_vgic_v2_state
-alternative_else
- bl __restore_vgic_v3_state
-alternative_endif
+ENTRY(__restore_vgic_state_insn)
+ bl __restore_vgic_v2_state // may update to __restore_vgic_v3_state
.endm
.macro save_timer_state
diff --git a/virt/kvm/arm/vgic.c b/virt/kvm/arm/vgic.c
index 66c66165e712..8b4215414d11 100644
--- a/virt/kvm/arm/vgic.c
+++ b/virt/kvm/arm/vgic.c
@@ -2502,6 +2502,9 @@ int kvm_vgic_hyp_init(void)
goto out_free_irq;
}
+ /* Callback into for arch code for setup */
+ vgic_arch_setup(vgic);
+
on_each_cpu(vgic_init_maintenance_interrupt, NULL, 1);
return 0;
--
2.1.2