[PATCH 0/2] Arm32: memset() & friends

[PATCH 0/2] Arm32: memset() & friends

[Jan Beulich]

Note that while the original Linux code has the same issue, I don't
really fancy sending there a patch similar to patch 1. That's because
my XSA-307 related "make find_next_{,zero_}bit() have well defined
behavior" was entirely ignored, so I would expect nothing better here.

1: correct string.h functions for "int" -> "unsigned char" conversion
2: tidy the memset() macro

Really I happened to be looking at the memset() macro, being a little
surprised of the special casing of 0 there. As a result I was curious to
see how much more efficient __memzero() really was compared to memset().
The first thing which actually caught my attention is that part of
__memzero()'s code lives outside of the function (i.e. ahead of
ENTRY()). This saves a branch (and interestingly memset() does _not_ do
the same), but (a) renders the ELF symbol table somewhat wrong and (b)
leaves debug info generated with gas 2.39 not covering the entire
function in the new subprogram DIE that it generates (line number info
and .debug_ranges are correct nevertheless).

Jan

[PATCH 1/2] Arm32: correct string.h functions for "int" -> "unsigned char" conversion

[Jan Beulich]

While Arm64 does so uniformly, for Arm32 only strchr() currently handles
this properly. Add the necessary conversion also to strrchr(), memchr(),
and memset().

As to the placement in memset(): Putting the new insn at the beginning
of the function could perhaps be deemed more "obvious", but the code
reachable without ever making it to the "1" label only ever does byte
stores.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/arch/arm/arm32/lib/memchr.S
+++ b/xen/arch/arm/arm32/lib/memchr.S
@@ -14,6 +14,7 @@
 	.text
 	.align	5
 ENTRY(memchr)
+	and	r1, r1, #0xff
 1:	subs	r2, r2, #1
 	bmi	2f
 	ldrb	r3, [r0], #1
--- a/xen/arch/arm/arm32/lib/memset.S
+++ b/xen/arch/arm/arm32/lib/memset.S
@@ -21,7 +21,8 @@ ENTRY(memset)
 /*
  * we know that the pointer in ip is aligned to a word boundary.
  */
-1:	orr	r1, r1, r1, lsl #8
+1:	and	r1, r1, #0xff
+	orr	r1, r1, r1, lsl #8
 	orr	r1, r1, r1, lsl #16
 	mov	r3, r1
 	cmp	r2, #16
--- a/xen/arch/arm/arm32/lib/strrchr.S
+++ b/xen/arch/arm/arm32/lib/strrchr.S
@@ -14,6 +14,7 @@
 		.text
 		.align	5
 ENTRY(strrchr)
+		and	r1, r1, #0xff
 		mov	r3, #0
 1:		ldrb	r2, [r0], #1
 		teq	r2, r1

[PATCH 2/2] Arm32: tidy the memset() macro

[Jan Beulich]

- add parentheses where they were missing (MISRA)
- make sure to evaluate also v exactly once (MISRA)
- remove excess parentheses
- rename local variables to not have leading underscores
- apply Xen coding style

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
I wonder whether "if ( n_ )" is really helpful: It's extra code in all
callers passing a non-constant size, just to cover a pretty rare case
which memset() is required to deal with correctly anyway, and which
__memzero() also handles quite fine from all I can tell.

--- a/xen/arch/arm/include/asm/string.h
+++ b/xen/arch/arm/include/asm/string.h
@@ -28,17 +28,19 @@
 
 void __memzero(void *ptr, size_t n);
 
-#define memset(p, v, n)                                                 \
-        ({                                                              \
-                void *__p = (p); size_t __n = n;                        \
-                if ((__n) != 0) {                                       \
-                        if (__builtin_constant_p((v)) && (v) == 0)      \
-                                __memzero((__p),(__n));                 \
-                        else                                            \
-                                memset((__p),(v),(__n));                \
-                }                                                       \
-                (__p);                                                  \
-        })
+#define memset(p, v, n)                                 \
+    ({                                                  \
+        void *p_ = (p); size_t n_ = (n);                \
+        typeof(v) v_ = (v);                             \
+        if ( n_ )                                       \
+        {                                               \
+            if ( __builtin_constant_p(v) && !v_ )       \
+                __memzero(p_, n_);                      \
+            else                                        \
+                memset(p_, v_, n_);                     \
+        }                                               \
+        p_;                                             \
+    })
 
 #endif
 

