Re: [PATCH v2 2/2] eal: add Arm WFET in power management intrinsics

[Thomas Monjalon <thomas@monjalon.net>]

19/06/2024 08:45, Wathsala Vithanage:
> --- a/lib/eal/arm/include/rte_cpuflags_64.h
> +++ b/lib/eal/arm/include/rte_cpuflags_64.h
> @@ -36,6 +36,7 @@ enum rte_cpu_flag_t {
>  	RTE_CPUFLAG_SVEF64MM,
>  	RTE_CPUFLAG_SVEBF16,
>  	RTE_CPUFLAG_AARCH64,
> +	RTE_CPUFLAG_WFXT,
>  };

It may be useful to add comments explaining each flag.
May be a separate patch in this series?


> - * Copyright(c) 2019 Arm Limited
> + * Copyright(c) 2024 Arm Limited

No, it's wrong to remove initial date,
and no, you don't need to update dates at all.


> -#ifdef RTE_WAIT_UNTIL_EQUAL_ARCH_DEFINED

Why removing this #ifdef?

> -/* Send a local event to quit WFE. */
> +/* Send a local event to quit WFE/WFxT. */
>  #define __RTE_ARM_SEVL() { asm volatile("sevl" : : : "memory"); }
>  
> -/* Send a global event to quit WFE for all cores. */
> +/* Send a global event to quit WFE/WFxT for all cores. */
>  #define __RTE_ARM_SEV() { asm volatile("sev" : : : "memory"); }
>  
>  /* Put processor into low power WFE(Wait For Event) state. */
>  #define __RTE_ARM_WFE() { asm volatile("wfe" : : : "memory"); }
>  
> +/* Put processor into low power WFET (WFE with Timeout) state. */
> +#ifdef RTE_ARM_FEATURE_WFXT
> +#define __RTE_ARM_WFET(t) {                              \
> +	asm volatile("wfet %x[to]"                        \
> +			:                                 \
> +			: [to] "r" (t)                    \
> +			: "memory");                      \
> +	}

Is there any intrinsic function available?


[...]
> --- a/lib/eal/arm/rte_cpuflags.c
> +++ b/lib/eal/arm/rte_cpuflags.c
> @@ -115,6 +115,7 @@ const struct feature_entry rte_cpu_feature_table[] = {
>  	FEAT_DEF(SVEF32MM,	REG_HWCAP2,   10)
>  	FEAT_DEF(SVEF64MM,	REG_HWCAP2,   11)
>  	FEAT_DEF(SVEBF16,	REG_HWCAP2,   12)
> +	FEAT_DEF(WFXT,		REG_HWCAP2,   31)
>  	FEAT_DEF(AARCH64,	REG_PLATFORM,  0)

Are you sure of alignment? (looks wrong in my email client)


[...]
>  rte_cpu_get_intrinsics_support(struct rte_cpu_intrinsics *intrinsics)
>  {
>  	memset(intrinsics, 0, sizeof(*intrinsics));
> -#ifdef RTE_ARM_USE_WFE
>  	intrinsics->power_monitor = 1;
> -#endif

Why removing this #ifdef?


> +uint8_t wfet_en;

It should be made static probably.
This variable will be unused in some cases, needs #ifdef.

> +
> +RTE_INIT(rte_power_intrinsics_init)
> +{
> +#ifdef RTE_ARCH_64
> +	if (rte_cpu_get_flag_enabled(RTE_CPUFLAG_WFXT))
> +		wfet_en = 1;
> +#endif
> +}
> +
> +/**
> + * This function uses WFE/WFET instruction to make lcore suspend
>   * execution on ARM.
> - * Note that timestamp based timeout is not supported yet.
>   */
>  int
>  rte_power_monitor(const struct rte_power_monitor_cond *pmc,
>  		const uint64_t tsc_timestamp)
>  {
> -	RTE_SET_USED(tsc_timestamp);
> -
> -#ifdef RTE_ARM_USE_WFE
> +#ifdef RTE_ARCH_64

It looks wrong.
If RTE_ARM_USE_WFE is disabled, you should not call __RTE_ARM_WFE().

>  	const unsigned int lcore_id = rte_lcore_id();
>  	uint64_t cur_value;
>  
> @@ -33,28 +44,30 @@ rte_power_monitor(const struct rte_power_monitor_cond *pmc,
>  
>  	switch (pmc->size) {
>  	case sizeof(uint8_t):
> -		__RTE_ARM_LOAD_EXC_8(pmc->addr, cur_value, rte_memory_order_relaxed)
> -		__RTE_ARM_WFE()
> +		__RTE_ARM_LOAD_EXC_8(pmc->addr, cur_value, rte_memory_order_relaxed);