From patchwork Wed Jun 14 22:25:29 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Thiago Jung Bauermann X-Patchwork-Id: 776012 Return-Path: X-Original-To: patchwork-incoming@ozlabs.org Delivered-To: patchwork-incoming@ozlabs.org Received: from lists.ozlabs.org (lists.ozlabs.org [IPv6:2401:3900:2:1::3]) (using TLSv1.2 with cipher ADH-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id 3wp1TB6jlVz9s65 for ; Thu, 15 Jun 2017 08:26:54 +1000 (AEST) Received: from lists.ozlabs.org (lists.ozlabs.org [IPv6:2401:3900:2:1::3]) by lists.ozlabs.org (Postfix) with ESMTP id 3wp1TB5wqdzDqPw for ; Thu, 15 Jun 2017 08:26:54 +1000 (AEST) X-Original-To: linuxppc-dev@lists.ozlabs.org Delivered-To: linuxppc-dev@lists.ozlabs.org Received: from mx0a-001b2d01.pphosted.com (mx0a-001b2d01.pphosted.com [148.163.156.1]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by lists.ozlabs.org (Postfix) with ESMTPS id 3wp1S36C5gzDqL3 for ; Thu, 15 Jun 2017 08:25:55 +1000 (AEST) Received: from pps.filterd (m0098399.ppops.net [127.0.0.1]) by mx0a-001b2d01.pphosted.com (8.16.0.20/8.16.0.20) with SMTP id v5EMNftW122238 for ; Wed, 14 Jun 2017 18:25:53 -0400 Received: from e24smtp05.br.ibm.com (e24smtp05.br.ibm.com [32.104.18.26]) by mx0a-001b2d01.pphosted.com with ESMTP id 2b36ms4v2j-1 (version=TLSv1.2 cipher=AES256-SHA bits=256 verify=NOT) for ; Wed, 14 Jun 2017 18:25:53 -0400 Received: from localhost by e24smtp05.br.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Wed, 14 Jun 2017 19:25:50 -0300 Received: from d24relay02.br.ibm.com (9.13.39.42) by e24smtp05.br.ibm.com (10.172.0.141) with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted; Wed, 14 Jun 2017 19:25:45 -0300 Received: from d24av04.br.ibm.com (d24av04.br.ibm.com [9.8.31.97]) by d24relay02.br.ibm.com (8.14.9/8.14.9/NCO v10.0) with ESMTP id v5EMPdhA6029788 for ; Wed, 14 Jun 2017 19:25:39 -0300 Received: from d24av04.br.ibm.com (localhost [127.0.0.1]) by d24av04.br.ibm.com (8.14.4/8.14.4/NCO v10.0 AVout) with ESMTP id v5EMPaZp007183 for ; Wed, 14 Jun 2017 19:25:36 -0300 Received: from morokweng ([9.85.196.50]) by d24av04.br.ibm.com (8.14.4/8.14.4/NCO v10.0 AVin) with ESMTP id v5EMPVSG007151 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=NO); Wed, 14 Jun 2017 19:25:34 -0300 References: <1496350947-30951-1-git-send-email-bauerman@linux.vnet.ibm.com> <1496350947-30951-8-git-send-email-bauerman@linux.vnet.ibm.com> <20170614001350.GB12998@us.ibm.com> From: Thiago Jung Bauermann To: Sukadev Bhattiprolu Subject: Re: [PATCH 7/8] powerpc/perf/hv-24x7: Support v2 of the hypervisor API In-reply-to: <20170614001350.GB12998@us.ibm.com> Date: Wed, 14 Jun 2017 19:25:29 -0300 MIME-Version: 1.0 X-TM-AS-MML: disable x-cbid: 17061422-0032-0000-0000-0000056A6659 X-IBM-AV-DETECTION: SAVI=unused REMOTE=unused XFE=unused x-cbparentid: 17061422-0033-0000-0000-000011F08AD4 Message-Id: <871sqmfbqe.fsf@linux.vnet.ibm.com> X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10432:, , definitions=2017-06-14_06:, , signatures=0 X-Proofpoint-Spam-Details: rule=outbound_notspam policy=outbound score=0 spamscore=0 suspectscore=1 malwarescore=0 phishscore=0 adultscore=0 bulkscore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.0.1-1703280000 definitions=main-1706140371 X-BeenThere: linuxppc-dev@lists.ozlabs.org X-Mailman-Version: 2.1.23 Precedence: list List-Id: Linux on PowerPC Developers Mail List List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: linuxppc-dev@lists.ozlabs.org Errors-To: linuxppc-dev-bounces+patchwork-incoming=ozlabs.org@lists.ozlabs.org Sender: "Linuxppc-dev" Hello Suka, Thanks for your review! Sukadev Bhattiprolu writes: > Thiago Jung Bauermann [bauerman@linux.vnet.ibm.com] wrote: >> @@ -166,9 +174,12 @@ DEFINE_PER_CPU(struct hv_24x7_hw, hv_24x7_hw); >> DEFINE_PER_CPU(char, hv_24x7_reqb[H24x7_DATA_BUFFER_SIZE]) __aligned(4096); >> DEFINE_PER_CPU(char, hv_24x7_resb[H24x7_DATA_BUFFER_SIZE]) __aligned(4096); >> >> -#define MAX_NUM_REQUESTS ((H24x7_DATA_BUFFER_SIZE - \ >> +#define MAX_NUM_REQUESTS_V1 ((H24x7_DATA_BUFFER_SIZE - \ >> + sizeof(struct hv_24x7_request_buffer)) \ >> + / H24x7_REQUEST_SIZE_V1) >> +#define MAX_NUM_REQUESTS_V2 ((H24x7_DATA_BUFFER_SIZE - \ >> sizeof(struct hv_24x7_request_buffer)) \ >> - / sizeof(struct hv_24x7_request)) >> + / H24x7_REQUEST_SIZE_V2) > > Nit: Can we define MAX_NUM_REQUESTS(version) - with a version parameter ? It > will... That's a good idea. I created a function instead of a macro, I think it makes the code clearer since the macro would be a bit harder to read. >> @@ -1101,9 +1112,13 @@ static int add_event_to_24x7_request(struct perf_event *event, >> { >> u16 idx; >> int i; >> + size_t req_size; >> struct hv_24x7_request *req; >> >> - if (request_buffer->num_requests >= MAX_NUM_REQUESTS) { >> + if ((request_buffer->interface_version == 1 >> + && request_buffer->num_requests >= MAX_NUM_REQUESTS_V1) >> + || (request_buffer->interface_version > 1 >> + && request_buffer->num_requests >= MAX_NUM_REQUESTS_V2)) { >> pr_devel("Too many requests for 24x7 HCALL %d\n", > > ...simplify this check to > > if (request->buffer->num_requests >= MAX_NUM_REQUESTS(version)) That's better indeed. >> request_buffer->num_requests); >> return -EINVAL; >> @@ -1120,8 +1135,11 @@ static int add_event_to_24x7_request(struct perf_event *event, >> idx = event_get_vcpu(event); >> } >> >> + req_size = request_buffer->interface_version == 1 ? >> + H24x7_REQUEST_SIZE_V1 : H24x7_REQUEST_SIZE_V2; >> + > > Maybe similarly, with H24x7_REQUEST_SIZE(version) ? I implement this suggestion too. >> +/** >> + * get_count_from_result - get event count from the given result >> + * >> + * @event: Event associated with @res. >> + * @resb: Result buffer containing @res. >> + * @res: Result to work on. >> + * @countp: Output variable containing the event count. >> + * @next: Optional output variable pointing to the next result in @resb. >> + */ >> +static int get_count_from_result(struct perf_event *event, >> + struct hv_24x7_data_result_buffer *resb, >> + struct hv_24x7_result *res, u64 *countp, >> + struct hv_24x7_result **next) >> +{ >> + u16 num_elements = be16_to_cpu(res->num_elements_returned); >> + u16 data_size = be16_to_cpu(res->result_element_data_size); >> + unsigned int data_offset; >> + void *element_data; >> + int ret = 0; >> + >> + /* >> + * We can bail out early if the result is empty. >> + */ >> + if (!num_elements) { >> + pr_debug("Result of request %hhu is empty, nothing to do\n", >> + res->result_ix); >> + >> + if (next) >> + *next = (struct hv_24x7_result *) res->elements; >> + >> + return -ENODATA; >> + } >> + >> + /* >> + * This code assumes that a result has only one element. >> + */ >> + if (num_elements != 1) { >> + pr_debug("Error: result of request %hhu has %hu elements\n", >> + res->result_ix, num_elements); > > Could this happen due to an user request or would this indicate a bug > in the way we submitted the request (perf should submit separate request > for each lpar/index - we set ->max_ix and ->max_num_lpars to cpu_be16(1). That's a good question. I don't know to be honest. > Minor inconsistency with proceeding, is that if the next element passes, > this return code is lost/over written. IOW, h_24x7_event_commit_txn()'s > return value depends on the last element we process, even if intermediate > ones encounter an error. Good point. Would it be better if h_24x7_event_commit_txn interrupted processing and returned an error instead of continuing? The version below addresses your comments, except the one above. Subject: [PATCH 7/8] powerpc/perf/hv-24x7: Support v2 of the hypervisor API POWER9 introduces a new version of the hypervisor API to access the 24x7 perf counters. The new version changed some of the structures used for requests and results. Signed-off-by: Thiago Jung Bauermann --- arch/powerpc/perf/hv-24x7.c | 143 +++++++++++++++++++++++++++------ arch/powerpc/perf/hv-24x7.h | 58 ++++++++++--- arch/powerpc/platforms/pseries/Kconfig | 2 +- 3 files changed, 169 insertions(+), 34 deletions(-) diff --git a/arch/powerpc/perf/hv-24x7.c b/arch/powerpc/perf/hv-24x7.c index 043cbc78be98..35010403f191 100644 --- a/arch/powerpc/perf/hv-24x7.c +++ b/arch/powerpc/perf/hv-24x7.c @@ -18,6 +18,7 @@ #include #include +#include #include #include #include @@ -27,6 +28,9 @@ #include "hv-24x7-catalog.h" #include "hv-common.h" +/* Version of the 24x7 hypervisor API that we should use in this machine. */ +static int interface_version; + static bool domain_is_valid(unsigned domain) { switch (domain) { @@ -74,7 +78,11 @@ static const char *domain_name(unsigned domain) static bool catalog_entry_domain_is_valid(unsigned domain) { - return is_physical_domain(domain); + /* POWER8 doesn't support virtual domains. */ + if (interface_version == 1) + return is_physical_domain(domain); + else + return domain_is_valid(domain); } /* @@ -166,9 +174,11 @@ DEFINE_PER_CPU(struct hv_24x7_hw, hv_24x7_hw); DEFINE_PER_CPU(char, hv_24x7_reqb[H24x7_DATA_BUFFER_SIZE]) __aligned(4096); DEFINE_PER_CPU(char, hv_24x7_resb[H24x7_DATA_BUFFER_SIZE]) __aligned(4096); -#define MAX_NUM_REQUESTS ((H24x7_DATA_BUFFER_SIZE - \ - sizeof(struct hv_24x7_request_buffer)) \ - / sizeof(struct hv_24x7_request)) +static unsigned int max_num_requests(int interface_version) +{ + return (H24x7_DATA_BUFFER_SIZE - sizeof(struct hv_24x7_request_buffer)) + / H24x7_REQUEST_SIZE(interface_version); +} static char *event_name(struct hv_24x7_event_data *ev, int *len) { @@ -1052,7 +1062,7 @@ static void init_24x7_request(struct hv_24x7_request_buffer *request_buffer, memset(request_buffer, 0, H24x7_DATA_BUFFER_SIZE); memset(result_buffer, 0, H24x7_DATA_BUFFER_SIZE); - request_buffer->interface_version = HV_24X7_IF_VERSION_CURRENT; + request_buffer->interface_version = interface_version; /* memset above set request_buffer->num_requests to 0 */ } @@ -1077,7 +1087,7 @@ static int make_24x7_request(struct hv_24x7_request_buffer *request_buffer, if (ret) { struct hv_24x7_request *req; - req = &request_buffer->requests[0]; + req = request_buffer->requests; pr_notice_ratelimited("hcall failed: [%d %#x %#x %d] => ret 0x%lx (%ld) detail=0x%x failing ix=%x\n", req->performance_domain, req->data_offset, req->starting_ix, req->starting_lpar_ix, @@ -1101,9 +1111,11 @@ static int add_event_to_24x7_request(struct perf_event *event, { u16 idx; int i; + size_t req_size; struct hv_24x7_request *req; - if (request_buffer->num_requests >= MAX_NUM_REQUESTS) { + if (request_buffer->num_requests >= + max_num_requests(request_buffer->interface_version)) { pr_devel("Too many requests for 24x7 HCALL %d\n", request_buffer->num_requests); return -EINVAL; @@ -1120,8 +1132,10 @@ static int add_event_to_24x7_request(struct perf_event *event, idx = event_get_vcpu(event); } + req_size = H24x7_REQUEST_SIZE(request_buffer->interface_version); + i = request_buffer->num_requests++; - req = &request_buffer->requests[i]; + req = (void *) request_buffer->requests + i * req_size; req->performance_domain = event_get_domain(event); req->data_size = cpu_to_be16(8); @@ -1131,14 +1145,97 @@ static int add_event_to_24x7_request(struct perf_event *event, req->starting_ix = cpu_to_be16(idx); req->max_ix = cpu_to_be16(1); + if (request_buffer->interface_version > 1 && + req->performance_domain != HV_PERF_DOMAIN_PHYS_CHIP) { + req->starting_thread_group_ix = idx % 2; + req->max_num_thread_groups = 1; + } + return 0; } +/** + * get_count_from_result - get event count from the given result + * + * @event: Event associated with @res. + * @resb: Result buffer containing @res. + * @res: Result to work on. + * @countp: Output variable containing the event count. + * @next: Optional output variable pointing to the next result in @resb. + */ +static int get_count_from_result(struct perf_event *event, + struct hv_24x7_data_result_buffer *resb, + struct hv_24x7_result *res, u64 *countp, + struct hv_24x7_result **next) +{ + u16 num_elements = be16_to_cpu(res->num_elements_returned); + u16 data_size = be16_to_cpu(res->result_element_data_size); + unsigned int data_offset; + void *element_data; + int ret = 0; + + /* + * We can bail out early if the result is empty. + */ + if (!num_elements) { + pr_debug("Result of request %hhu is empty, nothing to do\n", + res->result_ix); + + if (next) + *next = (struct hv_24x7_result *) res->elements; + + return -ENODATA; + } + + /* + * This code assumes that a result has only one element. + */ + if (num_elements != 1) { + pr_debug("Error: result of request %hhu has %hu elements\n", + res->result_ix, num_elements); + + if (!next) + return -ENOTSUPP; + + /* + * We still need to go through the motions so that we can return + * a pointer to the next result. + */ + ret = -ENOTSUPP; + } + + if (data_size != sizeof(u64)) { + pr_debug("Error: result of request %hhu has data of %hu bytes\n", + res->result_ix, data_size); + + if (!next) + return -ENOTSUPP; + + ret = -ENOTSUPP; + } + + if (resb->interface_version == 1) + data_offset = offsetof(struct hv_24x7_result_element_v1, + element_data); + else + data_offset = offsetof(struct hv_24x7_result_element_v2, + element_data); + + element_data = res->elements + data_offset; + + if (!ret) + *countp = be64_to_cpu(*((u64 *) element_data)); + + /* The next result is after the result element. */ + if (next) + *next = element_data + data_size; + + return ret; +} + static int single_24x7_request(struct perf_event *event, u64 *count) { int ret; - u16 num_elements; - struct hv_24x7_result *result; struct hv_24x7_request_buffer *request_buffer; struct hv_24x7_data_result_buffer *result_buffer; @@ -1158,14 +1255,9 @@ static int single_24x7_request(struct perf_event *event, u64 *count) if (ret) goto out; - result = result_buffer->results; - - /* This code assumes that a result has only one element. */ - num_elements = be16_to_cpu(result->num_elements_returned); - WARN_ON_ONCE(num_elements != 1); - /* process result from hcall */ - *count = be64_to_cpu(result->elements[0].element_data[0]); + ret = get_count_from_result(event, result_buffer, + result_buffer->results, count, NULL); out: put_cpu_var(hv_24x7_reqb); @@ -1425,16 +1517,13 @@ static int h_24x7_event_commit_txn(struct pmu *pmu) for (i = 0, res = result_buffer->results; i < result_buffer->num_results; i++, res = next_res) { struct perf_event *event = h24x7hw->events[res->result_ix]; - u16 num_elements = be16_to_cpu(res->num_elements_returned); - u16 data_size = be16_to_cpu(res->result_element_data_size); - /* This code assumes that a result has only one element. */ - WARN_ON_ONCE(num_elements != 1); + ret = get_count_from_result(event, result_buffer, res, &count, + &next_res); + if (ret) + continue; - count = be64_to_cpu(res->elements[0].element_data[0]); update_event_count(event, count); - - next_res = (void *) res->elements[0].element_data + data_size; } put_cpu_var(hv_24x7_hw); @@ -1486,6 +1575,12 @@ static int hv_24x7_init(void) return -ENODEV; } + /* POWER8 only supports v1, while POWER9 only supports v2. */ + if (cpu_has_feature(CPU_FTR_ARCH_300)) + interface_version = 2; + else + interface_version = 1; + hret = hv_perf_caps_get(&caps); if (hret) { pr_debug("could not obtain capabilities, not enabling, rc=%ld\n", diff --git a/arch/powerpc/perf/hv-24x7.h b/arch/powerpc/perf/hv-24x7.h index b95909400b2a..5092c4a222a6 100644 --- a/arch/powerpc/perf/hv-24x7.h +++ b/arch/powerpc/perf/hv-24x7.h @@ -10,6 +10,8 @@ enum hv_perf_domains { HV_PERF_DOMAIN_MAX, }; +#define H24x7_REQUEST_SIZE(iface_version) (iface_version == 1 ? 16 : 32) + struct hv_24x7_request { /* PHYSICAL domains require enabling via phyp/hmc. */ __u8 performance_domain; @@ -42,19 +44,47 @@ struct hv_24x7_request { /* chip, core, or virtual processor based on @performance_domain */ __be16 starting_ix; __be16 max_ix; + + /* The following fields were added in v2 of the 24x7 interface. */ + + __u8 starting_thread_group_ix; + + /* -1 means all thread groups starting at @starting_thread_group_ix */ + __u8 max_num_thread_groups; + + __u8 reserved2[0xE]; } __packed; struct hv_24x7_request_buffer { /* 0 - ? */ /* 1 - ? */ -#define HV_24X7_IF_VERSION_CURRENT 0x01 __u8 interface_version; __u8 num_requests; __u8 reserved[0xE]; - struct hv_24x7_request requests[1]; + struct hv_24x7_request requests[]; +} __packed; + +struct hv_24x7_result_element_v1 { + __be16 lpar_ix; + + /* + * represents the core, chip, or virtual processor based on the + * request's @performance_domain + */ + __be16 domain_ix; + + /* -1 if @performance_domain does not refer to a virtual processor */ + __be32 lpar_cfg_instance_id; + + /* size = @result_element_data_size of containing result. */ + __u64 element_data[]; } __packed; -struct hv_24x7_result_element { +/* + * We need a separate struct for v2 because the offset of @element_data changed + * between versions. + */ +struct hv_24x7_result_element_v2 { __be16 lpar_ix; /* @@ -66,8 +96,12 @@ struct hv_24x7_result_element { /* -1 if @performance_domain does not refer to a virtual processor */ __be32 lpar_cfg_instance_id; + __u8 thread_group_ix; + + __u8 reserved[7]; + /* size = @result_element_data_size of containing result. */ - __u64 element_data[1]; + __u64 element_data[]; } __packed; struct hv_24x7_result { @@ -94,10 +128,16 @@ struct hv_24x7_result { __be16 result_element_data_size; __u8 reserved[0x2]; - /* WARNING: only valid for first result element due to variable sizes - * of result elements */ - /* struct hv_24x7_result_element[@num_elements_returned] */ - struct hv_24x7_result_element elements[1]; + /* + * Either + * struct hv_24x7_result_element_v1[@num_elements_returned] + * or + * struct hv_24x7_result_element_v2[@num_elements_returned] + * + * depending on the interface_version field of the + * struct hv_24x7_data_result_buffer containing this result. + */ + char elements[]; } __packed; struct hv_24x7_data_result_buffer { @@ -113,7 +153,7 @@ struct hv_24x7_data_result_buffer { __u8 reserved2[0x8]; /* WARNING: only valid for the first result due to variable sizes of * results */ - struct hv_24x7_result results[1]; /* [@num_results] */ + struct hv_24x7_result results[]; /* [@num_results] */ } __packed; #endif diff --git a/arch/powerpc/platforms/pseries/Kconfig b/arch/powerpc/platforms/pseries/Kconfig index 913c54e23eea..3a6dfd14f64b 100644 --- a/arch/powerpc/platforms/pseries/Kconfig +++ b/arch/powerpc/platforms/pseries/Kconfig @@ -124,7 +124,7 @@ config HV_PERF_CTRS Enable access to hypervisor supplied counters in perf. Currently, this enables code that uses the hcall GetPerfCounterInfo and 24x7 interfaces to retrieve counters. GPCI exists on Power 6 and later - systems. 24x7 is available on Power 8 systems. + systems. 24x7 is available on Power 8 and later systems. If unsure, select Y.