Re: [PATCH 2/2] Arm32: tidy the memset() macro

[Julien Grall]

Hi Jan,

On 19/08/2022 08:50, Jan Beulich wrote:
> - add parentheses where they were missing (MISRA)
> - make sure to evaluate also v exactly once (MISRA)
> - remove excess parentheses
> - rename local variables to not have leading underscores
> - apply Xen coding style

This code has been taken from Linux. From you write above, I don't see 
any strong reason for us to modify it (even if it is small).

Cheers,

-- 
Julien Grall

RE: [PATCH 2/2] Arm32: tidy the memset() macro

[Wei Chen]

Hi Jan,

> -----Original Message-----
> From: Xen-devel <xen-devel-bounces@lists.xenproject.org> On Behalf Of Jan
> Beulich
> Sent: 2022年8月19日 15:50
> To: xen-devel@lists.xenproject.org
> Cc: Julien Grall <julien@xen.org>; Stefano Stabellini
> <sstabellini@kernel.org>; Volodymyr Babchuk <volodymyr_babchuk@epam.com>;
> Bertrand Marquis <Bertrand.Marquis@arm.com>
> Subject: [PATCH 2/2] Arm32: tidy the memset() macro
> 
> - add parentheses where they were missing (MISRA)
> - make sure to evaluate also v exactly once (MISRA)
> - remove excess parentheses
> - rename local variables to not have leading underscores
> - apply Xen coding style
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> ---
> I wonder whether "if ( n_ )" is really helpful: It's extra code in all
> callers passing a non-constant size, just to cover a pretty rare case
> which memset() is required to deal with correctly anyway, and which

What rare case we need to use n_ that can make memset happy? As your
comment, I read the code again, but it seems to work fine without n_.

Cheers,
Wei Chen

> __memzero() also handles quite fine from all I can tell.
> 
> --- a/xen/arch/arm/include/asm/string.h
> +++ b/xen/arch/arm/include/asm/string.h
> @@ -28,17 +28,19 @@
> 
>  void __memzero(void *ptr, size_t n);
> 
> -#define memset(p, v, n)                                                 \
> -        ({                                                              \
> -                void *__p = (p); size_t __n = n;                        \
> -                if ((__n) != 0) {                                       \
> -                        if (__builtin_constant_p((v)) && (v) == 0)      \
> -                                __memzero((__p),(__n));                 \
> -                        else                                            \
> -                                memset((__p),(v),(__n));                \
> -                }                                                       \
> -                (__p);                                                  \
> -        })
> +#define memset(p, v, n)                                 \
> +    ({                                                  \
> +        void *p_ = (p); size_t n_ = (n);                \
> +        typeof(v) v_ = (v);                             \
> +        if ( n_ )                                       \
> +        {                                               \
> +            if ( __builtin_constant_p(v) && !v_ )       \
> +                __memzero(p_, n_);                      \
> +            else                                        \
> +                memset(p_, v_, n_);                     \
> +        }                                               \
> +        p_;                                             \
> +    })
> 
>  #endif
> 
> 

Re: [PATCH 2/2] Arm32: tidy the memset() macro

[Jan Beulich]

On 19.08.2022 09:58, Julien Grall wrote:
> On 19/08/2022 08:50, Jan Beulich wrote:
>> - add parentheses where they were missing (MISRA)
>> - make sure to evaluate also v exactly once (MISRA)
>> - remove excess parentheses
>> - rename local variables to not have leading underscores
>> - apply Xen coding style
> 
> This code has been taken from Linux. From you write above, I don't see 
> any strong reason for us to modify it (even if it is small).

At least the MISRA issues want addressing, I suppose. Plus I wasn't
able to spot the macro in Linux anymore (nor __memzero()), so to me
there seemed to be little point to consider keeping anything "in sync"
here.

