Re: [PATCH v2 1/2] powerpc/perf: consolidate GPCI hcall structs into asm/hvcall.h

[Tyrel Datwyler <tyreld@linux.ibm.com>]

On 7/27/20 11:46 AM, Scott Cheloha wrote:
> The H_GetPerformanceCounterInfo (GPCI) hypercall input/output structs are
> useful to modules outside of perf/, so move them into asm/hvcall.h to live
> alongside the other powerpc hypercall structs.
> 
> Leave the perf-specific GPCI stuff in perf/hv-gpci.h.
> 
> Signed-off-by: Scott Cheloha <cheloha@linux.ibm.com>
> ---
>  arch/powerpc/include/asm/hvcall.h | 36 +++++++++++++++++++++++++++++++
>  arch/powerpc/perf/hv-gpci.c       |  9 --------
>  arch/powerpc/perf/hv-gpci.h       | 27 -----------------------
>  3 files changed, 36 insertions(+), 36 deletions(-)
> 
> diff --git a/arch/powerpc/include/asm/hvcall.h b/arch/powerpc/include/asm/hvcall.h
> index e90c073e437e..c338480b4551 100644
> --- a/arch/powerpc/include/asm/hvcall.h
> +++ b/arch/powerpc/include/asm/hvcall.h
> @@ -527,6 +527,42 @@ struct hv_guest_state {
>  /* Latest version of hv_guest_state structure */
>  #define HV_GUEST_STATE_VERSION	1
>  
> +/*
> + * From the document "H_GetPerformanceCounterInfo Interface" v1.07
> + *
> + * H_GET_PERF_COUNTER_INFO argument
> + */
> +struct hv_get_perf_counter_info_params {
> +	__be32 counter_request; /* I */
> +	__be32 starting_index;  /* IO */
> +	__be16 secondary_index; /* IO */
> +	__be16 returned_values; /* O */
> +	__be32 detail_rc; /* O, only needed when called via *_norets() */
> +
> +	/*
> +	 * O, size each of counter_value element in bytes, only set for version
> +	 * >= 0x3
> +	 */
> +	__be16 cv_element_size;
> +
> +	/* I, 0 (zero) for versions < 0x3 */
> +	__u8 counter_info_version_in;
> +
> +	/* O, 0 (zero) if version < 0x3. Must be set to 0 when making hcall */
> +	__u8 counter_info_version_out;
> +	__u8 reserved[0xC];
> +	__u8 counter_value[];
> +} __packed;
> +
> +#define HGPCI_REQ_BUFFER_SIZE	4096
> +#define HGPCI_MAX_DATA_BYTES \
> +	(HGPCI_REQ_BUFFER_SIZE - sizeof(struct hv_get_perf_counter_info_params))
> +
> +struct hv_gpci_request_buffer {
> +	struct hv_get_perf_counter_info_params params;
> +	uint8_t bytes[HGPCI_MAX_DATA_BYTES];
> +} __packed;
> +
>  #endif /* __ASSEMBLY__ */
>  #endif /* __KERNEL__ */
>  #endif /* _ASM_POWERPC_HVCALL_H */
> diff --git a/arch/powerpc/perf/hv-gpci.c b/arch/powerpc/perf/hv-gpci.c
> index 6884d16ec19b..1667315b82e9 100644
> --- a/arch/powerpc/perf/hv-gpci.c
> +++ b/arch/powerpc/perf/hv-gpci.c
> @@ -123,17 +123,8 @@ static const struct attribute_group *attr_groups[] = {
>  	NULL,
>  };
>  
> -#define HGPCI_REQ_BUFFER_SIZE	4096
> -#define HGPCI_MAX_DATA_BYTES \
> -	(HGPCI_REQ_BUFFER_SIZE - sizeof(struct hv_get_perf_counter_info_params))
> -
>  static DEFINE_PER_CPU(char, hv_gpci_reqb[HGPCI_REQ_BUFFER_SIZE]) __aligned(sizeof(uint64_t));
>  
> -struct hv_gpci_request_buffer {
> -	struct hv_get_perf_counter_info_params params;
> -	uint8_t bytes[HGPCI_MAX_DATA_BYTES];
> -} __packed;
> -
>  static unsigned long single_gpci_request(u32 req, u32 starting_index,
>  		u16 secondary_index, u8 version_in, u32 offset, u8 length,
>  		u64 *value)
> diff --git a/arch/powerpc/perf/hv-gpci.h b/arch/powerpc/perf/hv-gpci.h
> index a3053eda5dcc..4d108262bed7 100644
> --- a/arch/powerpc/perf/hv-gpci.h
> +++ b/arch/powerpc/perf/hv-gpci.h
> @@ -2,33 +2,6 @@
>  #ifndef LINUX_POWERPC_PERF_HV_GPCI_H_
>  #define LINUX_POWERPC_PERF_HV_GPCI_H_
>  
> -#include <linux/types.h>
> -
> -/* From the document "H_GetPerformanceCounterInfo Interface" v1.07 */
> -
> -/* H_GET_PERF_COUNTER_INFO argument */
> -struct hv_get_perf_counter_info_params {
> -	__be32 counter_request; /* I */
> -	__be32 starting_index;  /* IO */
> -	__be16 secondary_index; /* IO */
> -	__be16 returned_values; /* O */
> -	__be32 detail_rc; /* O, only needed when called via *_norets() */
> -
> -	/*
> -	 * O, size each of counter_value element in bytes, only set for version
> -	 * >= 0x3
> -	 */
> -	__be16 cv_element_size;
> -
> -	/* I, 0 (zero) for versions < 0x3 */
> -	__u8 counter_info_version_in;
> -
> -	/* O, 0 (zero) if version < 0x3. Must be set to 0 when making hcall */
> -	__u8 counter_info_version_out;
> -	__u8 reserved[0xC];
> -	__u8 counter_value[];
> -} __packed;
> -

Hmm, this pretty much guts this header which normally I'd be inclined to suggest
moving the whole thing. The remainder of that header for context:

>  /*
>   * counter info version => fw version/reference (spec version)
>   *
>
 * 8 => power8 (1.07)
 * [7 is skipped by spec 1.07]
 * 6 => TLBIE (1.07)
 * 5 => v7r7m0.phyp (1.05)
 * [4 skipped]
 * 3 => v7r6m0.phyp (?)
 * [1,2 skipped]
 * 0 => v7r{2,3,4}m0.phyp (?)
 */
#define COUNTER_INFO_VERSION_CURRENT 0x8

/* capability mask masks. */
enum {
        HV_GPCI_CM_GA = (1 << 7),
        HV_GPCI_CM_EXPANDED = (1 << 6),
        HV_GPCI_CM_LAB = (1 << 5)
};

#define REQUEST_FILE "../hv-gpci-requests.h"
#define NAME_LOWER hv_gpci
#define NAME_UPPER HV_GPCI
#include "req-gen/perf.h"
#undef REQUEST_FILE
#undef NAME_LOWER
#undef NAME_UPPER

The side effect of moving seems that we would have to drag "hv-gpci-requests.h"
along as well. So, maybe its best just moving the struct as you've done so it
can be used by code outside of perf.

-Tyrel