[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v3 5/9] target-arm: Add ARMMMUFaultInfo
From: |
Alex Bennée |
Subject: |
Re: [Qemu-devel] [PATCH v3 5/9] target-arm: Add ARMMMUFaultInfo |
Date: |
Wed, 14 Oct 2015 14:00:17 +0100 |
User-agent: |
mu4e 0.9.13; emacs 24.5.50.4 |
Edgar E. Iglesias <address@hidden> writes:
> On Wed, Oct 07, 2015 at 05:24:27PM +0100, Alex Bennée wrote:
>>
>> Edgar E. Iglesias <address@hidden> writes:
>>
>> > From: "Edgar E. Iglesias" <address@hidden>
>> >
>> > Introduce ARMMMUFaultInfo to propagate MMU Fault information
>> > across the MMU translation code path. This is in preparation for
>> > adding State-2 translation.
>>
>> s/State/stage/?
>>
>> >
>> > No functional changes.
>> >
>> > Signed-off-by: Edgar E. Iglesias <address@hidden>
>> > ---
>> > target-arm/helper.c | 32 ++++++++++++++++++++------------
>> > target-arm/internals.h | 15 ++++++++++++++-
>> > target-arm/op_helper.c | 3 ++-
>> > 3 files changed, 36 insertions(+), 14 deletions(-)
>> >
>> > diff --git a/target-arm/helper.c b/target-arm/helper.c
>> > index cbc1570..a429ff2 100644
>> > --- a/target-arm/helper.c
>> > +++ b/target-arm/helper.c
>> > @@ -18,7 +18,8 @@
>> > static bool get_phys_addr(CPUARMState *env, target_ulong address,
>> > int access_type, ARMMMUIdx mmu_idx,
>> > hwaddr *phys_ptr, MemTxAttrs *attrs, int *prot,
>> > - target_ulong *page_size, uint32_t *fsr);
>> > + target_ulong *page_size, uint32_t *fsr,
>> > + ARMMMUFaultInfo *fi);
>> >
>> > /* Definitions for the PMCCNTR and PMCR registers */
>> > #define PMCRD 0x8
>> > @@ -1774,9 +1775,10 @@ static uint64_t do_ats_write(CPUARMState *env,
>> > uint64_t value,
>> > bool ret;
>> > uint64_t par64;
>> > MemTxAttrs attrs = {};
>> > + ARMMMUFaultInfo fi = {};
>> >
>> > ret = get_phys_addr(env, value, access_type, mmu_idx,
>> > - &phys_addr, &attrs, &prot, &page_size, &fsr);
>> > + &phys_addr, &attrs, &prot, &page_size, &fsr, &fi);
>> > if (extended_addresses_enabled(env)) {
>> > /* fsr is a DFSR/IFSR value for the long descriptor
>> > * translation table format, but with WnR always clear.
>> > @@ -6167,7 +6169,8 @@ static uint64_t arm_ldq_ptw(CPUState *cs, hwaddr
>> > addr, bool is_secure)
>> > static bool get_phys_addr_v5(CPUARMState *env, uint32_t address,
>> > int access_type, ARMMMUIdx mmu_idx,
>> > hwaddr *phys_ptr, int *prot,
>> > - target_ulong *page_size, uint32_t *fsr)
>> > + target_ulong *page_size, uint32_t *fsr,
>> > + ARMMMUFaultInfo *fi)
>> > {
>> > CPUState *cs = CPU(arm_env_get_cpu(env));
>> > int code;
>> > @@ -6280,7 +6283,8 @@ do_fault:
>> > static bool get_phys_addr_v6(CPUARMState *env, uint32_t address,
>> > int access_type, ARMMMUIdx mmu_idx,
>> > hwaddr *phys_ptr, MemTxAttrs *attrs, int
>> > *prot,
>> > - target_ulong *page_size, uint32_t *fsr)
>> > + target_ulong *page_size, uint32_t *fsr,
>> > + ARMMMUFaultInfo *fi)
>> > {
>> > CPUState *cs = CPU(arm_env_get_cpu(env));
>> > int code;
>> > @@ -6431,7 +6435,8 @@ typedef enum {
>> > static bool get_phys_addr_lpae(CPUARMState *env, target_ulong address,
>> > int access_type, ARMMMUIdx mmu_idx,
>> > hwaddr *phys_ptr, MemTxAttrs *txattrs, int
>> > *prot,
>> > - target_ulong *page_size_ptr, uint32_t *fsr)
>> > + target_ulong *page_size_ptr, uint32_t *fsr,
>> > + ARMMMUFaultInfo *fi)
>> > {
>> > CPUState *cs = CPU(arm_env_get_cpu(env));
>> > /* Read an LPAE long-descriptor translation table. */
>> > @@ -6975,7 +6980,8 @@ static bool get_phys_addr_pmsav5(CPUARMState *env,
>> > uint32_t address,
>> > static bool get_phys_addr(CPUARMState *env, target_ulong address,
>> > int access_type, ARMMMUIdx mmu_idx,
>> > hwaddr *phys_ptr, MemTxAttrs *attrs, int *prot,
>> > - target_ulong *page_size, uint32_t *fsr)
>> > + target_ulong *page_size, uint32_t *fsr,
>> > + ARMMMUFaultInfo *fi)
>> > {
>> > if (mmu_idx == ARMMMUIdx_S12NSE0 || mmu_idx == ARMMMUIdx_S12NSE1) {
>> > /* TODO: when we support EL2 we should here call ourselves
>> > recursively
>> > @@ -7034,13 +7040,13 @@ static bool get_phys_addr(CPUARMState *env,
>> > target_ulong address,
>> >
>> > if (regime_using_lpae_format(env, mmu_idx)) {
>> > return get_phys_addr_lpae(env, address, access_type, mmu_idx,
>> > phys_ptr,
>> > - attrs, prot, page_size, fsr);
>> > + attrs, prot, page_size, fsr, fi);
>> > } else if (regime_sctlr(env, mmu_idx) & SCTLR_XP) {
>> > return get_phys_addr_v6(env, address, access_type, mmu_idx,
>> > phys_ptr,
>> > - attrs, prot, page_size, fsr);
>> > + attrs, prot, page_size, fsr, fi);
>> > } else {
>> > return get_phys_addr_v5(env, address, access_type, mmu_idx,
>> > phys_ptr,
>> > - prot, page_size, fsr);
>> > + prot, page_size, fsr, fi);
>> > }
>> > }
>> >
>> > @@ -7049,7 +7055,8 @@ static bool get_phys_addr(CPUARMState *env,
>> > target_ulong address,
>> > * fsr with ARM DFSR/IFSR fault register format value on failure.
>> > */
>> > bool arm_tlb_fill(CPUState *cs, vaddr address,
>> > - int access_type, int mmu_idx, uint32_t *fsr)
>> > + int access_type, int mmu_idx, uint32_t *fsr,
>> > + ARMMMUFaultInfo *fi)
>> > {
>> > ARMCPU *cpu = ARM_CPU(cs);
>> > CPUARMState *env = &cpu->env;
>> > @@ -7060,7 +7067,7 @@ bool arm_tlb_fill(CPUState *cs, vaddr address,
>> > MemTxAttrs attrs = {};
>> >
>> > ret = get_phys_addr(env, address, access_type, mmu_idx, &phys_addr,
>> > - &attrs, &prot, &page_size, fsr);
>> > + &attrs, &prot, &page_size, fsr, fi);
>> > if (!ret) {
>> > /* Map a single [sub]page. */
>> > phys_addr &= TARGET_PAGE_MASK;
>> > @@ -7083,9 +7090,10 @@ hwaddr arm_cpu_get_phys_page_debug(CPUState *cs,
>> > vaddr addr)
>> > bool ret;
>> > uint32_t fsr;
>> > MemTxAttrs attrs = {};
>> > + ARMMMUFaultInfo fi = {};
>> >
>> > ret = get_phys_addr(env, addr, 0, cpu_mmu_index(env, false),
>> > &phys_addr,
>> > - &attrs, &prot, &page_size, &fsr);
>> > + &attrs, &prot, &page_size, &fsr, &fi);
>> >
>> > if (ret) {
>> > return -1;
>> > diff --git a/target-arm/internals.h b/target-arm/internals.h
>> > index 36a56aa..3be23be 100644
>> > --- a/target-arm/internals.h
>> > +++ b/target-arm/internals.h
>> > @@ -389,8 +389,21 @@ bool arm_is_psci_call(ARMCPU *cpu, int excp_type);
>> > void arm_handle_psci_call(ARMCPU *cpu);
>> > #endif
>> >
>> > +/**
>> > + * ARMMMUFaultInfo: Information describing an ARM MMU Fault
>> > + * @s2addr: Address that caused a fault at stage 2
>> > + * @stage2: True if we faulted at stage 2
>> > + * @s1ptw: True if we faulted at stage 2 while doing a stage 1 page-table
>> > walk
>> > + */
>> > +typedef struct ARMMMUFaultInfo ARMMMUFaultInfo;
>> > +struct ARMMMUFaultInfo {
>> > + target_ulong s2addr;
>> > + bool stage2;
>> > + bool s1ptw;
>>
>> I guess the compiler packs the bools down pretty well but why not just
>> encode the faulting stage in a single variable? Perhaps I'm
>> misunderstanding the potential combinations here.
>>
>
> Do you mean using bitfields?
> e.g:
>
> struct ARMMMUFaultInfo {
> target_ulong s2addr;
> unsigned int stage2 : 1;
> unsigned int s1ptw : 1;
> }
> If so, I guess we could. If others agree, I can change to that.
> We could also maybe structure things differently. I didn't consider
> the encodings very much here to be honest...
No I didn't.
>
> Or do you mean why we need these at all?
> We need it because the places in the code where we figure out that
> a fault occurs don't have paths to propagate details about the error
> back to the callers that later need to generate the exception syndromes.
>
> The s1ptw field is additional detail to a stage-2 fault describing that
> the stage-2 fault happened while doing a page-table walk for a stage-1
> translation. s2addr is similar additional info to a stage-2 fault,
> passing the fault address so we can report it via the hypervisor IPA
> fault address reg.
My concern was making the meaning of the various variables intuitive.
>From what I can tell this info is only relevant to stage 2 faults which
may occur while walking stage 1 and stage 2 page tables? Will we ever
see a point where s2addr = NULL, stage2 = false and s1ptw = false?
I'll come back to this once I've gone over the later patches to see if
it makes more sense.
>
> Best regards,
> Edgar
--
Alex Bennée
- Re: [Qemu-devel] [PATCH v3 1/9] target-arm: Add HPFAR_EL2, (continued)
[Qemu-devel] [PATCH v3 5/9] target-arm: Add ARMMMUFaultInfo, Edgar E. Iglesias, 2015/10/08
Re: [Qemu-devel] [PATCH v3 5/9] target-arm: Add ARMMMUFaultInfo, Edgar E. Iglesias, 2015/10/09
[Qemu-devel] [PATCH v3 9/9] target-arm: Add support for S1 + S2 MMU translations, Edgar E. Iglesias, 2015/10/08