Re: [PATCH 1/2] xen: make include/xen/unaligned.h usable on all architectures

[Juergen Gross <jgross@xxxxxxxx>]

On 05.12.23 14:55, Jan Beulich wrote:
> On 05.12.2023 11:07, Juergen Gross wrote:
> > @@ -15,67 +7,126 @@
> >   #include <asm/byteorder.h>
> >   #endif
> >   
> > -#define get_unaligned(p) (*(p))
> > -#define put_unaligned(val, p) (*(p) = (val))
> > +/*
> > + * This is the most generic implementation of unaligned accesses
> > + * and should work almost anywhere.
> > + */
> > +
> > +#define __get_unaligned_t(type, ptr) ({                                        
> >         \
> > +       const struct { type x; } __packed *__pptr = (typeof(__pptr))(ptr);      
> > \
> > +       __pptr->x;                                                           \
> > +})
> > +
> > +#define __put_unaligned_t(type, val, ptr) do {                                 
> > \
> > +       struct { type x; } __packed *__pptr = (typeof(__pptr))(ptr);            
> > \
> > +       __pptr->x = (val);                                                   \
> > +} while (0)
> Please can we avoid gaining new (even double) leading underscore symbols,
> especially when they're in helper (i.e. not intended to be used directly)
> constructs? No reason to introduce further issues wrt Misra adoption.
>
> > +#define get_unaligned(ptr)     __get_unaligned_t(typeof(*(ptr)), (ptr))
> > +#define put_unaligned(val, ptr) __put_unaligned_t(typeof(*(ptr)), (val), (ptr))
> May I ask to omit excess parentheses where possible?
>
> > +static inline u16 get_unaligned_le16(const void *p)
> > +{
> > +       return le16_to_cpu(__get_unaligned_t(__le16, p));
> > +}
> > +
> > +static inline u32 get_unaligned_le32(const void *p)
> > +{
> > +       return le32_to_cpu(__get_unaligned_t(__le32, p));
> > +}
> > +
> > +static inline u64 get_unaligned_le64(const void *p)
> > +{
> > +       return le64_to_cpu(__get_unaligned_t(__le64, p));
> > +}
> > +
> > +static inline void put_unaligned_le16(u16 val, void *p)
> > +{
> > +       __put_unaligned_t(__le16, cpu_to_le16(val), p);
> > +}
> > +
> > +static inline void put_unaligned_le32(u32 val, void *p)
> > +{
> > +       __put_unaligned_t(__le32, cpu_to_le32(val), p);
> > +}
> > +
> > +static inline void put_unaligned_le64(u64 val, void *p)
> > +{
> > +       __put_unaligned_t(__le64, cpu_to_le64(val), p);
> > +}
> > +
> > +static inline u16 get_unaligned_be16(const void *p)
> > +{
> > +       return be16_to_cpu(__get_unaligned_t(__be16, p));
> > +}
> > +
> > +static inline u32 get_unaligned_be32(const void *p)
> > +{
> > +       return be32_to_cpu(__get_unaligned_t(__be32, p));
> > +}
> >   
> > -static inline uint16_t get_unaligned_be16(const void *p)
> > +static inline u64 get_unaligned_be64(const void *p)
> >   {
> > -       return be16_to_cpup(p);
> > +       return be64_to_cpu(__get_unaligned_t(__be64, p));
> >   }
> >   
> > -static inline void put_unaligned_be16(uint16_t val, void *p)
> > +static inline void put_unaligned_be16(u16 val, void *p)
> >   {
> > -       *(__force __be16*)p = cpu_to_be16(val);
> > +       __put_unaligned_t(__be16, cpu_to_be16(val), p);
> >   }
> >   
> > -static inline uint32_t get_unaligned_be32(const void *p)
> > +static inline void put_unaligned_be32(u32 val, void *p)
> >   {
> > -       return be32_to_cpup(p);
> > +       __put_unaligned_t(__be32, cpu_to_be32(val), p);
> >   }
> >   
> > -static inline void put_unaligned_be32(uint32_t val, void *p)
> > +static inline void put_unaligned_be64(u64 val, void *p)
> >   {
> > -       *(__force __be32*)p = cpu_to_be32(val);
> > +       __put_unaligned_t(__be64, cpu_to_be64(val), p);
> >   }
> >   
> > -static inline uint64_t get_unaligned_be64(const void *p)
> > +static inline u32 __get_unaligned_be24(const u8 *p)
> Here and below - do you foresee use of these 24-bit helpers? They weren't
> there before, and the description also doesn't justify their introduction.
I have just applied the patch from the kernel (see Origin: tag).

I can change the patch according to your comments, of course.


Juergen

Attachment: OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key

Attachment: OpenPGP_signature.asc
Description: OpenPGP digital signature