diff mbox series

powerpc/pseries/vas: Don't print an error when VAS is unavailable

Message ID 20211126052133.1664375-1-npiggin@gmail.com (mailing list archive)
State Accepted
Headers show
Series powerpc/pseries/vas: Don't print an error when VAS is unavailable | expand

Checks

Context Check Description
snowpatch_ozlabs/github-powerpc_ppctests success Successfully ran 8 jobs.
snowpatch_ozlabs/github-powerpc_selftests success Successfully ran 8 jobs.
snowpatch_ozlabs/github-powerpc_sparse success Successfully ran 4 jobs.
snowpatch_ozlabs/github-powerpc_clang success Successfully ran 7 jobs.
snowpatch_ozlabs/github-powerpc_kernel_qemu success Successfully ran 24 jobs.

Commit Message

Nicholas Piggin Nov. 26, 2021, 5:21 a.m. UTC 
KVM does not support VAS so guests always print a useless error on boot

    vas: HCALL(398) error -2, query_type 0, result buffer 0x57f2000

Change this to only print the message if the error is not H_FUNCTION.

Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
 arch/powerpc/platforms/pseries/vas.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

Comments

Cédric Le Goater Nov. 26, 2021, 7:13 a.m. UTC | #1 
On 11/26/21 06:21, Nicholas Piggin wrote:
> KVM does not support VAS so guests always print a useless error on boot
> 
>      vas: HCALL(398) error -2, query_type 0, result buffer 0x57f2000
> 
> Change this to only print the message if the error is not H_FUNCTION.


Just being curious, why is it even called since "ibm,compression" should
not be exposed in the DT ?

C.

> 
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> ---
>   arch/powerpc/platforms/pseries/vas.c | 11 +++++++++--
>   1 file changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/powerpc/platforms/pseries/vas.c b/arch/powerpc/platforms/pseries/vas.c
> index b043e3936d21..734523e2272f 100644
> --- a/arch/powerpc/platforms/pseries/vas.c
> +++ b/arch/powerpc/platforms/pseries/vas.c
> @@ -151,8 +151,15 @@ int h_query_vas_capabilities(const u64 hcall, u8 query_type, u64 result)
>   	if (rc == H_SUCCESS)
>   		return 0;
>   
> -	pr_err("HCALL(%llx) error %ld, query_type %u, result buffer 0x%llx\n",
> -			hcall, rc, query_type, result);
> +	/* H_FUNCTION means HV does not support VAS so don't print an error */
> +	if (rc != H_FUNCTION) {
> +		pr_err("%s error %ld, query_type %u, result buffer 0x%llx\n",
> +			(hcall == H_QUERY_VAS_CAPABILITIES) ?
> +				"H_QUERY_VAS_CAPABILITIES" :
> +				"H_QUERY_NX_CAPABILITIES",
> +			rc, query_type, result);
> +	}
> +
>   	return -EIO;
>   }
>   EXPORT_SYMBOL_GPL(h_query_vas_capabilities);
>
Nicholas Piggin Nov. 26, 2021, 10:31 a.m. UTC | #2 
Excerpts from Cédric Le Goater's message of November 26, 2021 5:13 pm:
> On 11/26/21 06:21, Nicholas Piggin wrote:
>> KVM does not support VAS so guests always print a useless error on boot
>> 
>>      vas: HCALL(398) error -2, query_type 0, result buffer 0x57f2000
>> 
>> Change this to only print the message if the error is not H_FUNCTION.
> 
> 
> Just being curious, why is it even called since "ibm,compression" should
> not be exposed in the DT ?

It looks like vas does not test for it. I guess in theory there can be 
other functions than compression implemented as an accelerator. Maybe
that's why?

Thanks,
Nick
Tyrel Datwyler Nov. 29, 2021, 6:13 p.m. UTC | #3 
On 11/26/21 2:31 AM, Nicholas Piggin wrote:
> Excerpts from Cédric Le Goater's message of November 26, 2021 5:13 pm:
>> On 11/26/21 06:21, Nicholas Piggin wrote:
>>> KVM does not support VAS so guests always print a useless error on boot
>>>
>>>      vas: HCALL(398) error -2, query_type 0, result buffer 0x57f2000
>>>
>>> Change this to only print the message if the error is not H_FUNCTION.
>>
>>
>> Just being curious, why is it even called since "ibm,compression" should
>> not be exposed in the DT ?
> 
> It looks like vas does not test for it. I guess in theory there can be 
> other functions than compression implemented as an accelerator. Maybe
> that's why?
> 
> Thanks,
> Nick
> 
Looks like pseries_vas_init() simply calls h_query_vas_capabilities() to test
for VAS coprocessor support. I would assume KVM doesn't expose hcall-vas or
hcall-nx in /rtas/ibm,hypertas-functions? Doesn't look like hcall-vas or
hcall-nx have been added to the hypertas_fw_feature matching, but maybe they
should and we can gate VAS initialization on those, or at the minimum
FW_FEATURE_VAS?

