[RFC PATCH 00/25] exec: Add load/store API for aligned pointers

Philippe Mathieu-Daudé posted 25 patches 2 years, 11 months ago
Test checkpatch failed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20210518183655.1711377-1-philmd@redhat.com
docs/devel/loads-stores.rst           |  27 +--
include/exec/memory.h                 |  18 +-
include/hw/virtio/virtio-access.h     | 239 +++++++++-----------------
include/qemu/bswap.h                  | 149 ++++++----------
include/exec/memory_ldst.h.inc        |  16 +-
include/exec/memory_ldst_cached.h.inc |  66 ++++---
include/exec/memory_ldst_phys.h.inc   |  72 ++++----
hw/virtio/virtio.c                    |  13 +-
memory_ldst.c.inc                     |  20 +--
9 files changed, 270 insertions(+), 350 deletions(-)
[RFC PATCH 00/25] exec: Add load/store API for aligned pointers
Posted by Philippe Mathieu-Daudé 2 years, 11 months ago
Hi,

This series is the result of a chat with Stefan after looking at
Peter response to Bibo on a problem with unoptimized memcpy()
leading to atomic issues:
https://www.mail-archive.com/qemu-devel@nongnu.org/msg808209.html

MIPS R6 (32/64-bit) does support unaligned accesses, but for the
older releases the compiler can't optimize the __builtin_memcpy().

When a pointer is known to be aligned, we can skip the checks for
misalignment and directly access the data. The pair of virtio vring
fields happen to be aligned.

To be able to call the aligned functions from the virtio layer, we
have to fill the gap in multiple API layers.

The series is decomposed as:
- cleanups (1-6)
- clean ldst API using macros (7-13)
- add aligned ldst methods (14)
- add aligned memory methods (15-16)
- similar changes in virtio (17-24)
- use the new methods on vring aligned values (25)

There are some checkpatch errors related to the macro used.

Regards,

Phil.

Philippe Mathieu-Daudé (25):
  exec/memory_ldst_cached: Sort declarations
  exec/memory_ldst_phys: Sort declarations
  exec/memory_ldst: Use correct type sizes
  exec/memory_ldst_phys: Use correct type sizes
  exec/memory_ldst_cached: Use correct type size
  exec/memory: Use correct type size
  qemu/bswap: Introduce ST_CONVERT() macro
  qemu/bswap: Use ST_CONVERT() macro to emit 16-bit load/store functions
  qemu/bswap: Introduce LD_CONVERT() macro
  qemu/bswap: Use LD_CONVERT macro to emit 16-bit signed load/store code
  qemu/bswap: Use LD_CONVERT macro to emit 16-bit unsigned load/store
    code
  qemu/bswap: Use LDST_CONVERT macro to emit 32-bit load/store functions
  qemu/bswap: Use LDST_CONVERT macro to emit 64-bit load/store functions
  qemu/bswap: Introduce load/store for aligned pointer
  exec/memory: Add methods for aligned pointer access (address space)
  exec/memory: Add methods for aligned pointer access (physical memory)
  hw/virtio: Use correct type sizes
  hw/virtio: Introduce VIRTIO_LD_CONVERT() macro
  hw/virtio: Use LD_CONVERT macro to emit 16-bit unsigned load/store
    code
  hw/virtio: Introduce VIRTIO_ST_CONVERT() macro
  hw/virtio: Use ST_CONVERT() macro to emit 16-bit load/store functions
  hw/virtio: Use LDST_CONVERT macro to emit 32-bit load/store functions
  hw/virtio: Use LDST_CONVERT macro to emit 64-bit load/store functions
  hw/virtio: Add methods for aligned pointer access
  hw/virtio: Optimize accesses on vring aligned pointers

 docs/devel/loads-stores.rst           |  27 +--
 include/exec/memory.h                 |  18 +-
 include/hw/virtio/virtio-access.h     | 239 +++++++++-----------------
 include/qemu/bswap.h                  | 149 ++++++----------
 include/exec/memory_ldst.h.inc        |  16 +-
 include/exec/memory_ldst_cached.h.inc |  66 ++++---
 include/exec/memory_ldst_phys.h.inc   |  72 ++++----
 hw/virtio/virtio.c                    |  13 +-
 memory_ldst.c.inc                     |  20 +--
 9 files changed, 270 insertions(+), 350 deletions(-)

