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

[Julien Grall <julien@xxxxxxx>]

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.
> Your choices are to either fix it yourself (after all, you are the
> maintainer who cares about this unrelated bug), or you ask Lin kindly if
> he has time to look into fixing the unrelated bug after this series is
> complete.
Or 3) keep *cpup* so there is only one place to fix it.

> It is not reasonable to say "this unrelated thing is broken, and you
> need to fix it first to get your series in".  Requests like that are,
> I'm sure, part of what Bertrand raised in the community call as
> unnecessary fiction getting work submitted.
To be honest, you put the contributor in this situation. I would have 
been perfectly happy if we keep *cpup* around as there would be only a 
place to fix.
With this approach, you are effectively going to increase the work later 
one because now we would have to chase all the open-coded version of 
*cpup* and check which one is not safe.
Cheers,

--
Julien Grall