Re: [Qemu-devel] [PATCH 0/7] target/sparc: Convert to do_transaction_failed hook

[Mark Cave-Ayland]

On 01/08/2019 19:30, Peter Maydell wrote:

> This patchset converts the SPARC target away from the old
> broken do_unassigned_access hook to the new (added in 2017...)
> do_transaction_failed hook. In the process it fixes a number
> of bugs in corner cases.
> 
> The SPARC ld/st-with-ASI helper functions are odd in that they
> make use of the cpu_unassigned_access() function as a way of
> raising an MMU fault. We start by making them just directly
> call a new sparc_raise_mmu_fault() function so they don't go
> through the hook function. This in-passing fixes a bug where
> the hook was using GETPC(), which won't work inside a function
> several levels deep in the callstack from a helper function.
> 
> The next four patches convert places that were using ld*_phys()
> and st*_phys() and thus getting "implicitly causes an exception
> via do_unassigned_access if it gets a bus error" behaviour.
> We make them all use address_space_ld*/st* functions instead,
> and explicitly handle the transaction-failure case. Variously:
>  * for MMU passthrough, this doesn't change behaviour
>  * for the MXCC stream source/destination ASI accesses,
>    this doesn't change behaviour, but the current behaviour
>    looks a bit weird, so a TODO comment is left in case anybody
>    wants to chase up the actual right behaviour in future
>  * for page table walks this fixes a bug where we would take
>    an exception instead of reporting the page table walk failure
>    with the correct fault status register information
>  * for the page table walk in mmu_probe() this fixes a bug where
>    we would take an exception when we are supposed to just report
>    failure. Note that the spec says that MMU probe operations
>    are supposed to set the fault status registers, but we do not;
>    again I leave this pre-existing bug as a TODO note.
> Next, we remove one entirely pointless and unused ldl_phys()
> call from dump_mmu().
> 
> Finally, the last patch can do the very small amount of work to
> change over to the new hook function. This will cause all the
> "handle error" code paths in the preceding patches to become
> active (when a do_unassigned_access hook is present the load
> or store functions will never return an error, because the hook
> will get called and throw an exception first).
> 
> I have tested that I can boot a sparc32 debian image, and
> that sparc64 boots up to the firmware prompt, but this could
> certainly use more extensive testing than I have given it.
> 
> (After SPARC, the only remaining unassigned-access-hook users
> are RISCV, which crept in while I wasn't looking, and MIPS,
> which is doing something complicated with the Jazz board that
> I haven't yet investigated.)
> 
> thanks
> --PMM
> 
> Peter Maydell (7):
>   target/sparc: Factor out the body of sparc_cpu_unassigned_access()
>   target/sparc: Check for transaction failures in MMU passthrough ASIs
>   target/sparc: Check for transaction failures in MXCC stream ASI
>     accesses
>   target/sparc: Correctly handle bus errors in page table walks
>   target/sparc: Handle bus errors in mmu_probe()
>   target/sparc: Remove unused ldl_phys from dump_mmu()
>   target/sparc: Switch to do_transaction_failed() hook
> 
>  target/sparc/cpu.h         |   8 +-
>  target/sparc/cpu.c         |   2 +-
>  target/sparc/ldst_helper.c | 319 +++++++++++++++++++++----------------
>  target/sparc/mmu_helper.c  |  57 +++++--
>  4 files changed, 238 insertions(+), 148 deletions(-)

I've just run this through my SPARC test images with a fairly recent git master 
and I
don't see any obvious regressions so:

Tested-by: Mark Cave-Ayland <address@hidden>

Thanks for taking the time to do the conversion for the SPARC target.


ATB,

Mark.