| Message ID | 20230719153018.1456180-5-jean-philippe@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | target/arm: Fixes for RME | expand |
On Wed, 19 Jul 2023 at 16:56, Jean-Philippe Brucker <jean-philippe@linaro.org> wrote: > > GPC checks are not performed on the output address for AT instructions, > as stated by ARM DDI 0487J in D8.12.2: > > When populating PAR_EL1 with the result of an address translation > instruction, granule protection checks are not performed on the final > output address of a successful translation. > > Rename get_phys_addr_with_secure(), since it's only used to handle AT > instructions. > > Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org> > --- > This incidentally fixes a problem with AT S1E1 instructions which can > output an IPA and should definitely not cause a GPC. > --- > target/arm/internals.h | 25 ++++++++++++++----------- > target/arm/helper.c | 8 ++++++-- > target/arm/ptw.c | 11 ++++++----- > 3 files changed, 26 insertions(+), 18 deletions(-) > > diff --git a/target/arm/internals.h b/target/arm/internals.h > index 0f01bc32a8..fc90c364f7 100644 > --- a/target/arm/internals.h > +++ b/target/arm/internals.h > @@ -1190,12 +1190,11 @@ typedef struct GetPhysAddrResult { > } GetPhysAddrResult; > > /** > - * get_phys_addr_with_secure: get the physical address for a virtual address > + * get_phys_addr: get the physical address for a virtual address > * @env: CPUARMState > * @address: virtual address to get physical address for > * @access_type: 0 for read, 1 for write, 2 for execute > * @mmu_idx: MMU index indicating required translation regime > - * @is_secure: security state for the access > * @result: set on translation success. > * @fi: set to fault info if the translation fails > * > @@ -1212,26 +1211,30 @@ typedef struct GetPhysAddrResult { > * * for PSMAv5 based systems we don't bother to return a full FSR format > * value. > */ > -bool get_phys_addr_with_secure(CPUARMState *env, target_ulong address, > - MMUAccessType access_type, > - ARMMMUIdx mmu_idx, bool is_secure, > - GetPhysAddrResult *result, ARMMMUFaultInfo *fi) > +bool get_phys_addr(CPUARMState *env, target_ulong address, > + MMUAccessType access_type, ARMMMUIdx mmu_idx, > + GetPhysAddrResult *result, ARMMMUFaultInfo *fi) > __attribute__((nonnull)); What is going on in this bit of the patch ? We already have a prototype for get_phys_addr() with a doc comment. Is this git's diff-output producing something silly for a change that is not logically touching get_phys_addr()'s prototype and comment at all? > /** > - * get_phys_addr: get the physical address for a virtual address > + * get_phys_addr_with_secure_nogpc: get the physical address for a virtual > + * address > * @env: CPUARMState > * @address: virtual address to get physical address for > * @access_type: 0 for read, 1 for write, 2 for execute > * @mmu_idx: MMU index indicating required translation regime > + * @is_secure: security state for the access > * @result: set on translation success. > * @fi: set to fault info if the translation fails > * > - * Similarly, but use the security regime of @mmu_idx. > + * Similar to get_phys_addr, but use the given security regime and don't perform > + * a Granule Protection Check on the resulting address. > */ > -bool get_phys_addr(CPUARMState *env, target_ulong address, > - MMUAccessType access_type, ARMMMUIdx mmu_idx, > - GetPhysAddrResult *result, ARMMMUFaultInfo *fi) > +bool get_phys_addr_with_secure_nogpc(CPUARMState *env, target_ulong address, > + MMUAccessType access_type, > + ARMMMUIdx mmu_idx, bool is_secure, > + GetPhysAddrResult *result, > + ARMMMUFaultInfo *fi) > __attribute__((nonnull)); thanks -- PMM
On Thu, 20 Jul 2023 at 17:39, Peter Maydell <peter.maydell@linaro.org> wrote: > > On Wed, 19 Jul 2023 at 16:56, Jean-Philippe Brucker > <jean-philippe@linaro.org> wrote: > > > > GPC checks are not performed on the output address for AT instructions, > > as stated by ARM DDI 0487J in D8.12.2: > > > > When populating PAR_EL1 with the result of an address translation > > instruction, granule protection checks are not performed on the final > > output address of a successful translation. > > > > Rename get_phys_addr_with_secure(), since it's only used to handle AT > > instructions. > > > > Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org> > > --- > > This incidentally fixes a problem with AT S1E1 instructions which can > > output an IPA and should definitely not cause a GPC. > > --- > > target/arm/internals.h | 25 ++++++++++++++----------- > > target/arm/helper.c | 8 ++++++-- > > target/arm/ptw.c | 11 ++++++----- > > 3 files changed, 26 insertions(+), 18 deletions(-) > > > > diff --git a/target/arm/internals.h b/target/arm/internals.h > > index 0f01bc32a8..fc90c364f7 100644 > > --- a/target/arm/internals.h > > +++ b/target/arm/internals.h > > @@ -1190,12 +1190,11 @@ typedef struct GetPhysAddrResult { > > } GetPhysAddrResult; > > > > /** > > - * get_phys_addr_with_secure: get the physical address for a virtual address > > + * get_phys_addr: get the physical address for a virtual address > > * @env: CPUARMState > > * @address: virtual address to get physical address for > > * @access_type: 0 for read, 1 for write, 2 for execute > > * @mmu_idx: MMU index indicating required translation regime > > - * @is_secure: security state for the access > > * @result: set on translation success. > > * @fi: set to fault info if the translation fails > > * > > @@ -1212,26 +1211,30 @@ typedef struct GetPhysAddrResult { > > * * for PSMAv5 based systems we don't bother to return a full FSR format > > * value. > > */ > > -bool get_phys_addr_with_secure(CPUARMState *env, target_ulong address, > > - MMUAccessType access_type, > > - ARMMMUIdx mmu_idx, bool is_secure, > > - GetPhysAddrResult *result, ARMMMUFaultInfo *fi) > > +bool get_phys_addr(CPUARMState *env, target_ulong address, > > + MMUAccessType access_type, ARMMMUIdx mmu_idx, > > + GetPhysAddrResult *result, ARMMMUFaultInfo *fi) > > __attribute__((nonnull)); > > > What is going on in this bit of the patch ? We already > have a prototype for get_phys_addr() with a doc comment. > Is this git's diff-output producing something silly > for a change that is not logically touching get_phys_addr()'s > prototype and comment at all? Looking more closely, that does seem to be what's happened, so Reviewed-by: Peter Maydell <peter.maydell@linaro.org> thanks -- PMM
On Thu, Jul 20, 2023 at 05:39:56PM +0100, Peter Maydell wrote: > On Wed, 19 Jul 2023 at 16:56, Jean-Philippe Brucker > <jean-philippe@linaro.org> wrote: > > > > GPC checks are not performed on the output address for AT instructions, > > as stated by ARM DDI 0487J in D8.12.2: > > > > When populating PAR_EL1 with the result of an address translation > > instruction, granule protection checks are not performed on the final > > output address of a successful translation. > > > > Rename get_phys_addr_with_secure(), since it's only used to handle AT > > instructions. > > > > Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org> > > --- > > This incidentally fixes a problem with AT S1E1 instructions which can > > output an IPA and should definitely not cause a GPC. > > --- > > target/arm/internals.h | 25 ++++++++++++++----------- > > target/arm/helper.c | 8 ++++++-- > > target/arm/ptw.c | 11 ++++++----- > > 3 files changed, 26 insertions(+), 18 deletions(-) > > > > diff --git a/target/arm/internals.h b/target/arm/internals.h > > index 0f01bc32a8..fc90c364f7 100644 > > --- a/target/arm/internals.h > > +++ b/target/arm/internals.h > > @@ -1190,12 +1190,11 @@ typedef struct GetPhysAddrResult { > > } GetPhysAddrResult; > > > > /** > > - * get_phys_addr_with_secure: get the physical address for a virtual address > > + * get_phys_addr: get the physical address for a virtual address > > * @env: CPUARMState > > * @address: virtual address to get physical address for > > * @access_type: 0 for read, 1 for write, 2 for execute > > * @mmu_idx: MMU index indicating required translation regime > > - * @is_secure: security state for the access > > * @result: set on translation success. > > * @fi: set to fault info if the translation fails > > * > > @@ -1212,26 +1211,30 @@ typedef struct GetPhysAddrResult { > > * * for PSMAv5 based systems we don't bother to return a full FSR format > > * value. > > */ > > -bool get_phys_addr_with_secure(CPUARMState *env, target_ulong address, > > - MMUAccessType access_type, > > - ARMMMUIdx mmu_idx, bool is_secure, > > - GetPhysAddrResult *result, ARMMMUFaultInfo *fi) > > +bool get_phys_addr(CPUARMState *env, target_ulong address, > > + MMUAccessType access_type, ARMMMUIdx mmu_idx, > > + GetPhysAddrResult *result, ARMMMUFaultInfo *fi) > > __attribute__((nonnull)); > > > What is going on in this bit of the patch ? We already > have a prototype for get_phys_addr() with a doc comment. > Is this git's diff-output producing something silly > for a change that is not logically touching get_phys_addr()'s > prototype and comment at all? I swapped the two prototypes in order to make the new comment for get_phys_addr_with_secure_nogpc() more clear. Tried to convey that get_phys_addr() is the normal function and get_phys_addr_with_secure_nogpc() is special. So git is working as expected and this is a style change, I can switch them back if you prefer. Thanks, Jean > > > /** > > - * get_phys_addr: get the physical address for a virtual address > > + * get_phys_addr_with_secure_nogpc: get the physical address for a virtual > > + * address > > * @env: CPUARMState > > * @address: virtual address to get physical address for > > * @access_type: 0 for read, 1 for write, 2 for execute > > * @mmu_idx: MMU index indicating required translation regime > > + * @is_secure: security state for the access > > * @result: set on translation success. > > * @fi: set to fault info if the translation fails > > * > > - * Similarly, but use the security regime of @mmu_idx. > > + * Similar to get_phys_addr, but use the given security regime and don't perform > > + * a Granule Protection Check on the resulting address. > > */ > > -bool get_phys_addr(CPUARMState *env, target_ulong address, > > - MMUAccessType access_type, ARMMMUIdx mmu_idx, > > - GetPhysAddrResult *result, ARMMMUFaultInfo *fi) > > +bool get_phys_addr_with_secure_nogpc(CPUARMState *env, target_ulong address, > > + MMUAccessType access_type, > > + ARMMMUIdx mmu_idx, bool is_secure, > > + GetPhysAddrResult *result, > > + ARMMMUFaultInfo *fi) > > __attribute__((nonnull)); > > thanks > -- PMM
diff --git a/target/arm/internals.h b/target/arm/internals.h index 0f01bc32a8..fc90c364f7 100644 --- a/target/arm/internals.h +++ b/target/arm/internals.h @@ -1190,12 +1190,11 @@ typedef struct GetPhysAddrResult { } GetPhysAddrResult; /** - * get_phys_addr_with_secure: get the physical address for a virtual address + * get_phys_addr: get the physical address for a virtual address * @env: CPUARMState * @address: virtual address to get physical address for * @access_type: 0 for read, 1 for write, 2 for execute * @mmu_idx: MMU index indicating required translation regime - * @is_secure: security state for the access * @result: set on translation success. * @fi: set to fault info if the translation fails * @@ -1212,26 +1211,30 @@ typedef struct GetPhysAddrResult { * * for PSMAv5 based systems we don't bother to return a full FSR format * value. */ -bool get_phys_addr_with_secure(CPUARMState *env, target_ulong address, - MMUAccessType access_type, - ARMMMUIdx mmu_idx, bool is_secure, - GetPhysAddrResult *result, ARMMMUFaultInfo *fi) +bool get_phys_addr(CPUARMState *env, target_ulong address, + MMUAccessType access_type, ARMMMUIdx mmu_idx, + GetPhysAddrResult *result, ARMMMUFaultInfo *fi) __attribute__((nonnull)); /** - * get_phys_addr: get the physical address for a virtual address + * get_phys_addr_with_secure_nogpc: get the physical address for a virtual + * address * @env: CPUARMState * @address: virtual address to get physical address for * @access_type: 0 for read, 1 for write, 2 for execute * @mmu_idx: MMU index indicating required translation regime + * @is_secure: security state for the access * @result: set on translation success. * @fi: set to fault info if the translation fails * - * Similarly, but use the security regime of @mmu_idx. + * Similar to get_phys_addr, but use the given security regime and don't perform + * a Granule Protection Check on the resulting address. */ -bool get_phys_addr(CPUARMState *env, target_ulong address, - MMUAccessType access_type, ARMMMUIdx mmu_idx, - GetPhysAddrResult *result, ARMMMUFaultInfo *fi) +bool get_phys_addr_with_secure_nogpc(CPUARMState *env, target_ulong address, + MMUAccessType access_type, + ARMMMUIdx mmu_idx, bool is_secure, + GetPhysAddrResult *result, + ARMMMUFaultInfo *fi) __attribute__((nonnull)); bool pmsav8_mpu_lookup(CPUARMState *env, uint32_t address, diff --git a/target/arm/helper.c b/target/arm/helper.c index 07a9ac70f5..3ee2bb5fe1 100644 --- a/target/arm/helper.c +++ b/target/arm/helper.c @@ -3365,8 +3365,12 @@ static uint64_t do_ats_write(CPUARMState *env, uint64_t value, ARMMMUFaultInfo fi = {}; GetPhysAddrResult res = {}; - ret = get_phys_addr_with_secure(env, value, access_type, mmu_idx, - is_secure, &res, &fi); + /* + * I_MXTJT: Granule protection checks are not performed on the final address + * of a successful translation. + */ + ret = get_phys_addr_with_secure_nogpc(env, value, access_type, mmu_idx, + is_secure, &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 6318e13b98..1aef2b8cef 100644 --- a/target/arm/ptw.c +++ b/target/arm/ptw.c @@ -3412,16 +3412,17 @@ static bool get_phys_addr_gpc(CPUARMState *env, S1Translate *ptw, return false; } -bool get_phys_addr_with_secure(CPUARMState *env, target_ulong address, - MMUAccessType access_type, ARMMMUIdx mmu_idx, - bool is_secure, GetPhysAddrResult *result, - ARMMMUFaultInfo *fi) +bool get_phys_addr_with_secure_nogpc(CPUARMState *env, target_ulong address, + MMUAccessType access_type, + ARMMMUIdx mmu_idx, bool is_secure, + GetPhysAddrResult *result, + ARMMMUFaultInfo *fi) { S1Translate ptw = { .in_mmu_idx = mmu_idx, .in_space = arm_secure_to_space(is_secure), }; - return get_phys_addr_gpc(env, &ptw, address, access_type, result, fi); + return get_phys_addr_nogpc(env, &ptw, address, access_type, result, fi); } bool get_phys_addr(CPUARMState *env, target_ulong address,
GPC checks are not performed on the output address for AT instructions, as stated by ARM DDI 0487J in D8.12.2: When populating PAR_EL1 with the result of an address translation instruction, granule protection checks are not performed on the final output address of a successful translation. Rename get_phys_addr_with_secure(), since it's only used to handle AT instructions. Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org> --- This incidentally fixes a problem with AT S1E1 instructions which can output an IPA and should definitely not cause a GPC. --- target/arm/internals.h | 25 ++++++++++++++----------- target/arm/helper.c | 8 ++++++-- target/arm/ptw.c | 11 ++++++----- 3 files changed, 26 insertions(+), 18 deletions(-)