Jan

Re: [PATCH 2/2] Arm32: tidy the memset() macro

[Jan Beulich]

On 19.08.2022 09:59, Wei Chen wrote:
>> -----Original Message-----
>> From: Xen-devel <xen-devel-bounces@lists.xenproject.org> On Behalf Of Jan
>> Beulich
>> Sent: 2022年8月19日 15:50
>>
>> - add parentheses where they were missing (MISRA)
>> - make sure to evaluate also v exactly once (MISRA)
>> - remove excess parentheses
>> - rename local variables to not have leading underscores
>> - apply Xen coding style
>>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>> ---
>> I wonder whether "if ( n_ )" is really helpful: It's extra code in all
>> callers passing a non-constant size, just to cover a pretty rare case
>> which memset() is required to deal with correctly anyway, and which
> 
> What rare case we need to use n_ that can make memset happy?

I'm afraid I don't understand the question.

Jan

> As your
> comment, I read the code again, but it seems to work fine without n_.
> 
> Cheers,
> Wei Chen

Re: [PATCH 2/2] Arm32: tidy the memset() macro

[Julien Grall]

Hi Jan,

On 19/08/2022 09:02, Jan Beulich wrote:
> On 19.08.2022 09:58, Julien Grall wrote:
>> On 19/08/2022 08:50, Jan Beulich wrote:
>>> - add parentheses where they were missing (MISRA)
>>> - make sure to evaluate also v exactly once (MISRA)
>>> - remove excess parentheses
>>> - rename local variables to not have leading underscores
>>> - apply Xen coding style
>>
>> This code has been taken from Linux. From you write above, I don't see
>> any strong reason for us to modify it (even if it is small).
> 
> At least the MISRA issues want addressing, I suppose. Plus I wasn't
> able to spot the macro in Linux anymore (nor __memzero()), so to me
> there seemed to be little point to consider keeping anything "in sync"
> here.
I read the last part as we want a re-sync of the code (we haven't done 
one in the past couple of years).

Cheers,

-- 
Julien Grall

Re: [PATCH 2/2] Arm32: tidy the memset() macro

[Jan Beulich]

On 19.08.2022 10:06, Julien Grall wrote:
> On 19/08/2022 09:02, Jan Beulich wrote:
>> On 19.08.2022 09:58, Julien Grall wrote:
>>> On 19/08/2022 08:50, Jan Beulich wrote:
>>>> - add parentheses where they were missing (MISRA)
>>>> - make sure to evaluate also v exactly once (MISRA)
>>>> - remove excess parentheses
>>>> - rename local variables to not have leading underscores
>>>> - apply Xen coding style
>>>
>>> This code has been taken from Linux. From you write above, I don't see
>>> any strong reason for us to modify it (even if it is small).
>>
>> At least the MISRA issues want addressing, I suppose. Plus I wasn't
>> able to spot the macro in Linux anymore (nor __memzero()), so to me
>> there seemed to be little point to consider keeping anything "in sync"
>> here.
> I read the last part as we want a re-sync of the code (we haven't done 
> one in the past couple of years).

I'm afraid I'm now really confused: Which last part? I don't see how
any of what I have said could be read that way. Quite the opposite:
By stating that Linux doesn't have this macro anymore, isn't it quite
clear that there's nothing to re-sync against?

Jan

Re: [PATCH 2/2] Arm32: tidy the memset() macro

[Julien Grall]

Hi Jan,

