* [PATCH] powerpc/nvdimm: Add support for multibyte read/write for metadata
@ 2019-06-02 4:43 Aneesh Kumar K.V
2019-06-03 0:25 ` Oliver
2019-06-05 8:02 ` Alexey Kardashevskiy
0 siblings, 2 replies; 7+ messages in thread
From: Aneesh Kumar K.V @ 2019-06-02 4:43 UTC (permalink / raw)
To: npiggin, paulus, mpe; +Cc: oohall, linuxppc-dev, Aneesh Kumar K.V
SCM_READ/WRITE_MEATADATA hcall supports multibyte read/write. This patch
updates the metadata read/write to use 1, 2, 4 or 8 byte read/write as
mentioned in PAPR document.
READ/WRITE_METADATA hcall supports the 1, 2, 4, or 8 bytes read/write.
For other values hcall results H_P3.
Hypervisor stores the metadata contents in big-endian format and in-order
to enable read/write in different granularity, we need to switch the contents
to big-endian before calling HCALL.
Based on an patch from Oliver O'Halloran <oohall@gmail.com>
Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
---
arch/powerpc/platforms/pseries/papr_scm.c | 104 +++++++++++++++++-----
1 file changed, 82 insertions(+), 22 deletions(-)
diff --git a/arch/powerpc/platforms/pseries/papr_scm.c b/arch/powerpc/platforms/pseries/papr_scm.c
index 0176ce66673f..e33cebb8ee6c 100644
--- a/arch/powerpc/platforms/pseries/papr_scm.c
+++ b/arch/powerpc/platforms/pseries/papr_scm.c
@@ -97,42 +97,102 @@ static int drc_pmem_unbind(struct papr_scm_priv *p)
}
static int papr_scm_meta_get(struct papr_scm_priv *p,
- struct nd_cmd_get_config_data_hdr *hdr)
+ struct nd_cmd_get_config_data_hdr *hdr)
{
unsigned long data[PLPAR_HCALL_BUFSIZE];
+ unsigned long offset, data_offset;
+ int len, read;
int64_t ret;
- if (hdr->in_offset >= p->metadata_size || hdr->in_length != 1)
+ if ((hdr->in_offset + hdr->in_length) >= p->metadata_size)
return -EINVAL;
- ret = plpar_hcall(H_SCM_READ_METADATA, data, p->drc_index,
- hdr->in_offset, 1);
-
- if (ret == H_PARAMETER) /* bad DRC index */
- return -ENODEV;
- if (ret)
- return -EINVAL; /* other invalid parameter */
-
- hdr->out_buf[0] = data[0] & 0xff;
-
+ for (len = hdr->in_length; len; len -= read) {
+
+ data_offset = hdr->in_length - len;
+ offset = hdr->in_offset + data_offset;
+
+ if (len >= 8)
+ read = 8;
+ else if (len >= 4)
+ read = 4;
+ else if ( len >= 2)
+ read = 2;
+ else
+ read = 1;
+
+ ret = plpar_hcall(H_SCM_READ_METADATA, data, p->drc_index,
+ offset, read);
+
+ if (ret == H_PARAMETER) /* bad DRC index */
+ return -ENODEV;
+ if (ret)
+ return -EINVAL; /* other invalid parameter */
+
+ switch (read) {
+ case 8:
+ *(uint64_t *)(hdr->out_buf + data_offset) = be64_to_cpu(data[0]);
+ break;
+ case 4:
+ *(uint32_t *)(hdr->out_buf + data_offset) = be32_to_cpu(data[0] & 0xffffffff);
+ break;
+
+ case 2:
+ *(uint16_t *)(hdr->out_buf + data_offset) = be16_to_cpu(data[0] & 0xffff);
+ break;
+
+ case 1:
+ *(uint32_t *)(hdr->out_buf + data_offset) = (data[0] & 0xff);
+ break;
+ }
+ }
return 0;
}
static int papr_scm_meta_set(struct papr_scm_priv *p,
- struct nd_cmd_set_config_hdr *hdr)
+ struct nd_cmd_set_config_hdr *hdr)
{
+ unsigned long offset, data_offset;
+ int len, wrote;
+ unsigned long data;
+ __be64 data_be;
int64_t ret;
- if (hdr->in_offset >= p->metadata_size || hdr->in_length != 1)
+ if ((hdr->in_offset + hdr->in_length) >= p->metadata_size)
return -EINVAL;
- ret = plpar_hcall_norets(H_SCM_WRITE_METADATA,
- p->drc_index, hdr->in_offset, hdr->in_buf[0], 1);
-
- if (ret == H_PARAMETER) /* bad DRC index */
- return -ENODEV;
- if (ret)
- return -EINVAL; /* other invalid parameter */
+ for (len = hdr->in_length; len; len -= wrote) {
+
+ data_offset = hdr->in_length - len;
+ offset = hdr->in_offset + data_offset;
+
+ if (len >= 8) {
+ data = *(uint64_t *)(hdr->in_buf + data_offset);
+ data_be = cpu_to_be64(data);
+ wrote = 8;
+ } else if (len >= 4) {
+ data = *(uint32_t *)(hdr->in_buf + data_offset);
+ data &= 0xffffffff;
+ data_be = cpu_to_be32(data);
+ wrote = 4;
+ } else if (len >= 2) {
+ data = *(uint16_t *)(hdr->in_buf + data_offset);
+ data &= 0xffff;
+ data_be = cpu_to_be16(data);
+ wrote = 2;
+ } else {
+ data_be = *(uint8_t *)(hdr->in_buf + data_offset);
+ data_be &= 0xff;
+ wrote = 1;
+ }
+
+ ret = plpar_hcall_norets(H_SCM_WRITE_METADATA, p->drc_index,
+ offset, data_be, wrote);
+ if (ret == H_PARAMETER) /* bad DRC index */
+ return -ENODEV;
+ if (ret)
+ return -EINVAL; /* other invalid parameter */
+ }
return 0;
}
@@ -154,7 +214,7 @@ int papr_scm_ndctl(struct nvdimm_bus_descriptor *nd_desc, struct nvdimm *nvdimm,
get_size_hdr = buf;
get_size_hdr->status = 0;
- get_size_hdr->max_xfer = 1;
+ get_size_hdr->max_xfer = 8;
get_size_hdr->config_size = p->metadata_size;
*cmd_rc = 0;
break;
--
2.21.0
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] powerpc/nvdimm: Add support for multibyte read/write for metadata
2019-06-02 4:43 [PATCH] powerpc/nvdimm: Add support for multibyte read/write for metadata Aneesh Kumar K.V
@ 2019-06-03 0:25 ` Oliver
2019-06-04 9:06 ` Aneesh Kumar K.V
2019-06-05 8:02 ` Alexey Kardashevskiy
1 sibling, 1 reply; 7+ messages in thread
From: Oliver @ 2019-06-03 0:25 UTC (permalink / raw)
To: Aneesh Kumar K.V; +Cc: Paul Mackerras, linuxppc-dev, Nicholas Piggin
On Sun, Jun 2, 2019 at 2:44 PM Aneesh Kumar K.V
<aneesh.kumar@linux.ibm.com> wrote:
>
> SCM_READ/WRITE_MEATADATA hcall supports multibyte read/write. This patch
> updates the metadata read/write to use 1, 2, 4 or 8 byte read/write as
> mentioned in PAPR document.
>
> READ/WRITE_METADATA hcall supports the 1, 2, 4, or 8 bytes read/write.
> For other values hcall results H_P3.
You should probably fold the second paragraph here into the first.
> Hypervisor stores the metadata contents in big-endian format and in-order
> to enable read/write in different granularity, we need to switch the contents
> to big-endian before calling HCALL.
>
> Based on an patch from Oliver O'Halloran <oohall@gmail.com>
> Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
> ---
> arch/powerpc/platforms/pseries/papr_scm.c | 104 +++++++++++++++++-----
> 1 file changed, 82 insertions(+), 22 deletions(-)
>
> diff --git a/arch/powerpc/platforms/pseries/papr_scm.c b/arch/powerpc/platforms/pseries/papr_scm.c
> index 0176ce66673f..e33cebb8ee6c 100644
> --- a/arch/powerpc/platforms/pseries/papr_scm.c
> +++ b/arch/powerpc/platforms/pseries/papr_scm.c
> @@ -97,42 +97,102 @@ static int drc_pmem_unbind(struct papr_scm_priv *p)
> }
>
> static int papr_scm_meta_get(struct papr_scm_priv *p,
> - struct nd_cmd_get_config_data_hdr *hdr)
> + struct nd_cmd_get_config_data_hdr *hdr)
> {
> unsigned long data[PLPAR_HCALL_BUFSIZE];
> + unsigned long offset, data_offset;
> + int len, read;
> int64_t ret;
>
> - if (hdr->in_offset >= p->metadata_size || hdr->in_length != 1)
> + if ((hdr->in_offset + hdr->in_length) >= p->metadata_size)
> return -EINVAL;
>
> - ret = plpar_hcall(H_SCM_READ_METADATA, data, p->drc_index,
> - hdr->in_offset, 1);
> -
> - if (ret == H_PARAMETER) /* bad DRC index */
> - return -ENODEV;
> - if (ret)
> - return -EINVAL; /* other invalid parameter */
> -
> - hdr->out_buf[0] = data[0] & 0xff;
> -
> + for (len = hdr->in_length; len; len -= read) {
> +
> + data_offset = hdr->in_length - len;
> + offset = hdr->in_offset + data_offset;
> +
> + if (len >= 8)
> + read = 8;
> + else if (len >= 4)
> + read = 4;
> + else if ( len >= 2)
> + read = 2;
> + else
> + read = 1;
> +
> + ret = plpar_hcall(H_SCM_READ_METADATA, data, p->drc_index,
> + offset, read);
> +
> + if (ret == H_PARAMETER) /* bad DRC index */
> + return -ENODEV;
> + if (ret)
> + return -EINVAL; /* other invalid parameter */
> +
> + switch (read) {
> + case 8:
> + *(uint64_t *)(hdr->out_buf + data_offset) = be64_to_cpu(data[0]);
> + break;
> + case 4:
> + *(uint32_t *)(hdr->out_buf + data_offset) = be32_to_cpu(data[0] & 0xffffffff);
> + break;
> +
> + case 2:
> + *(uint16_t *)(hdr->out_buf + data_offset) = be16_to_cpu(data[0] & 0xffff);
> + break;
> +
> + case 1:
> + *(uint32_t *)(hdr->out_buf + data_offset) = (data[0] & 0xff);
> + break;
> + }
> + }
> return 0;
> }
>
> static int papr_scm_meta_set(struct papr_scm_priv *p,
> - struct nd_cmd_set_config_hdr *hdr)
> + struct nd_cmd_set_config_hdr *hdr)
> {
> + unsigned long offset, data_offset;
> + int len, wrote;
> + unsigned long data;
> + __be64 data_be;
> int64_t ret;
>
> - if (hdr->in_offset >= p->metadata_size || hdr->in_length != 1)
> + if ((hdr->in_offset + hdr->in_length) >= p->metadata_size)
> return -EINVAL;
>
> - ret = plpar_hcall_norets(H_SCM_WRITE_METADATA,
> - p->drc_index, hdr->in_offset, hdr->in_buf[0], 1);
> -
> - if (ret == H_PARAMETER) /* bad DRC index */
> - return -ENODEV;
> - if (ret)
> - return -EINVAL; /* other invalid parameter */
> + for (len = hdr->in_length; len; len -= wrote) {
> +
> + data_offset = hdr->in_length - len;
> + offset = hdr->in_offset + data_offset;
> +
> + if (len >= 8) {
> + data = *(uint64_t *)(hdr->in_buf + data_offset);
> + data_be = cpu_to_be64(data);
> + wrote = 8;
> + } else if (len >= 4) {
> + data = *(uint32_t *)(hdr->in_buf + data_offset);
> + data &= 0xffffffff;
> + data_be = cpu_to_be32(data);
> + wrote = 4;
> + } else if (len >= 2) {
> + data = *(uint16_t *)(hdr->in_buf + data_offset);
> + data &= 0xffff;
> + data_be = cpu_to_be16(data);
> + wrote = 2;
> + } else {
> + data_be = *(uint8_t *)(hdr->in_buf + data_offset);
> + data_be &= 0xff;
> + wrote = 1;
> + }
> +
> + ret = plpar_hcall_norets(H_SCM_WRITE_METADATA, p->drc_index,
> + offset, data_be, wrote);
> + if (ret == H_PARAMETER) /* bad DRC index */
> + return -ENODEV;
> + if (ret)
> + return -EINVAL; /* other invalid parameter */
> + }
>
> return 0;
> }
> @@ -154,7 +214,7 @@ int papr_scm_ndctl(struct nvdimm_bus_descriptor *nd_desc, struct nvdimm *nvdimm,
> get_size_hdr = buf;
>
> get_size_hdr->status = 0;
> - get_size_hdr->max_xfer = 1;
> + get_size_hdr->max_xfer = 8;
> get_size_hdr->config_size = p->metadata_size;
> *cmd_rc = 0;
> break;
> --
> 2.21.0
I assume you got the qemu bits sorted out with Shiva? Looks good otherwise.
Reviewed-by: Oliver O'Halloran <oohall@gmail.com>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] powerpc/nvdimm: Add support for multibyte read/write for metadata
2019-06-03 0:25 ` Oliver
@ 2019-06-04 9:06 ` Aneesh Kumar K.V
2019-06-05 10:51 ` Michael Ellerman
0 siblings, 1 reply; 7+ messages in thread
From: Aneesh Kumar K.V @ 2019-06-04 9:06 UTC (permalink / raw)
To: Oliver; +Cc: linuxppc-dev, Paul Mackerras, Nicholas Piggin
Oliver <oohall@gmail.com> writes:
> On Sun, Jun 2, 2019 at 2:44 PM Aneesh Kumar K.V
> <aneesh.kumar@linux.ibm.com> wrote:
>>
>> SCM_READ/WRITE_MEATADATA hcall supports multibyte read/write. This patch
>> updates the metadata read/write to use 1, 2, 4 or 8 byte read/write as
>> mentioned in PAPR document.
>>
>> READ/WRITE_METADATA hcall supports the 1, 2, 4, or 8 bytes read/write.
>> For other values hcall results H_P3.
>
> You should probably fold the second paragraph here into the first.
>
>> Hypervisor stores the metadata contents in big-endian format and in-order
>> to enable read/write in different granularity, we need to switch the contents
>> to big-endian before calling HCALL.
>>
>> Based on an patch from Oliver O'Halloran <oohall@gmail.com>
>> Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
>> ---
>> arch/powerpc/platforms/pseries/papr_scm.c | 104 +++++++++++++++++-----
>> 1 file changed, 82 insertions(+), 22 deletions(-)
>>
>> diff --git a/arch/powerpc/platforms/pseries/papr_scm.c b/arch/powerpc/platforms/pseries/papr_scm.c
>> index 0176ce66673f..e33cebb8ee6c 100644
>> --- a/arch/powerpc/platforms/pseries/papr_scm.c
>> +++ b/arch/powerpc/platforms/pseries/papr_scm.c
>> @@ -97,42 +97,102 @@ static int drc_pmem_unbind(struct papr_scm_priv *p)
>> }
>>
>> static int papr_scm_meta_get(struct papr_scm_priv *p,
>> - struct nd_cmd_get_config_data_hdr *hdr)
>> + struct nd_cmd_get_config_data_hdr *hdr)
>> {
>> unsigned long data[PLPAR_HCALL_BUFSIZE];
>> + unsigned long offset, data_offset;
>> + int len, read;
>> int64_t ret;
>>
>> - if (hdr->in_offset >= p->metadata_size || hdr->in_length != 1)
>> + if ((hdr->in_offset + hdr->in_length) >= p->metadata_size)
>> return -EINVAL;
>>
>> - ret = plpar_hcall(H_SCM_READ_METADATA, data, p->drc_index,
>> - hdr->in_offset, 1);
>> -
>> - if (ret == H_PARAMETER) /* bad DRC index */
>> - return -ENODEV;
>> - if (ret)
>> - return -EINVAL; /* other invalid parameter */
>> -
>> - hdr->out_buf[0] = data[0] & 0xff;
>> -
>> + for (len = hdr->in_length; len; len -= read) {
>> +
>> + data_offset = hdr->in_length - len;
>> + offset = hdr->in_offset + data_offset;
>> +
>> + if (len >= 8)
>> + read = 8;
>> + else if (len >= 4)
>> + read = 4;
>> + else if ( len >= 2)
>> + read = 2;
>> + else
>> + read = 1;
>> +
>> + ret = plpar_hcall(H_SCM_READ_METADATA, data, p->drc_index,
>> + offset, read);
>> +
>> + if (ret == H_PARAMETER) /* bad DRC index */
>> + return -ENODEV;
>> + if (ret)
>> + return -EINVAL; /* other invalid parameter */
>> +
>> + switch (read) {
>> + case 8:
>> + *(uint64_t *)(hdr->out_buf + data_offset) = be64_to_cpu(data[0]);
>> + break;
>> + case 4:
>> + *(uint32_t *)(hdr->out_buf + data_offset) = be32_to_cpu(data[0] & 0xffffffff);
>> + break;
>> +
>> + case 2:
>> + *(uint16_t *)(hdr->out_buf + data_offset) = be16_to_cpu(data[0] & 0xffff);
>> + break;
>> +
>> + case 1:
>> + *(uint32_t *)(hdr->out_buf + data_offset) = (data[0] & 0xff);
>> + break;
>> + }
>> + }
>> return 0;
>> }
>>
>> static int papr_scm_meta_set(struct papr_scm_priv *p,
>> - struct nd_cmd_set_config_hdr *hdr)
>> + struct nd_cmd_set_config_hdr *hdr)
>> {
>> + unsigned long offset, data_offset;
>> + int len, wrote;
>> + unsigned long data;
>> + __be64 data_be;
>> int64_t ret;
>>
>> - if (hdr->in_offset >= p->metadata_size || hdr->in_length != 1)
>> + if ((hdr->in_offset + hdr->in_length) >= p->metadata_size)
>> return -EINVAL;
>>
>> - ret = plpar_hcall_norets(H_SCM_WRITE_METADATA,
>> - p->drc_index, hdr->in_offset, hdr->in_buf[0], 1);
>> -
>> - if (ret == H_PARAMETER) /* bad DRC index */
>> - return -ENODEV;
>> - if (ret)
>> - return -EINVAL; /* other invalid parameter */
>> + for (len = hdr->in_length; len; len -= wrote) {
>> +
>> + data_offset = hdr->in_length - len;
>> + offset = hdr->in_offset + data_offset;
>> +
>> + if (len >= 8) {
>> + data = *(uint64_t *)(hdr->in_buf + data_offset);
>> + data_be = cpu_to_be64(data);
>> + wrote = 8;
>> + } else if (len >= 4) {
>> + data = *(uint32_t *)(hdr->in_buf + data_offset);
>> + data &= 0xffffffff;
>> + data_be = cpu_to_be32(data);
>> + wrote = 4;
>> + } else if (len >= 2) {
>> + data = *(uint16_t *)(hdr->in_buf + data_offset);
>> + data &= 0xffff;
>> + data_be = cpu_to_be16(data);
>> + wrote = 2;
>> + } else {
>> + data_be = *(uint8_t *)(hdr->in_buf + data_offset);
>> + data_be &= 0xff;
>> + wrote = 1;
>> + }
>> +
>> + ret = plpar_hcall_norets(H_SCM_WRITE_METADATA, p->drc_index,
>> + offset, data_be, wrote);
>> + if (ret == H_PARAMETER) /* bad DRC index */
>> + return -ENODEV;
>> + if (ret)
>> + return -EINVAL; /* other invalid parameter */
>> + }
>>
>> return 0;
>> }
>> @@ -154,7 +214,7 @@ int papr_scm_ndctl(struct nvdimm_bus_descriptor *nd_desc, struct nvdimm *nvdimm,
>> get_size_hdr = buf;
>>
>> get_size_hdr->status = 0;
>> - get_size_hdr->max_xfer = 1;
>> + get_size_hdr->max_xfer = 8;
>> get_size_hdr->config_size = p->metadata_size;
>> *cmd_rc = 0;
>> break;
>> --
>> 2.21.0
>
> I assume you got the qemu bits sorted out with Shiva? Looks good otherwise.
That is correct. I also tested with different xfer values (1, 2, 4, 8)
on both Qemu and PowerVM.
>
> Reviewed-by: Oliver O'Halloran <oohall@gmail.com>
Thanks
-aneesh
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] powerpc/nvdimm: Add support for multibyte read/write for metadata
2019-06-02 4:43 [PATCH] powerpc/nvdimm: Add support for multibyte read/write for metadata Aneesh Kumar K.V
2019-06-03 0:25 ` Oliver
@ 2019-06-05 8:02 ` Alexey Kardashevskiy
2019-06-06 12:49 ` Aneesh Kumar K.V
1 sibling, 1 reply; 7+ messages in thread
From: Alexey Kardashevskiy @ 2019-06-05 8:02 UTC (permalink / raw)
To: Aneesh Kumar K.V, npiggin, paulus, mpe; +Cc: linuxppc-dev, oohall
On 02/06/2019 14:43, Aneesh Kumar K.V wrote:
> SCM_READ/WRITE_MEATADATA hcall supports multibyte read/write. This patch
> updates the metadata read/write to use 1, 2, 4 or 8 byte read/write as
> mentioned in PAPR document.
>
> READ/WRITE_METADATA hcall supports the 1, 2, 4, or 8 bytes read/write.
> For other values hcall results H_P3.
>
> Hypervisor stores the metadata contents in big-endian format and in-order
> to enable read/write in different granularity, we need to switch the contents
> to big-endian before calling HCALL.
>
> Based on an patch from Oliver O'Halloran <oohall@gmail.com>
> Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
> ---
> arch/powerpc/platforms/pseries/papr_scm.c | 104 +++++++++++++++++-----
> 1 file changed, 82 insertions(+), 22 deletions(-)
>
> diff --git a/arch/powerpc/platforms/pseries/papr_scm.c b/arch/powerpc/platforms/pseries/papr_scm.c
> index 0176ce66673f..e33cebb8ee6c 100644
> --- a/arch/powerpc/platforms/pseries/papr_scm.c
> +++ b/arch/powerpc/platforms/pseries/papr_scm.c
> @@ -97,42 +97,102 @@ static int drc_pmem_unbind(struct papr_scm_priv *p)
> }
>
> static int papr_scm_meta_get(struct papr_scm_priv *p,
> - struct nd_cmd_get_config_data_hdr *hdr)
> + struct nd_cmd_get_config_data_hdr *hdr)
> {
> unsigned long data[PLPAR_HCALL_BUFSIZE];
> + unsigned long offset, data_offset;
> + int len, read;
> int64_t ret;
>
> - if (hdr->in_offset >= p->metadata_size || hdr->in_length != 1)
> + if ((hdr->in_offset + hdr->in_length) >= p->metadata_size)
> return -EINVAL;
>
> - ret = plpar_hcall(H_SCM_READ_METADATA, data, p->drc_index,
> - hdr->in_offset, 1);
> -
> - if (ret == H_PARAMETER) /* bad DRC index */
> - return -ENODEV;
> - if (ret)
> - return -EINVAL; /* other invalid parameter */
> -
> - hdr->out_buf[0] = data[0] & 0xff;
> -
> + for (len = hdr->in_length; len; len -= read) {
> +
> + data_offset = hdr->in_length - len;
> + offset = hdr->in_offset + data_offset;
> +
> + if (len >= 8)
> + read = 8;
> + else if (len >= 4)
> + read = 4;
> + else if ( len >= 2)
Do not need a space before "len".
> + read = 2;
> + else
> + read = 1;
> +
> + ret = plpar_hcall(H_SCM_READ_METADATA, data, p->drc_index,
> + offset, read);
> +
> + if (ret == H_PARAMETER) /* bad DRC index */
> + return -ENODEV;
> + if (ret)
> + return -EINVAL; /* other invalid parameter */
> +
> + switch (read) {
> + case 8:
> + *(uint64_t *)(hdr->out_buf + data_offset) = be64_to_cpu(data[0]);
> + break;
> + case 4:
> + *(uint32_t *)(hdr->out_buf + data_offset) = be32_to_cpu(data[0] & 0xffffffff);
> + break;
> +
> + case 2:
> + *(uint16_t *)(hdr->out_buf + data_offset) = be16_to_cpu(data[0] & 0xffff);
> + break;
> +
> + case 1:
> + *(uint32_t *)(hdr->out_buf + data_offset) = (data[0] & 0xff);
Memory corruption, should be uint8_t*.
> + break;
> + }
> + }
> return 0;
> }
>
> static int papr_scm_meta_set(struct papr_scm_priv *p,
> - struct nd_cmd_set_config_hdr *hdr)
> + struct nd_cmd_set_config_hdr *hdr)
> {
> + unsigned long offset, data_offset;
> + int len, wrote;
> + unsigned long data;
> + __be64 data_be;
> int64_t ret;
>
> - if (hdr->in_offset >= p->metadata_size || hdr->in_length != 1)
> + if ((hdr->in_offset + hdr->in_length) >= p->metadata_size)
> return -EINVAL;
>
> - ret = plpar_hcall_norets(H_SCM_WRITE_METADATA,
> - p->drc_index, hdr->in_offset, hdr->in_buf[0], 1);
> -
> - if (ret == H_PARAMETER) /* bad DRC index */
> - return -ENODEV;
> - if (ret)
> - return -EINVAL; /* other invalid parameter */
> + for (len = hdr->in_length; len; len -= wrote) {
> +
> + data_offset = hdr->in_length - len;
> + offset = hdr->in_offset + data_offset;
> +
> + if (len >= 8) {
> + data = *(uint64_t *)(hdr->in_buf + data_offset);
> + data_be = cpu_to_be64(data);
> + wrote = 8;
> + } else if (len >= 4) {
> + data = *(uint32_t *)(hdr->in_buf + data_offset);
> + data &= 0xffffffff;
Why do you need &0xffffffff here and below (&0xffff, &0xff)? uint32_t is
unsigned type so the sign bit won't be extended.
> + data_be = cpu_to_be32(data);
> + wrote = 4;
> + } else if (len >= 2) {
> + data = *(uint16_t *)(hdr->in_buf + data_offset);
> + data &= 0xffff;
> + data_be = cpu_to_be16(data);
> + wrote = 2;
> + } else {
> + data_be = *(uint8_t *)(hdr->in_buf + data_offset);
> + data_be &= 0xff;
> + wrote = 1;
> + }
> +
> + ret = plpar_hcall_norets(H_SCM_WRITE_METADATA, p->drc_index,
> + offset, data_be, wrote);
> + if (ret == H_PARAMETER) /* bad DRC index */
> + return -ENODEV;
> + if (ret)
> + return -EINVAL; /* other invalid parameter */
> + }
>
> return 0;
> }
> @@ -154,7 +214,7 @@ int papr_scm_ndctl(struct nvdimm_bus_descriptor *nd_desc, struct nvdimm *nvdimm,
> get_size_hdr = buf;
>
> get_size_hdr->status = 0;
> - get_size_hdr->max_xfer = 1;
> + get_size_hdr->max_xfer = 8;
> get_size_hdr->config_size = p->metadata_size;
> *cmd_rc = 0;
> break;
>
--
Alexey
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] powerpc/nvdimm: Add support for multibyte read/write for metadata
2019-06-04 9:06 ` Aneesh Kumar K.V
@ 2019-06-05 10:51 ` Michael Ellerman
2019-06-06 12:50 ` Aneesh Kumar K.V
0 siblings, 1 reply; 7+ messages in thread
From: Michael Ellerman @ 2019-06-05 10:51 UTC (permalink / raw)
To: Aneesh Kumar K.V, Oliver; +Cc: Paul Mackerras, linuxppc-dev, Nicholas Piggin
"Aneesh Kumar K.V" <aneesh.kumar@linux.ibm.com> writes:
> Oliver <oohall@gmail.com> writes:
>> On Sun, Jun 2, 2019 at 2:44 PM Aneesh Kumar K.V
>> <aneesh.kumar@linux.ibm.com> wrote:
...
>>> diff --git a/arch/powerpc/platforms/pseries/papr_scm.c b/arch/powerpc/platforms/pseries/papr_scm.c
>>> index 0176ce66673f..e33cebb8ee6c 100644
>>> --- a/arch/powerpc/platforms/pseries/papr_scm.c
>>> +++ b/arch/powerpc/platforms/pseries/papr_scm.c
>>> @@ -97,42 +97,102 @@ static int drc_pmem_unbind(struct papr_scm_priv *p)
>>> }
>>>
>>> static int papr_scm_meta_get(struct papr_scm_priv *p,
>>> - struct nd_cmd_get_config_data_hdr *hdr)
>>> + struct nd_cmd_get_config_data_hdr *hdr)
>>> {
>>> unsigned long data[PLPAR_HCALL_BUFSIZE];
>>> + unsigned long offset, data_offset;
>>> + int len, read;
>>> int64_t ret;
>>>
>>> - if (hdr->in_offset >= p->metadata_size || hdr->in_length != 1)
>>> + if ((hdr->in_offset + hdr->in_length) >= p->metadata_size)
>>> return -EINVAL;
>>>
>>> - ret = plpar_hcall(H_SCM_READ_METADATA, data, p->drc_index,
>>> - hdr->in_offset, 1);
>>> -
>>> - if (ret == H_PARAMETER) /* bad DRC index */
>>> - return -ENODEV;
>>> - if (ret)
>>> - return -EINVAL; /* other invalid parameter */
>>> -
>>> - hdr->out_buf[0] = data[0] & 0xff;
>>> -
>>> + for (len = hdr->in_length; len; len -= read) {
>>> +
>>> + data_offset = hdr->in_length - len;
>>> + offset = hdr->in_offset + data_offset;
>>> +
>>> + if (len >= 8)
>>> + read = 8;
>>> + else if (len >= 4)
>>> + read = 4;
>>> + else if ( len >= 2)
>>> + read = 2;
>>> + else
>>> + read = 1;
>>> +
>>> + ret = plpar_hcall(H_SCM_READ_METADATA, data, p->drc_index,
>>> + offset, read);
>>> +
>>> + if (ret == H_PARAMETER) /* bad DRC index */
>>> + return -ENODEV;
>>> + if (ret)
>>> + return -EINVAL; /* other invalid parameter */
>>> +
>>> + switch (read) {
>>> + case 8:
>>> + *(uint64_t *)(hdr->out_buf + data_offset) = be64_to_cpu(data[0]);
>>> + break;
>>> + case 4:
>>> + *(uint32_t *)(hdr->out_buf + data_offset) = be32_to_cpu(data[0] & 0xffffffff);
>>> + break;
...
>>
>> I assume you got the qemu bits sorted out with Shiva? Looks good otherwise.
>
> That is correct. I also tested with different xfer values (1, 2, 4, 8)
> on both Qemu and PowerVM.
With a big endian kernel?
cheers
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] powerpc/nvdimm: Add support for multibyte read/write for metadata
2019-06-05 8:02 ` Alexey Kardashevskiy
@ 2019-06-06 12:49 ` Aneesh Kumar K.V
0 siblings, 0 replies; 7+ messages in thread
From: Aneesh Kumar K.V @ 2019-06-06 12:49 UTC (permalink / raw)
To: Alexey Kardashevskiy, npiggin, paulus, mpe; +Cc: oohall, linuxppc-dev
Alexey Kardashevskiy <aik@ozlabs.ru> writes:
> On 02/06/2019 14:43, Aneesh Kumar K.V wrote:
>> SCM_READ/WRITE_MEATADATA hcall supports multibyte read/write. This patch
>> updates the metadata read/write to use 1, 2, 4 or 8 byte read/write as
>> mentioned in PAPR document.
>>
>> READ/WRITE_METADATA hcall supports the 1, 2, 4, or 8 bytes read/write.
>> For other values hcall results H_P3.
>>
>> Hypervisor stores the metadata contents in big-endian format and in-order
>> to enable read/write in different granularity, we need to switch the contents
>> to big-endian before calling HCALL.
>>
>> Based on an patch from Oliver O'Halloran <oohall@gmail.com>
>> Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
>> ---
>> arch/powerpc/platforms/pseries/papr_scm.c | 104 +++++++++++++++++-----
>> 1 file changed, 82 insertions(+), 22 deletions(-)
>>
>> diff --git a/arch/powerpc/platforms/pseries/papr_scm.c b/arch/powerpc/platforms/pseries/papr_scm.c
>> index 0176ce66673f..e33cebb8ee6c 100644
>> --- a/arch/powerpc/platforms/pseries/papr_scm.c
>> +++ b/arch/powerpc/platforms/pseries/papr_scm.c
>> @@ -97,42 +97,102 @@ static int drc_pmem_unbind(struct papr_scm_priv *p)
>> }
>>
>> static int papr_scm_meta_get(struct papr_scm_priv *p,
>> - struct nd_cmd_get_config_data_hdr *hdr)
>> + struct nd_cmd_get_config_data_hdr *hdr)
>> {
>> unsigned long data[PLPAR_HCALL_BUFSIZE];
>> + unsigned long offset, data_offset;
>> + int len, read;
>> int64_t ret;
>>
>> - if (hdr->in_offset >= p->metadata_size || hdr->in_length != 1)
>> + if ((hdr->in_offset + hdr->in_length) >= p->metadata_size)
>> return -EINVAL;
>>
>> - ret = plpar_hcall(H_SCM_READ_METADATA, data, p->drc_index,
>> - hdr->in_offset, 1);
>> -
>> - if (ret == H_PARAMETER) /* bad DRC index */
>> - return -ENODEV;
>> - if (ret)
>> - return -EINVAL; /* other invalid parameter */
>> -
>> - hdr->out_buf[0] = data[0] & 0xff;
>> -
>> + for (len = hdr->in_length; len; len -= read) {
>> +
>> + data_offset = hdr->in_length - len;
>> + offset = hdr->in_offset + data_offset;
>> +
>> + if (len >= 8)
>> + read = 8;
>> + else if (len >= 4)
>> + read = 4;
>> + else if ( len >= 2)
>
> Do not need a space before "len".
Will fix in next update
>
>
>> + read = 2;
>> + else
>> + read = 1;
>> +
>> + ret = plpar_hcall(H_SCM_READ_METADATA, data, p->drc_index,
>> + offset, read);
>> +
>> + if (ret == H_PARAMETER) /* bad DRC index */
>> + return -ENODEV;
>> + if (ret)
>> + return -EINVAL; /* other invalid parameter */
>> +
>> + switch (read) {
>> + case 8:
>> + *(uint64_t *)(hdr->out_buf + data_offset) = be64_to_cpu(data[0]);
>> + break;
>> + case 4:
>> + *(uint32_t *)(hdr->out_buf + data_offset) = be32_to_cpu(data[0] & 0xffffffff);
>> + break;
>> +
>> + case 2:
>> + *(uint16_t *)(hdr->out_buf + data_offset) = be16_to_cpu(data[0] & 0xffff);
>> + break;
>> +
>> + case 1:
>> + *(uint32_t *)(hdr->out_buf + data_offset) = (data[0] & 0xff);
>
>
> Memory corruption, should be uint8_t*.
Good catch. That also resulted in an error on big endian kernel. Will
fix that in next update
>
>
>> + break;
>> + }
>> + }
>> return 0;
>> }
>>
>> static int papr_scm_meta_set(struct papr_scm_priv *p,
>> - struct nd_cmd_set_config_hdr *hdr)
>> + struct nd_cmd_set_config_hdr *hdr)
>> {
>> + unsigned long offset, data_offset;
>> + int len, wrote;
>> + unsigned long data;
>> + __be64 data_be;
>> int64_t ret;
>>
>> - if (hdr->in_offset >= p->metadata_size || hdr->in_length != 1)
>> + if ((hdr->in_offset + hdr->in_length) >= p->metadata_size)
>> return -EINVAL;
>>
>> - ret = plpar_hcall_norets(H_SCM_WRITE_METADATA,
>> - p->drc_index, hdr->in_offset, hdr->in_buf[0], 1);
>> -
>> - if (ret == H_PARAMETER) /* bad DRC index */
>> - return -ENODEV;
>> - if (ret)
>> - return -EINVAL; /* other invalid parameter */
>> + for (len = hdr->in_length; len; len -= wrote) {
>> +
>> + data_offset = hdr->in_length - len;
>> + offset = hdr->in_offset + data_offset;
>> +
>> + if (len >= 8) {
>> + data = *(uint64_t *)(hdr->in_buf + data_offset);
>> + data_be = cpu_to_be64(data);
>> + wrote = 8;
>> + } else if (len >= 4) {
>> + data = *(uint32_t *)(hdr->in_buf + data_offset);
>> + data &= 0xffffffff;
>
>
> Why do you need &0xffffffff here and below (&0xffff, &0xff)? uint32_t is
> unsigned type so the sign bit won't be extended.
Sure. I just want to make sure we don't take extra data. For now I will
keep it as it is and let Michael Ellerman decide to drop that?
>
>
>> + data_be = cpu_to_be32(data);
>> + wrote = 4;
>> + } else if (len >= 2) {
>> + data = *(uint16_t *)(hdr->in_buf + data_offset);
>> + data &= 0xffff;
>> + data_be = cpu_to_be16(data);
>> + wrote = 2;
>> + } else {
>> + data_be = *(uint8_t *)(hdr->in_buf + data_offset);
>> + data_be &= 0xff;
>> + wrote = 1;
>> + }
>> +
>> + ret = plpar_hcall_norets(H_SCM_WRITE_METADATA, p->drc_index,
>> + offset, data_be, wrote);
>> + if (ret == H_PARAMETER) /* bad DRC index */
>> + return -ENODEV;
>> + if (ret)
>> + return -EINVAL; /* other invalid parameter */
>> + }
>>
>> return 0;
>> }
>> @@ -154,7 +214,7 @@ int papr_scm_ndctl(struct nvdimm_bus_descriptor *nd_desc, struct nvdimm *nvdimm,
>> get_size_hdr = buf;
>>
>> get_size_hdr->status = 0;
>> - get_size_hdr->max_xfer = 1;
>> + get_size_hdr->max_xfer = 8;
>> get_size_hdr->config_size = p->metadata_size;
>> *cmd_rc = 0;
>> break;
>>
>
> --
> Alexey
Thanks for the review
-aneesh
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] powerpc/nvdimm: Add support for multibyte read/write for metadata
2019-06-05 10:51 ` Michael Ellerman
@ 2019-06-06 12:50 ` Aneesh Kumar K.V
0 siblings, 0 replies; 7+ messages in thread
From: Aneesh Kumar K.V @ 2019-06-06 12:50 UTC (permalink / raw)
To: Michael Ellerman, Oliver; +Cc: linuxppc-dev, Paul Mackerras, Nicholas Piggin
Michael Ellerman <mpe@ellerman.id.au> writes:
> "Aneesh Kumar K.V" <aneesh.kumar@linux.ibm.com> writes:
>> Oliver <oohall@gmail.com> writes:
>>> On Sun, Jun 2, 2019 at 2:44 PM Aneesh Kumar K.V
>>> <aneesh.kumar@linux.ibm.com> wrote:
> ...
>>>> diff --git a/arch/powerpc/platforms/pseries/papr_scm.c b/arch/powerpc/platforms/pseries/papr_scm.c
>>>> index 0176ce66673f..e33cebb8ee6c 100644
>>>> --- a/arch/powerpc/platforms/pseries/papr_scm.c
>>>> +++ b/arch/powerpc/platforms/pseries/papr_scm.c
>>>> @@ -97,42 +97,102 @@ static int drc_pmem_unbind(struct papr_scm_priv *p)
>>>> }
>>>>
>>>> static int papr_scm_meta_get(struct papr_scm_priv *p,
>>>> - struct nd_cmd_get_config_data_hdr *hdr)
>>>> + struct nd_cmd_get_config_data_hdr *hdr)
>>>> {
>>>> unsigned long data[PLPAR_HCALL_BUFSIZE];
>>>> + unsigned long offset, data_offset;
>>>> + int len, read;
>>>> int64_t ret;
>>>>
>>>> - if (hdr->in_offset >= p->metadata_size || hdr->in_length != 1)
>>>> + if ((hdr->in_offset + hdr->in_length) >= p->metadata_size)
>>>> return -EINVAL;
>>>>
>>>> - ret = plpar_hcall(H_SCM_READ_METADATA, data, p->drc_index,
>>>> - hdr->in_offset, 1);
>>>> -
>>>> - if (ret == H_PARAMETER) /* bad DRC index */
>>>> - return -ENODEV;
>>>> - if (ret)
>>>> - return -EINVAL; /* other invalid parameter */
>>>> -
>>>> - hdr->out_buf[0] = data[0] & 0xff;
>>>> -
>>>> + for (len = hdr->in_length; len; len -= read) {
>>>> +
>>>> + data_offset = hdr->in_length - len;
>>>> + offset = hdr->in_offset + data_offset;
>>>> +
>>>> + if (len >= 8)
>>>> + read = 8;
>>>> + else if (len >= 4)
>>>> + read = 4;
>>>> + else if ( len >= 2)
>>>> + read = 2;
>>>> + else
>>>> + read = 1;
>>>> +
>>>> + ret = plpar_hcall(H_SCM_READ_METADATA, data, p->drc_index,
>>>> + offset, read);
>>>> +
>>>> + if (ret == H_PARAMETER) /* bad DRC index */
>>>> + return -ENODEV;
>>>> + if (ret)
>>>> + return -EINVAL; /* other invalid parameter */
>>>> +
>>>> + switch (read) {
>>>> + case 8:
>>>> + *(uint64_t *)(hdr->out_buf + data_offset) = be64_to_cpu(data[0]);
>>>> + break;
>>>> + case 4:
>>>> + *(uint32_t *)(hdr->out_buf + data_offset) = be32_to_cpu(data[0] & 0xffffffff);
>>>> + break;
> ...
>>>
>>> I assume you got the qemu bits sorted out with Shiva? Looks good otherwise.
>>
>> That is correct. I also tested with different xfer values (1, 2, 4, 8)
>> on both Qemu and PowerVM.
>
> With a big endian kernel?
I completed this testing and found new bugs in other parts of the code.
Thanks for the sugestion.
-aneesh
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2019-06-06 12:54 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-06-02 4:43 [PATCH] powerpc/nvdimm: Add support for multibyte read/write for metadata Aneesh Kumar K.V
2019-06-03 0:25 ` Oliver
2019-06-04 9:06 ` Aneesh Kumar K.V
2019-06-05 10:51 ` Michael Ellerman
2019-06-06 12:50 ` Aneesh Kumar K.V
2019-06-05 8:02 ` Alexey Kardashevskiy
2019-06-06 12:49 ` Aneesh Kumar K.V
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.