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

[Andrew Cooper <Andrew.Cooper3@citrix.com>]

On 10/05/2022 12:47, Julien Grall wrote:
> Hi,
>
> On 10/05/2022 12:34, Andrew Cooper wrote:
>> On 10/05/2022 12:17, Julien Grall wrote:
>>>>
>>>>>
>>>>>> 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.
>>
>> No.  That is an inappropriate request to make.
>>
>> Lin's patch does not alter the broken-ness of unaligned on arm32, and
>> does improve the aspect of the hypervisor that it pertains to.  It
>> therefore stands on its own merit.
> I am not sure sure why switching from *cpup* improves things... and as
> usual you haven't answered to the clarification questions.

Adjust your tone back to something appropriate to the conversation.

The p helpers hide a unsafe typecast&dereference which will erroneously
compile both of these:

void foo(void *ptr)
{
    blah_p(ptr);
}

void bar(baz *ptr)
{
    blah_p(ptr);
}

and potentially malfunction as a consequence, not to mention that it's
sufficient obfuscation to trick a maintainer into believe an unrelated
area of code was safe when it wasn't.

Deleting the p helpers is a specific objective of the work, because it
forces the author to resolve to an integral type, and have the deference
chain visible in a single location which improves code clarity.

On a hunch, I checked the MISRA spec, and it turns out there is a rule
against the p helpers (specifically the type safety aspect), so this
series is removing a whole load of DIR 4.9 violations from the codebase.

~Andrew