-Tyrel
Michael Ellerman Nov. 29, 2021, 11:25 p.m. UTC | #4 
Nicholas Piggin <npiggin@gmail.com> writes:
> Excerpts from Cédric Le Goater's message of November 26, 2021 5:13 pm:
>> On 11/26/21 06:21, Nicholas Piggin wrote:
>>> KVM does not support VAS so guests always print a useless error on boot
>>> 
>>>      vas: HCALL(398) error -2, query_type 0, result buffer 0x57f2000
>>> 
>>> Change this to only print the message if the error is not H_FUNCTION.
>> 
>> 
>> Just being curious, why is it even called since "ibm,compression" should
>> not be exposed in the DT ?
>
> It looks like vas does not test for it. I guess in theory there can be 
> other functions than compression implemented as an accelerator. Maybe
> that's why?

Yeah I guess, or it's just not structured that well. The vas platform
code is a bit awkward, it's there to support drivers, but it's not
actually driver code.

I think we can probably rework it so the vas code does nothing until a
driver calls in to it.

eg. something like below.

cheers


diff --git a/arch/powerpc/platforms/pseries/vas.c b/arch/powerpc/platforms/pseries/vas.c
index b043e3936d21..dc3491fc919d 100644
--- a/arch/powerpc/platforms/pseries/vas.c
+++ b/arch/powerpc/platforms/pseries/vas.c
@@ -454,6 +454,8 @@ static const struct vas_user_win_ops vops_pseries = {
 	.close_win	= vas_deallocate_window, /* Close window */
 };
 
