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

[Julien Grall <julien@xxxxxxx>]

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