-- 
2.26.3


Re: [RFC PATCH 00/25] exec: Add load/store API for aligned pointers
Posted by no-reply@patchew.org 2 years, 11 months ago
Patchew URL: https://patchew.org/QEMU/20210518183655.1711377-1-philmd@redhat.com/



Hi,

This series seems to have some coding style problems. See output below for
more information:

Type: series
Message-id: 20210518183655.1711377-1-philmd@redhat.com
Subject: [RFC PATCH 00/25] exec: Add load/store API for aligned pointers

=== TEST SCRIPT BEGIN ===
#!/bin/bash
git rev-parse base > /dev/null || exit 0
git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram
./scripts/checkpatch.pl --mailback base..
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
From https://github.com/patchew-project/qemu
 * [new tag]         patchew/20210518183655.1711377-1-philmd@redhat.com -> patchew/20210518183655.1711377-1-philmd@redhat.com
Switched to a new branch 'test'
b51497a hw/virtio: Optimize accesses on vring aligned pointers
f7d21cb hw/virtio: Add methods for aligned pointer access
3ad61c3 hw/virtio: Use LDST_CONVERT macro to emit 64-bit load/store functions
c03a99d hw/virtio: Use LDST_CONVERT macro to emit 32-bit load/store functions
5f7cb6e hw/virtio: Use ST_CONVERT() macro to emit 16-bit load/store functions
d95d346 hw/virtio: Introduce VIRTIO_ST_CONVERT() macro
fed8880 hw/virtio: Use LD_CONVERT macro to emit 16-bit unsigned load/store code
cfffb55 hw/virtio: Introduce VIRTIO_LD_CONVERT() macro
0c89b29 hw/virtio: Use correct type sizes
66d8c15 exec/memory: Add methods for aligned pointer access (physical memory)
9a169b5 exec/memory: Add methods for aligned pointer access (address space)
493016c qemu/bswap: Introduce load/store for aligned pointer
5cfe6c7 qemu/bswap: Use LDST_CONVERT macro to emit 64-bit load/store functions
4256f3f qemu/bswap: Use LDST_CONVERT macro to emit 32-bit load/store functions
5e69992 qemu/bswap: Use LD_CONVERT macro to emit 16-bit unsigned load/store code
0fa8f4d qemu/bswap: Use LD_CONVERT macro to emit 16-bit signed load/store code
2a6c3d1 qemu/bswap: Introduce LD_CONVERT() macro
e7b6792 qemu/bswap: Use ST_CONVERT() macro to emit 16-bit load/store functions
0d0c3e4 qemu/bswap: Introduce ST_CONVERT() macro
1ca0493 exec/memory: Use correct type size
3010882 exec/memory_ldst_cached: Use correct type size
0beaa26 exec/memory_ldst_phys: Use correct type sizes
88d6831 exec/memory_ldst: Use correct type sizes
057db27 exec/memory_ldst_phys: Sort declarations
3a92956 exec/memory_ldst_cached: Sort declarations

