[RFC PATCH 1/6] qemu/bswap: Add const_le64()

ira.weiny@intel.com posted 6 patches 3 years, 4 months ago
There is a newer version of this series
[RFC PATCH 1/6] qemu/bswap: Add const_le64()
Posted by ira.weiny@intel.com 3 years, 4 months ago
From: Ira Weiny <ira.weiny@intel.com>

Gcc requires constant versions of cpu_to_le* calls.

Add a 64 bit version.

Signed-off-by: Ira Weiny <ira.weiny@intel.com>
---
 include/qemu/bswap.h | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/include/qemu/bswap.h b/include/qemu/bswap.h
index 346d05f2aab3..08e607821102 100644
--- a/include/qemu/bswap.h
+++ b/include/qemu/bswap.h
@@ -192,10 +192,20 @@ CPU_CONVERT(le, 64, uint64_t)
      (((_x) & 0x0000ff00U) <<  8) |              \
      (((_x) & 0x00ff0000U) >>  8) |              \
      (((_x) & 0xff000000U) >> 24))
+# define const_le64(_x)                          \
+    ((((_x) & 0x00000000000000ffU) << 56) |      \
+     (((_x) & 0x000000000000ff00U) << 40) |      \
+     (((_x) & 0x0000000000ff0000U) << 24) |      \
+     (((_x) & 0x00000000ff000000U) <<  8) |      \
+     (((_x) & 0x000000ff00000000U) >>  8) |      \
+     (((_x) & 0x0000ff0000000000U) >> 24) |      \
+     (((_x) & 0x00ff000000000000U) >> 40) |      \
+     (((_x) & 0xff00000000000000U) >> 56))
 # define const_le16(_x)                          \
     ((((_x) & 0x00ff) << 8) |                    \
      (((_x) & 0xff00) >> 8))
 #else
+# define const_le64(_x) (_x)
 # define const_le32(_x) (_x)
 # define const_le16(_x) (_x)
 #endif
-- 
2.37.2
Re: [RFC PATCH 1/6] qemu/bswap: Add const_le64()
Posted by Peter Maydell 3 years, 4 months ago
On Mon, 10 Oct 2022 at 23:48, <ira.weiny@intel.com> wrote:
>
> From: Ira Weiny <ira.weiny@intel.com>
>
> Gcc requires constant versions of cpu_to_le* calls.
>
> Add a 64 bit version.
>
> Signed-off-by: Ira Weiny <ira.weiny@intel.com>
> ---
>  include/qemu/bswap.h | 10 ++++++++++
>  1 file changed, 10 insertions(+)
>
> diff --git a/include/qemu/bswap.h b/include/qemu/bswap.h
> index 346d05f2aab3..08e607821102 100644
> --- a/include/qemu/bswap.h
> +++ b/include/qemu/bswap.h
> @@ -192,10 +192,20 @@ CPU_CONVERT(le, 64, uint64_t)
>       (((_x) & 0x0000ff00U) <<  8) |              \
>       (((_x) & 0x00ff0000U) >>  8) |              \
>       (((_x) & 0xff000000U) >> 24))
> +# define const_le64(_x)                          \
> +    ((((_x) & 0x00000000000000ffU) << 56) |      \
> +     (((_x) & 0x000000000000ff00U) << 40) |      \
> +     (((_x) & 0x0000000000ff0000U) << 24) |      \
> +     (((_x) & 0x00000000ff000000U) <<  8) |      \
> +     (((_x) & 0x000000ff00000000U) >>  8) |      \
> +     (((_x) & 0x0000ff0000000000U) >> 24) |      \
> +     (((_x) & 0x00ff000000000000U) >> 40) |      \
> +     (((_x) & 0xff00000000000000U) >> 56))

Can you add this in the right place, ie above the const_le32()
definition, please ?

>  # define const_le16(_x)                          \
>      ((((_x) & 0x00ff) << 8) |                    \
>       (((_x) & 0xff00) >> 8))
>  #else
> +# define const_le64(_x) (_x)
>  # define const_le32(_x) (_x)
>  # define const_le16(_x) (_x)
>  #endif

