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

[Peter Maydell]

On Tue, 5 May 2020 at 15:04, Richard Henderson
<address@hidden> wrote:
>
> On 5/5/20 2:49 AM, Peter Maydell wrote:
> > On Mon, 4 May 2020 at 17:03, Richard Henderson
> > <address@hidden> 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