=== OUTPUT BEGIN ===
1/25 Checking commit 3a9295699cb2 (exec/memory_ldst_cached: Sort declarations)
2/25 Checking commit 057db27b4129 (exec/memory_ldst_phys: Sort declarations)
3/25 Checking commit 88d6831d2688 (exec/memory_ldst: Use correct type sizes)
4/25 Checking commit 0beaa26979fa (exec/memory_ldst_phys: Use correct type sizes)
5/25 Checking commit 3010882b5604 (exec/memory_ldst_cached: Use correct type size)
6/25 Checking commit 1ca0493ecfd1 (exec/memory: Use correct type size)
7/25 Checking commit 0d0c3e4e5c2d (qemu/bswap: Introduce ST_CONVERT() macro)
8/25 Checking commit e7b679286247 (qemu/bswap: Use ST_CONVERT() macro to emit 16-bit load/store functions)
9/25 Checking commit 2a6c3d1c4aad (qemu/bswap: Introduce LD_CONVERT() macro)
10/25 Checking commit 0fa8f4d7d229 (qemu/bswap: Use LD_CONVERT macro to emit 16-bit signed load/store code)
11/25 Checking commit 5e69992e047b (qemu/bswap: Use LD_CONVERT macro to emit 16-bit unsigned load/store code)
12/25 Checking commit 4256f3f7f36a (qemu/bswap: Use LDST_CONVERT macro to emit 32-bit load/store functions)
13/25 Checking commit 5cfe6c7a8fd6 (qemu/bswap: Use LDST_CONVERT macro to emit 64-bit load/store functions)
14/25 Checking commit 493016c0c84e (qemu/bswap: Introduce load/store for aligned pointer)
ERROR: space required after that close brace '}'
#119: FILE: include/qemu/bswap.h:369:
+}\

ERROR: space required after that close brace '}'
#129: FILE: include/qemu/bswap.h:379:
+}\

total: 2 errors, 0 warnings, 107 lines checked

Patch 14/25 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

15/25 Checking commit 9a169b58ddf3 (exec/memory: Add methods for aligned pointer access (address space))
16/25 Checking commit 66d8c1562136 (exec/memory: Add methods for aligned pointer access (physical memory))
17/25 Checking commit 0c89b296d850 (hw/virtio: Use correct type sizes)
18/25 Checking commit cfffb555f1ca (hw/virtio: Introduce VIRTIO_LD_CONVERT() macro)
ERROR: space required after that close brace '}'
#31: FILE: include/hw/virtio/virtio-access.h:49:
+    }\

ERROR: space required after that close brace '}'
#33: FILE: include/hw/virtio/virtio-access.h:51:
+}\

ERROR: space required after that close brace '}'
#41: FILE: include/hw/virtio/virtio-access.h:59:
+    }\

ERROR: space required after that close brace '}'
#42: FILE: include/hw/virtio/virtio-access.h:60:
+}\

ERROR: space required after that close brace '}'
#49: FILE: include/hw/virtio/virtio-access.h:67:
+    }\

total: 5 errors, 0 warnings, 35 lines checked

Patch 18/25 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

19/25 Checking commit fed888096371 (hw/virtio: Use LD_CONVERT macro to emit 16-bit unsigned load/store code)
20/25 Checking commit d95d346351f6 (hw/virtio: Introduce VIRTIO_ST_CONVERT() macro)
ERROR: space required after that close brace '}'
#32: FILE: include/hw/virtio/virtio-access.h:79:
+    }\

ERROR: space required after that close brace '}'
#33: FILE: include/hw/virtio/virtio-access.h:80:
+}\

ERROR: space required after that close brace '}'
#43: FILE: include/hw/virtio/virtio-access.h:90:
+    }\

ERROR: space required after that close brace '}'
#44: FILE: include/hw/virtio/virtio-access.h:91:
+}\

ERROR: space required after that close brace '}'
#53: FILE: include/hw/virtio/virtio-access.h:100:
+    }\

total: 5 errors, 0 warnings, 38 lines checked

Patch 20/25 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

21/25 Checking commit 5f7cb6e5bf21 (hw/virtio: Use ST_CONVERT() macro to emit 16-bit load/store functions)
22/25 Checking commit c03a99d5100a (hw/virtio: Use LDST_CONVERT macro to emit 32-bit load/store functions)
23/25 Checking commit 3ad61c32db83 (hw/virtio: Use LDST_CONVERT macro to emit 64-bit load/store functions)
24/25 Checking commit f7d21cb701d1 (hw/virtio: Add methods for aligned pointer access)
ERROR: space required after that close brace '}'
#22: FILE: include/hw/virtio/virtio-access.h:69:
+}\

