From: Peter Maydell <peter.maydell@linaro.org>
To: Richard Henderson <richard.henderson@linaro.org>
Cc: "Alex Bennée" <alex.bennee@linaro.org>,
"QEMU Developers" <qemu-devel@nongnu.org>
Subject: Re: [PATCH v4 00/18] target/arm: sve load/store improvements
Date: Tue, 5 May 2020 15:15:57 +0100 [thread overview]
Message-ID: <CAFEAcA8s-YcfTFDDh+ht5HYQUjg0cyPODXb5ereBikufjQViiA@mail.gmail.com> (raw)
In-Reply-To: <a55cb2a3-9018-2288-bfe9-2fa3c2bf0050@linaro.org>
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
prev parent reply other threads:[~2020-05-05 14:17 UTC|newest]
Thread overview: 26+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-04-30 16:27 [PATCH v4 00/18] target/arm: sve load/store improvements Richard Henderson
2020-04-30 16:27 ` [PATCH v4 01/18] exec: Add block comments for watchpoint routines Richard Henderson
2020-04-30 16:27 ` [PATCH v4 02/18] exec: Fix cpu_watchpoint_address_matches address length Richard Henderson
2020-04-30 16:27 ` [PATCH v4 03/18] accel/tcg: Add block comment for probe_access Richard Henderson
2020-04-30 16:27 ` [PATCH v4 04/18] accel/tcg: Add probe_access_flags Richard Henderson
2020-04-30 16:28 ` [PATCH v4 05/18] accel/tcg: Add endian-specific cpu_{ld, st}* operations Richard Henderson
2020-04-30 16:28 ` [PATCH v4 06/18] target/arm: Use cpu_*_data_ra for sve_ldst_tlb_fn Richard Henderson
2020-04-30 16:28 ` [PATCH v4 07/18] target/arm: Drop manual handling of set/clear_helper_retaddr Richard Henderson
2020-04-30 16:28 ` [PATCH v4 08/18] target/arm: Add sve infrastructure for page lookup Richard Henderson
2020-04-30 16:28 ` [PATCH v4 09/18] target/arm: Adjust interface of sve_ld1_host_fn Richard Henderson
2020-04-30 16:28 ` [PATCH v4 10/18] target/arm: Use SVEContLdSt in sve_ld1_r Richard Henderson
2020-04-30 16:28 ` [PATCH v4 11/18] target/arm: Handle watchpoints " Richard Henderson
2020-04-30 16:28 ` [PATCH v4 12/18] target/arm: Use SVEContLdSt for multi-register contiguous loads Richard Henderson
2020-04-30 16:28 ` [PATCH v4 13/18] target/arm: Update contiguous first-fault and no-fault loads Richard Henderson
2020-05-04 9:42 ` Peter Maydell
2020-04-30 16:28 ` [PATCH v4 14/18] target/arm: Use SVEContLdSt for contiguous stores Richard Henderson
2020-04-30 16:28 ` [PATCH v4 15/18] target/arm: Reuse sve_probe_page for gather first-fault loads Richard Henderson
2020-04-30 16:28 ` [PATCH v4 16/18] target/arm: Reuse sve_probe_page for scatter stores Richard Henderson
2020-04-30 16:28 ` [PATCH v4 17/18] target/arm: Reuse sve_probe_page for gather loads Richard Henderson
2020-04-30 16:28 ` [PATCH v4 18/18] target/arm: Remove sve_memopidx Richard Henderson
2020-05-01 6:18 ` [PATCH v4 00/18] target/arm: sve load/store improvements no-reply
2020-05-04 9:43 ` Peter Maydell
2020-05-04 16:03 ` Richard Henderson
2020-05-05 9:49 ` Peter Maydell
2020-05-05 14:04 ` Richard Henderson
2020-05-05 14:15 ` Peter Maydell [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=CAFEAcA8s-YcfTFDDh+ht5HYQUjg0cyPODXb5ereBikufjQViiA@mail.gmail.com \
--to=peter.maydell@linaro.org \
--cc=alex.bennee@linaro.org \
--cc=qemu-devel@nongnu.org \
--cc=richard.henderson@linaro.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).