| Message ID | 20180716133302.25989-1-peter.maydell@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | [for-3.0] target/arm: Correctly handle overlapping small MPU regions | expand |
On 07/16/2018 06:33 AM, Peter Maydell wrote: > @@ -9963,6 +9994,21 @@ static bool pmsav8_mpu_lookup(CPUARMState *env, uint32_t address, > } > > if (address < base || address > limit) {> + /*> + * Address not in this region. We must check whether the> + * region covers addresses in the same page as our address.> + * In that case we must not report a size that covers the> + * whole page for a subsequent hit against a different MPU> + * region or the background region, because it would result in> + * incorrect TLB hits for subsequent accesses to addresses that> + * are in this MPU region.> + */> + if (limit >= base &&> + ranges_overlap(base, limit - base + 1,> + addr_page_base,> + TARGET_PAGE_SIZE)) {> + *is_subpage = true;> + } I don't understand why this is necessary in the v8m case. AP APL <----B1----|----L1-B2-A-------|---L2---> Your comment posits two regions [B1,L1] and [B2,L2], that A is not within [B1,L1] but is within [B2,L2] (otherwise we would not report a hit at all). Further, that [B1,L1] intersects [AP,APL] but does not intersect [B2,L2] (otherwise we would report a fault for overlapping regions). Surely this combination of ranges implies that [B2,L2] must itself set IS_SUBPAGE (otherwise the first region would not overlap the page of A, or would not overlap the second region). Because of the non-fault for region overlap in v7m, I can see that the test is required in get_phys_addr_pmsav7, but AFAICS only there. r~
On 17 July 2018 at 18:29, Richard Henderson <richard.henderson@linaro.org> wrote: > I don't understand why this is necessary in the v8m case. > > AP APL > <----B1----|----L1-B2-A-------|---L2---> > > > Your comment posits two regions [B1,L1] and [B2,L2], that A is not within > [B1,L1] but is within [B2,L2] (otherwise we would not report a hit at all). > Further, that [B1,L1] intersects [AP,APL] but does not intersect [B2,L2] > (otherwise we would report a fault for overlapping regions). > > Surely this combination of ranges implies that [B2,L2] must itself set > IS_SUBPAGE (otherwise the first region would not overlap the page of A, or > would not overlap the second region). (a) the overlap fault is only for addresses which are actually in the overlap (ie "you asked about address X and it hits in R1 and R2"); it doesn't imply that other addresses which are only in R1 fault just because some part of R1 overlaps with R2. So for instance: AP APL [B1 L1] [B2 L2] A1^ A2^ we must report a narrowed page for address A1, even though it hits only within one region (B1-L1) which covers the full page which A1 is within. (Otherwise if we later access A2 then we'll get the TLB entry for A1, rather than reporting the fault for it being in 2 regions.) (b) Consider the case where one of the regions is the "background region" that you get if you don't hit on anything. thanks -- PMM
On 07/17/2018 12:40 PM, Peter Maydell wrote: > On 17 July 2018 at 18:29, Richard Henderson > <richard.henderson@linaro.org> wrote: >> I don't understand why this is necessary in the v8m case. >> >> AP APL >> <----B1----|----L1-B2-A-------|---L2---> >> >> >> Your comment posits two regions [B1,L1] and [B2,L2], that A is not within >> [B1,L1] but is within [B2,L2] (otherwise we would not report a hit at all). >> Further, that [B1,L1] intersects [AP,APL] but does not intersect [B2,L2] >> (otherwise we would report a fault for overlapping regions). >> >> Surely this combination of ranges implies that [B2,L2] must itself set >> IS_SUBPAGE (otherwise the first region would not overlap the page of A, or >> would not overlap the second region). > > (a) the overlap fault is only for addresses which are actually in the > overlap (ie "you asked about address X and it hits in R1 and R2"); it > doesn't imply that other addresses which are only in R1 fault just > because some part of R1 overlaps with R2. Ah, that wasn't clear from the manual. > So for instance: > > AP APL > [B1 L1] > [B2 L2] > A1^ A2^ Thanks for the example, Reviewed-by: Richard Henderson <richard.henderson@linaro.org> r~
On 18 July 2018 at 05:02, Richard Henderson <richard.henderson@linaro.org> wrote: > On 07/17/2018 12:40 PM, Peter Maydell wrote: >> On 17 July 2018 at 18:29, Richard Henderson >> <richard.henderson@linaro.org> wrote: >>> I don't understand why this is necessary in the v8m case. >>> >>> AP APL >>> <----B1----|----L1-B2-A-------|---L2---> >>> >>> >>> Your comment posits two regions [B1,L1] and [B2,L2], that A is not within >>> [B1,L1] but is within [B2,L2] (otherwise we would not report a hit at all). >>> Further, that [B1,L1] intersects [AP,APL] but does not intersect [B2,L2] >>> (otherwise we would report a fault for overlapping regions). >>> >>> Surely this combination of ranges implies that [B2,L2] must itself set >>> IS_SUBPAGE (otherwise the first region would not overlap the page of A, or >>> would not overlap the second region). >> >> (a) the overlap fault is only for addresses which are actually in the >> overlap (ie "you asked about address X and it hits in R1 and R2"); it >> doesn't imply that other addresses which are only in R1 fault just >> because some part of R1 overlaps with R2. > > Ah, that wasn't clear from the manual. R_DCHP is the text rule that defines this bit of behaviour (generate fault for "an access to an address that matches in more than one region"); MPUCheck() is the pseudocode function. thanks -- PMM
diff --git a/target/arm/helper.c b/target/arm/helper.c index 0604a0efbe2..22d812240af 100644 --- a/target/arm/helper.c +++ b/target/arm/helper.c @@ -17,6 +17,7 @@ #include "exec/semihost.h" #include "sysemu/kvm.h" #include "fpu/softfloat.h" +#include "qemu/range.h" #define ARM_CPU_FREQ 1000000000 /* FIXME: 1 GHz, should be configurable */ @@ -9669,6 +9670,20 @@ static bool get_phys_addr_pmsav7(CPUARMState *env, uint32_t address, } if (address < base || address > base + rmask) { + /* + * Address not in this region. We must check whether the + * region covers addresses in the same page as our address. + * In that case we must not report a size that covers the + * whole page for a subsequent hit against a different MPU + * region or the background region, because it would result in + * incorrect TLB hits for subsequent accesses to addresses that + * are in this MPU region. + */ + if (ranges_overlap(base, rmask, + address & TARGET_PAGE_MASK, + TARGET_PAGE_SIZE)) { + *page_size = 1; + } continue; } @@ -9888,6 +9903,22 @@ static void v8m_security_lookup(CPUARMState *env, uint32_t address, sattrs->srvalid = true; sattrs->sregion = r; } + } else { + /* + * Address not in this region. We must check whether the + * region covers addresses in the same page as our address. + * In that case we must not report a size that covers the + * whole page for a subsequent hit against a different MPU + * region or the background region, because it would result + * in incorrect TLB hits for subsequent accesses to + * addresses that are in this MPU region. + */ + if (limit >= base && + ranges_overlap(base, limit - base + 1, + addr_page_base, + TARGET_PAGE_SIZE)) { + sattrs->subpage = true; + } } } } @@ -9963,6 +9994,21 @@ static bool pmsav8_mpu_lookup(CPUARMState *env, uint32_t address, } if (address < base || address > limit) { + /* + * Address not in this region. We must check whether the + * region covers addresses in the same page as our address. + * In that case we must not report a size that covers the + * whole page for a subsequent hit against a different MPU + * region or the background region, because it would result in + * incorrect TLB hits for subsequent accesses to addresses that + * are in this MPU region. + */ + if (limit >= base && + ranges_overlap(base, limit - base + 1, + addr_page_base, + TARGET_PAGE_SIZE)) { + *is_subpage = true; + } continue; }
To correctly handle small (less than TARGET_PAGE_SIZE) MPU regions, we must correctly handle the case where the address being looked up hits in an MPU region that is not small but the address is in the same page as a small region. For instance if MPU region 1 covers an entire page from 0x2000 to 0x2400 and MPU region 2 is small and covers only 0x2200 to 0x2280, then for an access to 0x2000 we must not return a result covering the full page even though we hit the page-sized region 1. Otherwise we will then cache that result in the TLB and accesses that should hit region 2 will incorrectly find the region 1 information. Check for the case where we miss an MPU region but it is still within the same page, and in that case narrow the size we will pass to tlb_set_page_with_attrs() for whatever the final outcome is of the MPU lookup. Reported-by: Adithya Baglody <adithya.nagaraj.baglody@intel.com> Signed-off-by: Peter Maydell <peter.maydell@linaro.org> --- This should go into 3.0 as it fixes a corner case in the newly added small-MPU-region handling. target/arm/helper.c | 46 +++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 46 insertions(+)