+static int pseries_vas_init(void);
+
 /*
  * Supporting only nx-gzip coprocessor type now, but this API code
  * extended to other coprocessor types later.
@@ -463,7 +465,8 @@ int vas_register_api_pseries(struct module *mod, enum vas_cop_type cop_type,
 {
 	int rc;
 
-	if (!copypaste_feat)
+	rc = pseries_vas_init();
+	if (rc || !copypaste_feat)
 		return -ENOTSUPP;
 
 	rc = vas_register_coproc_api(mod, cop_type, name, &vops_pseries);
@@ -531,7 +534,7 @@ static int get_vas_capabilities(u8 feat, enum vas_cop_feat_type type,
 	return 0;
 }
 
-static int __init pseries_vas_init(void)
+static int pseries_vas_init(void)
 {
 	struct hv_vas_cop_feat_caps *hv_cop_caps;
 	struct hv_vas_all_caps *hv_caps;
@@ -592,4 +595,3 @@ static int __init pseries_vas_init(void)
 	kfree(hv_caps);
 	return rc;
 }
-machine_device_initcall(pseries, pseries_vas_init);
Haren Myneni Nov. 30, 2021, 7:35 a.m. UTC | #5 
On Tue, 2021-11-30 at 10:25 +1100, Michael Ellerman wrote:
> Nicholas Piggin <npiggin@gmail.com> writes:
> > Excerpts from Cédric Le Goater's message of November 26, 2021 5:13
> > pm:
> > > On 11/26/21 06:21, Nicholas Piggin wrote:
> > > > KVM does not support VAS so guests always print a useless error
> > > > on boot
> > > > 
> > > >      vas: HCALL(398) error -2, query_type 0, result buffer
> > > > 0x57f2000
> > > > 
> > > > Change this to only print the message if the error is not
> > > > H_FUNCTION.
> > > 
> > > Just being curious, why is it even called since "ibm,compression"
> > > should
> > > not be exposed in the DT ?
> > 
> > It looks like vas does not test for it. I guess in theory there can
> > be 
> > other functions than compression implemented as an accelerator.
> > Maybe
> > that's why?
> 
> Yeah I guess, or it's just not structured that well. The vas platform
> code is a bit awkward, it's there to support drivers, but it's not
> actually driver code.
> 
> I think we can probably rework it so the vas code does nothing until
> a
> driver calls in to it.
> 
> eg. something like below.

Correct, Even though NXGZIP is the only usage right now, VAS is
accelerator switchboard which should support other coprocessor types
such as GZIP and 842 or SW type solutions such as fast thread wakeup
and fast memory copy. 

So can we leave VAS initialization separate from drivers and use some
feature such as FW_FEATURE_LPAR to differentiate from KVM guests?

Thanks
Haren

> 
> cheers
> 
> 
> diff --git a/arch/powerpc/platforms/pseries/vas.c
> b/arch/powerpc/platforms/pseries/vas.c
> index b043e3936d21..dc3491fc919d 100644
> --- a/arch/powerpc/platforms/pseries/vas.c
> +++ b/arch/powerpc/platforms/pseries/vas.c
> @@ -454,6 +454,8 @@ static const struct vas_user_win_ops vops_pseries
> = {
>  	.close_win	= vas_deallocate_window, /* Close window */
>  };
>  
> +static int pseries_vas_init(void);
> +
>  /*
>   * Supporting only nx-gzip coprocessor type now, but this API code
>   * extended to other coprocessor types later.
> @@ -463,7 +465,8 @@ int vas_register_api_pseries(struct module *mod,
> enum vas_cop_type cop_type,
>  {
>  	int rc;
>  
> -	if (!copypaste_feat)
> +	rc = pseries_vas_init();
> +	if (rc || !copypaste_feat)
>  		return -ENOTSUPP;
>  
>  	rc = vas_register_coproc_api(mod, cop_type, name,
> &vops_pseries);
> @@ -531,7 +534,7 @@ static int get_vas_capabilities(u8 feat, enum
> vas_cop_feat_type type,
>  	return 0;
>  }
>  
> -static int __init pseries_vas_init(void)
> +static int pseries_vas_init(void)
>  {
>  	struct hv_vas_cop_feat_caps *hv_cop_caps;
>  	struct hv_vas_all_caps *hv_caps;
> @@ -592,4 +595,3 @@ static int __init pseries_vas_init(void)
>  	kfree(hv_caps);
>  	return rc;
>  }
> -machine_device_initcall(pseries, pseries_vas_init);
Michael Ellerman Nov. 30, 2021, 11:21 a.m. UTC | #6 
Haren Myneni <haren@linux.ibm.com> writes:
> On Tue, 2021-11-30 at 10:25 +1100, Michael Ellerman wrote:
>> Nicholas Piggin <npiggin@gmail.com> writes:
>> > Excerpts from Cédric Le Goater's message of November 26, 2021 5:13
>> > pm:
>> > > On 11/26/21 06:21, Nicholas Piggin wrote:
>> > > > KVM does not support VAS so guests always print a useless error
>> > > > on boot
>> > > > 
>> > > >      vas: HCALL(398) error -2, query_type 0, result buffer
>> > > > 0x57f2000
>> > > > 
>> > > > Change this to only print the message if the error is not
>> > > > H_FUNCTION.
>> > > 
>> > > Just being curious, why is it even called since "ibm,compression"
>> > > should
>> > > not be exposed in the DT ?
>> > 
>> > It looks like vas does not test for it. I guess in theory there can
>> > be 
>> > other functions than compression implemented as an accelerator.
>> > Maybe
>> > that's why?
>> 
>> Yeah I guess, or it's just not structured that well. The vas platform
>> code is a bit awkward, it's there to support drivers, but it's not
>> actually driver code.
>> 
>> I think we can probably rework it so the vas code does nothing until
>> a
>> driver calls in to it.
>> 
>> eg. something like below.
>
> Correct, Even though NXGZIP is the only usage right now, VAS is
> accelerator switchboard which should support other coprocessor types
> such as GZIP and 842 or SW type solutions such as fast thread wakeup
> and fast memory copy. 
>
> So can we leave VAS initialization separate from drivers and use some
> feature such as FW_FEATURE_LPAR to differentiate from KVM guests?

FW_FEATURE_LPAR is true on KVM guests as well.

As Tyrel pointed out, you should be looking for "hcall-vas" in
"ibm,hypertas-functions" and setting a new FW_FEATURE_VAS based on that.
Then use that to gate the vas init routine.

cheers
Michael Ellerman Dec. 21, 2021, 12:14 p.m. UTC | #7 
On Fri, 26 Nov 2021 15:21:33 +1000, Nicholas Piggin wrote:
> KVM does not support VAS so guests always print a useless error on boot
> 
>     vas: HCALL(398) error -2, query_type 0, result buffer 0x57f2000
> 
> Change this to only print the message if the error is not H_FUNCTION.
> 
> 
> [...]

Applied to powerpc/next.

[1/1] powerpc/pseries/vas: Don't print an error when VAS is unavailable
      https://git.kernel.org/powerpc/c/0a006ace634dcaf1bbf9125fb8089a4a50bf33d6

cheers
diff mbox series

Patch

diff --git a/arch/powerpc/platforms/pseries/vas.c b/arch/powerpc/platforms/pseries/vas.c
index b043e3936d21..734523e2272f 100644
--- a/arch/powerpc/platforms/pseries/vas.c
+++ b/arch/powerpc/platforms/pseries/vas.c
@@ -151,8 +151,15 @@  int h_query_vas_capabilities(const u64 hcall, u8 query_type, u64 result)
 	if (rc == H_SUCCESS)
 		return 0;
 
-	pr_err("HCALL(%llx) error %ld, query_type %u, result buffer 0x%llx\n",
-			hcall, rc, query_type, result);
+	/* H_FUNCTION means HV does not support VAS so don't print an error */
+	if (rc != H_FUNCTION) {
+		pr_err("%s error %ld, query_type %u, result buffer 0x%llx\n",
+			(hcall == H_QUERY_VAS_CAPABILITIES) ?
+				"H_QUERY_VAS_CAPABILITIES" :
+				"H_QUERY_NX_CAPABILITIES",
+			rc, query_type, result);
+	}
+
 	return -EIO;
 }
 EXPORT_SYMBOL_GPL(h_query_vas_capabilities);