Re: [Qemu-devel] [PATCH v3] arm: implement cache/shareability attribute bits for PAR registers
On 31 October 2017 at 13:02, Andrew Baumannwrote: > 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 > > There's unfortunately one new wrinkle in this revision: the spec for > S2ConvertAttrsHints() calls S2CacheDisabled() and returns non-cached if the > cache is disabled. I assumed that we ought to emulate this. However, it wasn't > clear to me what to make of the "if ELUsingAArch32(EL2)" branch -- we have a > cp15.hcr_el2, but no HCR2 register. I just left a TODO comment, but it's quite > likely I've missed something. > +/* Translate from the 4-bit stage 2 representation of > + * memory attributes (without cache-allocation hints) to > + * the 8-bit representation of the stage 1 MAIR registers > + * (which includes allocation hints). > + * > + * ref: shared/translation/attrs/S2AttrDecode() > + * .../S2ConvertAttrsHints() > + */ > +static uint8_t convert_stage2_attrs(CPUARMState *env, uint8_t s2attrs) > +{ > +uint8_t hiattr = extract32(s2attrs, 2, 2); > +uint8_t loattr = extract32(s2attrs, 0, 2); > +uint8_t hihint = 0, lohint = 0; > + > +if (hiattr != 0) { /* normal memory */ > +/* TODO: to faithfully emulate S2CacheDisabled() for the case > + * when EL2 is aarch32, we should check HCR2 (not HCR_EL2), but > + * qemu doesn't presently model this register. > + */ We don't currently implement HCR2, but when we do it will be in the top 32 bits of cp15.hcr_el2, because that's where it's architecturally mapped. So the bit to be checked is in the same place regardless (AArch32 HCR2.ID is bit 1, and AArch64 HCR_EL2.ID is bit 33, and so on.) This is pretty frequently true -- it's rare that you need to actually look in a different place for aarch32 vs aarch64. Actually checking this bit is a little bit odd for QEMU, because mostly what we're implementing here is "we report in PAR the attrs that the page tables say, even if that's not what the implementation actually does", and "NC if HCR_EL2.CD is 1" is an "affects what the implementation does" bit of behaviour. That said, the ARM ARM is a bit vague in this area so following the pseudocode here is probably safest. > +if ((env->cp15.hcr_el2 & HCR_CD) != 0) { /* cache disabled */ > +hiattr = loattr = 1; /* non-cacheable */ > +} else { > +if (hiattr != 1) { /* Write-through or write-back */ > +hihint = 3; /* RW allocate */ > +} > +if (loattr != 1) { /* Write-through or write-back */ > +lohint = 3; /* RW allocate */ > +} > +} > +} > + > +return (hiattr << 6) | (hihint << 2) | (loattr << 2) | lohint; > +} thanks -- PMM
Re: [Qemu-devel] [PATCH v3] arm: implement cache/shareability attribute bits for PAR registers
> From: Andrew Baumann > Sent: Tuesday, 31 October 2017 21:02 [...] > +static uint8_t convert_stage2_attrs(CPUARMState *env, uint8_t s2attrs) > +{ > +uint8_t hiattr = extract32(s2attrs, 2, 2); > +uint8_t loattr = extract32(s2attrs, 0, 2); > +uint8_t hihint = 0, lohint = 0; > + > +if (hiattr != 0) { /* normal memory */ > +/* TODO: to faithfully emulate S2CacheDisabled() for the case > + * when EL2 is aarch32, we should check HCR2 (not HCR_EL2), but > + * qemu doesn't presently model this register. > + */ > +if ((env->cp15.hcr_el2 & HCR_CD) != 0) { /* cache disabled */ > +hiattr = loattr = 1; /* non-cacheable */ > +} else { > +if (hiattr != 1) { /* Write-through or write-back */ > +hihint = 3; /* RW allocate */ > +} > +if (loattr != 1) { /* Write-through or write-back */ > +lohint = 3; /* RW allocate */ > +} > +} > +} > + > +return (hiattr << 6) | (hihint << 2) | (loattr << 2) | lohint; > +} Right after sending this, I noticed a dumb copy-paste bug: the above should be hihint << 4. I'm obviously going a bit too fast, sorry. Andrew
[Qemu-devel] [PATCH v3] arm: implement cache/shareability attribute bits for PAR registers
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 There's unfortunately one new wrinkle in this revision: the spec for S2ConvertAttrsHints() calls S2CacheDisabled() and returns non-cached if the cache is disabled. I assumed that we ought to emulate this. However, it wasn't clear to me what to make of the "if ELUsingAArch32(EL2)" branch -- we have a cp15.hcr_el2, but no HCR2 register. I just left a TODO comment, but it's quite likely I've missed something. Thanks, Andrew target/arm/helper.c | 180 1 file changed, 166 insertions(+), 14 deletions(-) diff --git a/target/arm/helper.c b/target/arm/helper.c index 96113fe989..81fe9283ea 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