On 19/08/2022 09:11, Jan Beulich wrote:
> On 19.08.2022 10:06, Julien Grall wrote:
>> On 19/08/2022 09:02, Jan Beulich wrote:
>>> On 19.08.2022 09:58, Julien Grall wrote:
>>>> On 19/08/2022 08:50, Jan Beulich wrote:
>>>>> - add parentheses where they were missing (MISRA)
>>>>> - make sure to evaluate also v exactly once (MISRA)
>>>>> - remove excess parentheses
>>>>> - rename local variables to not have leading underscores
>>>>> - apply Xen coding style
>>>>
>>>> This code has been taken from Linux. From you write above, I don't see
>>>> any strong reason for us to modify it (even if it is small).
>>>
>>> At least the MISRA issues want addressing, I suppose. Plus I wasn't
>>> able to spot the macro in Linux anymore (nor __memzero()), so to me
>>> there seemed to be little point to consider keeping anything "in sync"
>>> here.
>> I read the last part as we want a re-sync of the code (we haven't done
>> one in the past couple of years).
> 
> I'm afraid I'm now really confused: Which last part? I don't see how
> any of what I have said could be read that way. Quite the opposite:
> By stating that Linux doesn't have this macro anymore, isn't it quite
> clear that there's nothing to re-sync against?
Your view here if we will never re-sync the code. This is incorrect, we 
still want to keep it close so we can benefit from improvement in the 
Linux code. So if you start tweaking the code just for coding style 
purpose, it will just make it more difficult for us (I appreciate this 
is limited here).

In this case, Linux has removed __memzero() is patch ff5fdafc9e97 "ARM: 
8745/1: get rid of __memzero()" because the performance difference with 
memset() was limited. For Xen, I think we should also remove the function.

With that, this patch becomes pointless.

Cheers,

-- 
Julien Grall

Re: [PATCH 1/2] Arm32: correct string.h functions for "int" -> "unsigned char" conversion

[Julien Grall]

Hi Jan,

On 19/08/2022 08:49, Jan Beulich wrote:
> While Arm64 does so uniformly, for Arm32 only strchr() currently handles
> this properly. Add the necessary conversion also to strrchr(), memchr(),
> and memset().
> 
> As to the placement in memset(): Putting the new insn at the beginning
> of the function could perhaps be deemed more "obvious", but the code
> reachable without ever making it to the "1" label only ever does byte
> stores.
So the assumption here is the rest of the code will always use byte 
stores. Given that this issue has been present for a long time, I think 
it would be wiser to do the conversion at the start of the function.

The changes in memchr() and strrchr() looks fine to me.

> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> --- a/xen/arch/arm/arm32/lib/memchr.S
> +++ b/xen/arch/arm/arm32/lib/memchr.S
> @@ -14,6 +14,7 @@
>   	.text
>   	.align	5
>   ENTRY(memchr)
> +	and	r1, r1, #0xff
>   1:	subs	r2, r2, #1
>   	bmi	2f
>   	ldrb	r3, [r0], #1
> --- a/xen/arch/arm/arm32/lib/memset.S
> +++ b/xen/arch/arm/arm32/lib/memset.S
> @@ -21,7 +21,8 @@ ENTRY(memset)
>   
>    * we know that the pointer in ip is aligned to a word boundary.
>    */
> -1:	orr	r1, r1, r1, lsl #8
> +1:	and	r1, r1, #0xff
> +	orr	r1, r1, r1, lsl #8
>   	orr	r1, r1, r1, lsl #16
>   	mov	r3, r1
>   	cmp	r2, #16
> --- a/xen/arch/arm/arm32/lib/strrchr.S
> +++ b/xen/arch/arm/arm32/lib/strrchr.S
> @@ -14,6 +14,7 @@
>   		.text
>   		.align	5
>   ENTRY(strrchr)
> +		and	r1, r1, #0xff
>   		mov	r3, #0
>   1:		ldrb	r2, [r0], #1
>   		teq	r2, r1
> 

Cheers,

-- 
Julien Grall

Re: [PATCH 2/2] Arm32: tidy the memset() macro

[Jan Beulich]

