Re: [PATCH v3 01/42] target/arm: Split s2walk_secure from ipa_secure in get_phys_addr

[Peter Maydell]

On Thu, 6 Oct 2022 at 16:10, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> On 10/6/22 07:27, Peter Maydell wrote:
> > On Sat, 1 Oct 2022 at 17:24, Richard Henderson
> > <richard.henderson@linaro.org> wrote:
> >>
> >> The starting security state comes with the translation regime,
> >> not the current state of arm_is_secure_below_el3().
> >>
> >> Create a new local variable, s2walk_secure, which does not need
> >> to be written back to result->attrs.secure -- we compute that
> >> value later, after the S2 walk is complete.
> >>
> >> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> >> ---
> >> v3: Do not modify ipa_secure, per review.
> >> ---
> >
> > Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
> >
> > I did find myself wondering if we should explicitly set
> >    result->attrs.secure = false;
> > in an else-branch of the last "if (is_secure)", though.
> > At the moment we rely on get_phys_addr_lpae() for the stage2
> > doing that for us, I think. Having the code here always set
> > result->attrs.secure before the 'return 0' avoids having to think
> > about that...
>
> Yes, we're currently (and predating this patch set) relying on the attrs 
> structure being
> cleared to start.  But I can certainly add the assignment if you like.

Yeah, cleared-at-start is fine. But here we're also relying on
the stage 2 call to get_phys_addr_lpae() not setting it to 1,
because we pass that the same 'result' pointer, not a fresh one.

-- PMM