This is kind of a weird API, because:
 * it only exists for little-endian, not big-endian
 * we use it in exactly two files (linux-user/elfload.c and
   hw/input/virtio-input-hid.c)

which leaves me wondering if there's a better way of doing
it that I'm missing. But maybe it's just that we never filled
out the missing bits of the API surface because we haven't
needed them yet. Richard ?

thanks
-- PMM
Re: [RFC PATCH 1/6] qemu/bswap: Add const_le64()
Posted by Richard Henderson 3 years, 4 months ago
On 10/11/22 02:48, Peter Maydell wrote:
>> +# define const_le64(_x) (_x)
>>   # define const_le32(_x) (_x)
>>   # define const_le16(_x) (_x)
>>   #endif
> 
> This is kind of a weird API, because:
>   * it only exists for little-endian, not big-endian
>   * we use it in exactly two files (linux-user/elfload.c and
>     hw/input/virtio-input-hid.c)
> 
> which leaves me wondering if there's a better way of doing
> it that I'm missing. But maybe it's just that we never filled
> out the missing bits of the API surface because we haven't
> needed them yet. Richard ?

It's piecemeal because, as you note, very few places require a version of byte swapping 
that must be applicable to static data.  I certainly don't want to completely fill this 
out and have most of it remain unused.


r~
Re: [RFC PATCH 1/6] qemu/bswap: Add const_le64()
Posted by Peter Maydell 3 years, 4 months ago
On Tue, 11 Oct 2022 at 16:22, Richard Henderson
<richard.henderson@linaro.org> wrote:
> On 10/11/22 02:48, Peter Maydell wrote:
> > This is kind of a weird API, because:
> >   * it only exists for little-endian, not big-endian
> >   * we use it in exactly two files (linux-user/elfload.c and
> >     hw/input/virtio-input-hid.c)
> >
> > which leaves me wondering if there's a better way of doing
> > it that I'm missing. But maybe it's just that we never filled
> > out the missing bits of the API surface because we haven't
> > needed them yet. Richard ?
>
> It's piecemeal because, as you note, very few places require a version of byte swapping
> that must be applicable to static data.  I certainly don't want to completely fill this
> out and have most of it remain unused.

Makes sense. In that case, other than ordering the definitions
64-32-16,

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM
Re: [RFC PATCH 1/6] qemu/bswap: Add const_le64()
Posted by Ira Weiny 3 years, 3 months ago
On Tue, Oct 11, 2022 at 04:45:57PM +0100, Peter Maydell wrote:
> On Tue, 11 Oct 2022 at 16:22, Richard Henderson
> <richard.henderson@linaro.org> wrote:
> > On 10/11/22 02:48, Peter Maydell wrote:
> > > This is kind of a weird API, because:
> > >   * it only exists for little-endian, not big-endian
> > >   * we use it in exactly two files (linux-user/elfload.c and
> > >     hw/input/virtio-input-hid.c)
> > >
> > > which leaves me wondering if there's a better way of doing
> > > it that I'm missing. But maybe it's just that we never filled
> > > out the missing bits of the API surface because we haven't
> > > needed them yet. Richard ?
> >
> > It's piecemeal because, as you note, very few places require a version of byte swapping
> > that must be applicable to static data.  I certainly don't want to completely fill this
> > out and have most of it remain unused.
> 
> Makes sense. In that case, other than ordering the definitions
> 64-32-16,

Done.

> 
> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

Thanks!
Ira
Re: [RFC PATCH 1/6] qemu/bswap: Add const_le64()
Posted by Jonathan Cameron via 3 years, 4 months ago
On Mon, 10 Oct 2022 15:29:39 -0700
ira.weiny@intel.com wrote:

> From: Ira Weiny <ira.weiny@intel.com>
> 
> Gcc requires constant versions of cpu_to_le* calls.
> 
> Add a 64 bit version.
> 
> Signed-off-by: Ira Weiny <ira.weiny@intel.com>

Seems reasonable to me but I'm not an expert in this stuff.
FWIW

Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

There are probably a lot of places in the CXL emulation where
our endian handling isn't correct but so far it hasn't mattered
as all the supported architectures are little endian.

Good to not introduce more cases however!

Jonathan


