[PATCH v4 00/18] target/arm: sve load/store improvements

Richard Henderson posted 18 patches 4 years ago
Test asan passed
Test docker-mingw@fedora passed
Test checkpatch failed
Test docker-quick@centos7 passed
Test FreeBSD passed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20200430162813.17671-1-richard.henderson@linaro.org
Maintainers: Richard Henderson <rth@twiddle.net>, Eduardo Habkost <ehabkost@redhat.com>, Peter Maydell <peter.maydell@linaro.org>, Paolo Bonzini <pbonzini@redhat.com>, Riku Voipio <riku.voipio@iki.fi>, Marcel Apfelbaum <marcel.apfelbaum@gmail.com>
There is a newer version of this series
docs/devel/loads-stores.rst |   39 +-
include/exec/cpu-all.h      |   13 +-
include/exec/cpu_ldst.h     |  283 +++--
include/exec/exec-all.h     |   39 +
include/hw/core/cpu.h       |   23 +
target/arm/internals.h      |    5 -
accel/tcg/cputlb.c          |  413 ++++---
accel/tcg/user-exec.c       |  256 +++-
exec.c                      |    2 +-
target/arm/sve_helper.c     | 2241 +++++++++++++++++++----------------
target/arm/translate-sve.c  |   17 +-
11 files changed, 1999 insertions(+), 1332 deletions(-)
[PATCH v4 00/18] target/arm: sve load/store improvements
Posted by Richard Henderson 4 years ago
The goal here is to support MTE, but there's some cleanup to do.

Technically, we have sufficient interfaces in cputlb.c now, but it
requires multiple tlb lookups on different interfaces to do so.

Adding probe_access_flags() allows probing the tlb and getting out
some of the flags buried in the tlb comparator, such as TLB_MMIO
and TLB_WATCHPOINT.  In addition, we get no-fault semantics,
which we don't have via probe_acccess().

Looking forward to MTE, we can examine the Tagged bit on a per-page
basis and avoid dozens of mte_check calls that must be Unchecked.
That comes later, in a new version of the MTE patch set, but I do
add comments for where the checks should be added.

Version 4 addresses some review comments.
Only 2 patches still unreviewed:

0004-accel-tcg-Add-probe_access_flags.patch
0013-target-arm-Update-contiguous-first-fault-and-no-f.patch


r~


Richard Henderson (18):
  exec: Add block comments for watchpoint routines
  exec: Fix cpu_watchpoint_address_matches address length
  accel/tcg: Add block comment for probe_access
  accel/tcg: Add probe_access_flags
  accel/tcg: Add endian-specific cpu_{ld,st}* operations
  target/arm: Use cpu_*_data_ra for sve_ldst_tlb_fn
  target/arm: Drop manual handling of set/clear_helper_retaddr
  target/arm: Add sve infrastructure for page lookup
  target/arm: Adjust interface of sve_ld1_host_fn
  target/arm: Use SVEContLdSt in sve_ld1_r
  target/arm: Handle watchpoints in sve_ld1_r
  target/arm: Use SVEContLdSt for multi-register contiguous loads
  target/arm: Update contiguous first-fault and no-fault loads
  target/arm: Use SVEContLdSt for contiguous stores
  target/arm: Reuse sve_probe_page for gather first-fault loads
  target/arm: Reuse sve_probe_page for scatter stores
  target/arm: Reuse sve_probe_page for gather loads
  target/arm: Remove sve_memopidx

 docs/devel/loads-stores.rst |   39 +-
 include/exec/cpu-all.h      |   13 +-
 include/exec/cpu_ldst.h     |  283 +++--
 include/exec/exec-all.h     |   39 +
 include/hw/core/cpu.h       |   23 +
 target/arm/internals.h      |    5 -
 accel/tcg/cputlb.c          |  413 ++++---
 accel/tcg/user-exec.c       |  256 +++-
 exec.c                      |    2 +-
 target/arm/sve_helper.c     | 2241 +++++++++++++++++++----------------
 target/arm/translate-sve.c  |   17 +-
 11 files changed, 1999 insertions(+), 1332 deletions(-)

-- 
2.20.1


Re: [PATCH v4 00/18] target/arm: sve load/store improvements
Posted by no-reply@patchew.org 4 years ago
Patchew URL: https://patchew.org/QEMU/20200430162813.17671-1-richard.henderson@linaro.org/



Hi,

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

Message-id: 20200430162813.17671-1-richard.henderson@linaro.org
Subject: [PATCH v4 00/18] target/arm: sve load/store improvements
Type: series

