Re: [PATCH v3 4/6] xen: Switch to byteswap

[George Dunlap <George.Dunlap@xxxxxxxxxx>]

> On May 11, 2022, at 9:34 AM, Julien Grall <julien@xxxxxxx> wrote:
> 
> Hi,
> 
> Please configure your e-mail client to send in plain text.
> 
> On 11/05/2022 07:30, Lin Liu （刘林） wrote:
>> Subject: Re: [PATCH v3 4/6] xen: Switch to byteswap
>> On 10/05/2022 11:15, Lin Liu wrote:
>>> Update to use byteswap to swap bytes.
>>> 
>>> No functional change.
>>> 
>>> Signed-off-by: Lin Liu <lin.liu@xxxxxxxxxx>
>>> ---
>>> Cc: Stefano Stabellini <sstabellini@xxxxxxxxxx>
>>> Cc: Julien Grall <julien@xxxxxxx>
>>> Cc: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
>>> Cc: George Dunlap <george.dunlap@xxxxxxxxxx>
>>> Cc: Jan Beulich <jbeulich@xxxxxxxx>
>>> Cc: Wei Liu <wl@xxxxxxx>
>>> Changes in v3:
>>> - Update xen/common/device_tree.c to use be32_to_cpu
>>> - Keep const in type cast in unaligned.h
>>> ---
>>>   xen/common/device_tree.c           | 44 +++++++++++++++---------------
>>>   xen/common/libelf/libelf-private.h |  6 ++--
>>>   xen/common/xz/private.h            |  2 +-
>>>   xen/include/xen/unaligned.h        | 24 ++++++++--------
>>>   4 files changed, 38 insertions(+), 38 deletions(-)
>>> 
>>> diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c
>>> index 4aae281e89..70d3be3be6 100644
>>> --- a/xen/common/device_tree.c
>>> +++ b/xen/common/device_tree.c
>>> @@ -171,7 +171,7 @@ bool_t dt_property_read_u32(const struct dt_device_node 
>>> *np,
>>>       if ( !val || len < sizeof(*out_value) )
>>>           return 0;
>>> 
>>> -    *out_value = be32_to_cpup(val);
>>> +    *out_value = be32_to_cpu(*val);
>>> This code has been taken from Linux and I would rather prefer to keep
>>> the *cpup* helpers to avoid any changes when backporting.
>>> diff --git a/xen/include/xen/unaligned.h b/xen/include/xen/unaligned.h
>>> index 0a2b16d05d..16b2e6f5f0 100644
>>> --- a/xen/include/xen/unaligned.h
>>> +++ b/xen/include/xen/unaligned.h
>>> @@ -20,62 +20,62 @@
>>> 
>>>   static inline uint16_t get_unaligned_be16(const void *p)
>>>   {
>>> -     return be16_to_cpup(p);
>>> +     return be16_to_cpu(*(const uint16_t *)p)
>>> I haven't checked the existing implementation of be16_to_cpup().
>>> However, this new approach would allow the compiler to use a single load
>>> instruction to read the 16-bit value from memory. So this change may
>>> break on platform where unaligned access is forbidden (such as arm32).
>>>   }
>>> 
>>>   static inline void put_unaligned_be16(uint16_t val, void *p)
>>>   {
>>> -     *(__force __be16*)p = cpu_to_be16(val);
>>> +     *(__be16 *)p = cpu_to_be16(val);
>>>> Why did you drop the __force?
>> Google told me __force is used in linux kernel to suppress warning in sparse,
>> https://stackoverflow.com/questions/53120610/what-does-the-attribute-force-do
>> Is sparse also used in xen?
> 
> I am not aware of any use of Sparse in Xen, but it would technically be 
> possible.
> 
> However, my point here is more that this change seems to be unrelated to what 
> the patch is meant to do (i.e. switching to byteswap). So if it is 
> unnecessary, then it should be dropped from this patch.

I think making people pull little changes like this out into separate patches 
is asking too much.  It’s a lot of extra effort on the part of the submitter 
for basically no value.  We commonly do little clean-ups like this in patches, 
and just require a comment at the bottom, like this:

8<—

While here:
- Drop ‘_force’ keyword, which is only needed when running the Sparse analysis 
tool

—>8

I do agree that minor changes like this need to be described, so that people 5 
years from now have some hope of figuring out what’s going on.

 -George

Attachment: signature.asc
Description: Message signed with OpenPGP