[RFC PATCH 0/4] No-op support for Arm FEAT_XS, feedback needed

Manos Pitsidianakis posted 4 patches 1 month, 1 week ago
target/arm/cpu-features.h          |   5 +
target/arm/helper.c                | 366 +++++++++++++++++++++----------------
target/arm/tcg/a64.decode          |   3 +
target/arm/tcg/cpu64.c             |   1 +
target/arm/tcg/translate-a64.c     |   6 +
tests/tcg/aarch64/system/feat-xs.c |  27 +++
6 files changed, 255 insertions(+), 153 deletions(-)
[RFC PATCH 0/4] No-op support for Arm FEAT_XS, feedback needed
Posted by Manos Pitsidianakis 1 month, 1 week ago
This series is an initial incomplete attempt at adding support for the
FEAT_XS feature in aarch64 TCG. This feature was introduced in ARMv8.7:
it adds a new memory attribute XS which indicates that a memory access
could take longer than usual to complete and also adds instruction
variants for TLBI maintenance and DSB.

These variants are implemented as no-ops, since QEMU TCG doesn't
implement caching.

This is my first foray into TCG and certain things weren't clear to me:

1. How to make sure the feature is implemented properly. Since we model
   cache maintenance as no-ops my understanding is the only
   functionality we need to provide is to expose the FEAT_XS feature bit
   and also make sure the nXS variants trap properly if configured with
   fine-grained traps.
2. Is there a point in adding a TCG test? If I read the manual
   correctly, the nXS variants should trap to the undefined instruction
   vector if unimplemented.

These patches lack support for FGT for now.

Signed-off-by: Manos Pitsidianakis <manos.pitsidianakis@linaro.org>
---
Manos Pitsidianakis (4):
      arm: Add FEAT_XS's TLBI NXS variants
      arm/tcg: add decodetree entry for DSB nXS variant
      arm/tcg/cpu64: add FEAT_XS feat in max cpu
      tests/tcg/aarch64: add system test for FEAT_XS

 target/arm/cpu-features.h          |   5 +
 target/arm/helper.c                | 366 +++++++++++++++++++++----------------
 target/arm/tcg/a64.decode          |   3 +
 target/arm/tcg/cpu64.c             |   1 +
 target/arm/tcg/translate-a64.c     |   6 +
 tests/tcg/aarch64/system/feat-xs.c |  27 +++
 6 files changed, 255 insertions(+), 153 deletions(-)
---
base-commit: 7e3b6d8063f245d27eecce5aabe624b5785f2a77
change-id: 20240919-arm-feat-xs-73eedb23d937

--
γαῖα πυρί μιχθήτω


Re: [RFC PATCH 0/4] No-op support for Arm FEAT_XS, feedback needed
Posted by Gustavo Romero 1 month, 1 week ago
Hi Manos,

On 10/14/24 07:48, Manos Pitsidianakis wrote:
> This series is an initial incomplete attempt at adding support for the
> FEAT_XS feature in aarch64 TCG. This feature was introduced in ARMv8.7:
> it adds a new memory attribute XS which indicates that a memory access
> could take longer than usual to complete and also adds instruction
> variants for TLBI maintenance and DSB.
> 
> These variants are implemented as no-ops, since QEMU TCG doesn't
> implement caching.
> 
> This is my first foray into TCG and certain things weren't clear to me:
> 
> 1. How to make sure the feature is implemented properly. Since we model
>     cache maintenance as no-ops my understanding is the only
>     functionality we need to provide is to expose the FEAT_XS feature bit
>     and also make sure the nXS variants trap properly if configured with
>     fine-grained traps.
> 2. Is there a point in adding a TCG test? If I read the manual
>     correctly, the nXS variants should trap to the undefined instruction
>     vector if unimplemented.

Yes, I think a test as the one you provided is worth to at least, as you
mentioned, check if QEMU doesn't crash when encountering NXS instruction
variants.


Cheers,
Gustavo


> These patches lack support for FGT for now.
> 
> Signed-off-by: Manos Pitsidianakis <manos.pitsidianakis@linaro.org>
> ---
> Manos Pitsidianakis (4):
>        arm: Add FEAT_XS's TLBI NXS variants
>        arm/tcg: add decodetree entry for DSB nXS variant
>        arm/tcg/cpu64: add FEAT_XS feat in max cpu
>        tests/tcg/aarch64: add system test for FEAT_XS
> 
>   target/arm/cpu-features.h          |   5 +
>   target/arm/helper.c                | 366 +++++++++++++++++++++----------------
>   target/arm/tcg/a64.decode          |   3 +
>   target/arm/tcg/cpu64.c             |   1 +
>   target/arm/tcg/translate-a64.c     |   6 +
>   tests/tcg/aarch64/system/feat-xs.c |  27 +++
>   6 files changed, 255 insertions(+), 153 deletions(-)
> ---
> base-commit: 7e3b6d8063f245d27eecce5aabe624b5785f2a77
> change-id: 20240919-arm-feat-xs-73eedb23d937
> 
> --
> γαῖα πυρί μιχθήτω
> 
> 


Re: [RFC PATCH 0/4] No-op support for Arm FEAT_XS, feedback needed
Posted by Peter Maydell 1 month, 1 week ago
On Mon, 14 Oct 2024 at 11:50, Manos Pitsidianakis
<manos.pitsidianakis@linaro.org> wrote:
>
> This series is an initial incomplete attempt at adding support for the
> FEAT_XS feature in aarch64 TCG. This feature was introduced in ARMv8.7:
> it adds a new memory attribute XS which indicates that a memory access
> could take longer than usual to complete and also adds instruction
> variants for TLBI maintenance and DSB.
>
> These variants are implemented as no-ops, since QEMU TCG doesn't
> implement caching.
>
> This is my first foray into TCG and certain things weren't clear to me:
>
> 1. How to make sure the feature is implemented properly. Since we model
>    cache maintenance as no-ops my understanding is the only
>    functionality we need to provide is to expose the FEAT_XS feature bit
>    and also make sure the nXS variants trap properly if configured with
>    fine-grained traps.

We also need to make HCRX_EL2 writes allow read and write of the
new FGTnXS and FnXS bits. (hcrx_write() constructs a valid_mask
and only allows those bits to be written, so the mask needs updating.)

> 2. Is there a point in adding a TCG test? If I read the manual
>    correctly, the nXS variants should trap to the undefined instruction
>    vector if unimplemented.
>
> These patches lack support for FGT for now.

We will need the FGT support, but I think it should be
straightforward to add:
 * add a new entry NXS to the FIELD() definitions of FGT in cpregs.h
   with the meaning "honour HCR_EL2.FGTnXS to suppress FGT",
   and in access_check_cp_reg skip the trap-check if NXS is
   set and arm_hcrx_el2_eff() has the FGTnXS bit set
 * when we add the new ARMCPRegInfo stanzas for the new NXS,
   make the .fgt fields be
     .fgt = FGT_TLBIwhatever | FGT_NXS
   so we apply the NXS logic to those ones
   (or if you like use macro magic in cpreg.h to define
   FGT_TLBIVAE1OSNXS as being FGT_TLBIVAE1OS | FGT_NXS,
   like what we do for including FGT_REV in the FGT_ constants)

-- PMM