ERROR: space required after that close brace '}'
#30: FILE: include/hw/virtio/virtio-access.h:77:
+    }\

ERROR: space required after that close brace '}'
#39: FILE: include/hw/virtio/virtio-access.h:111:
+}\

ERROR: space required after that close brace '}'
#49: FILE: include/hw/virtio/virtio-access.h:121:
+    }\

total: 4 errors, 0 warnings, 33 lines checked

Patch 24/25 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

25/25 Checking commit b51497a1e1f3 (hw/virtio: Optimize accesses on vring aligned pointers)
=== OUTPUT END ===

Test command exited with code: 1


The full log is available at
http://patchew.org/logs/20210518183655.1711377-1-philmd@redhat.com/testing.checkpatch/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com
Re: [RFC PATCH 00/25] exec: Add load/store API for aligned pointers
Posted by Richard Henderson 2 years, 11 months ago
On 5/18/21 1:36 PM, Philippe Mathieu-Daudé wrote:
> The series is decomposed as:
> - cleanups (1-6)
> - clean ldst API using macros (7-13)
> - add aligned ldst methods (14)
> - add aligned memory methods (15-16)
> - similar changes in virtio (17-24)
> - use the new methods on vring aligned values (25)
> 
> There are some checkpatch errors related to the macro used.

I think we should emphasize the atomicness of the access as opposed to the 
alignedness.  That's the only thing that's important to virtio.

Thus s/aligned/atomic/g and use qatomic_read/qatomic_set.


r~

Re: [RFC PATCH 00/25] exec: Add load/store API for aligned pointers
Posted by Philippe Mathieu-Daudé 2 years, 11 months ago
On 5/19/21 9:20 PM, Richard Henderson wrote:
> On 5/18/21 1:36 PM, Philippe Mathieu-Daudé wrote:
>> The series is decomposed as:
>> - cleanups (1-6)
>> - clean ldst API using macros (7-13)
>> - add aligned ldst methods (14)
>> - add aligned memory methods (15-16)
>> - similar changes in virtio (17-24)
>> - use the new methods on vring aligned values (25)
>>
>> There are some checkpatch errors related to the macro used.
> 
> I think we should emphasize the atomicness of the access as opposed to
> the alignedness.  That's the only thing that's important to virtio.
> 
> Thus s/aligned/atomic/g and use qatomic_read/qatomic_set.

OK.

Do you think patches 1-6, 17 (Use correct type sizes) could
go in meanwhile, or should I repost them separately?


Re: [RFC PATCH 00/25] exec: Add load/store API for aligned pointers
Posted by Richard Henderson 2 years, 11 months ago
On 5/19/21 2:40 PM, Philippe Mathieu-Daudé wrote:
> On 5/19/21 9:20 PM, Richard Henderson wrote:
>> On 5/18/21 1:36 PM, Philippe Mathieu-Daudé wrote:
>>> The series is decomposed as:
>>> - cleanups (1-6)
>>> - clean ldst API using macros (7-13)
>>> - add aligned ldst methods (14)
>>> - add aligned memory methods (15-16)
>>> - similar changes in virtio (17-24)
>>> - use the new methods on vring aligned values (25)
>>>
>>> There are some checkpatch errors related to the macro used.
>>
>> I think we should emphasize the atomicness of the access as opposed to
>> the alignedness.  That's the only thing that's important to virtio.
>>
>> Thus s/aligned/atomic/g and use qatomic_read/qatomic_set.
> 
> OK.
> 
> Do you think patches 1-6, 17 (Use correct type sizes) could
> go in meanwhile, or should I repost them separately?
> 

I think 1-6 can be queued; I'm had a question about 17.


r~