| Message ID | 20231011053711.93427-3-bgray@linux.ibm.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Miscellaneous Sparse fixes | expand |
Benjamin Gray <bgray@linux.ibm.com> writes: > Sparse reports an endian mismatch in hvc_get_chars(). > > At first it seemed like the retbuf should be __be64[], but actually > retbuf holds serialized registers returned by the hypervisor call, so > it's correctly CPU endian typed. > > Instead, it is the be64_to_cpu() that's misleading. The intent is to do > a byte swap on a little endian CPU. The swap is required because we > wanted to store the register values to memory without 'swapping' bytes, > so that the high order byte of the first register is the first byte > in the result buffer. > > In diagram form, on a LE CPU with the letters representing the return > string bytes: > > (register bytes) A B C D E F G H I J K L M N O P > (retbuf mem bytes) H G F E D C B A P O N M L K J I > (buf/lbuf mem bytes) A B C D E F G H I J K L M N O P > > So retbuf stores the registers in CPU endian, and buf always stores in > big endian. > > So replace the byte swap function with cpu_to_be64() and cast lbuf as an > array of __be64 to match the semantics closer. > > Signed-off-by: Benjamin Gray <bgray@linux.ibm.com> > --- > arch/powerpc/platforms/pseries/hvconsole.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/arch/powerpc/platforms/pseries/hvconsole.c b/arch/powerpc/platforms/pseries/hvconsole.c > index 1ac52963e08b..647718a15e78 100644 > --- a/arch/powerpc/platforms/pseries/hvconsole.c > +++ b/arch/powerpc/platforms/pseries/hvconsole.c > @@ -29,11 +29,11 @@ int hvc_get_chars(uint32_t vtermno, char *buf, int count) > { > long ret; > unsigned long retbuf[PLPAR_HCALL_BUFSIZE]; > - unsigned long *lbuf = (unsigned long *)buf; > + __be64 *lbuf = (__be64 __force *)buf; > > ret = plpar_hcall(H_GET_TERM_CHAR, retbuf, vtermno); > - lbuf[0] = be64_to_cpu(retbuf[1]); > - lbuf[1] = be64_to_cpu(retbuf[2]); > + lbuf[0] = cpu_to_be64(retbuf[1]); > + lbuf[1] = cpu_to_be64(retbuf[2]); > > if (ret == H_SUCCESS) > return retbuf[0]; > There is no functionality change in this patch. It is clarifying the details that it expect the buf to have the big-endian format and retbuf contains native endian format. Not sure why this was not picked. Reviewed-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
Michael ? Any reason for not picking that one ? Le 30/10/2023 à 14:16, Aneesh Kumar K.V a écrit : > Benjamin Gray <bgray@linux.ibm.com> writes: > >> Sparse reports an endian mismatch in hvc_get_chars(). >> >> At first it seemed like the retbuf should be __be64[], but actually >> retbuf holds serialized registers returned by the hypervisor call, so >> it's correctly CPU endian typed. >> >> Instead, it is the be64_to_cpu() that's misleading. The intent is to do >> a byte swap on a little endian CPU. The swap is required because we >> wanted to store the register values to memory without 'swapping' bytes, >> so that the high order byte of the first register is the first byte >> in the result buffer. >> >> In diagram form, on a LE CPU with the letters representing the return >> string bytes: >> >> (register bytes) A B C D E F G H I J K L M N O P >> (retbuf mem bytes) H G F E D C B A P O N M L K J I >> (buf/lbuf mem bytes) A B C D E F G H I J K L M N O P >> >> So retbuf stores the registers in CPU endian, and buf always stores in >> big endian. >> >> So replace the byte swap function with cpu_to_be64() and cast lbuf as an >> array of __be64 to match the semantics closer. >> >> Signed-off-by: Benjamin Gray <bgray@linux.ibm.com> >> --- >> arch/powerpc/platforms/pseries/hvconsole.c | 6 +++--- >> 1 file changed, 3 insertions(+), 3 deletions(-) >> >> diff --git a/arch/powerpc/platforms/pseries/hvconsole.c b/arch/powerpc/platforms/pseries/hvconsole.c >> index 1ac52963e08b..647718a15e78 100644 >> --- a/arch/powerpc/platforms/pseries/hvconsole.c >> +++ b/arch/powerpc/platforms/pseries/hvconsole.c >> @@ -29,11 +29,11 @@ int hvc_get_chars(uint32_t vtermno, char *buf, int count) >> { >> long ret; >> unsigned long retbuf[PLPAR_HCALL_BUFSIZE]; >> - unsigned long *lbuf = (unsigned long *)buf; >> + __be64 *lbuf = (__be64 __force *)buf; >> >> ret = plpar_hcall(H_GET_TERM_CHAR, retbuf, vtermno); >> - lbuf[0] = be64_to_cpu(retbuf[1]); >> - lbuf[1] = be64_to_cpu(retbuf[2]); >> + lbuf[0] = cpu_to_be64(retbuf[1]); >> + lbuf[1] = cpu_to_be64(retbuf[2]); >> >> if (ret == H_SUCCESS) >> return retbuf[0]; >> > > There is no functionality change in this patch. It is clarifying the > details that it expect the buf to have the big-endian format and retbuf > contains native endian format. > > Not sure why this was not picked. > > Reviewed-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
diff --git a/arch/powerpc/platforms/pseries/hvconsole.c b/arch/powerpc/platforms/pseries/hvconsole.c index 1ac52963e08b..647718a15e78 100644 --- a/arch/powerpc/platforms/pseries/hvconsole.c +++ b/arch/powerpc/platforms/pseries/hvconsole.c @@ -29,11 +29,11 @@ int hvc_get_chars(uint32_t vtermno, char *buf, int count) { long ret; unsigned long retbuf[PLPAR_HCALL_BUFSIZE]; - unsigned long *lbuf = (unsigned long *)buf; + __be64 *lbuf = (__be64 __force *)buf; ret = plpar_hcall(H_GET_TERM_CHAR, retbuf, vtermno); - lbuf[0] = be64_to_cpu(retbuf[1]); - lbuf[1] = be64_to_cpu(retbuf[2]); + lbuf[0] = cpu_to_be64(retbuf[1]); + lbuf[1] = cpu_to_be64(retbuf[2]); if (ret == H_SUCCESS) return retbuf[0];
Sparse reports an endian mismatch in hvc_get_chars(). At first it seemed like the retbuf should be __be64[], but actually retbuf holds serialized registers returned by the hypervisor call, so it's correctly CPU endian typed. Instead, it is the be64_to_cpu() that's misleading. The intent is to do a byte swap on a little endian CPU. The swap is required because we wanted to store the register values to memory without 'swapping' bytes, so that the high order byte of the first register is the first byte in the result buffer. In diagram form, on a LE CPU with the letters representing the return string bytes: (register bytes) A B C D E F G H I J K L M N O P (retbuf mem bytes) H G F E D C B A P O N M L K J I (buf/lbuf mem bytes) A B C D E F G H I J K L M N O P So retbuf stores the registers in CPU endian, and buf always stores in big endian. So replace the byte swap function with cpu_to_be64() and cast lbuf as an array of __be64 to match the semantics closer. Signed-off-by: Benjamin Gray <bgray@linux.ibm.com> --- arch/powerpc/platforms/pseries/hvconsole.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)