On 19.08.2022 10:24, Julien Grall wrote:
> On 19/08/2022 09:11, Jan Beulich wrote:
>> On 19.08.2022 10:06, Julien Grall wrote:
>>> On 19/08/2022 09:02, Jan Beulich wrote:
>>>> On 19.08.2022 09:58, Julien Grall wrote:
>>>>> On 19/08/2022 08:50, Jan Beulich wrote:
>>>>>> - add parentheses where they were missing (MISRA)
>>>>>> - make sure to evaluate also v exactly once (MISRA)
>>>>>> - remove excess parentheses
>>>>>> - rename local variables to not have leading underscores
>>>>>> - apply Xen coding style
>>>>>
>>>>> This code has been taken from Linux. From you write above, I don't see
>>>>> any strong reason for us to modify it (even if it is small).
>>>>
>>>> At least the MISRA issues want addressing, I suppose. Plus I wasn't
>>>> able to spot the macro in Linux anymore (nor __memzero()), so to me
>>>> there seemed to be little point to consider keeping anything "in sync"
>>>> here.
>>> I read the last part as we want a re-sync of the code (we haven't done
>>> one in the past couple of years).
>>
>> I'm afraid I'm now really confused: Which last part? I don't see how
>> any of what I have said could be read that way. Quite the opposite:
>> By stating that Linux doesn't have this macro anymore, isn't it quite
>> clear that there's nothing to re-sync against?
> Your view here if we will never re-sync the code. This is incorrect, we 
> still want to keep it close so we can benefit from improvement in the 
> Linux code. So if you start tweaking the code just for coding style 
> purpose, it will just make it more difficult for us (I appreciate this 
> is limited here).
> 
> In this case, Linux has removed __memzero() is patch ff5fdafc9e97 "ARM: 
> 8745/1: get rid of __memzero()" because the performance difference with 
> memset() was limited. For Xen, I think we should also remove the function.
> 
> With that, this patch becomes pointless.

Of course. I could have named this as an alternative in a post-commit-
message remark ... Looking forward to the re-syncing to be done then,
at which point I'll happily drop this patch.

Jan

Re: [PATCH 0/2] Arm32: memset() & friends

[Julien Grall]

On 19/08/2022 08:48, Jan Beulich wrote:
> Note that while the original Linux code has the same issue, I don't
> really fancy sending there a patch similar to patch 1. That's because
> my XSA-307 related "make find_next_{,zero_}bit() have well defined
> behavior" was entirely ignored, so I would expect nothing better here.

Would you be able to point me to the discussion?

Cheers,

-- 
Julien Grall

Re: [PATCH 2/2] Arm32: tidy the memset() macro

[Julien Grall]

Hi Jan,

On 19/08/2022 09:31, Jan Beulich wrote:
> On 19.08.2022 10:24, Julien Grall wrote:
>> On 19/08/2022 09:11, Jan Beulich wrote:
>>> On 19.08.2022 10:06, Julien Grall wrote:
>>>> On 19/08/2022 09:02, Jan Beulich wrote:
>>>>> On 19.08.2022 09:58, Julien Grall wrote:
>>>>>> On 19/08/2022 08:50, Jan Beulich wrote:
>>>>>>> - add parentheses where they were missing (MISRA)
>>>>>>> - make sure to evaluate also v exactly once (MISRA)
>>>>>>> - remove excess parentheses
>>>>>>> - rename local variables to not have leading underscores
>>>>>>> - apply Xen coding style
>>>>>>
>>>>>> This code has been taken from Linux. From you write above, I don't see
>>>>>> any strong reason for us to modify it (even if it is small).
>>>>>
>>>>> At least the MISRA issues want addressing, I suppose. Plus I wasn't
>>>>> able to spot the macro in Linux anymore (nor __memzero()), so to me
>>>>> there seemed to be little point to consider keeping anything "in sync"
>>>>> here.
>>>> I read the last part as we want a re-sync of the code (we haven't done
>>>> one in the past couple of years).
>>>
>>> I'm afraid I'm now really confused: Which last part? I don't see how
>>> any of what I have said could be read that way. Quite the opposite:
>>> By stating that Linux doesn't have this macro anymore, isn't it quite
>>> clear that there's nothing to re-sync against?
>> Your view here if we will never re-sync the code. This is incorrect, we
>> still want to keep it close so we can benefit from improvement in the
>> Linux code. So if you start tweaking the code just for coding style
>> purpose, it will just make it more difficult for us (I appreciate this
>> is limited here).
>>
>> In this case, Linux has removed __memzero() is patch ff5fdafc9e97 "ARM:
>> 8745/1: get rid of __memzero()" because the performance difference with
>> memset() was limited. For Xen, I think we should also remove the function.
>>
>> With that, this patch becomes pointless.
> 
> Of course. I could have named this as an alternative in a post-commit-
> message remark ... Looking forward to the re-syncing to be done then,
> at which point I'll happily drop this patch.

