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

Re: [PATCH 3/3] xen/arm: Add sb instruction support


  • To: Julien Grall <julien@xxxxxxx>
  • From: Bertrand Marquis <Bertrand.Marquis@xxxxxxx>
  • Date: Wed, 4 May 2022 07:24:52 +0000
  • Accept-language: en-GB, en-US
  • Arc-authentication-results: i=2; mx.microsoft.com 1; spf=pass (sender ip is 63.35.35.123) smtp.rcpttodomain=lists.xenproject.org smtp.mailfrom=arm.com; dmarc=pass (p=none sp=none pct=100) action=none header.from=arm.com; dkim=pass (signature was verified) header.d=armh.onmicrosoft.com; arc=pass (0 oda=1 ltdi=1 spf=[1,1,smtp.mailfrom=arm.com] dkim=[1,1,header.d=arm.com] dmarc=[1,1,header.from=arm.com])
  • Arc-authentication-results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=arm.com; dmarc=pass action=none header.from=arm.com; dkim=pass header.d=arm.com; arc=none
  • Arc-message-signature: i=2; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-AntiSpam-MessageData-ChunkCount:X-MS-Exchange-AntiSpam-MessageData-0:X-MS-Exchange-AntiSpam-MessageData-1; bh=F9fcqZ04iw2IdItrRajTwSyqV3xUNBwyExasmtmjCK0=; b=Ft38bnwoJh2eNX0QKiw8YkxkluCubA1EyZIXf1PRZ4Ajr4uGwfFgOhgxC8Hh4irKB19HKvJtBYY0J1q4tOc4ETNdQyF2nTV5K+/+nfkLHs5u6pwgWL4Yu3USLUDzRA6eyQVnlT2+oIhBX1NJwvL5uNAtp6bL9STS5h62Nn1lWKUFEp8/BjvuiYEiWC/STNJeBLN38x+d8b+WEnXn2jir/n2DCap4UmPqe698gWGJtTl/Q7IwXuna+5sekB246uilV2wSa6ArsQw1suCLw88uqsq6+QGowCtNGwvvnOdD0clfVVGqMEl1dNcxbFvev/RGqmRm3WT54EL49Qiq7uGXnw==
  • Arc-message-signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-AntiSpam-MessageData-ChunkCount:X-MS-Exchange-AntiSpam-MessageData-0:X-MS-Exchange-AntiSpam-MessageData-1; bh=F9fcqZ04iw2IdItrRajTwSyqV3xUNBwyExasmtmjCK0=; b=e/cuHNjDdZ7+u8BjFq8fRGRNfOLtt6jlK/c6kGMBAbs09myAeiSEww9kV6ku6+fjS+kRsD1z87LXpaGNfp6pqVk/NVYavPSttM+STzQuN5JCwPYvU7PpXXFam3uq/hn7ZlAF+8vvjMFzZ3p+AHwIRnnrO6o8rROUZLMJKiEym8AHclE/4+oV2jxVvRFTGUGh+cOvK870uMoPtdEIp6WoPY09y4F0nhPoYzU50+W1UzskXz9TiaF2b3R5ltbTON9JGfjD4BIzsU5oLEH1VfJbu9RR5Hk74xyMnliuL2bnKfpJusWv35A024qeXtIu+3X/4frPQURC9sraJy24prFcBw==
  • Arc-seal: i=2; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=pass; b=EwBQ0yrTymwqJ3V/bOHM5iU78coyBUarhUFnTdy+d4TuVcxEE3XaYX6Nr3EQaZieh4Vf6QdaIMN0ZYrn+P/3GsMzdDimVftD+zbSuCfbbzNqD9YpYMxARbXMAx0AD8j52AV6OyRGsrineOxwu8n4JEqJBSg9W82N42CmnLjg4WdCpiCxTafTyXXTAGYCSByzDUUFHNzbscb9BVLrwRsv1coYO/gmN7j0N81keRhHAariqzbqRDJv1VfdPTFv7zIBlqWhjNN4pIKapYsnheRAc5JCTvIDG3BpNz4fAXjvSxJUPSr8of6aFtxtZAeDIcSdysf5GoVpEfTKJ9O00seVIg==
  • Arc-seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=h9zLv7KysRB44FLxkA+Ga/Zs0qykciW7hYnxiBc6q4bsFTrxJB0jZJB62IH4upyU44T8nDHFYPdxwDAfUMxF+lpeJE12jpp8L0fQ4bpicksprMGFeF7xJ8WImjP0ZoanYa4QXcZGpOBPZKDEraWrXNH0nQ6j1d3I2iol450zvx7a9Oyp6fbKNz9vp6L3KpjQsunSFjISeFRI2zwPaRqzpg4qJA5UKPRgsyDdC9oamF+ef4QWrcOVy70sT8JqVP1aqGFaqlluFwra7hmCqX+LKaz/CJRIws+jMZh1A2w4s0aEqqSGoVOJwmpEm9VU+2jilkBzECumf9SuX4xG9coiXQ==
  • Authentication-results-original: dkim=none (message not signed) header.d=none;dmarc=none action=none header.from=arm.com;
  • Cc: "xen-devel@xxxxxxxxxxxxxxxxxxxx" <xen-devel@xxxxxxxxxxxxxxxxxxxx>, Stefano Stabellini <sstabellini@xxxxxxxxxx>, Volodymyr Babchuk <Volodymyr_Babchuk@xxxxxxxx>
  • Delivery-date: Wed, 04 May 2022 07:25:08 +0000
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>
  • Nodisclaimer: true
  • Original-authentication-results: dkim=none (message not signed) header.d=none;dmarc=none action=none header.from=arm.com;
  • Thread-index: AQHYXtGbLw8lUArxPEOQFl634Avmjq0NftKAgADToQA=
  • Thread-topic: [PATCH 3/3] xen/arm: Add sb instruction support

