| Message ID | 20221010222944.3923556-2-ira.weiny@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | QEMU CXL Provide mock CXL events and irq support | expand |
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
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
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~
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
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
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 >
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