| Message ID | 1410452204-7277-1-git-send-email-dianders@chromium.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Sep 11, 2014 at 05:16:44PM +0100, Doug Anderson wrote: > Some 32-bit (ARMv7) systems are architected like this: > > * The firmware doesn't know and doesn't care about hypervisor mode and > we don't want to add the complexity of hypervisor there. > > * The firmware isn't involved in SMP bringup or resume. > > * The ARCH timer come up with an uninitialized offset between the > virtual and physical counters. Each core gets a different random > offset. > > On systems like the above, it doesn't make sense to use the virtual > counter. There's nobody managing the offset and each time a core goes > down and comes back up it will get reinitialized to some other random > value. You probably need to rephrase this slightly, as there *is* still a requirement on the hypervisor/firmware (actually, two!). See below. > Let's add a property to the device tree to say that we shouldn't use > the virtual timer. Firmware could potentially remove this property > before passing the device tree to the kernel if it really wants the > kernel to use a virtual timer. > > Note that it's been said that ARM64 (ARMv8) systems the firmware and > kernel really can't be architected as described above. That means > using the physical timer like this really only makes sense for ARMv7 > systems. I'd go further: this only makes sense if you're booting in secure SVC mode. > In order for this patch to do anything useful, we also need Sonny's > patch at <https://patchwork.kernel.org/patch/4790921/> > > Signed-off-by: Doug Anderson <dianders@chromium.org> > Signed-off-by: Sonny Rao <sonnyrao@chromium.org> > --- > Changes in v2: > - Add "#ifdef CONFIG_ARM" as per Will Deacon > > Documentation/devicetree/bindings/arm/arch_timer.txt | 6 ++++++ > drivers/clocksource/arm_arch_timer.c | 5 +++++ > 2 files changed, 11 insertions(+) > > diff --git a/Documentation/devicetree/bindings/arm/arch_timer.txt b/Documentation/devicetree/bindings/arm/arch_timer.txt > index 37b2caf..876d32b 100644 > --- a/Documentation/devicetree/bindings/arm/arch_timer.txt > +++ b/Documentation/devicetree/bindings/arm/arch_timer.txt > @@ -22,6 +22,12 @@ to deliver its interrupts via SPIs. > - always-on : a boolean property. If present, the timer is powered through an > always-on power domain, therefore it never loses context. > > +** Optional properties: > + > +- arm,use-physical-timer : Don't ever use the virtual timer, just use the > + physical one. Not supported for ARM64. I'd say `Only supported for ARM' to better match what we've done. Probably also worth mentioning that this relies on the hypervisor/firmware having set CNTHCTL.PL1PCEN and CNTHCTL.EL1PCTEN (but assumedly made a mess of CNTVOFF ;) if you want to boot on the non-secure side (e.g. as a guest). Will
Will, On Thu, Sep 11, 2014 at 9:47 AM, Will Deacon <will.deacon@arm.com> wrote: > On Thu, Sep 11, 2014 at 05:16:44PM +0100, Doug Anderson wrote: >> Some 32-bit (ARMv7) systems are architected like this: >> >> * The firmware doesn't know and doesn't care about hypervisor mode and >> we don't want to add the complexity of hypervisor there. >> >> * The firmware isn't involved in SMP bringup or resume. >> >> * The ARCH timer come up with an uninitialized offset between the >> virtual and physical counters. Each core gets a different random >> offset. >> >> On systems like the above, it doesn't make sense to use the virtual >> counter. There's nobody managing the offset and each time a core goes >> down and comes back up it will get reinitialized to some other random >> value. > > You probably need to rephrase this slightly, as there *is* still a > requirement on the hypervisor/firmware (actually, two!). See below. Sure. I added two more bullet points. >> Let's add a property to the device tree to say that we shouldn't use >> the virtual timer. Firmware could potentially remove this property >> before passing the device tree to the kernel if it really wants the >> kernel to use a virtual timer. >> >> Note that it's been said that ARM64 (ARMv8) systems the firmware and >> kernel really can't be architected as described above. That means >> using the physical timer like this really only makes sense for ARMv7 >> systems. > > I'd go further: this only makes sense if you're booting in secure SVC > mode. OK. >> In order for this patch to do anything useful, we also need Sonny's >> patch at <https://patchwork.kernel.org/patch/4790921/> >> >> Signed-off-by: Doug Anderson <dianders@chromium.org> >> Signed-off-by: Sonny Rao <sonnyrao@chromium.org> >> --- >> Changes in v2: >> - Add "#ifdef CONFIG_ARM" as per Will Deacon >> >> Documentation/devicetree/bindings/arm/arch_timer.txt | 6 ++++++ >> drivers/clocksource/arm_arch_timer.c | 5 +++++ >> 2 files changed, 11 insertions(+) >> >> diff --git a/Documentation/devicetree/bindings/arm/arch_timer.txt b/Documentation/devicetree/bindings/arm/arch_timer.txt >> index 37b2caf..876d32b 100644 >> --- a/Documentation/devicetree/bindings/arm/arch_timer.txt >> +++ b/Documentation/devicetree/bindings/arm/arch_timer.txt >> @@ -22,6 +22,12 @@ to deliver its interrupts via SPIs. >> - always-on : a boolean property. If present, the timer is powered through an >> always-on power domain, therefore it never loses context. >> >> +** Optional properties: >> + >> +- arm,use-physical-timer : Don't ever use the virtual timer, just use the >> + physical one. Not supported for ARM64. > > I'd say `Only supported for ARM' to better match what we've done. Probably > also worth mentioning that this relies on the hypervisor/firmware having set > CNTHCTL.PL1PCEN and CNTHCTL.EL1PCTEN (but assumedly made a mess of CNTVOFF > ;) if you want to boot on the non-secure side (e.g. as a guest). Note that the reset value of CNTHCTL.PL1PCEN and CNTHCTL.PL1PCTEN are both 1 in my version of the ARM ARM. On the other hand CNTVOFF is documented to have an UNKNOWN reset value. If only ARM had guaranteed that CNTVOFF started out as 0 (which seems like it would have been sensible) we wouldn't be in this mess. :-/ I've adjusted the wording. Hopefully it looks good to you. -Doug
On 11/09/14 17:47, Will Deacon wrote: > On Thu, Sep 11, 2014 at 05:16:44PM +0100, Doug Anderson wrote: >> Some 32-bit (ARMv7) systems are architected like this: >> >> * The firmware doesn't know and doesn't care about hypervisor mode and >> we don't want to add the complexity of hypervisor there. >> >> * The firmware isn't involved in SMP bringup or resume. >> >> * The ARCH timer come up with an uninitialized offset between the >> virtual and physical counters. Each core gets a different random >> offset. >> >> On systems like the above, it doesn't make sense to use the virtual >> counter. There's nobody managing the offset and each time a core goes >> down and comes back up it will get reinitialized to some other random >> value. > > You probably need to rephrase this slightly, as there *is* still a > requirement on the hypervisor/firmware (actually, two!). See below. > >> Let's add a property to the device tree to say that we shouldn't use >> the virtual timer. Firmware could potentially remove this property >> before passing the device tree to the kernel if it really wants the >> kernel to use a virtual timer. >> >> Note that it's been said that ARM64 (ARMv8) systems the firmware and >> kernel really can't be architected as described above. That means >> using the physical timer like this really only makes sense for ARMv7 >> systems. > > I'd go further: this only makes sense if you're booting in secure SVC > mode. If that's the case, what's the problem? Enter monitor mode, set SCR.NS to one, nuke CNTVOFF, revert, job done. What am I missing? M.
On Thu, Sep 11, 2014 at 05:59:53PM +0100, Doug Anderson wrote: > On Thu, Sep 11, 2014 at 9:47 AM, Will Deacon <will.deacon@arm.com> wrote: > > I'd say `Only supported for ARM' to better match what we've done. Probably > > also worth mentioning that this relies on the hypervisor/firmware having set > > CNTHCTL.PL1PCEN and CNTHCTL.EL1PCTEN (but assumedly made a mess of CNTVOFF > > ;) if you want to boot on the non-secure side (e.g. as a guest). > > Note that the reset value of CNTHCTL.PL1PCEN and CNTHCTL.PL1PCTEN are > both 1 in my version of the ARM ARM. On the other hand CNTVOFF is > documented to have an UNKNOWN reset value. If only ARM had guaranteed > that CNTVOFF started out as 0 (which seems like it would have been > sensible) we wouldn't be in this mess. :-/ I'm afraid we went the opposite way -- in ARMv8 there are a tiny handful of EL3 registers that are well-defined out of reset, then the rest of the system is UNKNOWN. The hardware guys prefer that and it can also be useful for very low-level debugging (system crashes, do a reset, read out the state). Will
Hi, On Thu, Sep 11, 2014 at 10:00 AM, Marc Zyngier <marc.zyngier@arm.com> wrote: > On 11/09/14 17:47, Will Deacon wrote: >> On Thu, Sep 11, 2014 at 05:16:44PM +0100, Doug Anderson wrote: >>> Some 32-bit (ARMv7) systems are architected like this: >>> >>> * The firmware doesn't know and doesn't care about hypervisor mode and >>> we don't want to add the complexity of hypervisor there. >>> >>> * The firmware isn't involved in SMP bringup or resume. >>> >>> * The ARCH timer come up with an uninitialized offset between the >>> virtual and physical counters. Each core gets a different random >>> offset. >>> >>> On systems like the above, it doesn't make sense to use the virtual >>> counter. There's nobody managing the offset and each time a core goes >>> down and comes back up it will get reinitialized to some other random >>> value. >> >> You probably need to rephrase this slightly, as there *is* still a >> requirement on the hypervisor/firmware (actually, two!). See below. >> >>> Let's add a property to the device tree to say that we shouldn't use >>> the virtual timer. Firmware could potentially remove this property >>> before passing the device tree to the kernel if it really wants the >>> kernel to use a virtual timer. >>> >>> Note that it's been said that ARM64 (ARMv8) systems the firmware and >>> kernel really can't be architected as described above. That means >>> using the physical timer like this really only makes sense for ARMv7 >>> systems. >> >> I'd go further: this only makes sense if you're booting in secure SVC >> mode. > > If that's the case, what's the problem? Enter monitor mode, set SCR.NS > to one, nuke CNTVOFF, revert, job done. > > What am I missing? Stuff like this was talked about in the thread about Sonny's patch at <https://patchwork.kernel.org/patch/4790921/> ...in that case we were always talking about HYP mode, though. I don't think anyone has explicitly talked about just switching to monitor mode and then leaving ourselves in Secure SVC after we're done. It would be nice (especially for the VDSO guys) if we could just init the virtual offset... We would need to run this code potentially at processor bringup and after suspend/resume, but that seems possible too. Is the transition to monitor mode and back simple? Where would you suggest putting this code? It would definitely need to be pretty early. We'd also need to be able to detect that we're in Secure SVC and not mess up anyone else who happened to boot in Non Secure SVC. -Doug
Will, On Thu, Sep 11, 2014 at 10:07 AM, Will Deacon <will.deacon@arm.com> wrote: > On Thu, Sep 11, 2014 at 05:59:53PM +0100, Doug Anderson wrote: >> On Thu, Sep 11, 2014 at 9:47 AM, Will Deacon <will.deacon@arm.com> wrote: >> > I'd say `Only supported for ARM' to better match what we've done. Probably >> > also worth mentioning that this relies on the hypervisor/firmware having set >> > CNTHCTL.PL1PCEN and CNTHCTL.EL1PCTEN (but assumedly made a mess of CNTVOFF >> > ;) if you want to boot on the non-secure side (e.g. as a guest). >> >> Note that the reset value of CNTHCTL.PL1PCEN and CNTHCTL.PL1PCTEN are >> both 1 in my version of the ARM ARM. On the other hand CNTVOFF is >> documented to have an UNKNOWN reset value. If only ARM had guaranteed >> that CNTVOFF started out as 0 (which seems like it would have been >> sensible) we wouldn't be in this mess. :-/ > > I'm afraid we went the opposite way -- in ARMv8 there are a tiny handful > of EL3 registers that are well-defined out of reset, then the rest of the > system is UNKNOWN. The hardware guys prefer that and it can also be useful > for very low-level debugging (system crashes, do a reset, read out the > state). Well, I guess if that's the way it is then software will have to deal with it. It sure seems like having things default to some state (even if it's zero) at reset is really nice. Otherwise you get things where one 1 of every 10 times you boot the system it behaves differently and that's hard to track down. ...of course, then it maybe forces you to think about really initting all bits in software and that's not terrible. -Doug
On 11/09/14 18:11, Doug Anderson wrote: > Hi, > > On Thu, Sep 11, 2014 at 10:00 AM, Marc Zyngier <marc.zyngier@arm.com> wrote: >> On 11/09/14 17:47, Will Deacon wrote: >>> On Thu, Sep 11, 2014 at 05:16:44PM +0100, Doug Anderson wrote: >>>> Some 32-bit (ARMv7) systems are architected like this: >>>> >>>> * The firmware doesn't know and doesn't care about hypervisor mode and >>>> we don't want to add the complexity of hypervisor there. >>>> >>>> * The firmware isn't involved in SMP bringup or resume. >>>> >>>> * The ARCH timer come up with an uninitialized offset between the >>>> virtual and physical counters. Each core gets a different random >>>> offset. >>>> >>>> On systems like the above, it doesn't make sense to use the virtual >>>> counter. There's nobody managing the offset and each time a core goes >>>> down and comes back up it will get reinitialized to some other random >>>> value. >>> >>> You probably need to rephrase this slightly, as there *is* still a >>> requirement on the hypervisor/firmware (actually, two!). See below. >>> >>>> Let's add a property to the device tree to say that we shouldn't use >>>> the virtual timer. Firmware could potentially remove this property >>>> before passing the device tree to the kernel if it really wants the >>>> kernel to use a virtual timer. >>>> >>>> Note that it's been said that ARM64 (ARMv8) systems the firmware and >>>> kernel really can't be architected as described above. That means >>>> using the physical timer like this really only makes sense for ARMv7 >>>> systems. >>> >>> I'd go further: this only makes sense if you're booting in secure SVC >>> mode. >> >> If that's the case, what's the problem? Enter monitor mode, set SCR.NS >> to one, nuke CNTVOFF, revert, job done. >> >> What am I missing? > > Stuff like this was talked about in the thread about Sonny's patch at > <https://patchwork.kernel.org/patch/4790921/> > > ...in that case we were always talking about HYP mode, though. I That's because I always assumed that you'd be running non-secure, dropped there by some idiotic firmware without any way to go back up. > don't think anyone has explicitly talked about just switching to > monitor mode and then leaving ourselves in Secure SVC after we're > done. It would be nice (especially for the VDSO guys) if we could > just init the virtual offset... > > We would need to run this code potentially at processor bringup and > after suspend/resume, but that seems possible too. Note that this would be an ARMv7 only thing (you can't do that on ARMv8, at all). > Is the transition to monitor mode and back simple? Where would you > suggest putting this code? It would definitely need to be pretty > early. We'd also need to be able to detect that we're in Secure SVC > and not mess up anyone else who happened to boot in Non Secure SVC. This would have to live in some very early platform-specific code. The ugly part is that you cannot find out what world you're in (accessing SCR is going to send you to UNDEF-land if accessed from NS). If I was suicidal, I'd suggest you could pass a parameter to the command line, interpreted by the timer code... But I since I'm not, let's pretend I haven't said anything... ;-) M.
Marc, On Thu, Sep 11, 2014 at 10:22 AM, Marc Zyngier <marc.zyngier@arm.com> wrote: >> We would need to run this code potentially at processor bringup and >> after suspend/resume, but that seems possible too. > > Note that this would be an ARMv7 only thing (you can't do that on ARMv8, > at all). Yes, of course. >> Is the transition to monitor mode and back simple? Where would you >> suggest putting this code? It would definitely need to be pretty >> early. We'd also need to be able to detect that we're in Secure SVC >> and not mess up anyone else who happened to boot in Non Secure SVC. > > This would have to live in some very early platform-specific code. The > ugly part is that you cannot find out what world you're in (accessing > SCR is going to send you to UNDEF-land if accessed from NS). Yup, so the question is: would such code be accepted upstream, or are we going to embark on a big job for someone to figure out how to do this only to get NAKed? If there was some indication that folks would take this, I think we might be able to get it coded up. If someone else wanted to volunteer to code it that would make me even happier, but maybe that's pushing my luck. ;) > If I was suicidal, I'd suggest you could pass a parameter to the command > line, interpreted by the timer code... But I since I'm not, let's > pretend I haven't said anything... ;-) I did this in the past (again, see Sonny's thread), but didn't consider myself knowledgeable to know if that was truly a good test: asm volatile("mrc p15, 0, %0, c1, c1, 0" : "=r" (val)); pr_info("DOUG: val is %#010x", val); val |= (1 << 2); asm volatile("mcr p15, 0, %0, c1, c1, 0" : : "r" (val)); val = 0xffffffff; asm volatile("mrc p15, 0, %0, c1, c1, 0" : "=r" (val)); pr_info("DOUG: val is %#010x", val); The idea being that if you can make modifications to the SCR register (and see your changes take effect) then you must be in secure mode. In my case the first printout was 0x0 and the second was 0x4. -Doug
On 11/09/14 18:29, Doug Anderson wrote: > Marc, > > On Thu, Sep 11, 2014 at 10:22 AM, Marc Zyngier <marc.zyngier@arm.com> wrote: >>> We would need to run this code potentially at processor bringup and >>> after suspend/resume, but that seems possible too. >> >> Note that this would be an ARMv7 only thing (you can't do that on ARMv8, >> at all). > > Yes, of course. > > >>> Is the transition to monitor mode and back simple? Where would you >>> suggest putting this code? It would definitely need to be pretty >>> early. We'd also need to be able to detect that we're in Secure SVC >>> and not mess up anyone else who happened to boot in Non Secure SVC. >> >> This would have to live in some very early platform-specific code. The >> ugly part is that you cannot find out what world you're in (accessing >> SCR is going to send you to UNDEF-land if accessed from NS). > > Yup, so the question is: would such code be accepted upstream, or are > we going to embark on a big job for someone to figure out how to do > this only to get NAKed? > > If there was some indication that folks would take this, I think we > might be able to get it coded up. If someone else wanted to volunteer > to code it that would make me even happier, but maybe that's pushing > my luck. ;) Writing the code is a 5 minute job. Getting it accepted is another story, and I'm not sure everyone would agree on that. >> If I was suicidal, I'd suggest you could pass a parameter to the command >> line, interpreted by the timer code... But I since I'm not, let's >> pretend I haven't said anything... ;-) > > I did this in the past (again, see Sonny's thread), but didn't > consider myself knowledgeable to know if that was truly a good test: > > asm volatile("mrc p15, 0, %0, c1, c1, 0" : "=r" (val)); > pr_info("DOUG: val is %#010x", val); > val |= (1 << 2); > asm volatile("mcr p15, 0, %0, c1, c1, 0" : : "r" (val)); > val = 0xffffffff; > asm volatile("mrc p15, 0, %0, c1, c1, 0" : "=r" (val)); > pr_info("DOUG: val is %#010x", val); > > The idea being that if you can make modifications to the SCR register > (and see your changes take effect) then you must be in secure mode. > In my case the first printout was 0x0 and the second was 0x4. The main issue is when you're *not* in secure mode. It is likely that this will explode badly. This is why I suggested something that is set by the bootloader (after all. it knows which mode it is booted in), and that the timer driver can use when the CPU comes up. Still, very ugly... M.
Marc, On Thu, Sep 11, 2014 at 10:43 AM, Marc Zyngier <marc.zyngier@arm.com> wrote: > On 11/09/14 18:29, Doug Anderson wrote: >> Marc, >> >> On Thu, Sep 11, 2014 at 10:22 AM, Marc Zyngier <marc.zyngier@arm.com> wrote: >>>> We would need to run this code potentially at processor bringup and >>>> after suspend/resume, but that seems possible too. >>> >>> Note that this would be an ARMv7 only thing (you can't do that on ARMv8, >>> at all). >> >> Yes, of course. >> >> >>>> Is the transition to monitor mode and back simple? Where would you >>>> suggest putting this code? It would definitely need to be pretty >>>> early. We'd also need to be able to detect that we're in Secure SVC >>>> and not mess up anyone else who happened to boot in Non Secure SVC. >>> >>> This would have to live in some very early platform-specific code. The >>> ugly part is that you cannot find out what world you're in (accessing >>> SCR is going to send you to UNDEF-land if accessed from NS). >> >> Yup, so the question is: would such code be accepted upstream, or are >> we going to embark on a big job for someone to figure out how to do >> this only to get NAKed? >> >> If there was some indication that folks would take this, I think we >> might be able to get it coded up. If someone else wanted to volunteer >> to code it that would make me even happier, but maybe that's pushing >> my luck. ;) > > Writing the code is a 5 minute job. Getting it accepted is another > story, and I'm not sure everyone would agree on that. > >>> If I was suicidal, I'd suggest you could pass a parameter to the command >>> line, interpreted by the timer code... But I since I'm not, let's >>> pretend I haven't said anything... ;-) >> >> I did this in the past (again, see Sonny's thread), but didn't >> consider myself knowledgeable to know if that was truly a good test: >> >> asm volatile("mrc p15, 0, %0, c1, c1, 0" : "=r" (val)); >> pr_info("DOUG: val is %#010x", val); >> val |= (1 << 2); >> asm volatile("mcr p15, 0, %0, c1, c1, 0" : : "r" (val)); >> val = 0xffffffff; >> asm volatile("mrc p15, 0, %0, c1, c1, 0" : "=r" (val)); >> pr_info("DOUG: val is %#010x", val); >> >> The idea being that if you can make modifications to the SCR register >> (and see your changes take effect) then you must be in secure mode. >> In my case the first printout was 0x0 and the second was 0x4. > > The main issue is when you're *not* in secure mode. It is likely that > this will explode badly. This is why I suggested something that is set > by the bootloader (after all. it knows which mode it is booted in), and > that the timer driver can use when the CPU comes up. > > Still, very ugly... Ah, got it. Well, unless someone can suggest a clean way to do this, then I guess we'll keep what we've got... -Doug
On 09/11/14 10:43, Marc Zyngier wrote: >>> If I was suicidal, I'd suggest you could pass a parameter to the command >>> line, interpreted by the timer code... But I since I'm not, let's >>> pretend I haven't said anything... ;-) >> I did this in the past (again, see Sonny's thread), but didn't >> consider myself knowledgeable to know if that was truly a good test: >> >> asm volatile("mrc p15, 0, %0, c1, c1, 0" : "=r" (val)); >> pr_info("DOUG: val is %#010x", val); >> val |= (1 << 2); >> asm volatile("mcr p15, 0, %0, c1, c1, 0" : : "r" (val)); >> val = 0xffffffff; >> asm volatile("mrc p15, 0, %0, c1, c1, 0" : "=r" (val)); >> pr_info("DOUG: val is %#010x", val); >> >> The idea being that if you can make modifications to the SCR register >> (and see your changes take effect) then you must be in secure mode. >> In my case the first printout was 0x0 and the second was 0x4. > The main issue is when you're *not* in secure mode. It is likely that > this will explode badly. This is why I suggested something that is set > by the bootloader (after all. it knows which mode it is booted in), and > that the timer driver can use when the CPU comes up. Where does this platform jump to when a CPU comes up? Is it rockchip_secondary_startup()? I wonder if that path could have this little bit of assembly to poke the cntvoff in monitor mode and then jump to secondary_startup()? Before we boot any secondary CPUs we could also read the cntvoff for CPU0 in the platform specific layer (where we know we're running in secure mode) and then use that value as the "reset" value for the secondaries. Or does this platform boot up in secure mode some times and non-secure mode other times?
Stephen, On Thu, Sep 11, 2014 at 4:56 PM, Stephen Boyd <sboyd@codeaurora.org> wrote: > On 09/11/14 10:43, Marc Zyngier wrote: >>>> If I was suicidal, I'd suggest you could pass a parameter to the command >>>> line, interpreted by the timer code... But I since I'm not, let's >>>> pretend I haven't said anything... ;-) >>> I did this in the past (again, see Sonny's thread), but didn't >>> consider myself knowledgeable to know if that was truly a good test: >>> >>> asm volatile("mrc p15, 0, %0, c1, c1, 0" : "=r" (val)); >>> pr_info("DOUG: val is %#010x", val); >>> val |= (1 << 2); >>> asm volatile("mcr p15, 0, %0, c1, c1, 0" : : "r" (val)); >>> val = 0xffffffff; >>> asm volatile("mrc p15, 0, %0, c1, c1, 0" : "=r" (val)); >>> pr_info("DOUG: val is %#010x", val); >>> >>> The idea being that if you can make modifications to the SCR register >>> (and see your changes take effect) then you must be in secure mode. >>> In my case the first printout was 0x0 and the second was 0x4. >> The main issue is when you're *not* in secure mode. It is likely that >> this will explode badly. This is why I suggested something that is set >> by the bootloader (after all. it knows which mode it is booted in), and >> that the timer driver can use when the CPU comes up. > > Where does this platform jump to when a CPU comes up? Is it > rockchip_secondary_startup()? I wonder if that path could have this > little bit of assembly to poke the cntvoff in monitor mode and then jump > to secondary_startup()? Before we boot any secondary CPUs we could also > read the cntvoff for CPU0 in the platform specific layer (where we know > we're running in secure mode) and then use that value as the "reset" > value for the secondaries. Or does this platform boot up in secure mode > some times and non-secure mode other times? I guess it would depend a whole lot on the bootloader, wouldn't it? With our current "get out of the way" bootloader, Linux always sees "Secure SVC". ...but if someone decided to put a new bootloader on the system that wanted to do something different (implement security and boot the kernel in nonsecure HYP or implement a hypervisor and boot the kernel in nonsecure SVC) then everything would be different. If someone were to write a bootloader like that (or perhaps if we're running in a VM?) then I'd imagine that the whole world would be different. Somehow this secure bootloader and/or hypervisor would _have_ to be involved in processor bringup and suspend/resume. Since I've never looked at code implementing either of these I'm just making assumptions, though. -Doug
On Thu, Sep 11, 2014 at 6:17 PM, Stephen Boyd <sboyd@codeaurora.org> wrote: > On 09/11/14 17:14, Sonny Rao wrote: > > On Thu, Sep 11, 2014 at 4:56 PM, Stephen Boyd <sboyd@codeaurora.org> wrote: >> >> >> Where does this platform jump to when a CPU comes up? Is it >> rockchip_secondary_startup()? I wonder if that path could have this >> little bit of assembly to poke the cntvoff in monitor mode and then jump >> to secondary_startup()? Before we boot any secondary CPUs we could also >> read the cntvoff for CPU0 in the platform specific layer (where we know >> we're running in secure mode) and then use that value as the "reset" >> value for the secondaries. Or does this platform boot up in secure mode >> some times and non-secure mode other times? > > > Yes, In our case, with our firmware, we will go through some internal Rom > code and then jump to rockchip_secondary_startup, but I don't think it's > correct to force all users of this SoC to do it that way. > > > What's being forced? The way internal rom jumps to sram? Is there any other > way that secondary CPUs come out of reset on this SoC? From looking at the > code it seems like the only path is internal rom jumps to sram (where > rockchip_secondary_trampoline lives) which jumps to > rockchip_secondary_startup() which then does an invalidate and jump to > secondary_startup(). Linux controls everything besides the internal rom. Is > something different in your case? There are other ways it can be done, and I don't know all of the possibilities, but there seems to be some protocol with the iROM that tells it where to go, which the current SMP patches are using by putting a magic number and an address in SRAM. I think it's true that in our case, it really is pretty simple and we have secure SVC mode and not much else runs (besides the iROM). Since I don't know all of the possibilities, I didn't want to preclude the possibility that someone else handled things differently and entered the kernel in non-secure mode, and have some code there that broke in that instance, that's all I meant by "forced". > > If there were a reasonable way to determine for sure that we are in secure > mode, then yes we could do what you're suggesting, and I'd be happy to code > that up. > > > I think the problem is that there isn't a great way to determine whether > we're in secure mode or not, and this is maybe by design? I don't > particularly understand that design choice. It would be nice to hear some > rationale from ARM folks. > > > I'm thinking we would have a different boot-method for secure vs. non-secure > and then we would know to configure cntvoff or not based on the boot method. > Isn't that a reasonable way of knowing what should be done? It seems like we > can at least modify the DT for this SoC. Putting something into the device-tree is in fact the point of this patch, so it is sort of doing what you're suggesting, although this patch is about being able use to physical counters and doesn't indicate anything about secure vs non-secure. What else do you think could be used to differentiate between the two cases, besides putting it into the DT? > > I still wonder if there is such a bootloader/hypervisor/rom that's putting > this SoC into non-secure mode and not configuring cntvoff. Doug's comments > seem to suggest that the whole world would be different if this were true. > Maybe Heiko knows? As far as I'm aware, there's no bootloader/firmware that's ever putting the CPU into non-secure mode for our case. > -- > Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, > hosted by The Linux Foundation
On 12/09/14 01:01, Doug Anderson wrote: > Stephen, > > On Thu, Sep 11, 2014 at 4:56 PM, Stephen Boyd <sboyd@codeaurora.org> wrote: >> On 09/11/14 10:43, Marc Zyngier wrote: >>>>> If I was suicidal, I'd suggest you could pass a parameter to the command >>>>> line, interpreted by the timer code... But I since I'm not, let's >>>>> pretend I haven't said anything... ;-) >>>> I did this in the past (again, see Sonny's thread), but didn't >>>> consider myself knowledgeable to know if that was truly a good test: >>>> >>>> asm volatile("mrc p15, 0, %0, c1, c1, 0" : "=r" (val)); >>>> pr_info("DOUG: val is %#010x", val); >>>> val |= (1 << 2); >>>> asm volatile("mcr p15, 0, %0, c1, c1, 0" : : "r" (val)); >>>> val = 0xffffffff; >>>> asm volatile("mrc p15, 0, %0, c1, c1, 0" : "=r" (val)); >>>> pr_info("DOUG: val is %#010x", val); >>>> >>>> The idea being that if you can make modifications to the SCR register >>>> (and see your changes take effect) then you must be in secure mode. >>>> In my case the first printout was 0x0 and the second was 0x4. >>> The main issue is when you're *not* in secure mode. It is likely that >>> this will explode badly. This is why I suggested something that is set >>> by the bootloader (after all. it knows which mode it is booted in), and >>> that the timer driver can use when the CPU comes up. >> >> Where does this platform jump to when a CPU comes up? Is it >> rockchip_secondary_startup()? I wonder if that path could have this >> little bit of assembly to poke the cntvoff in monitor mode and then jump >> to secondary_startup()? Before we boot any secondary CPUs we could also >> read the cntvoff for CPU0 in the platform specific layer (where we know >> we're running in secure mode) and then use that value as the "reset" >> value for the secondaries. Or does this platform boot up in secure mode >> some times and non-secure mode other times? > > I guess it would depend a whole lot on the bootloader, wouldn't it? Yes, hence my suggestion of hooking such a thing from the timer code, where we could have a clue what to do (and what not to). > With our current "get out of the way" bootloader, Linux always sees > "Secure SVC". ...but if someone decided to put a new bootloader on > the system that wanted to do something different (implement security > and boot the kernel in nonsecure HYP or implement a hypervisor and > boot the kernel in nonsecure SVC) then everything would be different. > > If someone were to write a bootloader like that (or perhaps if we're > running in a VM?) then I'd imagine that the whole world would be > different. Somehow this secure bootloader and/or hypervisor would > _have_ to be involved in processor bringup and suspend/resume. Since > I've never looked at code implementing either of these I'm just making > assumptions, though. Exactly. That's why we're so hell bent on PSCI, because it solves all these issues (and that's why I've added some rudimentary support for it in u-boot). Thanks, M.
Hi Marc, On 09/11/2014 01:43 PM, Marc Zyngier wrote: > On 11/09/14 18:29, Doug Anderson wrote: >> Marc, >> >> On Thu, Sep 11, 2014 at 10:22 AM, Marc Zyngier <marc.zyngier@arm.com> wrote: >>>> We would need to run this code potentially at processor bringup and >>>> after suspend/resume, but that seems possible too. >>> >>> Note that this would be an ARMv7 only thing (you can't do that on ARMv8, >>> at all). >> >> Yes, of course. >> >> >>>> Is the transition to monitor mode and back simple? Where would you >>>> suggest putting this code? It would definitely need to be pretty >>>> early. We'd also need to be able to detect that we're in Secure SVC >>>> and not mess up anyone else who happened to boot in Non Secure SVC. >>> >>> This would have to live in some very early platform-specific code. The >>> ugly part is that you cannot find out what world you're in (accessing >>> SCR is going to send you to UNDEF-land if accessed from NS). >> >> Yup, so the question is: would such code be accepted upstream, or are >> we going to embark on a big job for someone to figure out how to do >> this only to get NAKed? >> >> If there was some indication that folks would take this, I think we >> might be able to get it coded up. If someone else wanted to volunteer >> to code it that would make me even happier, but maybe that's pushing >> my luck. ;) > > Writing the code is a 5 minute job. Getting it accepted is another > story, and I'm not sure everyone would agree on that. > >>> If I was suicidal, I'd suggest you could pass a parameter to the command >>> line, interpreted by the timer code... But I since I'm not, let's >>> pretend I haven't said anything... ;-) >> >> I did this in the past (again, see Sonny's thread), but didn't >> consider myself knowledgeable to know if that was truly a good test: >> >> asm volatile("mrc p15, 0, %0, c1, c1, 0" : "=r" (val)); >> pr_info("DOUG: val is %#010x", val); >> val |= (1 << 2); >> asm volatile("mcr p15, 0, %0, c1, c1, 0" : : "r" (val)); >> val = 0xffffffff; >> asm volatile("mrc p15, 0, %0, c1, c1, 0" : "=r" (val)); >> pr_info("DOUG: val is %#010x", val); >> >> The idea being that if you can make modifications to the SCR register >> (and see your changes take effect) then you must be in secure mode. >> In my case the first printout was 0x0 and the second was 0x4. > > The main issue is when you're *not* in secure mode. It is likely that > this will explode badly. This is why I suggested something that is set > by the bootloader (after all. it knows which mode it is booted in), and > that the timer driver can use when the CPU comes up. What exactly does "exploding badly" look like? Causing and undefined instruction exception? That's just a branch with a mode switch. Any reason the code couldn't deal with that or even use that to its advantage? Thanks, Christopher
Hi Christopher, On 12/09/14 12:43, Christopher Covington wrote: > Hi Marc, > > On 09/11/2014 01:43 PM, Marc Zyngier wrote: >> On 11/09/14 18:29, Doug Anderson wrote: >>> Marc, >>> >>> On Thu, Sep 11, 2014 at 10:22 AM, Marc Zyngier <marc.zyngier@arm.com> wrote: >>>>> We would need to run this code potentially at processor bringup and >>>>> after suspend/resume, but that seems possible too. >>>> >>>> Note that this would be an ARMv7 only thing (you can't do that on ARMv8, >>>> at all). >>> >>> Yes, of course. >>> >>> >>>>> Is the transition to monitor mode and back simple? Where would you >>>>> suggest putting this code? It would definitely need to be pretty >>>>> early. We'd also need to be able to detect that we're in Secure SVC >>>>> and not mess up anyone else who happened to boot in Non Secure SVC. >>>> >>>> This would have to live in some very early platform-specific code. The >>>> ugly part is that you cannot find out what world you're in (accessing >>>> SCR is going to send you to UNDEF-land if accessed from NS). >>> >>> Yup, so the question is: would such code be accepted upstream, or are >>> we going to embark on a big job for someone to figure out how to do >>> this only to get NAKed? >>> >>> If there was some indication that folks would take this, I think we >>> might be able to get it coded up. If someone else wanted to volunteer >>> to code it that would make me even happier, but maybe that's pushing >>> my luck. ;) >> >> Writing the code is a 5 minute job. Getting it accepted is another >> story, and I'm not sure everyone would agree on that. >> >>>> If I was suicidal, I'd suggest you could pass a parameter to the command >>>> line, interpreted by the timer code... But I since I'm not, let's >>>> pretend I haven't said anything... ;-) >>> >>> I did this in the past (again, see Sonny's thread), but didn't >>> consider myself knowledgeable to know if that was truly a good test: >>> >>> asm volatile("mrc p15, 0, %0, c1, c1, 0" : "=r" (val)); >>> pr_info("DOUG: val is %#010x", val); >>> val |= (1 << 2); >>> asm volatile("mcr p15, 0, %0, c1, c1, 0" : : "r" (val)); >>> val = 0xffffffff; >>> asm volatile("mrc p15, 0, %0, c1, c1, 0" : "=r" (val)); >>> pr_info("DOUG: val is %#010x", val); >>> >>> The idea being that if you can make modifications to the SCR register >>> (and see your changes take effect) then you must be in secure mode. >>> In my case the first printout was 0x0 and the second was 0x4. >> >> The main issue is when you're *not* in secure mode. It is likely that >> this will explode badly. This is why I suggested something that is set >> by the bootloader (after all. it knows which mode it is booted in), and >> that the timer driver can use when the CPU comes up. > > What exactly does "exploding badly" look like? Causing and undefined > instruction exception? That's just a branch with a mode switch. Any reason the > code couldn't deal with that or even use that to its advantage? We surely can handle the UNDEF and do something there. We just can't do it the way Doug described it above. Thanks, M.
On 09/12/14 05:14, Marc Zyngier wrote: > Hi Christopher, > > On 12/09/14 12:43, Christopher Covington wrote: >> Hi Marc, >> >> On 09/11/2014 01:43 PM, Marc Zyngier wrote: >>> On 11/09/14 18:29, Doug Anderson wrote: >>> >>>> I did this in the past (again, see Sonny's thread), but didn't >>>> consider myself knowledgeable to know if that was truly a good test: >>>> >>>> asm volatile("mrc p15, 0, %0, c1, c1, 0" : "=r" (val)); >>>> pr_info("DOUG: val is %#010x", val); >>>> val |= (1 << 2); >>>> asm volatile("mcr p15, 0, %0, c1, c1, 0" : : "r" (val)); >>>> val = 0xffffffff; >>>> asm volatile("mrc p15, 0, %0, c1, c1, 0" : "=r" (val)); >>>> pr_info("DOUG: val is %#010x", val); >>>> >>>> The idea being that if you can make modifications to the SCR register >>>> (and see your changes take effect) then you must be in secure mode. >>>> In my case the first printout was 0x0 and the second was 0x4. >>> The main issue is when you're *not* in secure mode. It is likely that >>> this will explode badly. This is why I suggested something that is set >>> by the bootloader (after all. it knows which mode it is booted in), and >>> that the timer driver can use when the CPU comes up. >> What exactly does "exploding badly" look like? Causing and undefined >> instruction exception? That's just a branch with a mode switch. Any reason the >> code couldn't deal with that or even use that to its advantage? > We surely can handle the UNDEF and do something there. We just can't do > it the way Doug described it above. > > I suggested doing that for something else a while ago and Will and Dave we're not thrilled[1]. The suggestion back then was to use DT to indicate what mode the kernel is running in. [1] http://lists.infradead.org/pipermail/linux-arm-kernel/2012-June/105321.html
On Fri, Sep 12, 2014 at 07:59:29PM +0100, Stephen Boyd wrote: > On 09/12/14 05:14, Marc Zyngier wrote: > > On 12/09/14 12:43, Christopher Covington wrote: > >> On 09/11/2014 01:43 PM, Marc Zyngier wrote: > >>> On 11/09/14 18:29, Doug Anderson wrote: > >>> > >>>> I did this in the past (again, see Sonny's thread), but didn't > >>>> consider myself knowledgeable to know if that was truly a good test: > >>>> > >>>> asm volatile("mrc p15, 0, %0, c1, c1, 0" : "=r" (val)); > >>>> pr_info("DOUG: val is %#010x", val); > >>>> val |= (1 << 2); > >>>> asm volatile("mcr p15, 0, %0, c1, c1, 0" : : "r" (val)); > >>>> val = 0xffffffff; > >>>> asm volatile("mrc p15, 0, %0, c1, c1, 0" : "=r" (val)); > >>>> pr_info("DOUG: val is %#010x", val); > >>>> > >>>> The idea being that if you can make modifications to the SCR register > >>>> (and see your changes take effect) then you must be in secure mode. > >>>> In my case the first printout was 0x0 and the second was 0x4. BTW, if you want to change the SCR.NS bit (and CNTVOFF), the kernel must run in Monitor mode (by setting the CPSR mode bits, 32-bit only). > >>> The main issue is when you're *not* in secure mode. It is likely that > >>> this will explode badly. This is why I suggested something that is set > >>> by the bootloader (after all. it knows which mode it is booted in), and > >>> that the timer driver can use when the CPU comes up. > >> > >> What exactly does "exploding badly" look like? Causing and undefined > >> instruction exception? That's just a branch with a mode switch. Any reason the > >> code couldn't deal with that or even use that to its advantage? > > > > We surely can handle the UNDEF and do something there. We just can't do > > it the way Doug described it above. > > I suggested doing that for something else a while ago and Will and Dave > we're not thrilled[1]. The suggestion back then was to use DT to > indicate what mode the kernel is running in. > > [1] > http://lists.infradead.org/pipermail/linux-arm-kernel/2012-June/105321.html I think the context was slightly different. As I re-read the thread, it seems that the discussion was around whether to use some SMC interface or not based on whether the kernel is running secure or non-secure. The argument made by Will was to actually specify the type of the firmware SMC interface in the DT and use it in the kernel (and probably assume the kernel is running in secure mode if no smc interface is specified in the DT; you could have both though, running in secure mode and also having firmware). In this arch timer case, we need to work around a firmware bug (or feature as 32-bit ARM kernels never required CNTVOFF initialisation by firmware, no matter how small such firmware is). We don't expect a specific SMC call to initialise CNTVOFF, so we can't describe it in the DT. One problem with undef for detecting whether the core is in secure or non-secure mode is that sometimes the initialisation code needs to run very early when the kernel hooks may not be fully initialised. We could only detect this mode on the booting CPU and save it in a global variable (of course, assuming the other CPUs boot in the same mode). Other code could make use of such information as appropriate. Of course, there is always a risk that it will be abused.
On 09/15/14 04:10, Catalin Marinas wrote: > On Fri, Sep 12, 2014 at 07:59:29PM +0100, Stephen Boyd wrote: >> On 09/12/14 05:14, Marc Zyngier wrote: >>> We surely can handle the UNDEF and do something there. We just can't do >>> it the way Doug described it above. >> I suggested doing that for something else a while ago and Will and Dave >> we're not thrilled[1]. The suggestion back then was to use DT to >> indicate what mode the kernel is running in. >> >> [1] >> http://lists.infradead.org/pipermail/linux-arm-kernel/2012-June/105321.html > I think the context was slightly different. As I re-read the thread, it > seems that the discussion was around whether to use some SMC interface > or not based on whether the kernel is running secure or non-secure. The > argument made by Will was to actually specify the type of the firmware > SMC interface in the DT and use it in the kernel (and probably assume > the kernel is running in secure mode if no smc interface is specified in > the DT; you could have both though, running in secure mode and also > having firmware). > > In this arch timer case, we need to work around a firmware bug (or > feature as 32-bit ARM kernels never required CNTVOFF initialisation by > firmware, no matter how small such firmware is). We don't expect a > specific SMC call to initialise CNTVOFF, so we can't describe it in the > DT. Agreed, we can't described SMC calls that don't exist. From my perspective it's just another part of the cpu boot sequence that needs to be handled in the kernel, so describing the requirement via the cpu-boot method seems appropriate. It seems like we're making it harder than it should be by handling the undef when we could have slightly different SMP boot code (and suspend/resume code) depending on the boot method property.
On Mon, Sep 15, 2014 at 1:33 PM, Stephen Boyd <sboyd@codeaurora.org> wrote: > > On 09/15/14 04:10, Catalin Marinas wrote: > > On Fri, Sep 12, 2014 at 07:59:29PM +0100, Stephen Boyd wrote: > >> On 09/12/14 05:14, Marc Zyngier wrote: > >>> We surely can handle the UNDEF and do something there. We just can't do > >>> it the way Doug described it above. > >> I suggested doing that for something else a while ago and Will and Dave > >> we're not thrilled[1]. The suggestion back then was to use DT to > >> indicate what mode the kernel is running in. > >> > >> [1] > >> http://lists.infradead.org/pipermail/linux-arm-kernel/2012-June/105321.html > > I think the context was slightly different. As I re-read the thread, it > > seems that the discussion was around whether to use some SMC interface > > or not based on whether the kernel is running secure or non-secure. The > > argument made by Will was to actually specify the type of the firmware > > SMC interface in the DT and use it in the kernel (and probably assume > > the kernel is running in secure mode if no smc interface is specified in > > the DT; you could have both though, running in secure mode and also > > having firmware). > > > > In this arch timer case, we need to work around a firmware bug (or > > feature as 32-bit ARM kernels never required CNTVOFF initialisation by > > firmware, no matter how small such firmware is). We don't expect a > > specific SMC call to initialise CNTVOFF, so we can't describe it in the > > DT. > > Agreed, we can't described SMC calls that don't exist. From my > perspective it's just another part of the cpu boot sequence that needs > to be handled in the kernel, so describing the requirement via the > cpu-boot method seems appropriate. It seems like we're making it harder > than it should be by handling the undef when we could have slightly > different SMP boot code (and suspend/resume code) depending on the boot > method property. +heiko So, for the case of rk3288, based on this discussion what I'm going to propose is to add code to rockchip.c which looks for a particular SMP enable method -- say something like "rockchip,rk3288-smp-secure-svc" which will then assume we have been booted in secure SVC mode and do the CNTVOFF fixup. I believe, it will need to do this on the boot CPU as well, so I think it will need to scan the DT fairly early on the boot CPU and also perform the function there. I'll look into implementing this and post code. Comments and suggestions appreciated, thanks. > > > -- > Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, > hosted by The Linux Foundation >
On 09/15/14 14:47, Sonny Rao wrote: > On Mon, Sep 15, 2014 at 1:33 PM, Stephen Boyd <sboyd@codeaurora.org> wrote: >> On 09/15/14 04:10, Catalin Marinas wrote: >>> On Fri, Sep 12, 2014 at 07:59:29PM +0100, Stephen Boyd wrote: >>>> On 09/12/14 05:14, Marc Zyngier wrote: >>>>> We surely can handle the UNDEF and do something there. We just can't do >>>>> it the way Doug described it above. >>>> I suggested doing that for something else a while ago and Will and Dave >>>> we're not thrilled[1]. The suggestion back then was to use DT to >>>> indicate what mode the kernel is running in. >>>> >>>> [1] >>>> http://lists.infradead.org/pipermail/linux-arm-kernel/2012-June/105321.html >>> I think the context was slightly different. As I re-read the thread, it >>> seems that the discussion was around whether to use some SMC interface >>> or not based on whether the kernel is running secure or non-secure. The >>> argument made by Will was to actually specify the type of the firmware >>> SMC interface in the DT and use it in the kernel (and probably assume >>> the kernel is running in secure mode if no smc interface is specified in >>> the DT; you could have both though, running in secure mode and also >>> having firmware). >>> >>> In this arch timer case, we need to work around a firmware bug (or >>> feature as 32-bit ARM kernels never required CNTVOFF initialisation by >>> firmware, no matter how small such firmware is). We don't expect a >>> specific SMC call to initialise CNTVOFF, so we can't describe it in the >>> DT. >> Agreed, we can't described SMC calls that don't exist. From my >> perspective it's just another part of the cpu boot sequence that needs >> to be handled in the kernel, so describing the requirement via the >> cpu-boot method seems appropriate. It seems like we're making it harder >> than it should be by handling the undef when we could have slightly >> different SMP boot code (and suspend/resume code) depending on the boot >> method property. > > +heiko > > So, for the case of rk3288, based on this discussion what I'm going to > propose is to add code to rockchip.c which looks for a particular SMP > enable method -- say something like "rockchip,rk3288-smp-secure-svc" > which will then assume we have been booted in secure SVC mode and do > the CNTVOFF fixup. I believe, it will need to do this on the boot CPU > as well, so I think it will need to scan the DT fairly early on the > boot CPU and also perform the function there. > > I'll look into implementing this and post code. Comments and > suggestions appreciated, thanks. What goes wrong if we read the cntvoff from the boot CPU during smp_prepare_cpus() phase and use that to set the cntvoff on the other CPUs? That avoids needing to do anything very early by making the value the same. It does mean that cntvoff is some random out of reset value for CPU0, but at least it's consistent.
On Mon, Sep 15, 2014 at 2:49 PM, Stephen Boyd <sboyd@codeaurora.org> wrote: > On 09/15/14 14:47, Sonny Rao wrote: >> On Mon, Sep 15, 2014 at 1:33 PM, Stephen Boyd <sboyd@codeaurora.org> wrote: >>> On 09/15/14 04:10, Catalin Marinas wrote: >>>> On Fri, Sep 12, 2014 at 07:59:29PM +0100, Stephen Boyd wrote: >>>>> On 09/12/14 05:14, Marc Zyngier wrote: >>>>>> We surely can handle the UNDEF and do something there. We just can't do >>>>>> it the way Doug described it above. >>>>> I suggested doing that for something else a while ago and Will and Dave >>>>> we're not thrilled[1]. The suggestion back then was to use DT to >>>>> indicate what mode the kernel is running in. >>>>> >>>>> [1] >>>>> http://lists.infradead.org/pipermail/linux-arm-kernel/2012-June/105321.html >>>> I think the context was slightly different. As I re-read the thread, it >>>> seems that the discussion was around whether to use some SMC interface >>>> or not based on whether the kernel is running secure or non-secure. The >>>> argument made by Will was to actually specify the type of the firmware >>>> SMC interface in the DT and use it in the kernel (and probably assume >>>> the kernel is running in secure mode if no smc interface is specified in >>>> the DT; you could have both though, running in secure mode and also >>>> having firmware). >>>> >>>> In this arch timer case, we need to work around a firmware bug (or >>>> feature as 32-bit ARM kernels never required CNTVOFF initialisation by >>>> firmware, no matter how small such firmware is). We don't expect a >>>> specific SMC call to initialise CNTVOFF, so we can't describe it in the >>>> DT. >>> Agreed, we can't described SMC calls that don't exist. From my >>> perspective it's just another part of the cpu boot sequence that needs >>> to be handled in the kernel, so describing the requirement via the >>> cpu-boot method seems appropriate. It seems like we're making it harder >>> than it should be by handling the undef when we could have slightly >>> different SMP boot code (and suspend/resume code) depending on the boot >>> method property. >> >> +heiko >> >> So, for the case of rk3288, based on this discussion what I'm going to >> propose is to add code to rockchip.c which looks for a particular SMP >> enable method -- say something like "rockchip,rk3288-smp-secure-svc" >> which will then assume we have been booted in secure SVC mode and do >> the CNTVOFF fixup. I believe, it will need to do this on the boot CPU >> as well, so I think it will need to scan the DT fairly early on the >> boot CPU and also perform the function there. >> >> I'll look into implementing this and post code. Comments and >> suggestions appreciated, thanks. > > What goes wrong if we read the cntvoff from the boot CPU during > smp_prepare_cpus() phase and use that to set the cntvoff on the other > CPUs? That avoids needing to do anything very early by making the value > the same. It does mean that cntvoff is some random out of reset value > for CPU0, but at least it's consistent. I think we cannot read the value if we're not in hyp mode. > > -- > Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, > hosted by The Linux Foundation >
On Mon, Sep 15, 2014 at 2:52 PM, Sonny Rao <sonnyrao@chromium.org> wrote: > On Mon, Sep 15, 2014 at 2:49 PM, Stephen Boyd <sboyd@codeaurora.org> wrote: >> On 09/15/14 14:47, Sonny Rao wrote: >>> On Mon, Sep 15, 2014 at 1:33 PM, Stephen Boyd <sboyd@codeaurora.org> wrote: >>>> On 09/15/14 04:10, Catalin Marinas wrote: >>>>> On Fri, Sep 12, 2014 at 07:59:29PM +0100, Stephen Boyd wrote: >>>>>> On 09/12/14 05:14, Marc Zyngier wrote: >>>>>>> We surely can handle the UNDEF and do something there. We just can't do >>>>>>> it the way Doug described it above. >>>>>> I suggested doing that for something else a while ago and Will and Dave >>>>>> we're not thrilled[1]. The suggestion back then was to use DT to >>>>>> indicate what mode the kernel is running in. >>>>>> >>>>>> [1] >>>>>> http://lists.infradead.org/pipermail/linux-arm-kernel/2012-June/105321.html >>>>> I think the context was slightly different. As I re-read the thread, it >>>>> seems that the discussion was around whether to use some SMC interface >>>>> or not based on whether the kernel is running secure or non-secure. The >>>>> argument made by Will was to actually specify the type of the firmware >>>>> SMC interface in the DT and use it in the kernel (and probably assume >>>>> the kernel is running in secure mode if no smc interface is specified in >>>>> the DT; you could have both though, running in secure mode and also >>>>> having firmware). >>>>> >>>>> In this arch timer case, we need to work around a firmware bug (or >>>>> feature as 32-bit ARM kernels never required CNTVOFF initialisation by >>>>> firmware, no matter how small such firmware is). We don't expect a >>>>> specific SMC call to initialise CNTVOFF, so we can't describe it in the >>>>> DT. >>>> Agreed, we can't described SMC calls that don't exist. From my >>>> perspective it's just another part of the cpu boot sequence that needs >>>> to be handled in the kernel, so describing the requirement via the >>>> cpu-boot method seems appropriate. It seems like we're making it harder >>>> than it should be by handling the undef when we could have slightly >>>> different SMP boot code (and suspend/resume code) depending on the boot >>>> method property. >>> >>> +heiko >>> >>> So, for the case of rk3288, based on this discussion what I'm going to >>> propose is to add code to rockchip.c which looks for a particular SMP >>> enable method -- say something like "rockchip,rk3288-smp-secure-svc" >>> which will then assume we have been booted in secure SVC mode and do >>> the CNTVOFF fixup. I believe, it will need to do this on the boot CPU >>> as well, so I think it will need to scan the DT fairly early on the >>> boot CPU and also perform the function there. >>> >>> I'll look into implementing this and post code. Comments and >>> suggestions appreciated, thanks. >> >> What goes wrong if we read the cntvoff from the boot CPU during >> smp_prepare_cpus() phase and use that to set the cntvoff on the other >> CPUs? That avoids needing to do anything very early by making the value >> the same. It does mean that cntvoff is some random out of reset value >> for CPU0, but at least it's consistent. > > I think we cannot read the value if we're not in hyp mode. Well, thinking about it a little more, I think you still have a good point. We don't need to do this early on, as long as we haven't started using the arch timers yet. If we are still able to do this at the point where we're executing the code in arch/arm/mach-rockchip/platsmp.c that finds the enable method then we can just handle it there. > > >> >> -- >> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, >> hosted by The Linux Foundation >>
Hi Sonny, On 09/15/2014 06:04 PM, Sonny Rao wrote: > On Mon, Sep 15, 2014 at 2:52 PM, Sonny Rao <sonnyrao@chromium.org> wrote: >> On Mon, Sep 15, 2014 at 2:49 PM, Stephen Boyd <sboyd@codeaurora.org> wrote: >>> On 09/15/14 14:47, Sonny Rao wrote: >>>> On Mon, Sep 15, 2014 at 1:33 PM, Stephen Boyd <sboyd@codeaurora.org> wrote: >>>>> On 09/15/14 04:10, Catalin Marinas wrote: >>>>>> On Fri, Sep 12, 2014 at 07:59:29PM +0100, Stephen Boyd wrote: >>>>>>> On 09/12/14 05:14, Marc Zyngier wrote: >>>>>>>> We surely can handle the UNDEF and do something there. We just can't do >>>>>>>> it the way Doug described it above. >>>>>>> I suggested doing that for something else a while ago and Will and Dave >>>>>>> we're not thrilled[1]. The suggestion back then was to use DT to >>>>>>> indicate what mode the kernel is running in. >>>>>>> >>>>>>> [1] >>>>>>> http://lists.infradead.org/pipermail/linux-arm-kernel/2012-June/105321.html >>>>>> I think the context was slightly different. As I re-read the thread, it >>>>>> seems that the discussion was around whether to use some SMC interface >>>>>> or not based on whether the kernel is running secure or non-secure. The >>>>>> argument made by Will was to actually specify the type of the firmware >>>>>> SMC interface in the DT and use it in the kernel (and probably assume >>>>>> the kernel is running in secure mode if no smc interface is specified in >>>>>> the DT; you could have both though, running in secure mode and also >>>>>> having firmware). >>>>>> >>>>>> In this arch timer case, we need to work around a firmware bug (or >>>>>> feature as 32-bit ARM kernels never required CNTVOFF initialisation by >>>>>> firmware, no matter how small such firmware is). We don't expect a >>>>>> specific SMC call to initialise CNTVOFF, so we can't describe it in the >>>>>> DT. >>>>> Agreed, we can't described SMC calls that don't exist. From my >>>>> perspective it's just another part of the cpu boot sequence that needs >>>>> to be handled in the kernel, so describing the requirement via the >>>>> cpu-boot method seems appropriate. It seems like we're making it harder >>>>> than it should be by handling the undef when we could have slightly >>>>> different SMP boot code (and suspend/resume code) depending on the boot >>>>> method property. >>>> >>>> +heiko >>>> >>>> So, for the case of rk3288, based on this discussion what I'm going to >>>> propose is to add code to rockchip.c which looks for a particular SMP >>>> enable method -- say something like "rockchip,rk3288-smp-secure-svc" >>>> which will then assume we have been booted in secure SVC mode and do >>>> the CNTVOFF fixup. I believe, it will need to do this on the boot CPU >>>> as well, so I think it will need to scan the DT fairly early on the >>>> boot CPU and also perform the function there. >>>> >>>> I'll look into implementing this and post code. Comments and >>>> suggestions appreciated, thanks. >>> >>> What goes wrong if we read the cntvoff from the boot CPU during >>> smp_prepare_cpus() phase and use that to set the cntvoff on the other >>> CPUs? That avoids needing to do anything very early by making the value >>> the same. It does mean that cntvoff is some random out of reset value >>> for CPU0, but at least it's consistent. >> >> I think we cannot read the value if we're not in hyp mode. > > Well, thinking about it a little more, I think you still have a good point. > > We don't need to do this early on, as long as we haven't started using > the arch timers yet. If we are still able to do this at the point > where we're executing the code in arch/arm/mach-rockchip/platsmp.c > that finds the enable method then we can just handle it there. I've been playing around with the probe-based approach and while I need to do a lot more testing, it seems to be working for the first tens of instructions. I hope to be able to share a draft of that soon. Basically, I just read the current NSACR value and write it back (although maybe in the long term we would want to make sure a few of those bits are set or cleared). If that succeeds, we know we're in secure SVC and can proceed to set up MON and HYP. Christopher
On Mon, Sep 15, 2014 at 3:51 PM, Christopher Covington <cov@codeaurora.org> wrote: > Hi Sonny, > > On 09/15/2014 06:04 PM, Sonny Rao wrote: >> On Mon, Sep 15, 2014 at 2:52 PM, Sonny Rao <sonnyrao@chromium.org> wrote: >>> On Mon, Sep 15, 2014 at 2:49 PM, Stephen Boyd <sboyd@codeaurora.org> wrote: >>>> On 09/15/14 14:47, Sonny Rao wrote: >>>>> On Mon, Sep 15, 2014 at 1:33 PM, Stephen Boyd <sboyd@codeaurora.org> wrote: >>>>>> On 09/15/14 04:10, Catalin Marinas wrote: >>>>>>> On Fri, Sep 12, 2014 at 07:59:29PM +0100, Stephen Boyd wrote: >>>>>>>> On 09/12/14 05:14, Marc Zyngier wrote: >>>>>>>>> We surely can handle the UNDEF and do something there. We just can't do >>>>>>>>> it the way Doug described it above. >>>>>>>> I suggested doing that for something else a while ago and Will and Dave >>>>>>>> we're not thrilled[1]. The suggestion back then was to use DT to >>>>>>>> indicate what mode the kernel is running in. >>>>>>>> >>>>>>>> [1] >>>>>>>> http://lists.infradead.org/pipermail/linux-arm-kernel/2012-June/105321.html >>>>>>> I think the context was slightly different. As I re-read the thread, it >>>>>>> seems that the discussion was around whether to use some SMC interface >>>>>>> or not based on whether the kernel is running secure or non-secure. The >>>>>>> argument made by Will was to actually specify the type of the firmware >>>>>>> SMC interface in the DT and use it in the kernel (and probably assume >>>>>>> the kernel is running in secure mode if no smc interface is specified in >>>>>>> the DT; you could have both though, running in secure mode and also >>>>>>> having firmware). >>>>>>> >>>>>>> In this arch timer case, we need to work around a firmware bug (or >>>>>>> feature as 32-bit ARM kernels never required CNTVOFF initialisation by >>>>>>> firmware, no matter how small such firmware is). We don't expect a >>>>>>> specific SMC call to initialise CNTVOFF, so we can't describe it in the >>>>>>> DT. >>>>>> Agreed, we can't described SMC calls that don't exist. From my >>>>>> perspective it's just another part of the cpu boot sequence that needs >>>>>> to be handled in the kernel, so describing the requirement via the >>>>>> cpu-boot method seems appropriate. It seems like we're making it harder >>>>>> than it should be by handling the undef when we could have slightly >>>>>> different SMP boot code (and suspend/resume code) depending on the boot >>>>>> method property. >>>>> >>>>> +heiko >>>>> >>>>> So, for the case of rk3288, based on this discussion what I'm going to >>>>> propose is to add code to rockchip.c which looks for a particular SMP >>>>> enable method -- say something like "rockchip,rk3288-smp-secure-svc" >>>>> which will then assume we have been booted in secure SVC mode and do >>>>> the CNTVOFF fixup. I believe, it will need to do this on the boot CPU >>>>> as well, so I think it will need to scan the DT fairly early on the >>>>> boot CPU and also perform the function there. >>>>> >>>>> I'll look into implementing this and post code. Comments and >>>>> suggestions appreciated, thanks. >>>> >>>> What goes wrong if we read the cntvoff from the boot CPU during >>>> smp_prepare_cpus() phase and use that to set the cntvoff on the other >>>> CPUs? That avoids needing to do anything very early by making the value >>>> the same. It does mean that cntvoff is some random out of reset value >>>> for CPU0, but at least it's consistent. >>> >>> I think we cannot read the value if we're not in hyp mode. >> >> Well, thinking about it a little more, I think you still have a good point. >> >> We don't need to do this early on, as long as we haven't started using >> the arch timers yet. If we are still able to do this at the point >> where we're executing the code in arch/arm/mach-rockchip/platsmp.c >> that finds the enable method then we can just handle it there. > > I've been playing around with the probe-based approach and while I need to do > a lot more testing, it seems to be working for the first tens of instructions. > I hope to be able to share a draft of that soon. Basically, I just read the > current NSACR value and write it back (although maybe in the long term we > would want to make sure a few of those bits are set or cleared). If that > succeeds, we know we're in secure SVC and can proceed to set up MON and HYP. Christopher, sounds promising, please do share, thanks! Marc or Will, what do you guys think about this approach? > > Christopher > > -- > Employee of Qualcomm Innovation Center, Inc. > Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, > hosted by the Linux Foundation.
On Mon, Sep 15, 2014 at 11:51:14PM +0100, Christopher Covington wrote: > Hi Sonny, > > On 09/15/2014 06:04 PM, Sonny Rao wrote: > > On Mon, Sep 15, 2014 at 2:52 PM, Sonny Rao <sonnyrao@chromium.org> wrote: > >> On Mon, Sep 15, 2014 at 2:49 PM, Stephen Boyd <sboyd@codeaurora.org> wrote: > >>> On 09/15/14 14:47, Sonny Rao wrote: > >>>> On Mon, Sep 15, 2014 at 1:33 PM, Stephen Boyd <sboyd@codeaurora.org> wrote: > >>>>> On 09/15/14 04:10, Catalin Marinas wrote: > >>>>>> On Fri, Sep 12, 2014 at 07:59:29PM +0100, Stephen Boyd wrote: > >>>>>>> On 09/12/14 05:14, Marc Zyngier wrote: > >>>>>>>> We surely can handle the UNDEF and do something there. We just can't do > >>>>>>>> it the way Doug described it above. > >>>>>>> I suggested doing that for something else a while ago and Will and Dave > >>>>>>> we're not thrilled[1]. The suggestion back then was to use DT to > >>>>>>> indicate what mode the kernel is running in. > >>>>>>> > >>>>>>> [1] > >>>>>>> http://lists.infradead.org/pipermail/linux-arm-kernel/2012-June/105321.html > >>>>>> I think the context was slightly different. As I re-read the thread, it > >>>>>> seems that the discussion was around whether to use some SMC interface > >>>>>> or not based on whether the kernel is running secure or non-secure. The > >>>>>> argument made by Will was to actually specify the type of the firmware > >>>>>> SMC interface in the DT and use it in the kernel (and probably assume > >>>>>> the kernel is running in secure mode if no smc interface is specified in > >>>>>> the DT; you could have both though, running in secure mode and also > >>>>>> having firmware). > >>>>>> > >>>>>> In this arch timer case, we need to work around a firmware bug (or > >>>>>> feature as 32-bit ARM kernels never required CNTVOFF initialisation by > >>>>>> firmware, no matter how small such firmware is). We don't expect a > >>>>>> specific SMC call to initialise CNTVOFF, so we can't describe it in the > >>>>>> DT. > >>>>> Agreed, we can't described SMC calls that don't exist. From my > >>>>> perspective it's just another part of the cpu boot sequence that needs > >>>>> to be handled in the kernel, so describing the requirement via the > >>>>> cpu-boot method seems appropriate. It seems like we're making it harder > >>>>> than it should be by handling the undef when we could have slightly > >>>>> different SMP boot code (and suspend/resume code) depending on the boot > >>>>> method property. > >>>> > >>>> +heiko > >>>> > >>>> So, for the case of rk3288, based on this discussion what I'm going to > >>>> propose is to add code to rockchip.c which looks for a particular SMP > >>>> enable method -- say something like "rockchip,rk3288-smp-secure-svc" > >>>> which will then assume we have been booted in secure SVC mode and do > >>>> the CNTVOFF fixup. I believe, it will need to do this on the boot CPU > >>>> as well, so I think it will need to scan the DT fairly early on the > >>>> boot CPU and also perform the function there. > >>>> > >>>> I'll look into implementing this and post code. Comments and > >>>> suggestions appreciated, thanks. > >>> > >>> What goes wrong if we read the cntvoff from the boot CPU during > >>> smp_prepare_cpus() phase and use that to set the cntvoff on the other > >>> CPUs? That avoids needing to do anything very early by making the value > >>> the same. It does mean that cntvoff is some random out of reset value > >>> for CPU0, but at least it's consistent. > >> > >> I think we cannot read the value if we're not in hyp mode. > > > > Well, thinking about it a little more, I think you still have a good point. > > > > We don't need to do this early on, as long as we haven't started using > > the arch timers yet. If we are still able to do this at the point > > where we're executing the code in arch/arm/mach-rockchip/platsmp.c > > that finds the enable method then we can just handle it there. > > I've been playing around with the probe-based approach and while I need to do > a lot more testing, it seems to be working for the first tens of instructions. > I hope to be able to share a draft of that soon. Basically, I just read the > current NSACR value and write it back (although maybe in the long term we > would want to make sure a few of those bits are set or cleared). If that > succeeds, we know we're in secure SVC and can proceed to set up MON and HYP. But when it doesn't succeed, you get an undefined instruction fault (since NSACR is only writable in secure mode).
On Mon, Sep 15, 2014 at 09:33:03PM +0100, Stephen Boyd wrote: > On 09/15/14 04:10, Catalin Marinas wrote: > > On Fri, Sep 12, 2014 at 07:59:29PM +0100, Stephen Boyd wrote: > >> On 09/12/14 05:14, Marc Zyngier wrote: > >>> We surely can handle the UNDEF and do something there. We just can't do > >>> it the way Doug described it above. > >> I suggested doing that for something else a while ago and Will and Dave > >> we're not thrilled[1]. The suggestion back then was to use DT to > >> indicate what mode the kernel is running in. > >> > >> [1] > >> http://lists.infradead.org/pipermail/linux-arm-kernel/2012-June/105321.html > > I think the context was slightly different. As I re-read the thread, it > > seems that the discussion was around whether to use some SMC interface > > or not based on whether the kernel is running secure or non-secure. The > > argument made by Will was to actually specify the type of the firmware > > SMC interface in the DT and use it in the kernel (and probably assume > > the kernel is running in secure mode if no smc interface is specified in > > the DT; you could have both though, running in secure mode and also > > having firmware). > > > > In this arch timer case, we need to work around a firmware bug (or > > feature as 32-bit ARM kernels never required CNTVOFF initialisation by > > firmware, no matter how small such firmware is). We don't expect a > > specific SMC call to initialise CNTVOFF, so we can't describe it in the > > DT. > > Agreed, we can't described SMC calls that don't exist. From my > perspective it's just another part of the cpu boot sequence that needs > to be handled in the kernel, so describing the requirement via the > cpu-boot method seems appropriate. It seems like we're making it harder > than it should be by handling the undef when we could have slightly > different SMP boot code (and suspend/resume code) depending on the boot > method property. For 32-bit ARM SoCs, I think you can describe this via some specific enable-method property. What I don't like though is the multitude of enable methods (trying to reduce them on arm64) and the fact that registers like CNTVOFF are rather architecture than SoC specific.
On 09/16/2014 06:42 AM, Catalin Marinas wrote: > On Mon, Sep 15, 2014 at 11:51:14PM +0100, Christopher Covington wrote: >> Hi Sonny, >> >> On 09/15/2014 06:04 PM, Sonny Rao wrote: >>> On Mon, Sep 15, 2014 at 2:52 PM, Sonny Rao <sonnyrao@chromium.org> wrote: >>>> On Mon, Sep 15, 2014 at 2:49 PM, Stephen Boyd <sboyd@codeaurora.org> wrote: >>>>> On 09/15/14 14:47, Sonny Rao wrote: >>>>>> On Mon, Sep 15, 2014 at 1:33 PM, Stephen Boyd <sboyd@codeaurora.org> wrote: >>>>>>> On 09/15/14 04:10, Catalin Marinas wrote: >>>>>>>> On Fri, Sep 12, 2014 at 07:59:29PM +0100, Stephen Boyd wrote: >>>>>>>>> On 09/12/14 05:14, Marc Zyngier wrote: >>>>>>>>>> We surely can handle the UNDEF and do something there. We just can't do >>>>>>>>>> it the way Doug described it above. >>>>>>>>> I suggested doing that for something else a while ago and Will and Dave >>>>>>>>> we're not thrilled[1]. The suggestion back then was to use DT to >>>>>>>>> indicate what mode the kernel is running in. >>>>>>>>> >>>>>>>>> [1] >>>>>>>>> http://lists.infradead.org/pipermail/linux-arm-kernel/2012-June/105321.html >>>>>>>> I think the context was slightly different. As I re-read the thread, it >>>>>>>> seems that the discussion was around whether to use some SMC interface >>>>>>>> or not based on whether the kernel is running secure or non-secure. The >>>>>>>> argument made by Will was to actually specify the type of the firmware >>>>>>>> SMC interface in the DT and use it in the kernel (and probably assume >>>>>>>> the kernel is running in secure mode if no smc interface is specified in >>>>>>>> the DT; you could have both though, running in secure mode and also >>>>>>>> having firmware). >>>>>>>> >>>>>>>> In this arch timer case, we need to work around a firmware bug (or >>>>>>>> feature as 32-bit ARM kernels never required CNTVOFF initialisation by >>>>>>>> firmware, no matter how small such firmware is). We don't expect a >>>>>>>> specific SMC call to initialise CNTVOFF, so we can't describe it in the >>>>>>>> DT. >>>>>>> Agreed, we can't described SMC calls that don't exist. From my >>>>>>> perspective it's just another part of the cpu boot sequence that needs >>>>>>> to be handled in the kernel, so describing the requirement via the >>>>>>> cpu-boot method seems appropriate. It seems like we're making it harder >>>>>>> than it should be by handling the undef when we could have slightly >>>>>>> different SMP boot code (and suspend/resume code) depending on the boot >>>>>>> method property. >>>>>> >>>>>> +heiko >>>>>> >>>>>> So, for the case of rk3288, based on this discussion what I'm going to >>>>>> propose is to add code to rockchip.c which looks for a particular SMP >>>>>> enable method -- say something like "rockchip,rk3288-smp-secure-svc" >>>>>> which will then assume we have been booted in secure SVC mode and do >>>>>> the CNTVOFF fixup. I believe, it will need to do this on the boot CPU >>>>>> as well, so I think it will need to scan the DT fairly early on the >>>>>> boot CPU and also perform the function there. >>>>>> >>>>>> I'll look into implementing this and post code. Comments and >>>>>> suggestions appreciated, thanks. >>>>> >>>>> What goes wrong if we read the cntvoff from the boot CPU during >>>>> smp_prepare_cpus() phase and use that to set the cntvoff on the other >>>>> CPUs? That avoids needing to do anything very early by making the value >>>>> the same. It does mean that cntvoff is some random out of reset value >>>>> for CPU0, but at least it's consistent. >>>> >>>> I think we cannot read the value if we're not in hyp mode. >>> >>> Well, thinking about it a little more, I think you still have a good point. >>> >>> We don't need to do this early on, as long as we haven't started using >>> the arch timers yet. If we are still able to do this at the point >>> where we're executing the code in arch/arm/mach-rockchip/platsmp.c >>> that finds the enable method then we can just handle it there. >> >> I've been playing around with the probe-based approach and while I need to do >> a lot more testing, it seems to be working for the first tens of instructions. >> I hope to be able to share a draft of that soon. Basically, I just read the >> current NSACR value and write it back (although maybe in the long term we >> would want to make sure a few of those bits are set or cleared). If that >> succeeds, we know we're in secure SVC and can proceed to set up MON and HYP. > > But when it doesn't succeed, you get an undefined instruction fault > (since NSACR is only writable in secure mode). Yes. I see it as a conditional branch to VBAR+4 with a mode switch side effect. Christopher
diff --git a/Documentation/devicetree/bindings/arm/arch_timer.txt b/Documentation/devicetree/bindings/arm/arch_timer.txt index 37b2caf..876d32b 100644 --- a/Documentation/devicetree/bindings/arm/arch_timer.txt +++ b/Documentation/devicetree/bindings/arm/arch_timer.txt @@ -22,6 +22,12 @@ to deliver its interrupts via SPIs. - always-on : a boolean property. If present, the timer is powered through an always-on power domain, therefore it never loses context. +** Optional properties: + +- arm,use-physical-timer : Don't ever use the virtual timer, just use the + physical one. Not supported for ARM64. + + Example: timer { diff --git a/drivers/clocksource/arm_arch_timer.c b/drivers/clocksource/arm_arch_timer.c index 5163ec1..e7aa256 100644 --- a/drivers/clocksource/arm_arch_timer.c +++ b/drivers/clocksource/arm_arch_timer.c @@ -649,6 +649,11 @@ static void __init arch_timer_init(struct device_node *np) arch_timer_ppi[i] = irq_of_parse_and_map(np, i); arch_timer_detect_rate(NULL, np); +#ifdef CONFIG_ARM + if (of_property_read_bool(np, "arm,use-physical-timer")) + arch_timer_use_virtual = false; +#endif + /* * If HYP mode is available, we know that the physical timer * has been configured to be accessible from PL1. Use it, so