=== 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
Switched to a new branch 'test'
82ee270 target/arm: Remove sve_memopidx
fab9c40 target/arm: Reuse sve_probe_page for gather loads
f70bb77 target/arm: Reuse sve_probe_page for scatter stores
4e596dc target/arm: Reuse sve_probe_page for gather first-fault loads
468a67f target/arm: Use SVEContLdSt for contiguous stores
9a0478c target/arm: Update contiguous first-fault and no-fault loads
918f6f4 target/arm: Use SVEContLdSt for multi-register contiguous loads
d9511b0 target/arm: Handle watchpoints in sve_ld1_r
c79af96 target/arm: Use SVEContLdSt in sve_ld1_r
e6c154a target/arm: Adjust interface of sve_ld1_host_fn
b4217ec target/arm: Add sve infrastructure for page lookup
3469d99 target/arm: Drop manual handling of set/clear_helper_retaddr
5910347 target/arm: Use cpu_*_data_ra for sve_ldst_tlb_fn
a0d4423 accel/tcg: Add endian-specific cpu_{ld, st}* operations
0b84297 accel/tcg: Add probe_access_flags
ac7b3fb accel/tcg: Add block comment for probe_access
00a9ec0 exec: Fix cpu_watchpoint_address_matches address length
10cd650 exec: Add block comments for watchpoint routines

=== OUTPUT BEGIN ===
1/18 Checking commit 10cd6507e74a (exec: Add block comments for watchpoint routines)
2/18 Checking commit 00a9ec0026c5 (exec: Fix cpu_watchpoint_address_matches address length)
3/18 Checking commit ac7b3fbdeb61 (accel/tcg: Add block comment for probe_access)
4/18 Checking commit 0b842976537b (accel/tcg: Add probe_access_flags)
5/18 Checking commit a0d4423f54d3 (accel/tcg: Add endian-specific cpu_{ld, st}* operations)
6/18 Checking commit 59103472222f (target/arm: Use cpu_*_data_ra for sve_ldst_tlb_fn)
ERROR: spaces required around that '*' (ctx:VxV)
#63: FILE: target/arm/sve_helper.c:4029:
+    TLB(env, addr, (TYPEM)*(TYPEE *)(vd + H(reg_off)), ra);                 \
                           ^

ERROR: spaces required around that '*' (ctx:WxV)
#153: FILE: target/arm/sve_helper.c:4162:
+                      sve_ldst1_tlb_fn *tlb_fn)
                                        ^

total: 2 errors, 0 warnings, 455 lines checked

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