I will add it in my queue. I don't think this will reach 4.17 (unless 
someone else has time).

Cheers,

-- 
Julien Grall

Re: [PATCH 0/2] Arm32: memset() & friends

[Jan Beulich]

On 19.08.2022 10:32, Julien Grall wrote:
> On 19/08/2022 08:48, Jan Beulich wrote:
>> Note that while the original Linux code has the same issue, I don't
>> really fancy sending there a patch similar to patch 1. That's because
>> my XSA-307 related "make find_next_{,zero_}bit() have well defined
>> behavior" was entirely ignored, so I would expect nothing better here.
> 
> Would you be able to point me to the discussion?

Hmm, I can't spot it on
https://patchwork.kernel.org/project/linux-arm-kernel/list/ , but
then again there are only very few entries there anyway which pre-
date July 2022. I've been able to easily spot it at
https://lore.kernel.org/linux-arm-kernel/5809eac9-aae3-e111-7301-a1aa76c0f626@suse.com/

Jan

RE: [PATCH 2/2] Arm32: tidy the memset() macro

[Wei Chen]

Hi Jan,

> -----Original Message-----
> From: Jan Beulich <jbeulich@suse.com>
> Sent: 2022年8月19日 16:04
> To: Wei Chen <Wei.Chen@arm.com>
> Cc: Julien Grall <julien@xen.org>; Stefano Stabellini
> <sstabellini@kernel.org>; Volodymyr Babchuk <volodymyr_babchuk@epam.com>;
> Bertrand Marquis <Bertrand.Marquis@arm.com>; xen-
> devel@lists.xenproject.org
> Subject: Re: [PATCH 2/2] Arm32: tidy the memset() macro
> 
> On 19.08.2022 09:59, Wei Chen wrote:
> >> -----Original Message-----
> >> From: Xen-devel <xen-devel-bounces@lists.xenproject.org> On Behalf Of
> Jan
> >> Beulich
> >> Sent: 2022年8月19日 15:50
> >>
> >> - add parentheses where they were missing (MISRA)
> >> - make sure to evaluate also v exactly once (MISRA)
> >> - remove excess parentheses
> >> - rename local variables to not have leading underscores
> >> - apply Xen coding style
> >>
> >> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> >> ---
> >> I wonder whether "if ( n_ )" is really helpful: It's extra code in all
> >> callers passing a non-constant size, just to cover a pretty rare case
> >> which memset() is required to deal with correctly anyway, and which
> >
> > What rare case we need to use n_ that can make memset happy?
> 
> I'm afraid I don't understand the question.
> 

Sorry I didn't describe the problem clearly in the last email. You mentioned
whether if (n_) is useful in your patch comments. I looked at the implementation
of the current memset macro, and I didn't feel it was too useful.

Then in the comments you mentioned that if (n_) is just to cover a very rare case.
Does the rare case is memset(p, v, 0)? If this is the case, I agree with you,
memset itself should be able to handle with size=0.

Sorry again for confusing you!

Thanks,
Wei Chen

> Jan
> 
> > As your
> > comment, I read the code again, but it seems to work fine without n_.
> >
> > Cheers,
> > Wei Chen

Re: [PATCH 2/2] Arm32: tidy the memset() macro

[Jan Beulich]