Hi Julien,

> On 3 May 2022, at 19:47, Julien Grall <julien@xxxxxxx> wrote:
> 
> Hi Bertrand,
> 
> On 03/05/2022 10:38, Bertrand Marquis wrote:
>> This patch is adding sb instruction support when it is supported by a
>> CPU on arm64.
>> To achieve this, the "sb" macro is moved to sub-arch macros.h so that we
>> can use sb instruction when available through alternative on arm64 and
>> keep the current behaviour on arm32.
> 
> SB is also supported on Arm32. So I would prefer to introduce the encoding 
> right now and avoid duplicating the .macro sb.

I will look into that.

> 
>> A new cpuerrata capability is introduced to enable the alternative
> 
> 'sb' is definitely not an erratum. Errata are for stuff that are meant to be 
> specific to one (or multiple) CPU and they are not part of the architecture.
> 
> This is the first time we introduce a feature in Xen. So we need to add a new 
> array in cpufeature.c that will cover 'SB' for now. In future we could add 
> feature like pointer auth, LSE atomics...

I am not quite sure why you would want to do that.

Using the sb instruction is definitely something to do to solve erratas, if a 
CPU is not impacted by those erratas, why would you want to use this ?

> 
>> code for sb when the support is detected using isa64 coprocessor
> 
> s/coprocessor/system/

Ack

> 
>> register.
>> The sb instruction is encoded using its hexadecimal value.
> 
> This is necessary to avoid recursive macro, right?

This is necessary for several reasons:
- support compilers not supporting sb instructions (need encoding)
- handle the alternative code (we do not want to repeat this everywhere)
- avoid recursive macro

> 
>> diff --git a/xen/arch/arm/include/asm/arm64/macros.h 
>> b/xen/arch/arm/include/asm/arm64/macros.h
>> index 140e223b4c..e639cec400 100644
>> --- a/xen/arch/arm/include/asm/arm64/macros.h
>> +++ b/xen/arch/arm/include/asm/arm64/macros.h
>> @@ -1,6 +1,24 @@
>>  #ifndef __ASM_ARM_ARM64_MACROS_H
>>  #define __ASM_ARM_ARM64_MACROS_H
>>  +#include <asm/alternative.h>
>> +
>> +    /*
>> +     * Speculative barrier
>> +     */
>> +    .macro sb
>> +alternative_if_not ARM64_HAS_SB
>> +    dsb nsh
>> +    isb
>> +alternative_else
>> +/*
>> + * SB encoding as given in chapter C6.2.264 of ARM ARM (DDI 0487H.a).
>> + */
> 
> NIT: Please align the comment with ".inst" below. I also don't think it is 
> necessary to mention the spec here. The instruction encoding is not going to 
> change.
Ack

> 
>> +    .inst 0xd50330ff
>> +    nop
> 
> Why do we need the NOP?

Alternative requires both sides to have the same size hence the nop to have 2 
instructions as in the if.

> 
>> +alternative_endif
>> +    .endm
>> +
>>      /*
>>       * @dst: Result of get_cpu_info()
>>       */
>> diff --git a/xen/arch/arm/include/asm/cpufeature.h 
>> b/xen/arch/arm/include/asm/cpufeature.h
>> index 4719de47f3..9370805900 100644
>> --- a/xen/arch/arm/include/asm/cpufeature.h
>> +++ b/xen/arch/arm/include/asm/cpufeature.h
>> @@ -67,8 +67,9 @@
>>  #define ARM_WORKAROUND_BHB_LOOP_24 13
>>  #define ARM_WORKAROUND_BHB_LOOP_32 14
>>  #define ARM_WORKAROUND_BHB_SMCC_3 15
>> +#define ARM64_HAS_SB 16
>>  -#define ARM_NCAPS           16
>> +#define ARM_NCAPS           17
>>    #ifndef __ASSEMBLY__
>>  diff --git a/xen/arch/arm/include/asm/macros.h 
>> b/xen/arch/arm/include/asm/macros.h
>> index 1aa373760f..91ea3505e4 100644
>> --- a/xen/arch/arm/include/asm/macros.h
>> +++ b/xen/arch/arm/include/asm/macros.h
>> @@ -5,15 +5,6 @@
>>  # error "This file should only be included in assembly file"
>>  #endif
>>  -    /*
>> -     * Speculative barrier
>> -     * XXX: Add support for the 'sb' instruction
>> -     */
>> -    .macro sb
>> -    dsb nsh
>> -    isb
>> -    .endm
>> -
>>  #if defined (CONFIG_ARM_32)
>>  # include <asm/arm32/macros.h>
>>  #elif defined(CONFIG_ARM_64)
> 
> Cheers,

Thanks for the review

Cheers
Bertrand




 


Rackspace

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