| Message ID | 20200630112752.31134-3-mark.cave-ayland@ilande.co.uk |
|---|---|
| State | New |
| Headers | show |
| Series | target/m68k: fix physical address translation in m68k_cpu_get_phys_page_debug() | expand |
Le 30/06/2020 à 13:27, Mark Cave-Ayland a écrit : > Since all callers to get_physical_address() now apply the same page offset to > the translation result, move the logic into get_physical_address() itself to > avoid duplication. > > Suggested-by: Philippe Mathieu-Daudé <f4bug@amsat.org> > Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> > --- > target/m68k/helper.c | 18 +++++++----------- > 1 file changed, 7 insertions(+), 11 deletions(-) > > diff --git a/target/m68k/helper.c b/target/m68k/helper.c > index 631eab7774..71c2376910 100644 > --- a/target/m68k/helper.c > +++ b/target/m68k/helper.c > @@ -643,7 +643,7 @@ static int get_physical_address(CPUM68KState *env, hwaddr *physical, > /* Transparent Translation Register bit */ > env->mmu.mmusr = M68K_MMU_T_040 | M68K_MMU_R_040; > } > - *physical = address & TARGET_PAGE_MASK; > + *physical = address; > *page_size = TARGET_PAGE_SIZE; > return 0; > } > @@ -771,7 +771,8 @@ static int get_physical_address(CPUM68KState *env, hwaddr *physical, > } > *page_size = 1 << page_bits; > page_mask = ~(*page_size - 1); > - *physical = next & page_mask; > + address &= TARGET_PAGE_MASK; I don't think you need TARGET_PAGE_MASK here: - TARGET_PAGE_MASK is 4096 - page_mask is either 4096 or 8192 > + *physical = (next & page_mask) + (address & (*page_size - 1)); > > if (access_type & ACCESS_PTEST) { > env->mmu.mmusr |= next & M68K_MMU_SR_MASK_040; > @@ -826,8 +827,6 @@ hwaddr m68k_cpu_get_phys_page_debug(CPUState *cs, vaddr addr) > return -1; > } > > - addr &= TARGET_PAGE_MASK; > - phys_addr += addr & (page_size - 1); > return phys_addr; > } > > @@ -891,10 +890,8 @@ bool m68k_cpu_tlb_fill(CPUState *cs, vaddr address, int size, > ret = get_physical_address(&cpu->env, &physical, &prot, > address, access_type, &page_size); > if (likely(ret == 0)) { > - address &= TARGET_PAGE_MASK; > - physical += address & (page_size - 1); > - tlb_set_page(cs, address, physical, > - prot, mmu_idx, TARGET_PAGE_SIZE); > + tlb_set_page(cs, address & TARGET_PAGE_MASK, > + physical & TARGET_PAGE_MASK, prot, mmu_idx, page_size); I had a look to tl_set_page() to see how it manages the entry when the addresses are not aligned to page_size, and it calls tlb_set_page_with_attrs() where we have a comment: /* Add a new TLB entry. At most one entry for a given virtual address * is permitted. Only a single TARGET_PAGE_SIZE region is mapped, the * supplied size is only used by tlb_flush_page. ... So I think it's correct to use TARGET_PAGE_MASK and page_size. Thanks, Laurent
On 30/06/2020 22:20, Laurent Vivier wrote: > Le 30/06/2020 à 13:27, Mark Cave-Ayland a écrit : >> Since all callers to get_physical_address() now apply the same page offset to >> the translation result, move the logic into get_physical_address() itself to >> avoid duplication. >> >> Suggested-by: Philippe Mathieu-Daudé <f4bug@amsat.org> >> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> >> --- >> target/m68k/helper.c | 18 +++++++----------- >> 1 file changed, 7 insertions(+), 11 deletions(-) >> >> diff --git a/target/m68k/helper.c b/target/m68k/helper.c >> index 631eab7774..71c2376910 100644 >> --- a/target/m68k/helper.c >> +++ b/target/m68k/helper.c >> @@ -643,7 +643,7 @@ static int get_physical_address(CPUM68KState *env, hwaddr *physical, >> /* Transparent Translation Register bit */ >> env->mmu.mmusr = M68K_MMU_T_040 | M68K_MMU_R_040; >> } >> - *physical = address & TARGET_PAGE_MASK; >> + *physical = address; >> *page_size = TARGET_PAGE_SIZE; >> return 0; >> } >> @@ -771,7 +771,8 @@ static int get_physical_address(CPUM68KState *env, hwaddr *physical, >> } >> *page_size = 1 << page_bits; >> page_mask = ~(*page_size - 1); >> - *physical = next & page_mask; >> + address &= TARGET_PAGE_MASK; > > I don't think you need TARGET_PAGE_MASK here: > - TARGET_PAGE_MASK is 4096 > - page_mask is either 4096 or 8192 Ah yes, of course - that will get handled fine by the statement below. >> + *physical = (next & page_mask) + (address & (*page_size - 1)); >> >> if (access_type & ACCESS_PTEST) { >> env->mmu.mmusr |= next & M68K_MMU_SR_MASK_040; >> @@ -826,8 +827,6 @@ hwaddr m68k_cpu_get_phys_page_debug(CPUState *cs, vaddr addr) >> return -1; >> } >> >> - addr &= TARGET_PAGE_MASK; >> - phys_addr += addr & (page_size - 1); >> return phys_addr; >> } >> >> @@ -891,10 +890,8 @@ bool m68k_cpu_tlb_fill(CPUState *cs, vaddr address, int size, >> ret = get_physical_address(&cpu->env, &physical, &prot, >> address, access_type, &page_size); >> if (likely(ret == 0)) { >> - address &= TARGET_PAGE_MASK; >> - physical += address & (page_size - 1); >> - tlb_set_page(cs, address, physical, >> - prot, mmu_idx, TARGET_PAGE_SIZE); >> + tlb_set_page(cs, address & TARGET_PAGE_MASK, >> + physical & TARGET_PAGE_MASK, prot, mmu_idx, page_size); > > I had a look to tl_set_page() to see how it manages the entry when the > addresses are not aligned to page_size, and it calls > tlb_set_page_with_attrs() where we have a comment: > > /* Add a new TLB entry. At most one entry for a given virtual address > * is permitted. Only a single TARGET_PAGE_SIZE region is mapped, the > * supplied size is only used by tlb_flush_page. > ... > > So I think it's correct to use TARGET_PAGE_MASK and page_size. Indeed, it certainly agrees with the documentation and what the majority of the callers are doing - I'm just surprised that tlb_set_page_with_attrs() doesn't assert() if any bits below TARGET_PAGE_MASK are set, rather than masking both vaddr and paddr itself. But I'm happy with this part anyhow. ATB, Mark.
diff --git a/target/m68k/helper.c b/target/m68k/helper.c index 631eab7774..71c2376910 100644 --- a/target/m68k/helper.c +++ b/target/m68k/helper.c @@ -643,7 +643,7 @@ static int get_physical_address(CPUM68KState *env, hwaddr *physical, /* Transparent Translation Register bit */ env->mmu.mmusr = M68K_MMU_T_040 | M68K_MMU_R_040; } - *physical = address & TARGET_PAGE_MASK; + *physical = address; *page_size = TARGET_PAGE_SIZE; return 0; } @@ -771,7 +771,8 @@ static int get_physical_address(CPUM68KState *env, hwaddr *physical, } *page_size = 1 << page_bits; page_mask = ~(*page_size - 1); - *physical = next & page_mask; + address &= TARGET_PAGE_MASK; + *physical = (next & page_mask) + (address & (*page_size - 1)); if (access_type & ACCESS_PTEST) { env->mmu.mmusr |= next & M68K_MMU_SR_MASK_040; @@ -826,8 +827,6 @@ hwaddr m68k_cpu_get_phys_page_debug(CPUState *cs, vaddr addr) return -1; } - addr &= TARGET_PAGE_MASK; - phys_addr += addr & (page_size - 1); return phys_addr; } @@ -891,10 +890,8 @@ bool m68k_cpu_tlb_fill(CPUState *cs, vaddr address, int size, ret = get_physical_address(&cpu->env, &physical, &prot, address, access_type, &page_size); if (likely(ret == 0)) { - address &= TARGET_PAGE_MASK; - physical += address & (page_size - 1); - tlb_set_page(cs, address, physical, - prot, mmu_idx, TARGET_PAGE_SIZE); + tlb_set_page(cs, address & TARGET_PAGE_MASK, + physical & TARGET_PAGE_MASK, prot, mmu_idx, page_size); return true; } @@ -1383,9 +1380,8 @@ void HELPER(ptest)(CPUM68KState *env, uint32_t addr, uint32_t is_read) ret = get_physical_address(env, &physical, &prot, addr, access_type, &page_size); if (ret == 0) { - addr &= TARGET_PAGE_MASK; - physical += addr & (page_size - 1); - tlb_set_page(env_cpu(env), addr, physical, + tlb_set_page(env_cpu(env), addr & TARGET_PAGE_MASK, + physical & TARGET_PAGE_MASK, prot, access_type & ACCESS_SUPER ? MMU_KERNEL_IDX : MMU_USER_IDX, page_size); }
Since all callers to get_physical_address() now apply the same page offset to the translation result, move the logic into get_physical_address() itself to avoid duplication. Suggested-by: Philippe Mathieu-Daudé <f4bug@amsat.org> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> --- target/m68k/helper.c | 18 +++++++----------- 1 file changed, 7 insertions(+), 11 deletions(-)