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

[Julien Grall <julien@xxxxxxx>]

On 11/05/2022 10:56, Andrew Cooper wrote:
> 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.
It was indeed harsh. Sorry for that.

> 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);
> }
I am assuming that blah would expect a (blah *).

> 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.
I looked at the helpers, they are static inline and use a proper type. 
Therefore, I am not sure why bar would compile in this situation.
In fact, to me it seems this is an inherent issue to C: any void pointer 
can be casted to anything. You are removing one here, but there are 
hundreds of other potential "unsafe" place in Xen.
> 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.
See above, I think dropping p helpers is not solving the underlying 
problem (we are not going to be able to remove pointers in Xen).
What would solve the problem is forbidding cast from void pointer to any 
pointer. At which point, keeping *cpup* is not going to be problem.
> 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.
I read through DIR 4.9., AFAIU it is about using function rather than 
macro. The current implementation of *cpup* are using function so I 
don't understand how removing *cpup* would help.
So I am afraid, I still see no justifications to drop *cpup* here and I 
actually prefer them the open-code version. I will not Nack it, but I 
will not support.
In any case, the commit message should contain some information why they 
are dropped.
Cheers,

--
Julien Grall