On 19.08.2022 11:41, Wei Chen wrote:
> Hi Jan,
> 
>> -----Original Message-----
>> From: Jan Beulich <jbeulich@suse.com>
>> Sent: 2022年8月19日 16:04
>> To: Wei Chen <Wei.Chen@arm.com>
>> Cc: Julien Grall <julien@xen.org>; Stefano Stabellini
>> <sstabellini@kernel.org>; Volodymyr Babchuk <volodymyr_babchuk@epam.com>;
>> Bertrand Marquis <Bertrand.Marquis@arm.com>; xen-
>> devel@lists.xenproject.org
>> Subject: Re: [PATCH 2/2] Arm32: tidy the memset() macro
>>
>> On 19.08.2022 09:59, Wei Chen wrote:
>>>> -----Original Message-----
>>>> From: Xen-devel <xen-devel-bounces@lists.xenproject.org> On Behalf Of
>> Jan
>>>> Beulich
>>>> Sent: 2022年8月19日 15:50
>>>>
>>>> - add parentheses where they were missing (MISRA)
>>>> - make sure to evaluate also v exactly once (MISRA)
>>>> - remove excess parentheses
>>>> - rename local variables to not have leading underscores
>>>> - apply Xen coding style
>>>>
>>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>>> ---
>>>> I wonder whether "if ( n_ )" is really helpful: It's extra code in all
>>>> callers passing a non-constant size, just to cover a pretty rare case
>>>> which memset() is required to deal with correctly anyway, and which
>>>
>>> What rare case we need to use n_ that can make memset happy?
>>
>> I'm afraid I don't understand the question.
>>
> 
> Sorry I didn't describe the problem clearly in the last email. You mentioned
> whether if (n_) is useful in your patch comments. I looked at the implementation
> of the current memset macro, and I didn't feel it was too useful.
> 
> Then in the comments you mentioned that if (n_) is just to cover a very rare case.
> Does the rare case is memset(p, v, 0)?

Yes, albeit not in the form you've written it, but with the last argument
being a variable which happens to be zero. With literal zero, the compiler
would dead-code eliminate the construct anyway.

Jan

> If this is the case, I agree with you,
> memset itself should be able to handle with size=0.
> 
> Sorry again for confusing you!
> 
> Thanks,
> Wei Chen

[PATCH v2] Arm32: correct string.h functions for "int" -> "unsigned char" conversion

[Jan Beulich]

While Arm64 does so uniformly, for Arm32 only strchr() currently handles
this properly. Add the necessary conversion also to strrchr(), memchr(),
and memset().

As to the placement in memset(): Putting the new insn at the beginning
of the function is apparently deemed more "obvious". It could be placed
later, as the code reachable without ever making it to the "1" label
only ever does byte stores.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
v2: For memset() use the "more obvious" adjustment.

--- a/xen/arch/arm/arm32/lib/memchr.S
+++ b/xen/arch/arm/arm32/lib/memchr.S
@@ -14,6 +14,7 @@
 	.text
 	.align	5
 ENTRY(memchr)
+	and	r1, r1, #0xff
 1:	subs	r2, r2, #1
 	bmi	2f
 	ldrb	r3, [r0], #1
--- a/xen/arch/arm/arm32/lib/memset.S
+++ b/xen/arch/arm/arm32/lib/memset.S
@@ -15,6 +15,7 @@
 	.align	5
 
 ENTRY(memset)
+	and	r1, r1, #0xff
 	ands	r3, r0, #3		@ 1 unaligned?
 	mov	ip, r0			@ preserve r0 as return value
 	bne	6f			@ 1
--- a/xen/arch/arm/arm32/lib/strrchr.S
+++ b/xen/arch/arm/arm32/lib/strrchr.S
@@ -14,6 +14,7 @@
 		.text
 		.align	5
 ENTRY(strrchr)
+		and	r1, r1, #0xff
 		mov	r3, #0
 1:		ldrb	r2, [r0], #1
 		teq	r2, r1

Re: [PATCH v2] Arm32: correct string.h functions for "int" -> "unsigned char" conversion

[Bertrand Marquis]

Hi Jan,

> On 24 Aug 2022, at 13:33, Jan Beulich <jbeulich@suse.com> wrote:
> 
> While Arm64 does so uniformly, for Arm32 only strchr() currently handles
> this properly. Add the necessary conversion also to strrchr(), memchr(),
> and memset().
> 
> As to the placement in memset(): Putting the new insn at the beginning
> of the function is apparently deemed more "obvious". It could be placed
> later, as the code reachable without ever making it to the "1" label
> only ever does byte stores.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com>

