Re: [Qemu-devel] [PATCH v4] arm: implement cache/shareability attribute bits for PAR registers

[Peter Maydell]

On 31 October 2017 at 22:38, Andrew Baumann
 wrote:
> On a successful address translation instruction, PAR is supposed to
> contain cacheability and shareability attributes determined by the
> translation. We previously returned 0 for these bits (in line with the
> general strategy of ignoring caches and memory attributes), but some
> guest OSes may depend on them.
>
> This patch collects the attribute bits in the page-table walk, and
> updates PAR with the correct attributes for all LPAE translations.
> Short descriptor formats still return 0 for these bits, as in the
> prior implementation.
>
> Signed-off-by: Andrew Baumann 
> ---
> v2:
>  * return attrs via out parameter from get_phys_addr, rather than MemTxAttrs
>  * move MAIR lookup/index inline, since it turned out to be simple
>  * implement attributes for stage 2 translations
>  * combine attributes from stages 1 and 2 when required
>
> v3:
>  * implement S2 allocation hints and check for cache-disabled
>  * fix stage 2 shareability bits
>  * fix combined allocation hints (always use stage 1 hints)
>  * remove LOG_UNIMP message
>
> v4:
>  * fix hihint shift buglet in convert_stage2_attrs
>  * remove TODO comment (what was there is complete)
>  * mention relevant pseudocode procedures in comments
>



Applied to target-arm.next, thanks.

-- PMM

[Qemu-devel] [PATCH v4] arm: implement cache/shareability attribute bits for PAR registers

[Andrew Baumann via Qemu-devel]

On a successful address translation instruction, PAR is supposed to
contain cacheability and shareability attributes determined by the
translation. We previously returned 0 for these bits (in line with the
general strategy of ignoring caches and memory attributes), but some
guest OSes may depend on them.

This patch collects the attribute bits in the page-table walk, and
updates PAR with the correct attributes for all LPAE translations.
Short descriptor formats still return 0 for these bits, as in the
prior implementation.

Signed-off-by: Andrew Baumann 
---
v2:
 * return attrs via out parameter from get_phys_addr, rather than MemTxAttrs
 * move MAIR lookup/index inline, since it turned out to be simple
 * implement attributes for stage 2 translations
 * combine attributes from stages 1 and 2 when required

v3:
 * implement S2 allocation hints and check for cache-disabled
 * fix stage 2 shareability bits
 * fix combined allocation hints (always use stage 1 hints)
 * remove LOG_UNIMP message

v4:
 * fix hihint shift buglet in convert_stage2_attrs
 * remove TODO comment (what was there is complete)
 * mention relevant pseudocode procedures in comments

Thanks,
Andrew

 target/arm/helper.c | 178 +++-
 1 file changed, 164 insertions(+), 14 deletions(-)

diff --git a/target/arm/helper.c b/target/arm/helper.c
index 96113fe989..f61fb3ef68 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -19,17 +19,23 @@
 #define ARM_CPU_FREQ 10 /* FIXME: 1 GHz, should be configurable */
 
 #ifndef CONFIG_USER_ONLY
+/* Cacheability and shareability attributes for a memory access */
+typedef struct ARMCacheAttrs {
+unsigned int attrs:8; /* as in the MAIR register encoding */
+unsigned int shareability:2; /* as in the SH field of the VMSAv8-64 PTEs */
+} ARMCacheAttrs;
+
 static bool get_phys_addr(CPUARMState *env, target_ulong address,
   MMUAccessType access_type, ARMMMUIdx mmu_idx,
   hwaddr *phys_ptr, MemTxAttrs *attrs, int *prot,
   target_ulong *page_size, uint32_t *fsr,
-  ARMMMUFaultInfo *fi);
+  ARMMMUFaultInfo *fi, ARMCacheAttrs *cacheattrs);
 
 static bool get_phys_addr_lpae(CPUARMState *env, target_ulong address,
MMUAccessType access_type, ARMMMUIdx mmu_idx,
hwaddr *phys_ptr, MemTxAttrs *txattrs, int 
*prot,
target_ulong *page_size_ptr, uint32_t *fsr,
-   ARMMMUFaultInfo *fi);
+   ARMMMUFaultInfo *fi, ARMCacheAttrs *cacheattrs);
 
 /* Security attributes for an address, as returned by v8m_security_lookup. */
 typedef struct V8M_SAttributes {
@@ -2159,9 +2165,10 @@ static uint64_t do_ats_write(CPUARMState *env, uint64_t 
value,
 uint64_t par64;
 MemTxAttrs attrs = {};
 ARMMMUFaultInfo fi = {};
+ARMCacheAttrs cacheattrs = {};
 
-ret = get_phys_addr(env, value, access_type, mmu_idx,
-_addr, , , _size, , );
+ret = get_phys_addr(env, value, access_type, mmu_idx, _addr, ,
+, _size, , , );
 if (extended_addresses_enabled(env)) {
 /* fsr is a DFSR/IFSR value for the long descriptor
  * translation table format, but with WnR always clear.
@@ -2173,7 +2180,8 @@ static uint64_t do_ats_write(CPUARMState *env, uint64_t 
value,
 if (!attrs.secure) {
 par64 |= (1 << 9); /* NS */
 }
-/* We don't set the ATTR or SH fields in the PAR. */
+par64 |= (uint64_t)cacheattrs.attrs << 56; /* ATTR */
+par64 |= cacheattrs.shareability << 7; /* SH */
 } else {
 par64 |= 1; /* F */
 par64 |= (fsr & 0x3f) << 1; /* FS */
@@ -6925,7 +6933,7 @@ static bool v7m_read_half_insn(ARMCPU *cpu, ARMMMUIdx 
mmu_idx,
 return false;
 }
 if (get_phys_addr(env, addr, MMU_INST_FETCH, mmu_idx,
-  , , , _size, , )) {
+  , , , _size, , , NULL)) {
 /* the MPU lookup failed */
 env->v7m.cfsr[env->v7m.secure] |= R_V7M_CFSR_IACCVIOL_MASK;
 armv7m_nvic_set_pending(env->nvic, ARMV7M_EXCP_MEM, env->v7m.secure);
@@ -8207,7 +8215,7 @@ static hwaddr S1_ptw_translate(CPUARMState *env, 
ARMMMUIdx mmu_idx,
 int ret;
 
 ret = get_phys_addr_lpae(env, addr, 0, ARMMMUIdx_S2NS, ,
- , , , fsr, fi);
+ , , , fsr, fi, NULL);
 if (ret) {
 fi->s2addr = addr;
 fi->stage2 = true;
@@ -8608,11 +8616,41 @@ static bool check_s2_mmu_setup(ARMCPU *cpu, bool 
is_aa64, int level,
 return true;
 }
 
+/* Translate from the 4-bit stage 2 representation of
+ * memory attributes (without cache-allocation hints) to
+ * the 8-bit representation of