| Message ID | 20241005200600.493604-15-richard.henderson@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | accel/tcg: Introduce tlb_fill_align hook | expand |
On 10/5/24 22:05, Richard Henderson wrote: > Zero is the safe do-nothing value for callers to use. > > Signed-off-by: Richard Henderson <richard.henderson@linaro.org> Reviewed-by: Helge Deller <deller@gmx.de> > --- > target/arm/internals.h | 3 ++- > target/arm/helper.c | 4 ++-- > target/arm/ptw.c | 2 +- > 3 files changed, 5 insertions(+), 4 deletions(-)
On Sat, 5 Oct 2024 at 21:06, Richard Henderson <richard.henderson@linaro.org> wrote: > > Zero is the safe do-nothing value for callers to use. > > Signed-off-by: Richard Henderson <richard.henderson@linaro.org> > --- > target/arm/internals.h | 3 ++- > target/arm/helper.c | 4 ++-- > target/arm/ptw.c | 2 +- > 3 files changed, 5 insertions(+), 4 deletions(-) > > diff --git a/target/arm/internals.h b/target/arm/internals.h > index 2b16579fa5..a6088d551c 100644 > --- a/target/arm/internals.h > +++ b/target/arm/internals.h > @@ -1461,6 +1461,7 @@ bool get_phys_addr(CPUARMState *env, vaddr address, > * @env: CPUARMState > * @address: virtual address to get physical address for > * @access_type: 0 for read, 1 for write, 2 for execute > + * @memop: memory operation feeding this access, or 0 for none > * @mmu_idx: MMU index indicating required translation regime > * @space: security space for the access > * @result: set on translation success. > @@ -1470,7 +1471,7 @@ bool get_phys_addr(CPUARMState *env, vaddr address, > * a Granule Protection Check on the resulting address. > */ > bool get_phys_addr_with_space_nogpc(CPUARMState *env, vaddr address, > - MMUAccessType access_type, > + MMUAccessType access_type, MemOp memop, > ARMMMUIdx mmu_idx, ARMSecuritySpace space, > GetPhysAddrResult *result, > ARMMMUFaultInfo *fi) > diff --git a/target/arm/helper.c b/target/arm/helper.c > index 3f77b40734..f2f329e00a 100644 > --- a/target/arm/helper.c > +++ b/target/arm/helper.c > @@ -3602,8 +3602,8 @@ static uint64_t do_ats_write(CPUARMState *env, uint64_t value, > * I_MXTJT: Granule protection checks are not performed on the final address > * of a successful translation. > */ > - ret = get_phys_addr_with_space_nogpc(env, value, access_type, mmu_idx, ss, > - &res, &fi); > + ret = get_phys_addr_with_space_nogpc(env, value, access_type, 0, > + mmu_idx, ss, &res, &fi); 0 is the correct thing here, because AT operations are effectively assuming an aligned address (cf AArch64.AT() setting "aligned = true" in the Arm ARM pseudocode). Is there a way to write this as something other than "0" as an indication of "we've definitely thought about this callsite and it's not just 0 for back-compat behaviour" ? Otherwise we could add a brief comment. Otherwise Reviewed-by: Peter Maydell <peter.maydell@linaro.org> thanks -- PMM
On 10/8/24 07:35, Peter Maydell wrote: >> - ret = get_phys_addr_with_space_nogpc(env, value, access_type, mmu_idx, ss, >> - &res, &fi); >> + ret = get_phys_addr_with_space_nogpc(env, value, access_type, 0, >> + mmu_idx, ss, &res, &fi); > > 0 is the correct thing here, because AT operations are effectively > assuming an aligned address (cf AArch64.AT() setting "aligned = true" > in the Arm ARM pseudocode). > > Is there a way to write this as something other than "0" as > an indication of "we've definitely thought about this callsite > and it's not just 0 for back-compat behaviour" ? Otherwise we > could add a brief comment. Nothing other than 0 is leaping to mind. The documentation for @memop includes "... or 0 for none". Perhaps /* This is a translation not a memory reference, so "memop = none = 0". */ r~
diff --git a/target/arm/internals.h b/target/arm/internals.h index 2b16579fa5..a6088d551c 100644 --- a/target/arm/internals.h +++ b/target/arm/internals.h @@ -1461,6 +1461,7 @@ bool get_phys_addr(CPUARMState *env, vaddr address, * @env: CPUARMState * @address: virtual address to get physical address for * @access_type: 0 for read, 1 for write, 2 for execute + * @memop: memory operation feeding this access, or 0 for none * @mmu_idx: MMU index indicating required translation regime * @space: security space for the access * @result: set on translation success. @@ -1470,7 +1471,7 @@ bool get_phys_addr(CPUARMState *env, vaddr address, * a Granule Protection Check on the resulting address. */ bool get_phys_addr_with_space_nogpc(CPUARMState *env, vaddr address, - MMUAccessType access_type, + MMUAccessType access_type, MemOp memop, ARMMMUIdx mmu_idx, ARMSecuritySpace space, GetPhysAddrResult *result, ARMMMUFaultInfo *fi) diff --git a/target/arm/helper.c b/target/arm/helper.c index 3f77b40734..f2f329e00a 100644 --- a/target/arm/helper.c +++ b/target/arm/helper.c @@ -3602,8 +3602,8 @@ static uint64_t do_ats_write(CPUARMState *env, uint64_t value, * I_MXTJT: Granule protection checks are not performed on the final address * of a successful translation. */ - ret = get_phys_addr_with_space_nogpc(env, value, access_type, mmu_idx, ss, - &res, &fi); + ret = get_phys_addr_with_space_nogpc(env, value, access_type, 0, + mmu_idx, ss, &res, &fi); /* * ATS operations only do S1 or S1+S2 translations, so we never diff --git a/target/arm/ptw.c b/target/arm/ptw.c index 373095a339..9af86da597 100644 --- a/target/arm/ptw.c +++ b/target/arm/ptw.c @@ -3559,7 +3559,7 @@ static bool get_phys_addr_gpc(CPUARMState *env, S1Translate *ptw, } bool get_phys_addr_with_space_nogpc(CPUARMState *env, vaddr address, - MMUAccessType access_type, + MMUAccessType access_type, MemOp memop, ARMMMUIdx mmu_idx, ARMSecuritySpace space, GetPhysAddrResult *result, ARMMMUFaultInfo *fi)
Zero is the safe do-nothing value for callers to use. Signed-off-by: Richard Henderson <richard.henderson@linaro.org> --- target/arm/internals.h | 3 ++- target/arm/helper.c | 4 ++-- target/arm/ptw.c | 2 +- 3 files changed, 5 insertions(+), 4 deletions(-)