| Message ID | 20190802160458.25681-2-peter.maydell@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | target/mips: Convert to do_transaction_failed hook | expand |
On 8/2/19 6:04 PM, Peter Maydell wrote: > The MIPS Jazz ('magnum' and 'pica61') boards have some code which > overrides the CPU's do_unassigned_access hook, so they can intercept > it and not raise exceptions on data accesses to invalid addresses, > only for instruction fetches. > > We want to switch MIPS over to using the do_transaction_failed > hook instead, so add an intercept for that as well, and make > the board code install whichever hook the CPU is actually using. > Once we've changed the CPU implementation we can remove the > redundant code for the old hook. > > Note: I am suspicious that the behaviour as implemented here may not > be what the hardware really does. It was added in commit > 54e755588cf1e90f0b14 to restore the behaviour that was broken by > commit c658b94f6e8c206c59d. But prior to commit c658b94f6e8c206c59d > every MIPS board generated exceptions for instruction access to > invalid addresses but not for data accesses; and other boards, > notably Malta, were fixed by making all invalid accesses behave as > reads-as-zero (see the call to empty_slot_init() in > mips_malta_init()). Hardware that raises exceptions for instruction > access and not data access seems to me to be an unlikely design, and > it's possible that the right way to emulate this is to make the Jazz > boards do what we did with Malta (or some variation of that). > Nonetheless, since I don't have access to real hardware to test > against I have taken the approach of "make QEMU continue to behave > the same way it did before this commit". I have updated the comment > to correct the parts that are no longer accurate and note that > the hardware might behave differently. > > The test case for the need for the hook-hijacking is in > https://bugs.launchpad.net/qemu/+bug/1245924 That BIOS will boot OK > either with this overriding of both hooks, or with a simple "global > memory region to ignore bad accesses of all types", so it doesn't > provide evidence either way, unfortunately. > > Signed-off-by: Peter Maydell <peter.maydell@linaro.org> > --- > hw/mips/mips_jazz.c | 54 ++++++++++++++++++++++++++++++++++++++------- > 1 file changed, 46 insertions(+), 8 deletions(-) > > diff --git a/hw/mips/mips_jazz.c b/hw/mips/mips_jazz.c > index fa8775d4284..c64b4c78809 100644 > --- a/hw/mips/mips_jazz.c > +++ b/hw/mips/mips_jazz.c > @@ -123,6 +123,28 @@ static void mips_jazz_do_unassigned_access(CPUState *cpu, hwaddr addr, > (*real_do_unassigned_access)(cpu, addr, is_write, is_exec, opaque, size); > } > > +static void (*real_do_transaction_failed)(CPUState *cpu, hwaddr physaddr, > + vaddr addr, unsigned size, > + MMUAccessType access_type, > + int mmu_idx, MemTxAttrs attrs, > + MemTxResult response, > + uintptr_t retaddr); > + > +static void mips_jazz_do_transaction_failed(CPUState *cs, hwaddr physaddr, > + vaddr addr, unsigned size, > + MMUAccessType access_type, > + int mmu_idx, MemTxAttrs attrs, > + MemTxResult response, > + uintptr_t retaddr) > +{ > + if (access_type != MMU_INST_FETCH) { > + /* ignore invalid access (ie do not raise exception) */ > + return; > + } > + (*real_do_transaction_failed)(cs, physaddr, addr, size, access_type, > + mmu_idx, attrs, response, retaddr); > +} > + > static void mips_jazz_init(MachineState *machine, > enum jazz_model_e jazz_model) > { > @@ -157,16 +179,32 @@ static void mips_jazz_init(MachineState *machine, > env = &cpu->env; > qemu_register_reset(main_cpu_reset, cpu); > > - /* Chipset returns 0 in invalid reads and do not raise data exceptions. > + /* > + * Chipset returns 0 in invalid reads and do not raise data exceptions. > * However, we can't simply add a global memory region to catch > - * everything, as memory core directly call unassigned_mem_read/write > - * on some invalid accesses, which call do_unassigned_access on the > - * CPU, which raise an exception. > - * Handle that case by hijacking the do_unassigned_access method on > - * the CPU, and do not raise exceptions for data access. */ > + * everything, as this would make all accesses including instruction > + * accesses be ignored and not raise exceptions. > + * So instead we hijack either the do_unassigned_access method or > + * the do_transaction_failed method on the CPU, and do not raise exceptions > + * for data access. > + * > + * NOTE: this behaviour of raising exceptions for bad instruction > + * fetches but not bad data accesses was added in commit 54e755588cf1e9 > + * to restore behaviour broken by c658b94f6e8c206, but it is not clear > + * whether the real hardware behaves this way. It is possible that > + * real hardware ignores bad instruction fetches as well -- if so then > + * we could replace this hijacking of CPU methods with a simple global > + * memory region that catches all memory accesses, as we do on Malta. Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com> > + */ > cc = CPU_GET_CLASS(cpu); > - real_do_unassigned_access = cc->do_unassigned_access; > - cc->do_unassigned_access = mips_jazz_do_unassigned_access; > + if (cc->do_unassigned_access) { > + real_do_unassigned_access = cc->do_unassigned_access; > + cc->do_unassigned_access = mips_jazz_do_unassigned_access; > + } > + if (cc->do_transaction_failed) { > + real_do_transaction_failed = cc->do_transaction_failed; > + cc->do_transaction_failed = mips_jazz_do_transaction_failed; > + } > > /* allocate RAM */ > memory_region_allocate_system_memory(ram, NULL, "mips_jazz.ram", >
Le 02/08/2019 à 18:04, Peter Maydell a écrit : > The MIPS Jazz ('magnum' and 'pica61') boards have some code which > overrides the CPU's do_unassigned_access hook, so they can intercept > it and not raise exceptions on data accesses to invalid addresses, > only for instruction fetches. > > We want to switch MIPS over to using the do_transaction_failed > hook instead, so add an intercept for that as well, and make > the board code install whichever hook the CPU is actually using. > Once we've changed the CPU implementation we can remove the > redundant code for the old hook. > > Note: I am suspicious that the behaviour as implemented here may not > be what the hardware really does. It was added in commit > 54e755588cf1e90f0b14 to restore the behaviour that was broken by > commit c658b94f6e8c206c59d. But prior to commit c658b94f6e8c206c59d > every MIPS board generated exceptions for instruction access to > invalid addresses but not for data accesses; and other boards, > notably Malta, were fixed by making all invalid accesses behave as > reads-as-zero (see the call to empty_slot_init() in > mips_malta_init()). Hardware that raises exceptions for instruction > access and not data access seems to me to be an unlikely design, and > it's possible that the right way to emulate this is to make the Jazz > boards do what we did with Malta (or some variation of that). > Nonetheless, since I don't have access to real hardware to test > against I have taken the approach of "make QEMU continue to behave > the same way it did before this commit". I have updated the comment > to correct the parts that are no longer accurate and note that > the hardware might behave differently. > > The test case for the need for the hook-hijacking is in > https://bugs.launchpad.net/qemu/+bug/1245924 That BIOS will boot OK > either with this overriding of both hooks, or with a simple "global > memory region to ignore bad accesses of all types", so it doesn't > provide evidence either way, unfortunately. > > Signed-off-by: Peter Maydell <peter.maydell@linaro.org> Tested-by: Hervé Poussineau <hpoussin@reactos.org>
diff --git a/hw/mips/mips_jazz.c b/hw/mips/mips_jazz.c index fa8775d4284..c64b4c78809 100644 --- a/hw/mips/mips_jazz.c +++ b/hw/mips/mips_jazz.c @@ -123,6 +123,28 @@ static void mips_jazz_do_unassigned_access(CPUState *cpu, hwaddr addr, (*real_do_unassigned_access)(cpu, addr, is_write, is_exec, opaque, size); } +static void (*real_do_transaction_failed)(CPUState *cpu, hwaddr physaddr, + vaddr addr, unsigned size, + MMUAccessType access_type, + int mmu_idx, MemTxAttrs attrs, + MemTxResult response, + uintptr_t retaddr); + +static void mips_jazz_do_transaction_failed(CPUState *cs, hwaddr physaddr, + vaddr addr, unsigned size, + MMUAccessType access_type, + int mmu_idx, MemTxAttrs attrs, + MemTxResult response, + uintptr_t retaddr) +{ + if (access_type != MMU_INST_FETCH) { + /* ignore invalid access (ie do not raise exception) */ + return; + } + (*real_do_transaction_failed)(cs, physaddr, addr, size, access_type, + mmu_idx, attrs, response, retaddr); +} + static void mips_jazz_init(MachineState *machine, enum jazz_model_e jazz_model) { @@ -157,16 +179,32 @@ static void mips_jazz_init(MachineState *machine, env = &cpu->env; qemu_register_reset(main_cpu_reset, cpu); - /* Chipset returns 0 in invalid reads and do not raise data exceptions. + /* + * Chipset returns 0 in invalid reads and do not raise data exceptions. * However, we can't simply add a global memory region to catch - * everything, as memory core directly call unassigned_mem_read/write - * on some invalid accesses, which call do_unassigned_access on the - * CPU, which raise an exception. - * Handle that case by hijacking the do_unassigned_access method on - * the CPU, and do not raise exceptions for data access. */ + * everything, as this would make all accesses including instruction + * accesses be ignored and not raise exceptions. + * So instead we hijack either the do_unassigned_access method or + * the do_transaction_failed method on the CPU, and do not raise exceptions + * for data access. + * + * NOTE: this behaviour of raising exceptions for bad instruction + * fetches but not bad data accesses was added in commit 54e755588cf1e9 + * to restore behaviour broken by c658b94f6e8c206, but it is not clear + * whether the real hardware behaves this way. It is possible that + * real hardware ignores bad instruction fetches as well -- if so then + * we could replace this hijacking of CPU methods with a simple global + * memory region that catches all memory accesses, as we do on Malta. + */ cc = CPU_GET_CLASS(cpu); - real_do_unassigned_access = cc->do_unassigned_access; - cc->do_unassigned_access = mips_jazz_do_unassigned_access; + if (cc->do_unassigned_access) { + real_do_unassigned_access = cc->do_unassigned_access; + cc->do_unassigned_access = mips_jazz_do_unassigned_access; + } + if (cc->do_transaction_failed) { + real_do_transaction_failed = cc->do_transaction_failed; + cc->do_transaction_failed = mips_jazz_do_transaction_failed; + } /* allocate RAM */ memory_region_allocate_system_memory(ram, NULL, "mips_jazz.ram",
The MIPS Jazz ('magnum' and 'pica61') boards have some code which overrides the CPU's do_unassigned_access hook, so they can intercept it and not raise exceptions on data accesses to invalid addresses, only for instruction fetches. We want to switch MIPS over to using the do_transaction_failed hook instead, so add an intercept for that as well, and make the board code install whichever hook the CPU is actually using. Once we've changed the CPU implementation we can remove the redundant code for the old hook. Note: I am suspicious that the behaviour as implemented here may not be what the hardware really does. It was added in commit 54e755588cf1e90f0b14 to restore the behaviour that was broken by commit c658b94f6e8c206c59d. But prior to commit c658b94f6e8c206c59d every MIPS board generated exceptions for instruction access to invalid addresses but not for data accesses; and other boards, notably Malta, were fixed by making all invalid accesses behave as reads-as-zero (see the call to empty_slot_init() in mips_malta_init()). Hardware that raises exceptions for instruction access and not data access seems to me to be an unlikely design, and it's possible that the right way to emulate this is to make the Jazz boards do what we did with Malta (or some variation of that). Nonetheless, since I don't have access to real hardware to test against I have taken the approach of "make QEMU continue to behave the same way it did before this commit". I have updated the comment to correct the parts that are no longer accurate and note that the hardware might behave differently. The test case for the need for the hook-hijacking is in https://bugs.launchpad.net/qemu/+bug/1245924 That BIOS will boot OK either with this overriding of both hooks, or with a simple "global memory region to ignore bad accesses of all types", so it doesn't provide evidence either way, unfortunately. Signed-off-by: Peter Maydell <peter.maydell@linaro.org> --- hw/mips/mips_jazz.c | 54 ++++++++++++++++++++++++++++++++++++++------- 1 file changed, 46 insertions(+), 8 deletions(-)