On Mon, Jan 11, 2016 at 12:15 PM, Artyom Tarasenko <atar4q...@gmail.com> wrote:
> Hi Richard,
>
> first of all, this is a very nice series.
> I really enjoy reading it, thank you very much.
> It makes the code is more readable and likely to be more performant.
> A nitpick below.
>
> On Thu, Dec 17, 2015 at 9:57 PM, Richard Henderson <r...@twiddle.net> wrote:
>> This gives us a trivial way to access physical addresses
>> (aka "real addresses", in sun4v terminology) directly from
>> qemu_ld/st, without having to go through another helper.
>>
>> This also fixes a bug in get_physical_address_code where
>> it inferred NUCLEUS from env->tl instead of from mmu_idx.
>>
>> Signed-off-by: Richard Henderson <r...@twiddle.net>
>> ---
>> target-sparc/cpu.h | 18 +++++++---
>> target-sparc/mmu_helper.c | 90
>> +++++++++++++++++++++++++++++------------------
>> 2 files changed, 69 insertions(+), 39 deletions(-)
>>
>> diff --git a/target-sparc/cpu.h b/target-sparc/cpu.h
>> index 7f4d47f..b1222a1 100644
>> --- a/target-sparc/cpu.h
>> +++ b/target-sparc/cpu.h
>> @@ -220,9 +220,9 @@ enum {
>> #define MAX_NWINDOWS 32
>>
>> #if !defined(TARGET_SPARC64)
>> -#define NB_MMU_MODES 2
>> +#define NB_MMU_MODES 3
>> #else
>> -#define NB_MMU_MODES 6
>> +#define NB_MMU_MODES 7
>> typedef struct trap_state {
>> uint64_t tpc;
>> uint64_t tnpc;
>> @@ -612,11 +612,13 @@ int cpu_sparc_signal_handler(int host_signum, void
>> *pinfo, void *puc);
>> #define MMU_MODE4_SUFFIX _nucleus
>> #define MMU_HYPV_IDX 5
>> #define MMU_MODE5_SUFFIX _hypv
>> +#define MMU_REAL_IDX 6
>> #else
>> #define MMU_USER_IDX 0
>> #define MMU_MODE0_SUFFIX _user
>> #define MMU_KERNEL_IDX 1
>> #define MMU_MODE1_SUFFIX _kernel
>> +#define MMU_REAL_IDX 2
>> #endif
>>
>> #if defined (TARGET_SPARC64)
>> @@ -641,9 +643,17 @@ static inline int cpu_mmu_index(CPUSPARCState *env1,
>> bool ifetch)
>> #if defined(CONFIG_USER_ONLY)
>> return MMU_USER_IDX;
>> #elif !defined(TARGET_SPARC64)
>> - return env1->psrs;
>> + if (!(env1->mmuregs[0] & MMU_E)) {
>> + return MMU_REAL_IDX; /* MMU disabled */
>> + } else {
>> + return env1->psrs;
>> + }
>> #else
>> - if (env1->tl > 0) {
>> + if (ifetch
>> + ? !(env1->lsu & IMMU_E) || (env1->pstate & PS_RED)
>> + : !(env1->lsu & DMMU_E)) {
>> + return MMU_REAL_IDX; /* MMU disabled */
>> + } else if (env1->tl > 0) {
>> return MMU_NUCLEUS_IDX;
>> } else if (cpu_hypervisor_mode(env1)) {
>> return MMU_HYPV_IDX;
>> diff --git a/target-sparc/mmu_helper.c b/target-sparc/mmu_helper.c
>> index 7495406..105f00d 100644
>> --- a/target-sparc/mmu_helper.c
>> +++ b/target-sparc/mmu_helper.c
>> @@ -90,7 +90,7 @@ static int get_physical_address(CPUSPARCState *env, hwaddr
>> *physical,
>>
>> is_user = mmu_idx == MMU_USER_IDX;
>>
>> - if ((env->mmuregs[0] & MMU_E) == 0) { /* MMU disabled */
>> + if (mmu_idx == MMU_REAL_IDX) { /* MMU bypass access */
>> *page_size = TARGET_PAGE_SIZE;
>> /* Boot mode: instruction fetches are taken from PROM */
>> if (rw == 2 && (env->mmuregs[0] & env->def->mmu_bm)) {
>> @@ -492,33 +492,40 @@ static int get_physical_address_data(CPUSPARCState
>> *env,
>> unsigned int i;
>> uint64_t context;
>> uint64_t sfsr = 0;
>> + bool is_user = false;
>>
>> - int is_user = (mmu_idx == MMU_USER_IDX ||
>> - mmu_idx == MMU_USER_SECONDARY_IDX);
>> -
>> - if ((env->lsu & DMMU_E) == 0) { /* DMMU disabled */
>
> ^^^^
>
> in case of ASI instructions, mmu_idx can come from dc->mem_idx, right?
> So the check may be still necessary here,
>
>> + switch (mmu_idx) {
>> + case MMU_REAL_IDX:
>> + /* MMU bypass access */
>> *physical = ultrasparc_truncate_physical(address);
>> - *prot = PAGE_READ | PAGE_WRITE;
>> + *prot = PAGE_EXEC | PAGE_READ | PAGE_WRITE;
>> return 0;
>> - }
>>
>> - switch (mmu_idx) {
>> + case MMU_NUCLEUS_IDX:
>> + sfsr |= SFSR_CT_NUCLEUS;
>> + /* fallthru */
>> + case MMU_HYPV_IDX:
>> + /* No context. */
>> + context = 0;
>> + break;
>> case MMU_USER_IDX:
>> + is_user = true;
>> + /* fallthru */
>> case MMU_KERNEL_IDX:
>> + /* PRIMARY context */
>> context = env->dmmu.mmu_primary_context & 0x1fff;
>> sfsr |= SFSR_CT_PRIMARY;
>> break;
>> case MMU_USER_SECONDARY_IDX:
>> + is_user = true;
>> + /* fallthru */
>> case MMU_KERNEL_SECONDARY_IDX:
>> + /* PRIMARY context */
>> context = env->dmmu.mmu_secondary_context & 0x1fff;
>> sfsr |= SFSR_CT_SECONDARY;
>> break;
>> - case MMU_NUCLEUS_IDX:
>> - sfsr |= SFSR_CT_NUCLEUS;
>> - /* FALLTHRU */
>> default:
>> - context = 0;
>> - break;
>> + g_assert_not_reached();
>> }
>>
>> if (rw == 1) {
>> @@ -573,8 +580,8 @@ static int get_physical_address_data(CPUSPARCState *env,
>> }
>>
>> if (env->dmmu.sfsr & SFSR_VALID_BIT) { /* Fault status register
>> */
>> - sfsr |= SFSR_OW_BIT; /* overflow (not read before
>> - another fault) */
>> + /* overflow (not read before another fault) */
>> + sfsr |= SFSR_OW_BIT;
>> }
>>
>> if (env->pstate & PS_PRIV) {
>> @@ -611,23 +618,41 @@ static int get_physical_address_code(CPUSPARCState
>> *env,
>> CPUState *cs = CPU(sparc_env_get_cpu(env));
>> unsigned int i;
>> uint64_t context;
>> + uint64_t sfsr = 0;
>> + bool is_user = false;
>>
>> - int is_user = (mmu_idx == MMU_USER_IDX ||
>> - mmu_idx == MMU_USER_SECONDARY_IDX);
>> -
>> - if ((env->lsu & IMMU_E) == 0 || (env->pstate & PS_RED) != 0) {
>> - /* IMMU disabled */
>
> and here.
On a second thought, IMMU check is not necessary, because there is no
ASI access for instructions.
So, here no check is necessary, only above.
Artyom
>
>> + switch (mmu_idx) {
>> + case MMU_REAL_IDX:
>> + /* MMU bypass access */
>> *physical = ultrasparc_truncate_physical(address);
>> - *prot = PAGE_EXEC;
>> + *prot = PAGE_EXEC | PAGE_READ | PAGE_WRITE;
>> return 0;
>> - }
>>
>> - if (env->tl == 0) {
>> + case MMU_NUCLEUS_IDX:
>> + sfsr |= SFSR_CT_NUCLEUS;
>> + /* fallthru */
>> + case MMU_HYPV_IDX:
>> + /* No context. */
>> + context = 0;
>> + break;
>> + case MMU_USER_IDX:
>> + is_user = true;
>> + /* fallthru */
>> + case MMU_KERNEL_IDX:
>> /* PRIMARY context */
>> context = env->dmmu.mmu_primary_context & 0x1fff;
>> - } else {
>> - /* NUCLEUS context */
>> - context = 0;
>> + sfsr |= SFSR_CT_PRIMARY;
>> + break;
>> + case MMU_USER_SECONDARY_IDX:
>> + is_user = true;
>> + /* fallthru */
>> + case MMU_KERNEL_SECONDARY_IDX:
>> + /* PRIMARY context */
>> + context = env->dmmu.mmu_secondary_context & 0x1fff;
>> + sfsr |= SFSR_CT_SECONDARY;
>> + break;
>> + default:
>> + g_assert_not_reached();
>> }
>>
>> for (i = 0; i < 64; i++) {
>> @@ -638,20 +663,15 @@ static int get_physical_address_code(CPUSPARCState
>> *env,
>> if (TTE_IS_PRIV(env->itlb[i].tte) && is_user) {
>> /* Fault status register */
>> if (env->immu.sfsr & SFSR_VALID_BIT) {
>> - env->immu.sfsr = SFSR_OW_BIT; /* overflow (not read
>> before
>> - another fault) */
>> - } else {
>> - env->immu.sfsr = 0;
>> + /* overflow (not read before another fault) */
>> + sfsr |= SFSR_OW_BIT;
>> }
>> if (env->pstate & PS_PRIV) {
>> - env->immu.sfsr |= SFSR_PR_BIT;
>> - }
>> - if (env->tl > 0) {
>> - env->immu.sfsr |= SFSR_CT_NUCLEUS;
>> + sfsr |= SFSR_PR_BIT;
>> }
>>
>> /* FIXME: ASI field in SFSR must be set */
>> - env->immu.sfsr |= SFSR_FT_PRIV_BIT | SFSR_VALID_BIT;
>> + env->immu.sfsr |= sfsr | SFSR_FT_PRIV_BIT | SFSR_VALID_BIT;
>> cs->exception_index = TT_TFAULT;
>>
>> env->immu.tag_access = (address & ~0x1fffULL) | context;
>> --
>> 2.5.0
>>
>>
>
>
>
> --
> Regards,
> Artyom Tarasenko
>
> SPARC and PPC PReP under qemu blog: http://tyom.blogspot.com/search/label/qemu
