| Message ID | 20210320220949.40965-2-mrolnik@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Set AVR TARGET_PAGE_BITS to be 10 instead of 8 | expand |
* Michael Rolnik (mrolnik@gmail.com) wrote: > Signed-off-by: Michael Rolnik <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 >
how long? On Tue, Mar 23, 2021 at 2:46 PM Dr. David Alan Gilbert <dgilbert@redhat.com> wrote: > * Michael Rolnik (mrolnik@gmail.com) wrote: > > Signed-off-by: Michael Rolnik <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 / Manchester, UK > >
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> wrote: > how long? > > On Tue, Mar 23, 2021 at 2:46 PM Dr. David Alan Gilbert < > dgilbert@redhat.com> wrote: > >> * Michael Rolnik (mrolnik@gmail.com) wrote: >> > Signed-off-by: Michael Rolnik <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 / Manchester, UK >> >> > > -- > Best Regards, > Michael Rolnik >
Please review. On Tue, Mar 23, 2021 at 10:28 PM Michael Rolnik <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> wrote: > >> how long? >> >> On Tue, Mar 23, 2021 at 2:46 PM Dr. David Alan Gilbert < >> dgilbert@redhat.com> wrote: >> >>> * Michael Rolnik (mrolnik@gmail.com) wrote: >>> > Signed-off-by: Michael Rolnik <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 / Manchester, UK >>> >>> >> >> -- >> Best Regards, >> Michael Rolnik >> > > > -- > Best Regards, > Michael Rolnik >
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
On Sun, 11 Apr 2021 at 16:15, Richard Henderson <richard.henderson@linaro.org> wrote: > > 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. Migration is irrelevant, but every target benefits from snapshot save-and-restore, and I think that uses the same codepaths ? -- PMM
* Peter Maydell (peter.maydell@linaro.org) wrote: > On Sun, 11 Apr 2021 at 16:15, Richard Henderson > <richard.henderson@linaro.org> wrote: > > > > 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. > > Migration is irrelevant, but every target benefits from snapshot > save-and-restore, and I think that uses the same codepaths ? Yes it does. My main problem for wanting this fixed is that I really wanted to add an assert to stop us tripping over the page size/migration bits clash. Dave > -- PMM >
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 - */ -#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;
Signed-off-by: Michael Rolnik <mrolnik@gmail.com> --- target/avr/cpu-param.h | 8 +------- target/avr/helper.c | 2 -- 2 files changed, 1 insertion(+), 9 deletions(-)