[v6,1/4] bswap: Add the ability to store to an unaligned 24 bit field

Commit Message

Jonathan Cameron May 19, 2023, 2:18 p.m. UTC

From: Ira Weiny <[e-mail]>

CXL has 24 bit unaligned fields which need to be stored to.  CXL is
specified as little endian.

Define st24_le_p() and the supporting functions to store such a field
from a 32 bit host native value.

The use of b, w, l, q as the size specifier is limiting.  So "24" was
used for the size part of the function name.

Signed-off-by: Ira Weiny <[e-mail]>
Signed-off-by: Jonathan Cameron <[e-mail]>
---
 docs/devel/loads-stores.rst |  1 +
 include/qemu/bswap.h        | 27 +++++++++++++++++++++++++++
 2 files changed, 28 insertions(+)

Comments

Philippe Mathieu-Daudé May 19, 2023, 4:08 p.m. UTC | #1

On 19/5/23 16:18, Jonathan Cameron wrote:
> From: Ira Weiny <[e-mail]>
> 
> CXL has 24 bit unaligned fields which need to be stored to.  CXL is
> specified as little endian.
> 
> Define st24_le_p() and the supporting functions to store such a field
> from a 32 bit host native value.
> 
> The use of b, w, l, q as the size specifier is limiting.  So "24" was
> used for the size part of the function name.
> 
> Signed-off-by: Ira Weiny <[e-mail]>
> Signed-off-by: Jonathan Cameron <[e-mail]>
> ---
>   docs/devel/loads-stores.rst |  1 +
>   include/qemu/bswap.h        | 27 +++++++++++++++++++++++++++
>   2 files changed, 28 insertions(+)
> 
> diff --git a/docs/devel/loads-stores.rst b/docs/devel/loads-stores.rst
> index d2cefc77a2..82a79e91d9 100644
> --- a/docs/devel/loads-stores.rst
> +++ b/docs/devel/loads-stores.rst
> @@ -36,6 +36,7 @@ store: ``st{size}_{endian}_p(ptr, val)``
>   ``size``
>    - ``b`` : 8 bits
>    - ``w`` : 16 bits
> + - ``24`` : 24 bits
>    - ``l`` : 32 bits
>    - ``q`` : 64 bits
>   
> diff --git a/include/qemu/bswap.h b/include/qemu/bswap.h
> index 15a78c0db5..f546b1fc06 100644
> --- a/include/qemu/bswap.h
> +++ b/include/qemu/bswap.h
> @@ -8,11 +8,25 @@
>   #undef  bswap64
>   #define bswap64(_x) __builtin_bswap64(_x)
>   
> +static inline uint32_t bswap24(uint32_t x)
> +{
> +    assert((x & 0xff000000U) == 0);

Asserting here is a bit violent. In particular because there is no
contract description that x should be less than N bits for bswapN()
in "qemu/bswap.h" API.

But if you rather to assert ...

> +
> +    return (((x & 0x000000ffU) << 16) |
> +            ((x & 0x0000ff00U) <<  0) |
> +            ((x & 0x00ff0000U) >> 16));
> +}
> +
>   static inline void bswap16s(uint16_t *s)
>   {
>       *s = __builtin_bswap16(*s);
>   }
>   
> +static inline void bswap24s(uint32_t *s)
> +{
> +    *s = bswap24(*s & 0x00ffffffU);

... and sanitize the value here ...

> +}
> +
>   static inline void bswap32s(uint32_t *s)
>   {
>       *s = __builtin_bswap32(*s);
> @@ -26,11 +40,13 @@ static inline void bswap64s(uint64_t *s)
>   #if HOST_BIG_ENDIAN
>   #define be_bswap(v, size) (v)
>   #define le_bswap(v, size) glue(__builtin_bswap, size)(v)
> +#define le_bswap24(v) bswap24(v)

... then shouldn't you sanitize also here?

Personally I'd just drop the assertion.

>   #define be_bswaps(v, size)
>   #define le_bswaps(p, size) \
>               do { *p = glue(__builtin_bswap, size)(*p); } while (0)
>   #else
>   #define le_bswap(v, size) (v)
> +#define le_bswap24(v) (v)
>   #define be_bswap(v, size) glue(__builtin_bswap, size)(v)
>   #define le_bswaps(v, size)
>   #define be_bswaps(p, size) \
> @@ -176,6 +192,7 @@ CPU_CONVERT(le, 64, uint64_t)
>    * size is:
>    *   b: 8 bits
>    *   w: 16 bits
> + *   24: 24 bits
>    *   l: 32 bits
>    *   q: 64 bits
>    *
> @@ -248,6 +265,11 @@ static inline void stw_he_p(void *ptr, uint16_t v)
>       __builtin_memcpy(ptr, &v, sizeof(v));
>   }
>   
> +static inline void st24_he_p(void *ptr, uint32_t v)
> +{
> +    __builtin_memcpy(ptr, &v, 3);
> +}
> +
>   static inline int ldl_he_p(const void *ptr)
>   {
>       int32_t r;
> @@ -297,6 +319,11 @@ static inline void stw_le_p(void *ptr, uint16_t v)
>       stw_he_p(ptr, le_bswap(v, 16));
>   }
>   
> +static inline void st24_le_p(void *ptr, uint32_t v)
> +{
> +    st24_he_p(ptr, le_bswap24(v));
> +}
> +
>   static inline void stl_le_p(void *ptr, uint32_t v)
>   {
>       stl_he_p(ptr, le_bswap(v, 32));

Conditional to removing the assertion in bswap24():
Reviewed-by: Philippe Mathieu-Daudé <[e-mail]>

Jonathan Cameron May 19, 2023, 4:24 p.m. UTC | #2

On Fri, 19 May 2023 18:08:30 +0200
Philippe Mathieu-Daudé <[e-mail]> wrote:

> On 19/5/23 16:18, Jonathan Cameron wrote:
> > From: Ira Weiny <[e-mail]>
> > 
> > CXL has 24 bit unaligned fields which need to be stored to.  CXL is
> > specified as little endian.
> > 
> > Define st24_le_p() and the supporting functions to store such a field
> > from a 32 bit host native value.
> > 
> > The use of b, w, l, q as the size specifier is limiting.  So "24" was
> > used for the size part of the function name.
> > 
> > Signed-off-by: Ira Weiny <[e-mail]>
> > Signed-off-by: Jonathan Cameron <[e-mail]>
> > ---
> >   docs/devel/loads-stores.rst |  1 +
> >   include/qemu/bswap.h        | 27 +++++++++++++++++++++++++++
> >   2 files changed, 28 insertions(+)
> > 
> > diff --git a/docs/devel/loads-stores.rst b/docs/devel/loads-stores.rst
> > index d2cefc77a2..82a79e91d9 100644
> > --- a/docs/devel/loads-stores.rst
> > +++ b/docs/devel/loads-stores.rst
> > @@ -36,6 +36,7 @@ store: ``st{size}_{endian}_p(ptr, val)``
> >   ``size``
> >    - ``b`` : 8 bits
> >    - ``w`` : 16 bits
> > + - ``24`` : 24 bits
> >    - ``l`` : 32 bits
> >    - ``q`` : 64 bits
> >   
> > diff --git a/include/qemu/bswap.h b/include/qemu/bswap.h
> > index 15a78c0db5..f546b1fc06 100644
> > --- a/include/qemu/bswap.h
> > +++ b/include/qemu/bswap.h
> > @@ -8,11 +8,25 @@
> >   #undef  bswap64
> >   #define bswap64(_x) __builtin_bswap64(_x)
> >   
> > +static inline uint32_t bswap24(uint32_t x)
> > +{
> > +    assert((x & 0xff000000U) == 0);  
> 
> Asserting here is a bit violent. In particular because there is no
> contract description that x should be less than N bits for bswapN()
> in "qemu/bswap.h" API.
> 
> But if you rather to assert ...


I'm fine either way.  You asked for it when reviewing v4...
https://lore.kernel.org/all/[e-mail]/


> 
> > +
> > +    return (((x & 0x000000ffU) << 16) |
> > +            ((x & 0x0000ff00U) <<  0) |
> > +            ((x & 0x00ff0000U) >> 16));
> > +}
> > +
> >   static inline void bswap16s(uint16_t *s)
> >   {
> >       *s = __builtin_bswap16(*s);
> >   }
> >   
> > +static inline void bswap24s(uint32_t *s)
> > +{
> > +    *s = bswap24(*s & 0x00ffffffU);  
> 
> ... and sanitize the value here ...
> 
> > +}
> > +
> >   static inline void bswap32s(uint32_t *s)
> >   {
> >       *s = __builtin_bswap32(*s);
> > @@ -26,11 +40,13 @@ static inline void bswap64s(uint64_t *s)
> >   #if HOST_BIG_ENDIAN
> >   #define be_bswap(v, size) (v)
> >   #define le_bswap(v, size) glue(__builtin_bswap, size)(v)
> > +#define le_bswap24(v) bswap24(v)  
> 
> ... then shouldn't you sanitize also here?
> 

Good point. I forgot that detail whilst fighting with s390
cross builds earlier ;)

> Personally I'd just drop the assertion.

I'm fine with doing that.

Jonathan

> 
> >   #define be_bswaps(v, size)
> >   #define le_bswaps(p, size) \
> >               do { *p = glue(__builtin_bswap, size)(*p); } while (0)
> >   #else
> >   #define le_bswap(v, size) (v)
> > +#define le_bswap24(v) (v)
> >   #define be_bswap(v, size) glue(__builtin_bswap, size)(v)
> >   #define le_bswaps(v, size)
> >   #define be_bswaps(p, size) \
> > @@ -176,6 +192,7 @@ CPU_CONVERT(le, 64, uint64_t)
> >    * size is:
> >    *   b: 8 bits
> >    *   w: 16 bits
> > + *   24: 24 bits
> >    *   l: 32 bits
> >    *   q: 64 bits
> >    *
> > @@ -248,6 +265,11 @@ static inline void stw_he_p(void *ptr, uint16_t v)
> >       __builtin_memcpy(ptr, &v, sizeof(v));
> >   }
> >   
> > +static inline void st24_he_p(void *ptr, uint32_t v)
> > +{
> > +    __builtin_memcpy(ptr, &v, 3);
> > +}
> > +
> >   static inline int ldl_he_p(const void *ptr)
> >   {
> >       int32_t r;
> > @@ -297,6 +319,11 @@ static inline void stw_le_p(void *ptr, uint16_t v)
> >       stw_he_p(ptr, le_bswap(v, 16));
> >   }
> >   
> > +static inline void st24_le_p(void *ptr, uint32_t v)
> > +{
> > +    st24_he_p(ptr, le_bswap24(v));
> > +}
> > +
> >   static inline void stl_le_p(void *ptr, uint32_t v)
> >   {
> >       stl_he_p(ptr, le_bswap(v, 32));  
> 
> Conditional to removing the assertion in bswap24():
> Reviewed-by: Philippe Mathieu-Daudé <[e-mail]>
>

Philippe Mathieu-Daudé May 19, 2023, 4:43 p.m. UTC | #3

On 19/5/23 18:24, Jonathan Cameron wrote:
> On Fri, 19 May 2023 18:08:30 +0200
> Philippe Mathieu-Daudé <[e-mail]> wrote:
> 
>> On 19/5/23 16:18, Jonathan Cameron wrote:
>>> From: Ira Weiny <[e-mail]>
>>>
>>> CXL has 24 bit unaligned fields which need to be stored to.  CXL is
>>> specified as little endian.
>>>
>>> Define st24_le_p() and the supporting functions to store such a field
>>> from a 32 bit host native value.
>>>
>>> The use of b, w, l, q as the size specifier is limiting.  So "24" was
>>> used for the size part of the function name.
>>>
>>> Signed-off-by: Ira Weiny <[e-mail]>
>>> Signed-off-by: Jonathan Cameron <[e-mail]>
>>> ---
>>>    docs/devel/loads-stores.rst |  1 +
>>>    include/qemu/bswap.h        | 27 +++++++++++++++++++++++++++
>>>    2 files changed, 28 insertions(+)
>>>
>>> diff --git a/docs/devel/loads-stores.rst b/docs/devel/loads-stores.rst
>>> index d2cefc77a2..82a79e91d9 100644
>>> --- a/docs/devel/loads-stores.rst
>>> +++ b/docs/devel/loads-stores.rst
>>> @@ -36,6 +36,7 @@ store: ``st{size}_{endian}_p(ptr, val)``
>>>    ``size``
>>>     - ``b`` : 8 bits
>>>     - ``w`` : 16 bits
>>> + - ``24`` : 24 bits
>>>     - ``l`` : 32 bits
>>>     - ``q`` : 64 bits
>>>    
>>> diff --git a/include/qemu/bswap.h b/include/qemu/bswap.h
>>> index 15a78c0db5..f546b1fc06 100644
>>> --- a/include/qemu/bswap.h
>>> +++ b/include/qemu/bswap.h
>>> @@ -8,11 +8,25 @@
>>>    #undef  bswap64
>>>    #define bswap64(_x) __builtin_bswap64(_x)
>>>    
>>> +static inline uint32_t bswap24(uint32_t x)
>>> +{
>>> +    assert((x & 0xff000000U) == 0);
>>
>> Asserting here is a bit violent. In particular because there is no
>> contract description that x should be less than N bits for bswapN()
>> in "qemu/bswap.h" API.
>>
>> But if you rather to assert ...
> 
> 
> I'm fine either way.  You asked for it when reviewing v4...
> https://lore.kernel.org/all/[e-mail]/

Never too late to improve oneself ;)

One month later I'm afraid the assertion would fire too often,
resulting in unnecessary pain to developers (in particular the
non-QEMU ones).

>>
>>> +
>>> +    return (((x & 0x000000ffU) << 16) |
>>> +            ((x & 0x0000ff00U) <<  0) |
>>> +            ((x & 0x00ff0000U) >> 16));
>>> +}
>>> +
>>>    static inline void bswap16s(uint16_t *s)
>>>    {
>>>        *s = __builtin_bswap16(*s);
>>>    }
>>>    
>>> +static inline void bswap24s(uint32_t *s)
>>> +{
>>> +    *s = bswap24(*s & 0x00ffffffU);
>>
>> ... and sanitize the value here ...
>>
>>> +}
>>> +
>>>    static inline void bswap32s(uint32_t *s)
>>>    {
>>>        *s = __builtin_bswap32(*s);
>>> @@ -26,11 +40,13 @@ static inline void bswap64s(uint64_t *s)
>>>    #if HOST_BIG_ENDIAN
>>>    #define be_bswap(v, size) (v)
>>>    #define le_bswap(v, size) glue(__builtin_bswap, size)(v)
>>> +#define le_bswap24(v) bswap24(v)
>>
>> ... then shouldn't you sanitize also here?
>>
> 
> Good point. I forgot that detail whilst fighting with s390
> cross builds earlier ;)
> 
>> Personally I'd just drop the assertion.
> 
> I'm fine with doing that.
> 
> Jonathan
> 
>>
>>>    #define be_bswaps(v, size)
>>>    #define le_bswaps(p, size) \
>>>                do { *p = glue(__builtin_bswap, size)(*p); } while (0)
>>>    #else
>>>    #define le_bswap(v, size) (v)
>>> +#define le_bswap24(v) (v)
>>>    #define be_bswap(v, size) glue(__builtin_bswap, size)(v)
>>>    #define le_bswaps(v, size)
>>>    #define be_bswaps(p, size) \
>>> @@ -176,6 +192,7 @@ CPU_CONVERT(le, 64, uint64_t)
>>>     * size is:
>>>     *   b: 8 bits
>>>     *   w: 16 bits
>>> + *   24: 24 bits
>>>     *   l: 32 bits
>>>     *   q: 64 bits
>>>     *
>>> @@ -248,6 +265,11 @@ static inline void stw_he_p(void *ptr, uint16_t v)
>>>        __builtin_memcpy(ptr, &v, sizeof(v));
>>>    }
>>>    
>>> +static inline void st24_he_p(void *ptr, uint32_t v)
>>> +{
>>> +    __builtin_memcpy(ptr, &v, 3);
>>> +}
>>> +
>>>    static inline int ldl_he_p(const void *ptr)
>>>    {
>>>        int32_t r;
>>> @@ -297,6 +319,11 @@ static inline void stw_le_p(void *ptr, uint16_t v)
>>>        stw_he_p(ptr, le_bswap(v, 16));
>>>    }
>>>    
>>> +static inline void st24_le_p(void *ptr, uint32_t v)
>>> +{
>>> +    st24_he_p(ptr, le_bswap24(v));
>>> +}
>>> +
>>>    static inline void stl_le_p(void *ptr, uint32_t v)
>>>    {
>>>        stl_he_p(ptr, le_bswap(v, 32));
>>
>> Conditional to removing the assertion in bswap24():
>> Reviewed-by: Philippe Mathieu-Daudé <[e-mail]>
>>
>

Peter Maydell May 20, 2023, 12:37 p.m. UTC | #4

On Fri, 19 May 2023 at 15:19, Jonathan Cameron via
<[e-mail]> wrote:
>
> From: Ira Weiny <[e-mail]>
>
> CXL has 24 bit unaligned fields which need to be stored to.  CXL is
> specified as little endian.
>
> Define st24_le_p() and the supporting functions to store such a field
> from a 32 bit host native value.
>
> The use of b, w, l, q as the size specifier is limiting.  So "24" was
> used for the size part of the function name.
>
> Signed-off-by: Ira Weiny <[e-mail]>
> Signed-off-by: Jonathan Cameron <[e-mail]>
> ---
>  docs/devel/loads-stores.rst |  1 +
>  include/qemu/bswap.h        | 27 +++++++++++++++++++++++++++
>  2 files changed, 28 insertions(+)
>
> diff --git a/docs/devel/loads-stores.rst b/docs/devel/loads-stores.rst
> index d2cefc77a2..82a79e91d9 100644
> --- a/docs/devel/loads-stores.rst
> +++ b/docs/devel/loads-stores.rst
> @@ -36,6 +36,7 @@ store: ``st{size}_{endian}_p(ptr, val)``
>  ``size``
>   - ``b`` : 8 bits
>   - ``w`` : 16 bits
> + - ``24`` : 24 bits
>   - ``l`` : 32 bits
>   - ``q`` : 64 bits

Can you also update the "Regexes for git grep" section
below to account for the new size value, please?

thanks
-- PMM

BALATON Zoltan May 20, 2023, 1:15 p.m. UTC | #5

On Sat, 20 May 2023, Peter Maydell wrote:
> On Fri, 19 May 2023 at 15:19, Jonathan Cameron via
> <[e-mail]> wrote:
>>
>> From: Ira Weiny <[e-mail]>
>>
>> CXL has 24 bit unaligned fields which need to be stored to.  CXL is
>> specified as little endian.
>>
>> Define st24_le_p() and the supporting functions to store such a field
>> from a 32 bit host native value.
>>
>> The use of b, w, l, q as the size specifier is limiting.  So "24" was
>> used for the size part of the function name.

Maybe it's clearer to use 24 but if we want to keep these somewhat 
consistent how about using t for Triplet, Three-bytes or Twenty-four?

Regards,
BALATON Zoltan

>> Signed-off-by: Ira Weiny <[e-mail]>
>> Signed-off-by: Jonathan Cameron <[e-mail]>
>> ---
>>  docs/devel/loads-stores.rst |  1 +
>>  include/qemu/bswap.h        | 27 +++++++++++++++++++++++++++
>>  2 files changed, 28 insertions(+)
>>
>> diff --git a/docs/devel/loads-stores.rst b/docs/devel/loads-stores.rst
>> index d2cefc77a2..82a79e91d9 100644
>> --- a/docs/devel/loads-stores.rst
>> +++ b/docs/devel/loads-stores.rst
>> @@ -36,6 +36,7 @@ store: ``st{size}_{endian}_p(ptr, val)``
>>  ``size``
>>   - ``b`` : 8 bits
>>   - ``w`` : 16 bits
>> + - ``24`` : 24 bits
>>   - ``l`` : 32 bits
>>   - ``q`` : 64 bits
>
> Can you also update the "Regexes for git grep" section
> below to account for the new size value, please?
>
> thanks
> -- PMM
>
>

Richard Henderson May 20, 2023, 3:15 p.m. UTC | #6

On 5/20/23 06:15, BALATON Zoltan wrote:
> On Sat, 20 May 2023, Peter Maydell wrote:
>> On Fri, 19 May 2023 at 15:19, Jonathan Cameron via
>> <[e-mail]> wrote:
>>>
>>> From: Ira Weiny <[e-mail]>
>>>
>>> CXL has 24 bit unaligned fields which need to be stored to.  CXL is
>>> specified as little endian.
>>>
>>> Define st24_le_p() and the supporting functions to store such a field
>>> from a 32 bit host native value.
>>>
>>> The use of b, w, l, q as the size specifier is limiting.  So "24" was
>>> used for the size part of the function name.
> 
> Maybe it's clearer to use 24 but if we want to keep these somewhat consistent how about 
> using t for Triplet, Three-bytes or Twenty-four?

I think it's clearer to use '3'.
When I added 128-bit support I used cpu_ld16_mmu.

I think it would be clearer to not use letters anywhere, and to use units of bytes instead 
of units of bits (no one can store just a bit), but changing everything is a big job.


r~

Philippe Mathieu-Daudé May 20, 2023, 5:08 p.m. UTC | #7

On 20/5/23 17:15, Richard Henderson wrote:
> On 5/20/23 06:15, BALATON Zoltan wrote:
>> On Sat, 20 May 2023, Peter Maydell wrote:
>>> On Fri, 19 May 2023 at 15:19, Jonathan Cameron via
>>> <[e-mail]> wrote:
>>>>
>>>> From: Ira Weiny <[e-mail]>
>>>>
>>>> CXL has 24 bit unaligned fields which need to be stored to.  CXL is
>>>> specified as little endian.
>>>>
>>>> Define st24_le_p() and the supporting functions to store such a field
>>>> from a 32 bit host native value.
>>>>
>>>> The use of b, w, l, q as the size specifier is limiting.  So "24" was
>>>> used for the size part of the function name.
>>
>> Maybe it's clearer to use 24 but if we want to keep these somewhat 
>> consistent how about using t for Triplet, Three-bytes or Twenty-four?
> 
> I think it's clearer to use '3'.
> When I added 128-bit support I used cpu_ld16_mmu.

There is also ld8u / ld8s / st8.

> I think it would be clearer to not use letters anywhere, and to use 
> units of bytes instead of units of bits (no one can store just a bit), 
> but changing everything is a big job.

So:

ldub ->  ld1u,

lduw_le -> ld2u_le,

virtio_stl -> virtio_st4,

stq_be -> st8_be.

Right?

Also we have:

cpu_ld/st_*
virtio_ld/st_*
ld/st_*_phys
ld/st_*_pci_dma
address_space_ld/st

While mass-changing, we could use FOO_ld/st_BAR with FOO
for API and BAR for API variant (endian, mmuidx, ra, ...):

So:

ld/st_*_pci_dma -> pci_dma_ld/st_*

for ld/st_*_phys I'm not sure.

Michael S. Tsirkin May 21, 2023, 9:05 a.m. UTC | #8

On Sat, May 20, 2023 at 07:08:22PM +0200, Philippe Mathieu-Daudé wrote:
> On 20/5/23 17:15, Richard Henderson wrote:
> > On 5/20/23 06:15, BALATON Zoltan wrote:
> > > On Sat, 20 May 2023, Peter Maydell wrote:
> > > > On Fri, 19 May 2023 at 15:19, Jonathan Cameron via
> > > > <[e-mail]> wrote:
> > > > > 
> > > > > From: Ira Weiny <[e-mail]>
> > > > > 
> > > > > CXL has 24 bit unaligned fields which need to be stored to.  CXL is
> > > > > specified as little endian.
> > > > > 
> > > > > Define st24_le_p() and the supporting functions to store such a field
> > > > > from a 32 bit host native value.
> > > > > 
> > > > > The use of b, w, l, q as the size specifier is limiting.  So "24" was
> > > > > used for the size part of the function name.
> > > 
> > > Maybe it's clearer to use 24 but if we want to keep these somewhat
> > > consistent how about using t for Triplet, Three-bytes or
> > > Twenty-four?
> > 
> > I think it's clearer to use '3'.
> > When I added 128-bit support I used cpu_ld16_mmu.
> 
> There is also ld8u / ld8s / st8.
> 
> > I think it would be clearer to not use letters anywhere, and to use
> > units of bytes instead of units of bits (no one can store just a bit),
> > but changing everything is a big job.
> 
> So:
> 
> ldub ->  ld1u,
> 
> lduw_le -> ld2u_le,
> 
> virtio_stl -> virtio_st4,
> 
> stq_be -> st8_be.
> 
> Right?


No, using bits is so ingrained by now, that people will think
st8 is a single byte.

And yes, you can store a bit - you have to read modify write but
hey.

> Also we have:
> 
> cpu_ld/st_*
> virtio_ld/st_*
> ld/st_*_phys
> ld/st_*_pci_dma
> address_space_ld/st
> 
> While mass-changing, we could use FOO_ld/st_BAR with FOO
> for API and BAR for API variant (endian, mmuidx, ra, ...):
> 
> So:
> 
> ld/st_*_pci_dma -> pci_dma_ld/st_*
> 
> for ld/st_*_phys I'm not sure.

Jonathan Cameron May 22, 2023, 11:59 a.m. UTC | #9

On Sat, 20 May 2023 13:37:42 +0100
Peter Maydell <[e-mail]> wrote:

> On Fri, 19 May 2023 at 15:19, Jonathan Cameron via
> <[e-mail]> wrote:
> >
> > From: Ira Weiny <[e-mail]>
> >
> > CXL has 24 bit unaligned fields which need to be stored to.  CXL is
> > specified as little endian.
> >
> > Define st24_le_p() and the supporting functions to store such a field
> > from a 32 bit host native value.
> >
> > The use of b, w, l, q as the size specifier is limiting.  So "24" was
> > used for the size part of the function name.
> >
> > Signed-off-by: Ira Weiny <[e-mail]>
> > Signed-off-by: Jonathan Cameron <[e-mail]>
> > ---
> >  docs/devel/loads-stores.rst |  1 +
> >  include/qemu/bswap.h        | 27 +++++++++++++++++++++++++++
> >  2 files changed, 28 insertions(+)
> >
> > diff --git a/docs/devel/loads-stores.rst b/docs/devel/loads-stores.rst
> > index d2cefc77a2..82a79e91d9 100644
> > --- a/docs/devel/loads-stores.rst
> > +++ b/docs/devel/loads-stores.rst
> > @@ -36,6 +36,7 @@ store: ``st{size}_{endian}_p(ptr, val)``
> >  ``size``
> >   - ``b`` : 8 bits
> >   - ``w`` : 16 bits
> > + - ``24`` : 24 bits
> >   - ``l`` : 32 bits
> >   - ``q`` : 64 bits  
> 
> Can you also update the "Regexes for git grep" section
> below to account for the new size value, please?

Ok. My regex isn't great, but I think this would require either some
separate entries or a switch to git grep -E to allow for the multiple
character matching.

So I've added
 - ``\st24\(_[hbl]e\)\?_p\>``




> 
> thanks
> -- PMM

Jonathan Cameron May 22, 2023, 12:09 p.m. UTC | #10

On Sat, 20 May 2023 19:08:22 +0200
Philippe Mathieu-Daudé <[e-mail]> wrote:

> On 20/5/23 17:15, Richard Henderson wrote:
> > On 5/20/23 06:15, BALATON Zoltan wrote:  
> >> On Sat, 20 May 2023, Peter Maydell wrote:  
> >>> On Fri, 19 May 2023 at 15:19, Jonathan Cameron via
> >>> <[e-mail]> wrote:  
> >>>>
> >>>> From: Ira Weiny <[e-mail]>
> >>>>
> >>>> CXL has 24 bit unaligned fields which need to be stored to.  CXL is
> >>>> specified as little endian.
> >>>>
> >>>> Define st24_le_p() and the supporting functions to store such a field
> >>>> from a 32 bit host native value.
> >>>>
> >>>> The use of b, w, l, q as the size specifier is limiting.  So "24" was
> >>>> used for the size part of the function name.  
> >>
> >> Maybe it's clearer to use 24 but if we want to keep these somewhat 
> >> consistent how about using t for Triplet, Three-bytes or Twenty-four?  
> > 
> > I think it's clearer to use '3'.
> > When I added 128-bit support I used cpu_ld16_mmu.  

As an aside on that - you didn't update the docs when you added that
(I was looking for it to copy your regex ;)

> 
> There is also ld8u / ld8s / st8.
> 
> > I think it would be clearer to not use letters anywhere, and to use 
> > units of bytes instead of units of bits (no one can store just a bit), 
> > but changing everything is a big job.  
> 
> So:
> 
> ldub ->  ld1u,
> 
> lduw_le -> ld2u_le,
> 
> virtio_stl -> virtio_st4,
> 
> stq_be -> st8_be.
> 
> Right?
> 
> Also we have:
> 
> cpu_ld/st_*
> virtio_ld/st_*
> ld/st_*_phys
> ld/st_*_pci_dma
> address_space_ld/st
> 
> While mass-changing, we could use FOO_ld/st_BAR with FOO
> for API and BAR for API variant (endian, mmuidx, ra, ...):
> 
> So:
> 
> ld/st_*_pci_dma -> pci_dma_ld/st_*
> 
> for ld/st_*_phys I'm not sure.

Patch

diff --git a/docs/devel/loads-stores.rst b/docs/devel/loads-stores.rst
index d2cefc77a2..82a79e91d9 100644
--- a/docs/devel/loads-stores.rst
+++ b/docs/devel/loads-stores.rst
@@ -36,6 +36,7 @@  store: ``st{size}_{endian}_p(ptr, val)``
 ``size``
  - ``b`` : 8 bits
  - ``w`` : 16 bits
+ - ``24`` : 24 bits
  - ``l`` : 32 bits
  - ``q`` : 64 bits
 
diff --git a/include/qemu/bswap.h b/include/qemu/bswap.h
index 15a78c0db5..f546b1fc06 100644
--- a/include/qemu/bswap.h
+++ b/include/qemu/bswap.h
@@ -8,11 +8,25 @@ 
 #undef  bswap64
 #define bswap64(_x) __builtin_bswap64(_x)
 
+static inline uint32_t bswap24(uint32_t x)
+{
+    assert((x & 0xff000000U) == 0);
+
+    return (((x & 0x000000ffU) << 16) |
+            ((x & 0x0000ff00U) <<  0) |
+            ((x & 0x00ff0000U) >> 16));
+}
+
 static inline void bswap16s(uint16_t *s)
 {
     *s = __builtin_bswap16(*s);
 }
 
+static inline void bswap24s(uint32_t *s)
+{
+    *s = bswap24(*s & 0x00ffffffU);
+}
+
 static inline void bswap32s(uint32_t *s)
 {
     *s = __builtin_bswap32(*s);
@@ -26,11 +40,13 @@  static inline void bswap64s(uint64_t *s)
 #if HOST_BIG_ENDIAN
 #define be_bswap(v, size) (v)
 #define le_bswap(v, size) glue(__builtin_bswap, size)(v)
+#define le_bswap24(v) bswap24(v)
 #define be_bswaps(v, size)
 #define le_bswaps(p, size) \
             do { *p = glue(__builtin_bswap, size)(*p); } while (0)
 #else
 #define le_bswap(v, size) (v)
+#define le_bswap24(v) (v)
 #define be_bswap(v, size) glue(__builtin_bswap, size)(v)
 #define le_bswaps(v, size)
 #define be_bswaps(p, size) \
@@ -176,6 +192,7 @@  CPU_CONVERT(le, 64, uint64_t)
  * size is:
  *   b: 8 bits
  *   w: 16 bits
+ *   24: 24 bits
  *   l: 32 bits
  *   q: 64 bits
  *
@@ -248,6 +265,11 @@  static inline void stw_he_p(void *ptr, uint16_t v)
     __builtin_memcpy(ptr, &v, sizeof(v));
 }
 
+static inline void st24_he_p(void *ptr, uint32_t v)
+{
+    __builtin_memcpy(ptr, &v, 3);
+}
+
 static inline int ldl_he_p(const void *ptr)
 {
     int32_t r;
@@ -297,6 +319,11 @@  static inline void stw_le_p(void *ptr, uint16_t v)
     stw_he_p(ptr, le_bswap(v, 16));
 }
 
+static inline void st24_le_p(void *ptr, uint32_t v)
+{
+    st24_he_p(ptr, le_bswap24(v));
+}
+
 static inline void stl_le_p(void *ptr, uint32_t v)
 {
     stl_he_p(ptr, le_bswap(v, 32));