Re: SMCCC 1.1 support for psci(4)

2018-05-22 Thread Mike Larkin 
On Tue, May 22, 2018 at 07:35:59PM +0200, Mark Kettenis wrote:
> > Date: Fri, 4 May 2018 20:14:38 +0200 (CEST)
> > From: Mark Kettenis 
> > 
> > > Date: Tue, 1 May 2018 11:55:00 +0200 (CEST)
> > > From: Mark Kettenis 
> > > 
> > > So after adding a quick hack to mitigate Spectre variant 2 to ARM
> > > Trusted Firmware (ATF), ARM actually designed a proper solution that
> > > minimizes the performance loss and makes the presence of the
> > > workaround detectable.  This is all documented in an update of the SMC
> > > Calling Convention (SMCCC) standard.
> > > 
> > > The diff below implements support for this solution while keeping
> > > support for the hack.  While ARM strongly suggests vendors to update
> > > to a version of ATF that implements SMCCC 1.1 the current ATF for the
> > > Marvell ARMADA 8040 hasn't been updated yet (but does include the
> > > initial hack).
> > 
> > So it turns out that while I wasn't paying attention, the Marvell
> > folks backported SMCCC 1.1 support to their ATF fork.  This means we
> > can drop support for the PSCI_VERSION hack without leaving
> > MACCHIATOBin owener out in the cold.  I think the code simplification
> > is worth it.  It also means we can tell whether a machine is safe or
> > not from a dmesg.
> > 
> > ok?
> 
> And in light of Spectre variant 4, I think there is even less reason
> to continue support for firmware that doesn't implement SMCCC 1.1.
> 
> I'd like to commit this diff such that I can improve the way we signal
> that mitigations are present in the firmware.
> 
> ok?
> 

Diff and reasoning make sense to me. ok mlarkin