Cheers
Bertrand
> ---
> v2: For memset() use the "more obvious" adjustment.
> 
> --- a/xen/arch/arm/arm32/lib/memchr.S
> +++ b/xen/arch/arm/arm32/lib/memchr.S
> @@ -14,6 +14,7 @@
> 	.text
> 	.align	5
> ENTRY(memchr)
> +	and	r1, r1, #0xff
> 1:	subs	r2, r2, #1
> 	bmi	2f
> 	ldrb	r3, [r0], #1
> --- a/xen/arch/arm/arm32/lib/memset.S
> +++ b/xen/arch/arm/arm32/lib/memset.S
> @@ -15,6 +15,7 @@
> 	.align	5
> 
> ENTRY(memset)
> +	and	r1, r1, #0xff
> 	ands	r3, r0, #3		@ 1 unaligned?
> 	mov	ip, r0			@ preserve r0 as return value
> 	bne	6f			@ 1
> --- a/xen/arch/arm/arm32/lib/strrchr.S
> +++ b/xen/arch/arm/arm32/lib/strrchr.S
> @@ -14,6 +14,7 @@
> 		.text
> 		.align	5
> ENTRY(strrchr)
> +		and	r1, r1, #0xff
> 		mov	r3, #0
> 1:		ldrb	r2, [r0], #1
> 		teq	r2, r1

Re: [PATCH v2] Arm32: correct string.h functions for "int" -> "unsigned char" conversion

[Julien Grall]

On 24/08/2022 13:44, Bertrand Marquis wrote:
>> On 24 Aug 2022, at 13:33, Jan Beulich <jbeulich@suse.com> wrote:
>>
>> While Arm64 does so uniformly, for Arm32 only strchr() currently handles
>> this properly. Add the necessary conversion also to strrchr(), memchr(),
>> and memset().
>>
>> As to the placement in memset(): Putting the new insn at the beginning
>> of the function is apparently deemed more "obvious". It could be placed
>> later, as the code reachable without ever making it to the "1" label
>> only ever does byte stores.
>>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com>

It is now committed.

Cheers,

-- 
Julien Grall

Re: [PATCH v2] Arm32: correct string.h functions for "int" -> "unsigned char" conversion

[Jan Beulich]

On 25.08.2022 16:31, Julien Grall wrote:
> On 24/08/2022 13:44, Bertrand Marquis wrote:
>>> On 24 Aug 2022, at 13:33, Jan Beulich <jbeulich@suse.com> wrote:
>>>
>>> While Arm64 does so uniformly, for Arm32 only strchr() currently handles
>>> this properly. Add the necessary conversion also to strrchr(), memchr(),
>>> and memset().
>>>
>>> As to the placement in memset(): Putting the new insn at the beginning
>>> of the function is apparently deemed more "obvious". It could be placed
>>> later, as the code reachable without ever making it to the "1" label
>>> only ever does byte stores.
>>>
>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>> Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com>
> 
> It is now committed.

But then perhaps not pushed yet?

Jan

Re: [PATCH v2] Arm32: correct string.h functions for "int" -> "unsigned char" conversion

[Julien Grall]

Hi Jan,

On 25/08/2022 15:34, Jan Beulich wrote:
> On 25.08.2022 16:31, Julien Grall wrote:
>> On 24/08/2022 13:44, Bertrand Marquis wrote:
>>>> On 24 Aug 2022, at 13:33, Jan Beulich <jbeulich@suse.com> wrote:
>>>>
>>>> While Arm64 does so uniformly, for Arm32 only strchr() currently handles
>>>> this properly. Add the necessary conversion also to strrchr(), memchr(),
>>>> and memset().
>>>>
>>>> As to the placement in memset(): Putting the new insn at the beginning
>>>> of the function is apparently deemed more "obvious". It could be placed
>>>> later, as the code reachable without ever making it to the "1" label
>>>> only ever does byte stores.
>>>>
>>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>> Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com>
>>
>> It is now committed.
> 
> But then perhaps not pushed yet?

Yes. I tend to send the message just after I apply it. I will push when 
I am done with a batch (in this case, this is the only patch I pushed).

Cheers,

-- 
Julien Grall