Re: [PATCH v3 2/3] powerpc/papr_scm: Update drc_pmem_unbind() to use H_SCM_UNBIND_ALL
Thanks for reviewing this patch Aneesh,
"Aneesh Kumar K.V" writes:
> On 6/26/19 7:34 PM, Vaibhav Jain wrote:
>> The new hcall named H_SCM_UNBIND_ALL has been introduce that can
>> unbind all or specific scm memory assigned to an lpar. This is
>> more efficient than using H_SCM_UNBIND_MEM as currently we don't
>> support partial unbind of scm memory.
>>
>> Hence this patch proposes following changes to drc_pmem_unbind():
>>
>> * Update drc_pmem_unbind() to replace hcall H_SCM_UNBIND_MEM to
>>H_SCM_UNBIND_ALL.
>>
>> * Update drc_pmem_unbind() to handles cases when PHYP asks the guest
>>kernel to wait for specific amount of time before retrying the
>>hcall via the 'LONG_BUSY' return value.
>>
>> * Ensure appropriate error code is returned back from the function
>>in case of an error.
>>
>> Reviewed-by: Oliver O'Halloran
>> Signed-off-by: Vaibhav Jain
>> ---
>> Change-log:
>>
>> v3:
>> * Fixed a build warning reported by kbuild-robot.
>> * Updated patch description to put emphasis on 'scm memory' instead of
>>'scm drc memory blocks' as for phyp there is a stark difference
>>between how drc are managed for scm memory v/s regular memory. [Oliver]
>>
>> v2:
>> * Added a dev_dbg when unbind operation succeeds [Oliver]
>> * Changed type of variable 'rc' to int64_t [Oliver]
>> * Removed the code that was logging a warning in case bind operation
>>takes >1-seconds [Oliver]
>> * Spinned off changes to hvcall.h as a separate patch. [Oliver]
>> ---
>> arch/powerpc/platforms/pseries/papr_scm.c | 29 +--
>> 1 file changed, 22 insertions(+), 7 deletions(-)
>>
>> diff --git a/arch/powerpc/platforms/pseries/papr_scm.c
>> b/arch/powerpc/platforms/pseries/papr_scm.c
>> index 96c53b23e58f..c01a03fd3ee7 100644
>> --- a/arch/powerpc/platforms/pseries/papr_scm.c
>> +++ b/arch/powerpc/platforms/pseries/papr_scm.c
>> @@ -11,6 +11,7 @@
>> #include
>> #include
>> #include
>> +#include
>>
>> #include
>>
>> @@ -77,22 +78,36 @@ static int drc_pmem_bind(struct papr_scm_priv *p)
>> static int drc_pmem_unbind(struct papr_scm_priv *p)
>> {
>> unsigned long ret[PLPAR_HCALL_BUFSIZE];
>> -uint64_t rc, token;
>> +uint64_t token = 0;
>> +int64_t rc;
>>
>> -token = 0;
>> +dev_dbg(>pdev->dev, "unbind drc %x\n", p->drc_index);
>>
>> -/* NB: unbind has the same retry requirements mentioned above */
>> +/* NB: unbind has the same retry requirements as drc_pmem_bind() */
>> do {
>> -rc = plpar_hcall(H_SCM_UNBIND_MEM, ret, p->drc_index,
>> -p->bound_addr, p->blocks, token);
>> +
>> +/* Unbind of all SCM resources associated with drcIndex */
>> +rc = plpar_hcall(H_SCM_UNBIND_ALL, ret, H_UNBIND_SCOPE_DRC,
>> + p->drc_index, token);
>> token = ret[0];
>> -cond_resched();
>> +
>> +/* Check if we are stalled for some time */
>> +if (H_IS_LONG_BUSY(rc)) {
>> +msleep(get_longbusy_msecs(rc));
>> +rc = H_BUSY;
>> +} else if (rc == H_BUSY) {
>> +cond_resched();
>> +}
>> +
>> } while (rc == H_BUSY);
>>
>> if (rc)
>> dev_err(>pdev->dev, "unbind error: %lld\n", rc);
>> +else
>> +dev_dbg(>pdev->dev, "unbind drc %x complete\n",
>> +p->drc_index);
>>
> Can we add p->drc_index as part of these messages? Also s/%x/0x%x ?
the scm platform device has the name of the form
"ibm,persistent-memory:ibm,pmemory@4412" which also contains the
drc_index. So i think printing it again in this message would be
redundant.
>
>
>> -return !!rc;
>> +return rc == H_SUCCESS ? 0 : -ENXIO;
>> }
>>
> The error -ENXIO is confusing. Can we keep the HCALL error as return for
> this? We don't check error from this. If we can't take any action based
> on the return. Then may be make it void?
>
Wanted to keep the behaviour of this function symmetric to
drc_pmem_bind() which when updated in the next patch returns a kernel
error code instead of hcall error.
Agree that we arent using the return
value of this function right now but this may change in the future.
>
>> static int papr_scm_meta_get(struct papr_scm_priv *p,
>>
>
>
> -aneesh
--
Vaibhav Jain
Linux Technology Center, IBM India Pvt. Ltd.
Re: [PATCH v3 2/3] powerpc/papr_scm: Update drc_pmem_unbind() to use H_SCM_UNBIND_ALL
On 6/26/19 7:34 PM, Vaibhav Jain wrote:
The new hcall named H_SCM_UNBIND_ALL has been introduce that can
unbind all or specific scm memory assigned to an lpar. This is
more efficient than using H_SCM_UNBIND_MEM as currently we don't
support partial unbind of scm memory.
Hence this patch proposes following changes to drc_pmem_unbind():
* Update drc_pmem_unbind() to replace hcall H_SCM_UNBIND_MEM to
H_SCM_UNBIND_ALL.
* Update drc_pmem_unbind() to handles cases when PHYP asks the guest
kernel to wait for specific amount of time before retrying the
hcall via the 'LONG_BUSY' return value.
* Ensure appropriate error code is returned back from the function
in case of an error.
Reviewed-by: Oliver O'Halloran
Signed-off-by: Vaibhav Jain
---
Change-log:
v3:
* Fixed a build warning reported by kbuild-robot.
* Updated patch description to put emphasis on 'scm memory' instead of
'scm drc memory blocks' as for phyp there is a stark difference
between how drc are managed for scm memory v/s regular memory. [Oliver]
v2:
* Added a dev_dbg when unbind operation succeeds [Oliver]
* Changed type of variable 'rc' to int64_t [Oliver]
* Removed the code that was logging a warning in case bind operation
takes >1-seconds [Oliver]
* Spinned off changes to hvcall.h as a separate patch. [Oliver]
---
arch/powerpc/platforms/pseries/papr_scm.c | 29 +--
1 file changed, 22 insertions(+), 7 deletions(-)
diff --git a/arch/powerpc/platforms/pseries/papr_scm.c
b/arch/powerpc/platforms/pseries/papr_scm.c
index 96c53b23e58f..c01a03fd3ee7 100644
--- a/arch/powerpc/platforms/pseries/papr_scm.c
+++ b/arch/powerpc/platforms/pseries/papr_scm.c
@@ -11,6 +11,7 @@
#include
#include
#include
+#include
#include
@@ -77,22 +78,36 @@ static int drc_pmem_bind(struct papr_scm_priv *p)
static int drc_pmem_unbind(struct papr_scm_priv *p)
{
unsigned long ret[PLPAR_HCALL_BUFSIZE];
- uint64_t rc, token;
+ uint64_t token = 0;
+ int64_t rc;
- token = 0;
+ dev_dbg(>pdev->dev, "unbind drc %x\n", p->drc_index);
- /* NB: unbind has the same retry requirements mentioned above */
+ /* NB: unbind has the same retry requirements as drc_pmem_bind() */
do {
- rc = plpar_hcall(H_SCM_UNBIND_MEM, ret, p->drc_index,
- p->bound_addr, p->blocks, token);
+
+ /* Unbind of all SCM resources associated with drcIndex */
+ rc = plpar_hcall(H_SCM_UNBIND_ALL, ret, H_UNBIND_SCOPE_DRC,
+p->drc_index, token);
token = ret[0];
- cond_resched();
+
+ /* Check if we are stalled for some time */
+ if (H_IS_LONG_BUSY(rc)) {
+ msleep(get_longbusy_msecs(rc));
+ rc = H_BUSY;
+ } else if (rc == H_BUSY) {
+ cond_resched();
+ }
+
} while (rc == H_BUSY);
if (rc)
dev_err(>pdev->dev, "unbind error: %lld\n", rc);
+ else
+ dev_dbg(>pdev->dev, "unbind drc %x complete\n",
+ p->drc_index);
Can we add p->drc_index as part of these messages? Also s/%x/0x%x ?
- return !!rc;
+ return rc == H_SUCCESS ? 0 : -ENXIO;
}
The error -ENXIO is confusing. Can we keep the HCALL error as return for
this? We don't check error from this. If we can't take any action based
on the return. Then may be make it void?
static int papr_scm_meta_get(struct papr_scm_priv *p,
-aneesh
[PATCH v3 2/3] powerpc/papr_scm: Update drc_pmem_unbind() to use H_SCM_UNBIND_ALL
The new hcall named H_SCM_UNBIND_ALL has been introduce that can
unbind all or specific scm memory assigned to an lpar. This is
more efficient than using H_SCM_UNBIND_MEM as currently we don't
support partial unbind of scm memory.
Hence this patch proposes following changes to drc_pmem_unbind():
* Update drc_pmem_unbind() to replace hcall H_SCM_UNBIND_MEM to
H_SCM_UNBIND_ALL.
* Update drc_pmem_unbind() to handles cases when PHYP asks the guest
kernel to wait for specific amount of time before retrying the
hcall via the 'LONG_BUSY' return value.
* Ensure appropriate error code is returned back from the function
in case of an error.
Reviewed-by: Oliver O'Halloran
Signed-off-by: Vaibhav Jain
---
Change-log:
v3:
* Fixed a build warning reported by kbuild-robot.
* Updated patch description to put emphasis on 'scm memory' instead of
'scm drc memory blocks' as for phyp there is a stark difference
between how drc are managed for scm memory v/s regular memory. [Oliver]
v2:
* Added a dev_dbg when unbind operation succeeds [Oliver]
* Changed type of variable 'rc' to int64_t [Oliver]
* Removed the code that was logging a warning in case bind operation
takes >1-seconds [Oliver]
* Spinned off changes to hvcall.h as a separate patch. [Oliver]
---
arch/powerpc/platforms/pseries/papr_scm.c | 29 +--
1 file changed, 22 insertions(+), 7 deletions(-)
diff --git a/arch/powerpc/platforms/pseries/papr_scm.c
b/arch/powerpc/platforms/pseries/papr_scm.c
index 96c53b23e58f..c01a03fd3ee7 100644
--- a/arch/powerpc/platforms/pseries/papr_scm.c
+++ b/arch/powerpc/platforms/pseries/papr_scm.c
@@ -11,6 +11,7 @@
#include
#include
#include
+#include
#include
@@ -77,22 +78,36 @@ static int drc_pmem_bind(struct papr_scm_priv *p)
static int drc_pmem_unbind(struct papr_scm_priv *p)
{
unsigned long ret[PLPAR_HCALL_BUFSIZE];
- uint64_t rc, token;
+ uint64_t token = 0;
+ int64_t rc;
- token = 0;
+ dev_dbg(>pdev->dev, "unbind drc %x\n", p->drc_index);
- /* NB: unbind has the same retry requirements mentioned above */
+ /* NB: unbind has the same retry requirements as drc_pmem_bind() */
do {
- rc = plpar_hcall(H_SCM_UNBIND_MEM, ret, p->drc_index,
- p->bound_addr, p->blocks, token);
+
+ /* Unbind of all SCM resources associated with drcIndex */
+ rc = plpar_hcall(H_SCM_UNBIND_ALL, ret, H_UNBIND_SCOPE_DRC,
+p->drc_index, token);
token = ret[0];
- cond_resched();
+
+ /* Check if we are stalled for some time */
+ if (H_IS_LONG_BUSY(rc)) {
+ msleep(get_longbusy_msecs(rc));
+ rc = H_BUSY;
+ } else if (rc == H_BUSY) {
+ cond_resched();
+ }
+
} while (rc == H_BUSY);
if (rc)
dev_err(>pdev->dev, "unbind error: %lld\n", rc);
+ else
+ dev_dbg(>pdev->dev, "unbind drc %x complete\n",
+ p->drc_index);
- return !!rc;
+ return rc == H_SUCCESS ? 0 : -ENXIO;
}
static int papr_scm_meta_get(struct papr_scm_priv *p,
--
2.21.0
