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

[Julien Grall <julien@xen.org>]

Hi,

On 10/05/2022 12:09, Andrew Cooper wrote:
> On 10/05/2022 11:51, Julien Grall wrote:
>> On 10/05/2022 11:15, Lin Liu wrote:
>>> 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.
> 
> I specifically requested that this be de-obfuscated.  Hiding indirection
> is a fantastic way to introduce bugs, and we've had XSAs in the past
> because of it (admittedly in libxl, but still...).

Care providing a link to those XSAs? But I don't really see what's the 
problem here, this is no better no worth than passing pointer to other 
functions...

> 
> This file is already Xen style, not Linux, so won't be taking backports
> directly, and the resulting compiler diagnostic will make it obvious
> what is going on. be32_to_cpu(*val) works fine on older versions of Xen too.
> 
> In this case, the cost of changing is well worth the improvements and
> simplifications gained.  See the 0/6 diffstat and see that the compiler
> can make better optimisations when it can see the builtin.

I take your point... However, the commit message provides virtually zero 
justification into why we should switch to be32_to_cpup(). So to me, the 
changes so far looks unwanted.

> 
>>
>>> 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().
> 
> It's a plain dereference, just like this.  AFAICT, it wasn't unaligned
> safe before, either.

Well, technically an architecture could provide an override for the 
copy. I agree that arm32 is already bogus but...

> 
> It should be reasonably easy to fix in a followup patch.  Just memcpy()
> to/from the void pointer to a stack variable of the appropriate type.
... I disagree that it should be fixed in a follow-up patch. It should 
be fixed now as this is where the badness is spread to any architecture.

Cheers,

-- 
Julien Grall