| Message ID | 20241230152519.86291-1-philmd@linaro.org |
|---|---|
| Headers | show |
| Series | hppa CPU reset and speedup | expand |
Hi Philippe, On 12/30/24 16:25, Philippe Mathieu-Daudé wrote: > Respin of: > https://lore.kernel.org/qemu-devel/20241229234154.32250-1-deller@kernel.org/ > "Add CPU reset function and speed up runtime and translation." > > - Remove hppa_cpu_init() Thanks for picking up my patches and integrating them properly with the reset code. But sadly your changes somehow break hppa 64-bit CPU support. I think it's to when the reset code is called. Easy reproducer (no other options/disc/network needed): ./qemu-system-hppa -smp cpus=4 -nographic -machine C3700 > - Reset PSW using M bit (untested) We haven't implemented PSW-M support and the first thing the firmware does is to reprogram PSW. So, basically it's not needed. > Helge, could we add a functional test booting Linux? What exactly are you looking for? Some trivial preinstalled disc image with kernels? Any examples? Helge > Helge Deller (4): > target/hppa: Convert hppa_cpu_init() to ResetHold handler > hw/hppa: Reset vCPUs calling resettable_reset() > target/hppa: Set PC on vCPU reset > target/hppa: Speed up hppa_is_pa20() > > Philippe Mathieu-Daudé (1): > target/hppa: Only set PSW 'M' bit on reset > > target/hppa/cpu.h | 11 +++++++++-- > hw/hppa/machine.c | 6 +++--- > target/hppa/cpu.c | 20 +++++++++++++++++--- > 3 files changed, 29 insertions(+), 8 deletions(-) >
On 30/12/24 21:24, Helge Deller wrote: > Hi Philippe, > > On 12/30/24 16:25, Philippe Mathieu-Daudé wrote: >> Respin of: >> https://lore.kernel.org/qemu-devel/20241229234154.32250-1- >> deller@kernel.org/ >> "Add CPU reset function and speed up runtime and translation." >> >> - Remove hppa_cpu_init() > > Thanks for picking up my patches and integrating them properly > with the reset code. > But sadly your changes somehow break hppa 64-bit CPU support. > I think it's to when the reset code is called. > > Easy reproducer (no other options/disc/network needed): > ./qemu-system-hppa -smp cpus=4 -nographic -machine C3700 [*] > >> - Reset PSW using M bit (untested) > > We haven't implemented PSW-M support and the first > thing the firmware does is to reprogram PSW. > So, basically it's not needed. > >> Helge, could we add a functional test booting Linux? > > What exactly are you looking for? > Some trivial preinstalled disc image with kernels? > Any examples? Your [*] example running SeaBIOS is perfect. See tests/functional/test_riscv_opensbi.py, but using: wait_for_console_pattern('SeaBIOS PA-RISC 64-bit Firmware') > > Helge > >> Helge Deller (4): >> target/hppa: Convert hppa_cpu_init() to ResetHold handler >> hw/hppa: Reset vCPUs calling resettable_reset() >> target/hppa: Set PC on vCPU reset >> target/hppa: Speed up hppa_is_pa20() >> >> Philippe Mathieu-Daudé (1): >> target/hppa: Only set PSW 'M' bit on reset >> >> target/hppa/cpu.h | 11 +++++++++-- >> hw/hppa/machine.c | 6 +++--- >> target/hppa/cpu.c | 20 +++++++++++++++++--- >> 3 files changed, 29 insertions(+), 8 deletions(-) >> >
On 30/12/24 21:24, Helge Deller wrote: > Hi Philippe, > > On 12/30/24 16:25, Philippe Mathieu-Daudé wrote: >> Respin of: >> https://lore.kernel.org/qemu-devel/20241229234154.32250-1- >> deller@kernel.org/ >> "Add CPU reset function and speed up runtime and translation." >> >> - Remove hppa_cpu_init() > > Thanks for picking up my patches and integrating them properly > with the reset code. > But sadly your changes somehow break hppa 64-bit CPU support. > I think it's to when the reset code is called. Likely hppa_is_pa20() called *before* CPU reset...? > > Easy reproducer (no other options/disc/network needed): > ./qemu-system-hppa -smp cpus=4 -nographic -machine C3700
On 30/12/24 21:39, Philippe Mathieu-Daudé wrote: > On 30/12/24 21:24, Helge Deller wrote: >> Hi Philippe, >> >> On 12/30/24 16:25, Philippe Mathieu-Daudé wrote: >>> Respin of: >>> https://lore.kernel.org/qemu-devel/20241229234154.32250-1- >>> deller@kernel.org/ >>> "Add CPU reset function and speed up runtime and translation." >>> >>> - Remove hppa_cpu_init() >> >> Thanks for picking up my patches and integrating them properly >> with the reset code. >> But sadly your changes somehow break hppa 64-bit CPU support. >> I think it's to when the reset code is called. > > Likely hppa_is_pa20() called *before* CPU reset...? Indeed: (lldb) bt * thread #1, queue = 'com.apple.main-thread', stop reason = breakpoint 1.10 * frame #0: 0x000000010024df30 qemu-system-hppa`hppa_ptlbe [inlined] hppa_is_pa20(env=0x000000011f822230) at cpu.h:304:17 frame #1: 0x000000010024df30 qemu-system-hppa`hppa_ptlbe [inlined] HPPA_BTLB_ENTRIES(env=0x000000011f822230) at cpu.h:309:12 frame #2: 0x000000010024df30 qemu-system-hppa`hppa_ptlbe(env=0x000000011f822230) at mem_helper.c:642:29 frame #3: 0x0000000100250564 qemu-system-hppa`hppa_cpu_realizefn(dev=0x000000011f81fa00, errp=<unavailable>) at cpu.c:188:9 frame #4: 0x00000001002f812c qemu-system-hppa`device_set_realized(obj=<unavailable>, value=<unavailable>, errp=0x000000016fdfebc0) at qdev.c:495:13 > >> >> Easy reproducer (no other options/disc/network needed): >> ./qemu-system-hppa -smp cpus=4 -nographic -machine C3700
On 30/12/24 21:41, Philippe Mathieu-Daudé wrote: > On 30/12/24 21:39, Philippe Mathieu-Daudé wrote: >> On 30/12/24 21:24, Helge Deller wrote: >>> Hi Philippe, >>> >>> On 12/30/24 16:25, Philippe Mathieu-Daudé wrote: >>>> Respin of: >>>> https://lore.kernel.org/qemu-devel/20241229234154.32250-1- >>>> deller@kernel.org/ >>>> "Add CPU reset function and speed up runtime and translation." >>>> >>>> - Remove hppa_cpu_init() >>> >>> Thanks for picking up my patches and integrating them properly >>> with the reset code. >>> But sadly your changes somehow break hppa 64-bit CPU support. >>> I think it's to when the reset code is called. >> >> Likely hppa_is_pa20() called *before* CPU reset...? > > Indeed: > > (lldb) bt > * thread #1, queue = 'com.apple.main-thread', stop reason = breakpoint 1.10 > * frame #0: 0x000000010024df30 qemu-system-hppa`hppa_ptlbe [inlined] > hppa_is_pa20(env=0x000000011f822230) at cpu.h:304:17 > frame #1: 0x000000010024df30 qemu-system-hppa`hppa_ptlbe [inlined] > HPPA_BTLB_ENTRIES(env=0x000000011f822230) at cpu.h:309:12 > frame #2: 0x000000010024df30 qemu-system- > hppa`hppa_ptlbe(env=0x000000011f822230) at mem_helper.c:642:29 > frame #3: 0x0000000100250564 qemu-system- > hppa`hppa_cpu_realizefn(dev=0x000000011f81fa00, errp=<unavailable>) at > cpu.c:188:9 > frame #4: 0x00000001002f812c qemu-system- > hppa`device_set_realized(obj=<unavailable>, value=<unavailable>, > errp=0x000000016fdfebc0) at qdev.c:495:13 Assigning is_pa20 in instance_init() fixes it: -- >8 -- diff --git a/target/hppa/cpu.c b/target/hppa/cpu.c index 6e5434a8e99..b0bc9d35e4c 100644 --- a/target/hppa/cpu.c +++ b/target/hppa/cpu.c @@ -193,6 +193,13 @@ static void hppa_cpu_realizefn(DeviceState *dev, Error **errp) tcg_cflags_set(cs, CF_PCREL); } +static void hppa_cpu_initfn(Object *obj) +{ + CPUHPPAState *env = cpu_env(CPU(obj)); + + env->is_pa20 = !!object_dynamic_cast(obj, TYPE_HPPA64_CPU); +} + static void hppa_cpu_reset_hold(Object *obj, ResetType type) { HPPACPUClass *scc = HPPA_CPU_GET_CLASS(obj); @@ -209,8 +216,6 @@ static void hppa_cpu_reset_hold(Object *obj, ResetType type) memset(env, 0, offsetof(CPUHPPAState, end_reset_fields)); - env->is_pa20 = !!object_dynamic_cast(obj, TYPE_HPPA64_CPU); - cpu_hppa_loaded_fr0(env); cpu_hppa_put_psw(env, PSW_M); } @@ -284,6 +289,7 @@ static const TypeInfo hppa_cpu_type_infos[] = { .parent = TYPE_CPU, .instance_size = sizeof(HPPACPU), .instance_align = __alignof(HPPACPU), + .instance_init = hppa_cpu_initfn, .abstract = false, .class_size = sizeof(HPPACPUClass), .class_init = hppa_cpu_class_init, ---
On 30/12/24 21:24, Helge Deller wrote: > Hi Philippe, > > On 12/30/24 16:25, Philippe Mathieu-Daudé wrote: >> Respin of: >> https://lore.kernel.org/qemu-devel/20241229234154.32250-1- >> deller@kernel.org/ >> "Add CPU reset function and speed up runtime and translation." >> >> - Remove hppa_cpu_init() > > Thanks for picking up my patches and integrating them properly > with the reset code. >> - Reset PSW using M bit (untested) > > We haven't implemented PSW-M support and the first > thing the firmware does is to reprogram PSW. > So, basically it's not needed. Good to know it isn't needed yet (I grepped and noticed very few uses). Are you OK with the patch as is (as it matches the spec)?
On 12/30/24 21:36, Philippe Mathieu-Daudé wrote: > On 30/12/24 21:24, Helge Deller wrote: >> Hi Philippe, >> >> On 12/30/24 16:25, Philippe Mathieu-Daudé wrote: >>> Respin of: >>> https://lore.kernel.org/qemu-devel/20241229234154.32250-1- deller@kernel.org/ >>> "Add CPU reset function and speed up runtime and translation." >>> >>> - Remove hppa_cpu_init() >> >> Thanks for picking up my patches and integrating them properly >> with the reset code. >> But sadly your changes somehow break hppa 64-bit CPU support. >> I think it's to when the reset code is called. >> >> Easy reproducer (no other options/disc/network needed): >> ./qemu-system-hppa -smp cpus=4 -nographic -machine C3700 > > [*] > >> >>> - Reset PSW using M bit (untested) >> >> We haven't implemented PSW-M support and the first >> thing the firmware does is to reprogram PSW. >> So, basically it's not needed. >> >>> Helge, could we add a functional test booting Linux? >> >> What exactly are you looking for? >> Some trivial preinstalled disc image with kernels? >> Any examples? > > Your [*] example running SeaBIOS is perfect. > > See tests/functional/test_riscv_opensbi.py, but using: > > wait_for_console_pattern('SeaBIOS PA-RISC 64-bit Firmware') Then I suggest 2 testcases (32-bit and 64-bit CPU, dropped SMP flag): ./qemu-system-hppa -nographic -machine B160L ./qemu-system-hppa -nographic -machine C3700 Helge > >> >> Helge >> >>> Helge Deller (4): >>> target/hppa: Convert hppa_cpu_init() to ResetHold handler >>> hw/hppa: Reset vCPUs calling resettable_reset() >>> target/hppa: Set PC on vCPU reset >>> target/hppa: Speed up hppa_is_pa20() >>> >>> Philippe Mathieu-Daudé (1): >>> target/hppa: Only set PSW 'M' bit on reset >>> >>> target/hppa/cpu.h | 11 +++++++++-- >>> hw/hppa/machine.c | 6 +++--- >>> target/hppa/cpu.c | 20 +++++++++++++++++--- >>> 3 files changed, 29 insertions(+), 8 deletions(-) >>> >> >
On 12/30/24 21:47, Philippe Mathieu-Daudé wrote: > On 30/12/24 21:24, Helge Deller wrote: >> Hi Philippe, >> >> On 12/30/24 16:25, Philippe Mathieu-Daudé wrote: >>> Respin of: >>> https://lore.kernel.org/qemu-devel/20241229234154.32250-1- deller@kernel.org/ >>> "Add CPU reset function and speed up runtime and translation." >>> >>> - Remove hppa_cpu_init() >> >> Thanks for picking up my patches and integrating them properly >> with the reset code. > > >>> - Reset PSW using M bit (untested) >> >> We haven't implemented PSW-M support and the first >> thing the firmware does is to reprogram PSW. >> So, basically it's not needed. > > Good to know it isn't needed yet (I grepped and noticed > very few uses). Are you OK with the patch as is (as it > matches the spec)? At least I don't have any objections as long as 32- and 64-bit CPUs still boot. Will you respin the series, then I can test and review? Helge
On 30/12/24 22:30, Helge Deller wrote: > On 12/30/24 21:47, Philippe Mathieu-Daudé wrote: >> On 30/12/24 21:24, Helge Deller wrote: >>> Hi Philippe, >>> >>> On 12/30/24 16:25, Philippe Mathieu-Daudé wrote: >>>> Respin of: >>>> https://lore.kernel.org/qemu-devel/20241229234154.32250-1- >>>> deller@kernel.org/ >>>> "Add CPU reset function and speed up runtime and translation." >>>> >>>> - Remove hppa_cpu_init() >>> >>> Thanks for picking up my patches and integrating them properly >>> with the reset code. >> >> >>>> - Reset PSW using M bit (untested) >>> >>> We haven't implemented PSW-M support and the first >>> thing the firmware does is to reprogram PSW. >>> So, basically it's not needed. >> >> Good to know it isn't needed yet (I grepped and noticed >> very few uses). Are you OK with the patch as is (as it >> matches the spec)? > > At least I don't have any objections as long as 32- and 64-bit > CPUs still boot. > > Will you respin the series, then I can test and review? OK!