| Message ID | 20190110121736.23448-5-richard.henderson@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | target/arm: Implement ARMv8.5-BTI | expand |
On Thu, 10 Jan 2019 at 12:17, Richard Henderson <richard.henderson@linaro.org> wrote: > > This isn't really a transaction attribute, but that's the most > convenient place to hold a random bit of information within the > softmmu tlb. > > Signed-off-by: Richard Henderson <richard.henderson@linaro.org> > --- > include/exec/memattrs.h | 2 ++ > target/arm/helper.c | 6 ++++++ > 2 files changed, 8 insertions(+) > > diff --git a/include/exec/memattrs.h b/include/exec/memattrs.h > index d4a1642098..39d61188e1 100644 > --- a/include/exec/memattrs.h > +++ b/include/exec/memattrs.h > @@ -35,6 +35,8 @@ typedef struct MemTxAttrs { > unsigned int secure:1; > /* Memory access is usermode (unprivileged) */ > unsigned int user:1; > + /* Page is marked as "guarded" */ > + unsigned int guarded:1; Given that this isn't a real transaction attribute in the traditional sense, and it's pretty Arm-specific, I think we could do with a more expansive comment than this... > /* Requester ID (for MSI for example) */ > unsigned int requester_id:16; > } MemTxAttrs; > diff --git a/target/arm/helper.c b/target/arm/helper.c > index 138d9d5565..4e9ea2ed39 100644 > --- a/target/arm/helper.c > +++ b/target/arm/helper.c > @@ -9927,6 +9927,7 @@ static bool get_phys_addr_lpae(CPUARMState *env, target_ulong address, > bool ttbr1_valid; > uint64_t descaddrmask; > bool aarch64 = arm_el_is_aa64(env, el); > + bool guarded = false; > > /* TODO: > * This code does not handle the different format TCR for VTCR_EL2. > @@ -10098,6 +10099,7 @@ static bool get_phys_addr_lpae(CPUARMState *env, target_ulong address, > } > /* Merge in attributes from table descriptors */ > attrs |= nstable << 3; /* NS */ > + guarded |= extract64(descriptor, 50, 1); /* GP */ Do we need to do the logical-OR here? Since this is a block/page entry bit with no similar bit in the table descriptors, there's no merging to be done (ie we only execute this code once and 'guarded' will always be 'false' before execution of the |=.) > if (param.hpd) { > /* HPD disables all the table attributes except NSTable. */ > break; > @@ -10143,6 +10145,10 @@ static bool get_phys_addr_lpae(CPUARMState *env, target_ulong address, > */ > txattrs->secure = false; > } > + /* When in aarch64 mode, and BTI is enabled, remember GP in the IOTLB. */ > + if (aarch64 && guarded && cpu_isar_feature(aa64_bti, cpu)) { > + txattrs->guarded = true; > + } > > if (cacheattrs != NULL) { > if (mmu_idx == ARMMMUIdx_S2NS) { > -- > 2.17.2 > thanks -- PMM
On 1/22/19 5:26 AM, Peter Maydell wrote: > On Thu, 10 Jan 2019 at 12:17, Richard Henderson > <richard.henderson@linaro.org> wrote: >> >> This isn't really a transaction attribute, but that's the most >> convenient place to hold a random bit of information within the >> softmmu tlb. >> >> Signed-off-by: Richard Henderson <richard.henderson@linaro.org> >> --- >> include/exec/memattrs.h | 2 ++ >> target/arm/helper.c | 6 ++++++ >> 2 files changed, 8 insertions(+) >> >> diff --git a/include/exec/memattrs.h b/include/exec/memattrs.h >> index d4a1642098..39d61188e1 100644 >> --- a/include/exec/memattrs.h >> +++ b/include/exec/memattrs.h >> @@ -35,6 +35,8 @@ typedef struct MemTxAttrs { >> unsigned int secure:1; >> /* Memory access is usermode (unprivileged) */ >> unsigned int user:1; >> + /* Page is marked as "guarded" */ >> + unsigned int guarded:1; > > Given that this isn't a real transaction attribute in the traditional > sense, and it's pretty Arm-specific, I think we could do with a > more expansive comment than this... I have split this out to a separate patch, rearranged this to target_tlb_bit[0-2], with a large block comment. We will need some more of these bits for for system mode v8.5-MemTag anyway. >> + guarded |= extract64(descriptor, 50, 1); /* GP */ > > Do we need to do the logical-OR here? Since this is a > block/page entry bit with no similar bit in the table > descriptors, there's no merging to be done (ie we > only execute this code once and 'guarded' will always > be 'false' before execution of the |=.) The document that I have has exactly one sentence about this, and does not specify whether the bit is akin to the page table attributes (which appear at every table level) or not. As written above, this will execute more than once. r~
On Mon, 28 Jan 2019 at 21:08, Richard Henderson <richard.henderson@linaro.org> wrote: > > On 1/22/19 5:26 AM, Peter Maydell wrote: > >> + guarded |= extract64(descriptor, 50, 1); /* GP */ > > > > Do we need to do the logical-OR here? Since this is a > > block/page entry bit with no similar bit in the table > > descriptors, there's no merging to be done (ie we > > only execute this code once and 'guarded' will always > > be 'false' before execution of the |=.) > > The document that I have has exactly one sentence about this, and does not > specify whether the bit is akin to the page table attributes (which appear at > every table level) or not. Translation table descriptor formats come in four flavours: * Invalid * Table (which gives the address of the next level table) * Block (which gives the address of a large lump of memory) * Page (which gives the address of a page) The GP bit documented to be in Block and Page entries, not Table (which is how you've coded it). > As written above, this will execute more than once. I don't see how -- all the code paths forward from "guarded |= extract64(descriptor, 50, 1);" reach a "break" statement that terminates the loop, don't they? thanks -- PMM
On 1/29/19 1:55 AM, Peter Maydell wrote: >> As written above, this will execute more than once. > > I don't see how -- all the code paths forward from > "guarded |= extract64(descriptor, 50, 1);" reach a > "break" statement that terminates the loop, don't they? You're right. I've misread the surrounding code. r~
diff --git a/include/exec/memattrs.h b/include/exec/memattrs.h index d4a1642098..39d61188e1 100644 --- a/include/exec/memattrs.h +++ b/include/exec/memattrs.h @@ -35,6 +35,8 @@ typedef struct MemTxAttrs { unsigned int secure:1; /* Memory access is usermode (unprivileged) */ unsigned int user:1; + /* Page is marked as "guarded" */ + unsigned int guarded:1; /* Requester ID (for MSI for example) */ unsigned int requester_id:16; } MemTxAttrs; diff --git a/target/arm/helper.c b/target/arm/helper.c index 138d9d5565..4e9ea2ed39 100644 --- a/target/arm/helper.c +++ b/target/arm/helper.c @@ -9927,6 +9927,7 @@ static bool get_phys_addr_lpae(CPUARMState *env, target_ulong address, bool ttbr1_valid; uint64_t descaddrmask; bool aarch64 = arm_el_is_aa64(env, el); + bool guarded = false; /* TODO: * This code does not handle the different format TCR for VTCR_EL2. @@ -10098,6 +10099,7 @@ static bool get_phys_addr_lpae(CPUARMState *env, target_ulong address, } /* Merge in attributes from table descriptors */ attrs |= nstable << 3; /* NS */ + guarded |= extract64(descriptor, 50, 1); /* GP */ if (param.hpd) { /* HPD disables all the table attributes except NSTable. */ break; @@ -10143,6 +10145,10 @@ static bool get_phys_addr_lpae(CPUARMState *env, target_ulong address, */ txattrs->secure = false; } + /* When in aarch64 mode, and BTI is enabled, remember GP in the IOTLB. */ + if (aarch64 && guarded && cpu_isar_feature(aa64_bti, cpu)) { + txattrs->guarded = true; + } if (cacheattrs != NULL) { if (mmu_idx == ARMMMUIdx_S2NS) {
This isn't really a transaction attribute, but that's the most convenient place to hold a random bit of information within the softmmu tlb. Signed-off-by: Richard Henderson <richard.henderson@linaro.org> --- include/exec/memattrs.h | 2 ++ target/arm/helper.c | 6 ++++++ 2 files changed, 8 insertions(+) -- 2.17.2