> ---
>  include/qemu/bswap.h | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/include/qemu/bswap.h b/include/qemu/bswap.h
> index 346d05f2aab3..08e607821102 100644
> --- a/include/qemu/bswap.h
> +++ b/include/qemu/bswap.h
> @@ -192,10 +192,20 @@ CPU_CONVERT(le, 64, uint64_t)
>       (((_x) & 0x0000ff00U) <<  8) |              \
>       (((_x) & 0x00ff0000U) >>  8) |              \
>       (((_x) & 0xff000000U) >> 24))
> +# define const_le64(_x)                          \
> +    ((((_x) & 0x00000000000000ffU) << 56) |      \
> +     (((_x) & 0x000000000000ff00U) << 40) |      \
> +     (((_x) & 0x0000000000ff0000U) << 24) |      \
> +     (((_x) & 0x00000000ff000000U) <<  8) |      \
> +     (((_x) & 0x000000ff00000000U) >>  8) |      \
> +     (((_x) & 0x0000ff0000000000U) >> 24) |      \
> +     (((_x) & 0x00ff000000000000U) >> 40) |      \
> +     (((_x) & 0xff00000000000000U) >> 56))
>  # define const_le16(_x)                          \
>      ((((_x) & 0x00ff) << 8) |                    \
>       (((_x) & 0xff00) >> 8))
>  #else
> +# define const_le64(_x) (_x)
>  # define const_le32(_x) (_x)
>  # define const_le16(_x) (_x)
>  #endif
Re: [RFC PATCH 1/6] qemu/bswap: Add const_le64()
Posted by Ira Weiny 3 years, 3 months ago
On Tue, Oct 11, 2022 at 10:03:00AM +0100, Jonathan Cameron wrote:
> On Mon, 10 Oct 2022 15:29:39 -0700
> ira.weiny@intel.com wrote:
> 
> > From: Ira Weiny <ira.weiny@intel.com>
> > 
> > Gcc requires constant versions of cpu_to_le* calls.
> > 
> > Add a 64 bit version.
> > 
> > Signed-off-by: Ira Weiny <ira.weiny@intel.com>
> 
> Seems reasonable to me but I'm not an expert in this stuff.
> FWIW
> 
> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> 
> There are probably a lot of places in the CXL emulation where
> our endian handling isn't correct but so far it hasn't mattered
> as all the supported architectures are little endian.
> 
> Good to not introduce more cases however!

Agreed. Thanks!
Ira

> 
> Jonathan
> 
> 
> > ---
> >  include/qemu/bswap.h | 10 ++++++++++
> >  1 file changed, 10 insertions(+)
> > 
> > diff --git a/include/qemu/bswap.h b/include/qemu/bswap.h
> > index 346d05f2aab3..08e607821102 100644
> > --- a/include/qemu/bswap.h
> > +++ b/include/qemu/bswap.h
> > @@ -192,10 +192,20 @@ CPU_CONVERT(le, 64, uint64_t)
> >       (((_x) & 0x0000ff00U) <<  8) |              \
> >       (((_x) & 0x00ff0000U) >>  8) |              \
> >       (((_x) & 0xff000000U) >> 24))
> > +# define const_le64(_x)                          \
> > +    ((((_x) & 0x00000000000000ffU) << 56) |      \
> > +     (((_x) & 0x000000000000ff00U) << 40) |      \
> > +     (((_x) & 0x0000000000ff0000U) << 24) |      \
> > +     (((_x) & 0x00000000ff000000U) <<  8) |      \
> > +     (((_x) & 0x000000ff00000000U) >>  8) |      \
> > +     (((_x) & 0x0000ff0000000000U) >> 24) |      \
> > +     (((_x) & 0x00ff000000000000U) >> 40) |      \
> > +     (((_x) & 0xff00000000000000U) >> 56))
> >  # define const_le16(_x)                          \
> >      ((((_x) & 0x00ff) << 8) |                    \
> >       (((_x) & 0xff00) >> 8))
> >  #else
> > +# define const_le64(_x) (_x)
> >  # define const_le32(_x) (_x)
> >  # define const_le16(_x) (_x)
> >  #endif
>