> > Index: dev/fdt/psci.c
> > ===
> > RCS file: /cvs/src/sys/dev/fdt/psci.c,v
> > retrieving revision 1.7
> > diff -u -p -r1.7 psci.c
> > --- dev/fdt/psci.c  3 May 2018 09:45:57 -   1.7
> > +++ dev/fdt/psci.c  4 May 2018 18:01:04 -
> > @@ -54,7 +54,6 @@ struct psci_softc {
> > uint32_t sc_system_reset;
> > uint32_t sc_cpu_on;
> >  
> > -   uint32_t sc_version;
> > uint32_t sc_smccc_version;
> >  };
> >  
> > @@ -98,6 +97,7 @@ psci_attach(struct device *parent, struc
> > struct psci_softc *sc = (struct psci_softc *)self;
> > struct fdt_attach_args *faa = aux;
> > char method[128];
> > +   uint32_t version;
> >  
> > if (OF_getprop(faa->fa_node, "method", method, sizeof(method))) {
> > if (strcmp(method, "hvc") == 0)
> > @@ -127,10 +127,10 @@ psci_attach(struct device *parent, struc
> >  
> > psci_sc = sc;
> >  
> > -   sc->sc_version = psci_version();
> > -   printf(": PSCI %d.%d", sc->sc_version >> 16, sc->sc_version & 0x);
> > +   version = psci_version();
> > +   printf(": PSCI %d.%d", version >> 16, version & 0x);
> >  
> > -   if (sc->sc_version >= 0x1) {
> > +   if (version >= 0x1) {
> > if (psci_features(SMCCC_VERSION) == PSCI_SUCCESS) {
> > sc->sc_smccc_version = smccc_version();
> > printf(", SMCCC %d.%d", sc->sc_smccc_version >> 16,
> > @@ -165,11 +165,9 @@ psci_powerdown(void)
> >  }
> >  
> >  /*
> > - * Firmware-based workaround for CVE-2017-5715.  We pick the
> > - * appropriate mechanism based on the PSCI and SMCCC versions.  We
> > - * determine whether the workaround is actually needed the first time
> > - * we are invoked such that we only make the firmware call if we
> > - * really need to.
> > + * Firmware-based workaround for CVE-2017-5715.  We determine whether
> > + * the workaround is actually implemented and needed the first time we
> > + * are invoked such that we only make the firmware call when appropriate.
> >   */
> >  
> >  void
> > @@ -178,14 +176,6 @@ psci_flush_bp_none(void)
> >  }
> >  
> >  void
> > -psci_flush_bp_psci_version(void)
> > -{
> > -   struct psci_softc *sc = psci_sc;
> > -
> > -   (*sc->sc_callfn)(PSCI_VERSION, 0, 0, 0);
> > -}
> > -
> > -void
> >  psci_flush_bp_smccc_arch_workaround_1(void)
> >  {
> > struct psci_softc *sc = psci_sc;
> > @@ -199,32 +189,17 @@ psci_flush_bp(void)
> > struct psci_softc *sc = psci_sc;
> > struct cpu_info *ci = curcpu();
> >  
> > -   /* No PSCI or an old version of PSCI; nothing we can do. */
> > -   if (sc == NULL || sc->sc_version < 0x1) {
> > -   ci->ci_flush_bp = psci_flush_bp_none;
> > -   return;
> > -   }
> > -
> > -   /*
> > -* PSCI 1.0 or later with SMCCC 1.0; invoke PSCI_VERSION and
> > -* hope for the best.
> > -*/
> > -   if (sc->sc_smccc_version < 0x10001) {
> > -   ci->ci_flush_bp = psci_flush_bp_psci_version;
> > -   ci->ci_flush_bp();
> > -   return;
> > -   }
> > -
> > /*
> > -* SMCCC 1.1 or later; we can actually detect if the
> > -* workaround is implemented and needed.
> > +* SMCCC 1.1 allows us to detect if the workaround is
> > +* implemented and needed.
> >  */
> > -   if

Re: SMCCC 1.1 support for psci(4)

2018-05-22 Thread Mark Kettenis 
> Date: Fri, 4 May 2018 20:14:38 +0200 (CEST)
> From: Mark Kettenis 
> 
> > Date: Tue, 1 May 2018 11:55:00 +0200 (CEST)
> > From: Mark Kettenis 
> > 
> > So after adding a quick hack to mitigate Spectre variant 2 to ARM
> > Trusted Firmware (ATF), ARM actually designed a proper solution that
> > minimizes the performance loss and makes the presence of the
> > workaround detectable.  This is all documented in an update of the SMC
> > Calling Convention (SMCCC) standard.
> > 
> > The diff below implements support for this solution while keeping
> > support for the hack.  While ARM strongly suggests vendors to update
> > to a version of ATF that implements SMCCC 1.1 the current ATF for the
> > Marvell ARMADA 8040 hasn't been updated yet (but does include the
> > initial hack).
> 
> So it turns out that while I wasn't paying attention, the Marvell
> folks backported SMCCC 1.1 support to their ATF fork.  This means we
> can drop support for the PSCI_VERSION hack without leaving
> MACCHIATOBin owener out in the cold.  I think the code simplification
> is worth it.  It also means we can tell whether a machine is safe or
> not from a dmesg.
> 
> ok?

And in light of Spectre variant 4, I think there is even less reason
to continue support for firmware that doesn't implement SMCCC 1.1.

I'd like to commit this diff such that I can improve the way we signal
that mitigations are present in the firmware.

ok?

> Index: dev/fdt/psci.c
> ===
> RCS file: /cvs/src/sys/dev/fdt/psci.c,v
> retrieving revision 1.7
> diff -u -p -r1.7 psci.c
> --- dev/fdt/psci.c3 May 2018 09:45:57 -   1.7
> +++ dev/fdt/psci.c4 May 2018 18:01:04 -
> @@ -54,7 +54,6 @@ struct psci_softc {
>   uint32_t sc_system_reset;
>   uint32_t sc_cpu_on;
>  
> - uint32_t sc_version;
>   uint32_t sc_smccc_version;
>  };
>  
> @@ -98,6 +97,7 @@ psci_attach(struct device *parent, struc
>   struct psci_softc *sc = (struct psci_softc *)self;
>   struct fdt_attach_args *faa = aux;
>   char method[128];
> + uint32_t version;
>  
>   if (OF_getprop(faa->fa_node, "method", method, sizeof(method))) {
>   if (strcmp(method, "hvc") == 0)
> @@ -127,10 +127,10 @@ psci_attach(struct device *parent, struc
>  
>   psci_sc = sc;
>  
> - sc->sc_version = psci_version();
> - printf(": PSCI %d.%d", sc->sc_version >> 16, sc->sc_version & 0x);
> + version = psci_version();
> + printf(": PSCI %d.%d", version >> 16, version & 0x);
>  
> - if (sc->sc_version >= 0x1) {
> + if (version >= 0x1) {
>   if (psci_features(SMCCC_VERSION) == PSCI_SUCCESS) {
>   sc->sc_smccc_version = smccc_version();
>   printf(", SMCCC %d.%d", sc->sc_smccc_version >> 16,
> @@ -165,11 +165,9 @@ psci_powerdown(void)
>  }
>  
>  /*
> - * Firmware-based workaround for CVE-2017-5715.  We pick the
> - * appropriate mechanism based on the PSCI and SMCCC versions.  We
> - * determine whether the workaround is actually needed the first time
> - * we are invoked such that we only make the firmware call if we
> - * really need to.
> + * Firmware-based workaround for CVE-2017-5715.  We determine whether
> + * the workaround is actually implemented and needed the first time we
> + * are invoked such that we only make the firmware call when appropriate.
>   */
>  
>  void
> @@ -178,14 +176,6 @@ psci_flush_bp_none(void)
>  }
>  
>  void
> -psci_flush_bp_psci_version(void)
> -{
> - struct psci_softc *sc = psci_sc;
> -
> - (*sc->sc_callfn)(PSCI_VERSION, 0, 0, 0);
> -}
> -
> -void
>  psci_flush_bp_smccc_arch_workaround_1(void)
>  {
>   struct psci_softc *sc = psci_sc;
> @@ -199,32 +189,17 @@ psci_flush_bp(void)
>   struct psci_softc *sc = psci_sc;
>   struct cpu_info *ci = curcpu();
>  
> - /* No PSCI or an old version of PSCI; nothing we can do. */
> - if (sc == NULL || sc->sc_version < 0x1) {
> - ci->ci_flush_bp = psci_flush_bp_none;
> - return;
> - }
> -
> - /*
> -  * PSCI 1.0 or later with SMCCC 1.0; invoke PSCI_VERSION and
> -  * hope for the best.
> -  */
> - if (sc->sc_smccc_version < 0x10001) {
> - ci->ci_flush_bp = psci_flush_bp_psci_version;
> - ci->ci_flush_bp();
> - return;
> - }
> -
>   /*
> -  * SMCCC 1.1 or later; we can actually detect if the
> -  * workaround is implemented and needed.
> +  * SMCCC 1.1 allows us to detect if the workaround is
> +  * implemented and needed.
>*/
> - if (smccc_arch_features(SMCCC_ARCH_WORKAROUND_1) == 0) {
> + if (sc && sc->sc_smccc_version >= 0x10001 &&
> + smccc_arch_features(SMCCC_ARCH_WORKAROUND_1) == 0) {
>   /* Workaround implemented and needed. */
>   ci->ci_flush_bp =

Re: SMCCC 1.1 support for psci(4)

2018-05-04 Thread Mark Kettenis 
> Date: Tue, 1 May 2018 11:55:00 +0200 (CEST)
> From: Mark Kettenis 
> 
> So after adding a quick hack to mitigate Spectre variant 2 to ARM
> Trusted Firmware (ATF), ARM actually designed a proper solution that
> minimizes the performance loss and makes the presence of the
> workaround detectable.  This is all documented in an update of the SMC
> Calling Convention (SMCCC) standard.
> 
> The diff below implements support for this solution while keeping
> support for the hack.  While ARM strongly suggests vendors to update
> to a version of ATF that implements SMCCC 1.1 the current ATF for the
> Marvell ARMADA 8040 hasn't been updated yet (but does include the
> initial hack).

So it turns out that while I wasn't paying attention, the Marvell
folks backported SMCCC 1.1 support to their ATF fork.  This means we
can drop support for the PSCI_VERSION hack without leaving
MACCHIATOBin owener out in the cold.  I think the code simplification
is worth it.  It also means we can tell whether a machine is safe or
not from a dmesg.

ok?

Index: dev/fdt/psci.c
===
RCS file: /cvs/src/sys/dev/fdt/psci.c,v
retrieving revision 1.7
diff -u -p -r1.7 psci.c
--- dev/fdt/psci.c  3 May 2018 09:45:57 -   1.7
+++ dev/fdt/psci.c  4 May 2018 18:01:04 -
@@ -54,7 +54,6 @@ struct psci_softc {
uint32_t sc_system_reset;
uint32_t sc_cpu_on;
 
-   uint32_t sc_version;
uint32_t sc_smccc_version;
 };
 
@@ -98,6 +97,7 @@ psci_attach(struct device *parent, struc
struct psci_softc *sc = (struct psci_softc *)self;
struct fdt_attach_args *faa = aux;
char method[128];
+   uint32_t version;
 
if (OF_getprop(faa->fa_node, "method", method, sizeof(method))) {
if (strcmp(method, "hvc") == 0)
@@ -127,10 +127,10 @@ psci_attach(struct device *parent, struc
 
psci_sc = sc;
 
-   sc->sc_version = psci_version();
-   printf(": PSCI %d.%d", sc->sc_version >> 16, sc->sc_version & 0x);
+   version = psci_version();
+   printf(": PSCI %d.%d", version >> 16, version & 0x);
 
-   if (sc->sc_version >= 0x1) {
+   if (version >= 0x1) {
if (psci_features(SMCCC_VERSION) == PSCI_SUCCESS) {
sc->sc_smccc_version = smccc_version();
printf(", SMCCC %d.%d", sc->sc_smccc_version >> 16,
@@ -165,11 +165,9 @@ psci_powerdown(void)
 }
 
 /*
- * Firmware-based workaround for CVE-2017-5715.  We pick the
- * appropriate mechanism based on the PSCI and SMCCC versions.  We
- * determine whether the workaround is actually needed the first time
- * we are invoked such that we only make the firmware call if we
- * really need to.
+ * Firmware-based workaround for CVE-2017-5715.  We determine whether
+ * the workaround is actually implemented and needed the first time we
+ * are invoked such that we only make the firmware call when appropriate.
  */
 
 void
@@ -178,14 +176,6 @@ psci_flush_bp_none(void)
 }
 
 void
-psci_flush_bp_psci_version(void)
-{
-   struct psci_softc *sc = psci_sc;
-
-   (*sc->sc_callfn)(PSCI_VERSION, 0, 0, 0);
-}
-
-void
 psci_flush_bp_smccc_arch_workaround_1(void)
 {
struct psci_softc *sc = psci_sc;
@@ -199,32 +189,17 @@ psci_flush_bp(void)
struct psci_softc *sc = psci_sc;
struct cpu_info *ci = curcpu();
 
-   /* No PSCI or an old version of PSCI; nothing we can do. */
-   if (sc == NULL || sc->sc_version < 0x1) {
-   ci->ci_flush_bp = psci_flush_bp_none;
-   return;
-   }
-
-   /*
-* PSCI 1.0 or later with SMCCC 1.0; invoke PSCI_VERSION and
-* hope for the best.
-*/
-   if (sc->sc_smccc_version < 0x10001) {
-   ci->ci_flush_bp = psci_flush_bp_psci_version;
-   ci->ci_flush_bp();
-   return;
-   }
-
/*
-* SMCCC 1.1 or later; we can actually detect if the
-* workaround is implemented and needed.
+* SMCCC 1.1 allows us to detect if the workaround is
+* implemented and needed.
 */
-   if (smccc_arch_features(SMCCC_ARCH_WORKAROUND_1) == 0) {
+   if (sc && sc->sc_smccc_version >= 0x10001 &&
+   smccc_arch_features(SMCCC_ARCH_WORKAROUND_1) == 0) {
/* Workaround implemented and needed. */
ci->ci_flush_bp = psci_flush_bp_smccc_arch_workaround_1;
ci->ci_flush_bp();
} else {
-   /* No workaround needed. */
+   /* Workaround isn't implemented or isn't needed. */
ci->ci_flush_bp = psci_flush_bp_none;
}
 }

Re: SMCCC 1.1 support for psci(4)

2018-05-03 Thread Dimitris Papastamos 
On Thu, May 03, 2018 at 11:32:42AM +0200, Mark Kettenis wrote:
> > Date: Thu, 3 May 2018 08:19:01 +0100
> > From: Dimitris Papastamos 
> 
> Hi Dimitris,
> 
> > On Tue, May 01, 2018 at 11:55:00AM +0200, Mark Kettenis wrote:
> > > So after adding a quick hack to mitigate Spectre variant 2 to ARM
> > > Trusted Firmware (ATF), ARM actually designed a proper solution that
> > > minimizes the performance loss and makes the presence of the
> > > workaround detectable.  This is all documented in an update of the SMC
> > > Calling Convention (SMCCC) standard.
> > > 
> > > The diff below implements support for this solution while keeping
> > > support for the hack.  While ARM strongly suggests vendors to update
> > > to a version of ATF that implements SMCCC 1.1 the current ATF for the
> > > Marvell ARMADA 8040 hasn't been updated yet (but does include the
> > > initial hack).
> > > 
> > > Unfortunately the SMCCC 1.1 implementation in ATF doesn't quite
> > > implement the spec.  As a result we have to check whether the
> > > workaround is implemented by issuing the relevant calls on each of the
> > > CPUs that might be affected.  This is important for big.LITTLE designs
> > > such as the RK3399 that include both Cortex-A53 cores that aren't
> > > vulnerable and Cortex-A72 cores that are.
> > 
> > Can you explain a bit the inconsistency between the spec and the TF
> > implementation?  If there is an issue in the TF implementation I can
> > fix it.
> > 
> > On a big.LITTLE system the expected behaviour is for
> > SMCCC_ARCH_FEATURES() to return 1 for the unaffected cores
> > (Cortex-A53) and 0 for the affected cores (Cortex-A72).  The call will
> > only return -1 if TF is built without mitigation support for
> > CVE-2017-5715.
> 
> Looks like I managed to confuse myself and drew the wrong conclusion
> from the description of SMCCC_ARCH_WORKAROUND_1 in section 2.2.4.4:
> 
>   In heterogeneous systems with some PEs that require mitigation and
>   others that do not, the firmware must provide a safe implementation
>   of this function on all PEs.
> 
> I now notice that 2.2.4.2 explicitly spells out the results of the
> SMCCC_ARCH)FEATURES call that matches the behaviour you indicate above
> and doesn't actually contradict what's written in 2.2.4.4.  I'll
> adjust the comments in the OpenBSD code.  Thanks for enlightening me!

Ok, sounds good!

> > You might also want this patch:
> > 
> > https://github.com/ARM-software/arm-trusted-firmware/commit/59dc4ef48757e4de2dc2de13d13e43acd5d91aa0#diff-3de7e205aa9233a71ea7940aae6579fe
> > 
> 
> That certainly makes the code easier to understand.  I'll see if I can
> trick the Marvell folks into backporting that to their TF fork.
> 
> Thanks,
> 
> Mark
> 
> P.S. I'm still thinking about "leveraging" trusted firmware to improve
>  the security of OpenBSD.  I have some ideas, but no concrete
>  plans yet to implement anything.

Sounds interesting!

Re: SMCCC 1.1 support for psci(4)

2018-05-03 Thread Mark Kettenis 
> Date: Thu, 3 May 2018 08:19:01 +0100
> From: Dimitris Papastamos 

Hi Dimitris,

> On Tue, May 01, 2018 at 11:55:00AM +0200, Mark Kettenis wrote:
> > So after adding a quick hack to mitigate Spectre variant 2 to ARM
> > Trusted Firmware (ATF), ARM actually designed a proper solution that
> > minimizes the performance loss and makes the presence of the
> > workaround detectable.  This is all documented in an update of the SMC
> > Calling Convention (SMCCC) standard.
> > 
> > The diff below implements support for this solution while keeping
> > support for the hack.  While ARM strongly suggests vendors to update
> > to a version of ATF that implements SMCCC 1.1 the current ATF for the
> > Marvell ARMADA 8040 hasn't been updated yet (but does include the
> > initial hack).
> > 
> > Unfortunately the SMCCC 1.1 implementation in ATF doesn't quite
> > implement the spec.  As a result we have to check whether the
> > workaround is implemented by issuing the relevant calls on each of the
> > CPUs that might be affected.  This is important for big.LITTLE designs
> > such as the RK3399 that include both Cortex-A53 cores that aren't
> > vulnerable and Cortex-A72 cores that are.
> 
> Can you explain a bit the inconsistency between the spec and the TF
> implementation?  If there is an issue in the TF implementation I can
> fix it.
> 
> On a big.LITTLE system the expected behaviour is for
> SMCCC_ARCH_FEATURES() to return 1 for the unaffected cores
> (Cortex-A53) and 0 for the affected cores (Cortex-A72).  The call will
> only return -1 if TF is built without mitigation support for
> CVE-2017-5715.

Looks like I managed to confuse myself and drew the wrong conclusion
from the description of SMCCC_ARCH_WORKAROUND_1 in section 2.2.4.4:

  In heterogeneous systems with some PEs that require mitigation and
  others that do not, the firmware must provide a safe implementation
  of this function on all PEs.

I now notice that 2.2.4.2 explicitly spells out the results of the
SMCCC_ARCH)FEATURES call that matches the behaviour you indicate above
and doesn't actually contradict what's written in 2.2.4.4.  I'll
adjust the comments in the OpenBSD code.  Thanks for enlightening me!

> You might also want this patch:
> 
> https://github.com/ARM-software/arm-trusted-firmware/commit/59dc4ef48757e4de2dc2de13d13e43acd5d91aa0#diff-3de7e205aa9233a71ea7940aae6579fe
> 

That certainly makes the code easier to understand.  I'll see if I can
trick the Marvell folks into backporting that to their TF fork.

Thanks,

Mark

P.S. I'm still thinking about "leveraging" trusted firmware to improve
 the security of OpenBSD.  I have some ideas, but no concrete
 plans yet to implement anything.

Re: SMCCC 1.1 support for psci(4)

2018-05-03 Thread Dimitris Papastamos 
On Tue, May 01, 2018 at 11:55:00AM +0200, Mark Kettenis wrote:
> So after adding a quick hack to mitigate Spectre variant 2 to ARM
> Trusted Firmware (ATF), ARM actually designed a proper solution that
> minimizes the performance loss and makes the presence of the
> workaround detectable.  This is all documented in an update of the SMC
> Calling Convention (SMCCC) standard.
> 
> The diff below implements support for this solution while keeping
> support for the hack.  While ARM strongly suggests vendors to update
> to a version of ATF that implements SMCCC 1.1 the current ATF for the
> Marvell ARMADA 8040 hasn't been updated yet (but does include the
> initial hack).
> 
> Unfortunately the SMCCC 1.1 implementation in ATF doesn't quite
> implement the spec.  As a result we have to check whether the
> workaround is implemented by issuing the relevant calls on each of the
> CPUs that might be affected.  This is important for big.LITTLE designs
> such as the RK3399 that include both Cortex-A53 cores that aren't
> vulnerable and Cortex-A72 cores that are.

Can you explain a bit the inconsistency between the spec and the TF
implementation?  If there is an issue in the TF implementation I can
fix it.

On a big.LITTLE system the expected behaviour is for
SMCCC_ARCH_FEATURES() to return 1 for the unaffected cores
(Cortex-A53) and 0 for the affected cores (Cortex-A72).  The call will
only return -1 if TF is built without mitigation support for
CVE-2017-5715.

You might also want this patch:

https://github.com/ARM-software/arm-trusted-firmware/commit/59dc4ef48757e4de2dc2de13d13e43acd5d91aa0#diff-3de7e205aa9233a71ea7940aae6579fe

Re: SMCCC 1.1 support for psci(4)

2018-05-03 Thread Dimitris Papastamos 
On Thu, May 03, 2018 at 08:19:01AM +0100, Dimitris Papastamos wrote:
> On Tue, May 01, 2018 at 11:55:00AM +0200, Mark Kettenis wrote:
> > So after adding a quick hack to mitigate Spectre variant 2 to ARM
> > Trusted Firmware (ATF), ARM actually designed a proper solution that
> > minimizes the performance loss and makes the presence of the
> > workaround detectable.  This is all documented in an update of the SMC
> > Calling Convention (SMCCC) standard.
> > 
> > The diff below implements support for this solution while keeping
> > support for the hack.  While ARM strongly suggests vendors to update
> > to a version of ATF that implements SMCCC 1.1 the current ATF for the
> > Marvell ARMADA 8040 hasn't been updated yet (but does include the
> > initial hack).
> > 
> > Unfortunately the SMCCC 1.1 implementation in ATF doesn't quite
> > implement the spec.  As a result we have to check whether the
> > workaround is implemented by issuing the relevant calls on each of the
> > CPUs that might be affected.  This is important for big.LITTLE designs
> > such as the RK3399 that include both Cortex-A53 cores that aren't
> > vulnerable and Cortex-A72 cores that are.
> 
> Can you explain a bit the inconsistency between the spec and the TF
> implementation?  If there is an issue in the TF implementation I can
> fix it.
> 
> On a big.LITTLE system the expected behaviour is for
> SMCCC_ARCH_FEATURES() to return 1 for the unaffected cores
> (Cortex-A53) and 0 for the affected cores (Cortex-A72).  The call will
> only return -1 if TF is built without mitigation support for
> CVE-2017-5715.
> 
> You might also want this patch:
> 
> https://github.com/ARM-software/arm-trusted-firmware/commit/59dc4ef48757e4de2dc2de13d13e43acd5d91aa0#diff-3de7e205aa9233a71ea7940aae6579fe

And this patch as well:

https://github.com/ARM-software/arm-trusted-firmware/commit/a205a56ea891c354c642713701075fec28906c40#diff-3de7e205aa9233a71ea7940aae6579fe

Re: SMCCC 1.1 support for psci(4)

2018-05-02 Thread Jonathan Gray 
On Tue, May 01, 2018 at 11:55:00AM +0200, Mark Kettenis wrote:
> So after adding a quick hack to mitigate Spectre variant 2 to ARM
> Trusted Firmware (ATF), ARM actually designed a proper solution that
> minimizes the performance loss and makes the presence of the
> workaround detectable.  This is all documented in an update of the SMC
> Calling Convention (SMCCC) standard.
> 
> The diff below implements support for this solution while keeping
> support for the hack.  While ARM strongly suggests vendors to update
> to a version of ATF that implements SMCCC 1.1 the current ATF for the
> Marvell ARMADA 8040 hasn't been updated yet (but does include the
> initial hack).
> 
> Unfortunately the SMCCC 1.1 implementation in ATF doesn't quite
> implement the spec.  As a result we have to check whether the
> workaround is implemented by issuing the relevant calls on each of the
> CPUs that might be affected.  This is important for big.LITTLE designs
> such as the RK3399 that include both Cortex-A53 cores that aren't
> vulnerable and Cortex-A72 cores that are.
> 
> ok?
> 
> 
> Index: dev/fdt/psci.c
> ===
> RCS file: /cvs/src/sys/dev/fdt/psci.c,v
> retrieving revision 1.6
> diff -u -p -r1.6 psci.c
> --- dev/fdt/psci.c23 Feb 2018 19:08:56 -  1.6
> +++ dev/fdt/psci.c1 May 2018 09:35:14 -

> +int32_t
> +smccc_version(void)
> +{
> + struct psci_softc *sc = psci_sc;
> +
> + if (sc && sc->sc_callfn)
> + return (*sc->sc_callfn)(SMCCC_VERSION, 0, 0, 0);

According to
https://developer.arm.com/-/media/developer/pdf/ARM_DEN_0070A_Firmware_interfaces_for_mitigating_CVE-2017-5715.pdf?revision=14854bab-e163-479e-b65b-05b491b31736=en

SMCCC_VERSION returning NOT_SUPPORTED/-1 should be treated as 1.0.
Should we return 0x1 here or in the callers?

Though it seems all the callers just test for >= 1.1 at the moment
so I'm ok with this version going in.