Re: [PATCH 1/1] Set TARGET_PAGE_BITS to be 10 instead of 8 bits

[Richard Henderson <richard.henderson@linaro.org>]

On 4/10/21 10:24 AM, Michael Rolnik wrote:
> Please review.


The first 256b is i/o, the next 768b are ram.  But having changed the page 
size, it should mean that the first 1k are now treated as i/o.

We do have a path by which instructions in i/o pages can be executed.  This 
happens on some ARM board setups during cold boot.  But we do not save those 
translations, so they run much much slower than it should.

But perhaps in the case of AVR, "much much slower" really isn't visible?

In general, I think changing the page size is wrong.  I also assume that 
migration is largely irrelevant to this target.


r~

> 
> On Tue, Mar 23, 2021 at 10:28 PM Michael Rolnik <mrolnik@gmail.com 
> <mailto:mrolnik@gmail.com>> wrote:
> 
>     If I set TARGET_PAGE_BITS to 12 this *assert assert(v_l2_levels >= 0);*
>     will fail (page_table_config_init function) because
>     TARGET_PHYS_ADDR_SPACE_BITS is 24 bits, because AVR has 24 is the longest
>     pointer AVR has. I can set TARGET_PHYS_ADDR_SPACE_BITS to 32 and
>     TARGET_PAGE_BITS to 12 and everything will work fine.
>     What do you think?
> 
>     btw, wrote the original comment, you David referred to, when I did not know
>     that QEMU could map several regions to the same page, which is not true.
>     That's why I could change 8 to 10.
> 
>     On Tue, Mar 23, 2021 at 10:11 PM Michael Rolnik <mrolnik@gmail.com
>     <mailto:mrolnik@gmail.com>> wrote:
> 
>         how long?
> 
>         On Tue, Mar 23, 2021 at 2:46 PM Dr. David Alan Gilbert
>         <dgilbert@redhat.com <mailto:dgilbert@redhat.com>> wrote:
> 
>             * Michael Rolnik (mrolnik@gmail.com <mailto:mrolnik@gmail.com>) wrote:
>              > Signed-off-by: Michael Rolnik <mrolnik@gmail.com
>             <mailto:mrolnik@gmail.com>>
>              > ---
>              >  target/avr/cpu-param.h | 8 +-------
>              >  target/avr/helper.c    | 2 --
>              >  2 files changed, 1 insertion(+), 9 deletions(-)
>              >
>              > diff --git a/target/avr/cpu-param.h b/target/avr/cpu-param.h
>              > index 7ef4e7c679..9765a9d0db 100644
>              > --- a/target/avr/cpu-param.h
>              > +++ b/target/avr/cpu-param.h
>              > @@ -22,13 +22,7 @@
>              >  #define AVR_CPU_PARAM_H
>              >
>              >  #define TARGET_LONG_BITS 32
>              > -/*
>              > - * TARGET_PAGE_BITS cannot be more than 8 bits because
>              > - * 1.  all IO registers occupy [0x0000 .. 0x00ff] address
>             range, and they
>              > - *     should be implemented as a device and not memory
>              > - * 2.  SRAM starts at the address 0x0100
> 
>             I don't know AVR; but that seems to say why you can't make it any
>             larger
>             - how do you solve that?
> 
>             Dave
> 
>              > -#define TARGET_PAGE_BITS 8
>              > +#define TARGET_PAGE_BITS 10
>              >  #define TARGET_PHYS_ADDR_SPACE_BITS 24
>              >  #define TARGET_VIRT_ADDR_SPACE_BITS 24
>              >  #define NB_MMU_MODES 2
>              > diff --git a/target/avr/helper.c b/target/avr/helper.c
>              > index 35e1019594..da658afed3 100644
>              > --- a/target/avr/helper.c
>              > +++ b/target/avr/helper.c
>              > @@ -111,8 +111,6 @@ bool avr_cpu_tlb_fill(CPUState *cs, vaddr
>             address, int size,
>              >      MemTxAttrs attrs = {};
>              >      uint32_t paddr;
>              >
>              > -    address &= TARGET_PAGE_MASK;
>              > -
>              >      if (mmu_idx == MMU_CODE_IDX) {
>              >          /* access to code in flash */
>              >          paddr = OFFSET_CODE + address;
>              > --
>              > 2.25.1
>              >
>             -- 
>             Dr. David Alan Gilbert / dgilbert@redhat.com
>             <mailto:dgilbert@redhat.com> / Manchester, UK
> 
> 
> 
>         -- 
>         Best Regards,
>         Michael Rolnik
> 
> 
> 
>     -- 
>     Best Regards,
>     Michael Rolnik
> 
> 
> 
> -- 
> Best Regards,
> Michael Rolnik