As per the "Load and Store APIs" documentation (docs/devel/loads-stores.rst),
"No signed load operations are provided."
Update lduw_he_p() to return as unsigned.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 include/qemu/bswap.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/include/qemu/bswap.h b/include/qemu/bswap.h
index 3f28f661b1..613978f838 100644
--- a/include/qemu/bswap.h
+++ b/include/qemu/bswap.h
@@ -330,9 +330,9 @@ static inline void stw_he_p(void *ptr, uint16_t v)
     memcpy(ptr, &v, sizeof(v));
 }
 
-static inline int ldl_he_p(const void *ptr)
+static inline uint32_t ldl_he_p(const void *ptr)
 {
-    int32_t r;
+    uint32_t r;
     memcpy(&r, ptr, sizeof(r));
     return r;
 }
-- 
2.17.0

On 23 April 2018 at 17:25, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> As per the "Load and Store APIs" documentation (docs/devel/loads-stores.rst),
> "No signed load operations are provided."

That phrase is used in the documentation sections for other
kinds of load/store function, but not in the section for ld*_p
and st*_p, which do provide both signed and unsigned flavours.

> Update lduw_he_p() to return as unsigned.

Code is changing a different function to the one named here...

> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>  include/qemu/bswap.h | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/include/qemu/bswap.h b/include/qemu/bswap.h
> index 3f28f661b1..613978f838 100644
> --- a/include/qemu/bswap.h
> +++ b/include/qemu/bswap.h
> @@ -330,9 +330,9 @@ static inline void stw_he_p(void *ptr, uint16_t v)
>      memcpy(ptr, &v, sizeof(v));
>  }
>
> -static inline int ldl_he_p(const void *ptr)
> +static inline uint32_t ldl_he_p(const void *ptr)

This would make it not match the other ldl*_p functions
(ldl_le_p, ldl_be_p).

The expectation with the ldl functions is that you're
putting it into a variable of the right type and size,
and so we don't need to provide both a signed 32 bit load
and an unsigned 32 bit load.

thanks
-- PMM

On 04/23/2018 01:56 PM, Peter Maydell wrote:
> On 23 April 2018 at 17:25, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>> As per the "Load and Store APIs" documentation (docs/devel/loads-stores.rst),
>> "No signed load operations are provided."
> 
> That phrase is used in the documentation sections for other
> kinds of load/store function, but not in the section for ld*_p
> and st*_p, which do provide both signed and unsigned flavours.
> 
>> Update lduw_he_p() to return as unsigned.
> 
> Code is changing a different function to the one named here...
> 
>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>> ---
>>  include/qemu/bswap.h | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/include/qemu/bswap.h b/include/qemu/bswap.h
>> index 3f28f661b1..613978f838 100644
>> --- a/include/qemu/bswap.h
>> +++ b/include/qemu/bswap.h
>> @@ -330,9 +330,9 @@ static inline void stw_he_p(void *ptr, uint16_t v)
>>      memcpy(ptr, &v, sizeof(v));
>>  }
>>
>> -static inline int ldl_he_p(const void *ptr)
>> +static inline uint32_t ldl_he_p(const void *ptr)
> 
> This would make it not match the other ldl*_p functions
> (ldl_le_p, ldl_be_p).
> 
> The expectation with the ldl functions is that you're
> putting it into a variable of the right type and size,
> and so we don't need to provide both a signed 32 bit load
> and an unsigned 32 bit load.

OK, thank you for the clarification.

Regards,

Phil.

On 04/23/2018 06:25 AM, Philippe Mathieu-Daudé wrote:
> As per the "Load and Store APIs" documentation (docs/devel/loads-stores.rst),
> "No signed load operations are provided."
> Update lduw_he_p() to return as unsigned.
> 
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>  include/qemu/bswap.h | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/include/qemu/bswap.h b/include/qemu/bswap.h
> index 3f28f661b1..613978f838 100644
> --- a/include/qemu/bswap.h
> +++ b/include/qemu/bswap.h
> @@ -330,9 +330,9 @@ static inline void stw_he_p(void *ptr, uint16_t v)
>      memcpy(ptr, &v, sizeof(v));
>  }
>  
> -static inline int ldl_he_p(const void *ptr)
> +static inline uint32_t ldl_he_p(const void *ptr)
>  {
> -    int32_t r;
> +    uint32_t r;

Nack, not without auditing all users.

The documentation is clear about ldl being an oddball,
primarily because we didn't audit all users last time.

In the short term, just cast your user, which is what
we wind up doing elsewhere in the codebase.

r~

On 23/04/2018 18:25, Philippe Mathieu-Daudé wrote:
> As per the "Load and Store APIs" documentation (docs/devel/loads-stores.rst),
> "No signed load operations are provided."
> Update lduw_he_p() to return as unsigned.
> 
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

This is ldl_he_p, not lduw_he_p.

However, this patch should not be necessary; using uint16_t is needed
for shorts because they implicitly extend to int, so a "short -0x1234"
becomes 0xffff1234 when assigned to uint32_t.  Instead, an int can be
assigned to a uint32_t (which is the type of fdt32_to_cpu's argument)
with no change in value.

Paolo

> ---
>  include/qemu/bswap.h | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/include/qemu/bswap.h b/include/qemu/bswap.h
> index 3f28f661b1..613978f838 100644
> --- a/include/qemu/bswap.h
> +++ b/include/qemu/bswap.h
> @@ -330,9 +330,9 @@ static inline void stw_he_p(void *ptr, uint16_t v)
>      memcpy(ptr, &v, sizeof(v));
>  }
>  
> -static inline int ldl_he_p(const void *ptr)
> +static inline uint32_t ldl_he_p(const void *ptr)
>  {
> -    int32_t r;
> +    uint32_t r;
>      memcpy(&r, ptr, sizeof(r));
>      return r;
>  }
>