7/18 Checking commit 3469d99d0d94 (target/arm: Drop manual handling of set/clear_helper_retaddr)
8/18 Checking commit b4217ec4779a (target/arm: Add sve infrastructure for page lookup)
WARNING: Block comments use a leading /* on a separate line
#32: FILE: target/arm/sve_helper.c:1633:
+/* Big-endian hosts need to frob the byte indices.  If the copy

total: 0 errors, 1 warnings, 281 lines checked

Patch 8/18 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
9/18 Checking commit e6c154a82e83 (target/arm: Adjust interface of sve_ld1_host_fn)
10/18 Checking commit c79af9613ec5 (target/arm: Use SVEContLdSt in sve_ld1_r)
11/18 Checking commit d9511b04ab6a (target/arm: Handle watchpoints in sve_ld1_r)
12/18 Checking commit 918f6f478a6f (target/arm: Use SVEContLdSt for multi-register contiguous loads)
13/18 Checking commit 9a0478c3a5e6 (target/arm: Update contiguous first-fault and no-fault loads)
14/18 Checking commit 468a67f2b0ff (target/arm: Use SVEContLdSt for contiguous stores)
15/18 Checking commit 4e596dc2ae1a (target/arm: Reuse sve_probe_page for gather first-fault loads)
16/18 Checking commit f70bb771ff66 (target/arm: Reuse sve_probe_page for scatter stores)
17/18 Checking commit fab9c404e5d8 (target/arm: Reuse sve_probe_page for gather loads)
18/18 Checking commit 82ee27018cd7 (target/arm: Remove sve_memopidx)
=== OUTPUT END ===

Test command exited with code: 1


The full log is available at
http://patchew.org/logs/20200430162813.17671-1-richard.henderson@linaro.org/testing.checkpatch/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com
Re: [PATCH v4 00/18] target/arm: sve load/store improvements
Posted by Peter Maydell 4 years ago
On Thu, 30 Apr 2020 at 17:28, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> The goal here is to support MTE, but there's some cleanup to do.
>
> Technically, we have sufficient interfaces in cputlb.c now, but it
> requires multiple tlb lookups on different interfaces to do so.
>
> Adding probe_access_flags() allows probing the tlb and getting out
> some of the flags buried in the tlb comparator, such as TLB_MMIO
> and TLB_WATCHPOINT.  In addition, we get no-fault semantics,
> which we don't have via probe_acccess().
>
> Looking forward to MTE, we can examine the Tagged bit on a per-page
> basis and avoid dozens of mte_check calls that must be Unchecked.
> That comes later, in a new version of the MTE patch set, but I do
> add comments for where the checks should be added.
>
> Version 4 addresses some review comments.
> Only 2 patches still unreviewed:
>
> 0004-accel-tcg-Add-probe_access_flags.patch
> 0013-target-arm-Update-contiguous-first-fault-and-no-f.patch

I've reviewed patch 13, but I still don't understand why you've
made the size-related changes in patch 4, so I've continued
our conversation in the thread on the v3 version of that patch.

thanks
-- PMM

Re: [PATCH v4 00/18] target/arm: sve load/store improvements
Posted by Richard Henderson 4 years ago
On 5/4/20 2:43 AM, Peter Maydell wrote:
> I've reviewed patch 13, but I still don't understand why you've
> made the size-related changes in patch 4, so I've continued
> our conversation in the thread on the v3 version of that patch.

I've changed that here in v4.  Please have another look at this one.


r~


Re: [PATCH v4 00/18] target/arm: sve load/store improvements
Posted by Peter Maydell 4 years ago
On Mon, 4 May 2020 at 17:03, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> On 5/4/20 2:43 AM, Peter Maydell wrote:
> > I've reviewed patch 13, but I still don't understand why you've
> > made the size-related changes in patch 4, so I've continued
> > our conversation in the thread on the v3 version of that patch.
>
> I've changed that here in v4.  Please have another look at this one.

The page_check_range() call still seems to be passing a fixed
size of '1' ?

thanks
-- PMM

Re: [PATCH v4 00/18] target/arm: sve load/store improvements
Posted by Richard Henderson 4 years ago
On 5/5/20 2:49 AM, Peter Maydell wrote:
> On Mon, 4 May 2020 at 17:03, Richard Henderson
> <richard.henderson@linaro.org> wrote:
>>
>> On 5/4/20 2:43 AM, Peter Maydell wrote:
>>> I've reviewed patch 13, but I still don't understand why you've
>>> made the size-related changes in patch 4, so I've continued
>>> our conversation in the thread on the v3 version of that patch.
>>
>> I've changed that here in v4.  Please have another look at this one.
> 
> The page_check_range() call still seems to be passing a fixed
> size of '1' ?

We only need to validate one page, so validating one byte on the page is
sufficient.  The size argument to page_check_range is so that it can validate
multiple pages at a time.

The target exception is raised by cc->tlb_fill, and there we pass either the
actual access size (probe_access) or 0 to indicate unknown access size
(probe_access_flags, or probe_access passed down from the caller).


r~

Re: [PATCH v4 00/18] target/arm: sve load/store improvements
Posted by Peter Maydell 4 years ago
On Tue, 5 May 2020 at 15:04, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> On 5/5/20 2:49 AM, Peter Maydell wrote:
> > On Mon, 4 May 2020 at 17:03, Richard Henderson
> > <richard.henderson@linaro.org> wrote:
> >>
> >> On 5/4/20 2:43 AM, Peter Maydell wrote:
> >>> I've reviewed patch 13, but I still don't understand why you've
> >>> made the size-related changes in patch 4, so I've continued
> >>> our conversation in the thread on the v3 version of that patch.
> >>
> >> I've changed that here in v4.  Please have another look at this one.
> >
> > The page_check_range() call still seems to be passing a fixed
> > size of '1' ?
>
> We only need to validate one page, so validating one byte on the page is
> sufficient.  The size argument to page_check_range is so that it can validate
> multiple pages at a time.

Yes, but why *change* what we're doing? If the patch doesn't
change details like this then I can review it by looking at
it as a code refactor. If it changes this sort of thing then
now I have to look into the details of what exactly page_check_range()
is doing and whether it's possible to get here for an access
that crosses a page boundary, and the review job gets harder.

If passing 1 rather than the actual size we have is the right
thing, then split that into its own patch with its own commit
message that can explain why it needs to be changed. If
it doesn't matter whether we pass 1 or size, then please don't
change it in a refactoring patch.

thanks
-- PMM