[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Xen-devel] [PATCH v3 10/17] xen/arm64: Add ARM_SMCCC_ARCH_WORKAROUND_1 BP hardening support



Hi,

On 21/02/18 17:35, Stefano Stabellini wrote:
On Wed, 21 Feb 2018, Julien Grall wrote:
Hi Stefano,

On 21/02/2018 00:35, Stefano Stabellini wrote:
On Thu, 15 Feb 2018, Julien Grall wrote:
Add the detection and runtime code for ARM_SMCCC_ARCH_WORKAROUND_1.

Signed-off-by: Julien Grall <julien.grall@xxxxxxx>

---
      Changes in v3:
          - Add the missing call to smc #0.

      Changes in v2:
          - Patch added
---
   xen/arch/arm/arm64/bpi.S    | 13 +++++++++++++
   xen/arch/arm/cpuerrata.c    | 32 +++++++++++++++++++++++++++++++-
   xen/include/asm-arm/smccc.h |  1 +
   3 files changed, 45 insertions(+), 1 deletion(-)

diff --git a/xen/arch/arm/arm64/bpi.S b/xen/arch/arm/arm64/bpi.S
index 4b7f1dc21f..981fb83a88 100644
--- a/xen/arch/arm/arm64/bpi.S
+++ b/xen/arch/arm/arm64/bpi.S
@@ -16,6 +16,8 @@
    * along with this program.  If not, see <http://www.gnu.org/licenses/>.
    */
   +#include <asm/smccc.h>
+
   .macro ventry target
       .rept 31
       nop
@@ -81,6 +83,17 @@ ENTRY(__psci_hyp_bp_inval_start)
       add     sp, sp, #(8 * 18)
   ENTRY(__psci_hyp_bp_inval_end)
   +ENTRY(__smccc_workaround_1_smc_start)
+    sub     sp, sp, #(8 * 4)
+    stp     x2, x3, [sp, #(8 * 0)]
+    stp     x0, x1, [sp, #(8 * 2)]
+    mov     w0, #ARM_SMCCC_ARCH_WORKAROUND_1_FID
+    smc     #0
+    ldp     x2, x3, [sp, #(8 * 0)]
+    ldp     x0, x1, [sp, #(8 * 2)]
+    add     sp, sp, #(8 * 4)
+ENTRY(__smccc_workaround_1_smc_end)
+
   /*
    * Local variables:
    * mode: ASM
diff --git a/xen/arch/arm/cpuerrata.c b/xen/arch/arm/cpuerrata.c
index 8d5f8d372a..dec9074422 100644
--- a/xen/arch/arm/cpuerrata.c
+++ b/xen/arch/arm/cpuerrata.c
@@ -147,6 +147,34 @@ install_bp_hardening_vec(const struct
arm_cpu_capabilities *entry,
       return ret;
   }
   +extern char __smccc_workaround_1_smc_start[],
__smccc_workaround_1_smc_end[];
+
+static bool
+check_smccc_arch_workaround_1(const struct arm_cpu_capabilities *entry)
+{
+    struct arm_smccc_res res;
+
+    /*
+     * Enable callbacks are called on every CPU based on the
+     * capabilities. So double-check whether the CPU matches the
+     * entry.
+     */
+    if ( !entry->matches(entry) )
+        return false;

I think this should be return true?

Both are valid. It depends how you consider the workflow here. If you return:
        - true: You say that this helper already took care of that CPU. So no
need to continue further.
        - false: This CPU does not match, let's fallback to a different
method. That method will bailout later (see install_bp_hardening_vec).

I choose the latte because the SMCCC workaround is considered as an
alternative method. So we want to fallback to the other one if it does not
work at the cost of few extra instructions. But that's boot and going to be
reworked in patch #11. Indeed this is just a temporary solution to plumb the
new hardening method before we kill the PSCI_GET_VERSION one.

Yeah, I noticed that this is moot given the next patches in the series.
Given that you are already resending the series, I would also change this
to return true because I think it makes more sense, but it is
unimportant so either way:

I will keep false, because it make little sense to return true here, It will actually bring more confusion as we return false just after for a similar case.



Reviewed-by: Stefano Stabellini <sstabellini@xxxxxxxxxx>


I am going to drop it as I need some rework base on Andre's comment.

Cheers,

--
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel

 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.