[PATCH] powerpc/nvdimm: Add support for multibyte read/write for metadata

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 [off-list ref]
Signed-off-by: Aneesh Kumar K.V <redacted>
---
 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

On Sun, Jun 2, 2019 at 2:44 PM Aneesh Kumar K.V
[off-list ref] 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.

I assume you got the qemu bits sorted out with Shiva? Looks good otherwise.

Reviewed-by: Oliver O'Halloran <oohall@gmail.com>

Oliver [off-list ref] writes:

On Sun, Jun 2, 2019 at 2:44 PM Aneesh Kumar K.V
[off-list ref] wrote:

quoted

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.

quoted

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 [off-list ref]
Signed-off-by: Aneesh Kumar K.V <redacted>
---
 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

On 02/06/2019 14:43, Aneesh Kumar K.V wrote:

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.

-- 
Alexey

"Aneesh Kumar K.V" [off-list ref] writes:

Oliver [off-list ref] writes:

quoted

On Sun, Jun 2, 2019 at 2:44 PM Aneesh Kumar K.V
[off-list ref] wrote:

...

quoted

quoted

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;

...

quoted

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

Alexey Kardashevskiy [off-list ref] writes:

On 02/06/2019 14:43, Aneesh Kumar K.V wrote:

quoted

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 [off-list ref]
Signed-off-by: Aneesh Kumar K.V <redacted>
---
 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

quoted

+			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

quoted

+			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?

quoted

+			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

Michael Ellerman [off-list ref] writes:

"Aneesh Kumar K.V" [off-list ref] writes:

quoted

Oliver [off-list ref] writes:

quoted

On Sun, Jun 2, 2019 at 2:44 PM Aneesh Kumar K.V
[off-list ref] wrote:

...

quoted

quoted

quoted

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;

...

quoted

quoted

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