| Message ID | 20250206182711.2420505-1-pbonzini@redhat.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | target/riscv: declarative CPU definitions | expand |
On Fri, Feb 7, 2025 at 4:28 AM Paolo Bonzini <pbonzini@redhat.com> wrote: > > Hi Alastair, > > the subject is a slightly underhanded description, in that what I really > wanted to achieve was removing RISC-V's use of .instance_post_init; that's > because RISC-V operate with an opposite conception of .instance_post_init > compared to everyone else: RISC-V wants to register properties there, > whereas x86 and hw/pci-bridge/pcie_root_port.c want to set them. > While it's possible to move RISC-V's code to instance_init, the others > have to run after global properties have been set by device_post_init(). > > However, I think the result is an improvement anyway, in that it makes > CPU definitions entirely declarative. Previously, multiple instance_init > functions each override the properties that were set by the superclass, > and the code used a mix of subclassing and direct invocation of other > functions. Now, instead, after .class_init all the settings for each > model are available in a RISCVCPUDef struct, and the result is copied > into the RISCVCPU at .instance_init time. This is done with a single > function that starts from the parent's RISCVCPUDef and applies the delta > stored in the CPU's class_data. That is nice! I don't love the ifdef-ery around `#include "cpu_cfg_fields.h.inc"` but overall the patches look fine. > > Apart from the small reduction in line count, one advantage is that > more validation of the models can be done unconditionally at startup, > instead of happening dynamically if a CPU model is chosen. > > Tested by running query-cpu-model-expansion on all concrete models, > before and after applying the patches, with no change except the bugfix > noted in patch 10. The 64-bit variant of the script is as follows: > > for i in \ > "max" "max32" "rv32" "rv64" "x-rv128" "rv32i" "rv32e" "rv64i" "rv64e" \ > "rva22u64" "rva22s64" "lowrisc-ibex" "shakti-c" "sifive-e31" "sifive-e34" \ > "sifive-e51" "sifive-u34" "sifive-u54" "thead-c906" "veyron-v1" \ > "tt-ascalon" "xiangshan-nanhu" > do > echo $i > echo " > {'execute': 'qmp_capabilities'} > {'execute': 'query-cpu-model-expansion', 'arguments':{'type': 'full', 'model': {'name': '$i'}}} > {'execute': 'quit'} > " | ./qemu-system-riscv64 -qmp stdio -display none -M none | jq .return.model > list-new/$i > echo " > {'execute': 'qmp_capabilities'} > {'execute': 'query-cpu-model-expansion', 'arguments':{'type': 'full', 'model': {'name': '$i'}}} > {'execute': 'quit'} > " | ../../qemu-rust/+build/qemu-system-riscv64 -qmp stdio -display none -M none | jq .return.model > list-old/$i > done > > Do you think this is a good approach? Seems fine to me :) Alistair
On 2/18/25 01:25, Alistair Francis wrote: > On Fri, Feb 7, 2025 at 4:28 AM Paolo Bonzini <pbonzini@redhat.com> wrote: >> >> Hi Alastair, >> >> the subject is a slightly underhanded description, in that what I really >> wanted to achieve was removing RISC-V's use of .instance_post_init; that's >> because RISC-V operate with an opposite conception of .instance_post_init >> compared to everyone else: RISC-V wants to register properties there, >> whereas x86 and hw/pci-bridge/pcie_root_port.c want to set them. >> While it's possible to move RISC-V's code to instance_init, the others >> have to run after global properties have been set by device_post_init(). >> >> However, I think the result is an improvement anyway, in that it makes >> CPU definitions entirely declarative. Previously, multiple instance_init >> functions each override the properties that were set by the superclass, >> and the code used a mix of subclassing and direct invocation of other >> functions. Now, instead, after .class_init all the settings for each >> model are available in a RISCVCPUDef struct, and the result is copied >> into the RISCVCPU at .instance_init time. This is done with a single >> function that starts from the parent's RISCVCPUDef and applies the delta >> stored in the CPU's class_data. > > That is nice! > > I don't love the ifdef-ery around `#include "cpu_cfg_fields.h.inc"` > but overall the patches look fine. No problem, if you're okay with the final "target/riscv: move SATP modes out of CPUConfig" I can move it earlier in the series and get rid of the #ifdefs in cpu_cfg_fields.h.inc. It's only needed because satp_modes are not merged by the class_init function (they're not even initialized in fact). >> Do you think this is a good approach? > > Seems fine to me :) Ok, then I'll repost as soon as the patches for read-only class_data are ready. Paolo
Hi Alastair, the subject is a slightly underhanded description, in that what I really wanted to achieve was removing RISC-V's use of .instance_post_init; that's because RISC-V operate with an opposite conception of .instance_post_init compared to everyone else: RISC-V wants to register properties there, whereas x86 and hw/pci-bridge/pcie_root_port.c want to set them. While it's possible to move RISC-V's code to instance_init, the others have to run after global properties have been set by device_post_init(). However, I think the result is an improvement anyway, in that it makes CPU definitions entirely declarative. Previously, multiple instance_init functions each override the properties that were set by the superclass, and the code used a mix of subclassing and direct invocation of other functions. Now, instead, after .class_init all the settings for each model are available in a RISCVCPUDef struct, and the result is copied into the RISCVCPU at .instance_init time. This is done with a single function that starts from the parent's RISCVCPUDef and applies the delta stored in the CPU's class_data. Apart from the small reduction in line count, one advantage is that more validation of the models can be done unconditionally at startup, instead of happening dynamically if a CPU model is chosen. Tested by running query-cpu-model-expansion on all concrete models, before and after applying the patches, with no change except the bugfix noted in patch 10. The 64-bit variant of the script is as follows: for i in \ "max" "max32" "rv32" "rv64" "x-rv128" "rv32i" "rv32e" "rv64i" "rv64e" \ "rva22u64" "rva22s64" "lowrisc-ibex" "shakti-c" "sifive-e31" "sifive-e34" \ "sifive-e51" "sifive-u34" "sifive-u54" "thead-c906" "veyron-v1" \ "tt-ascalon" "xiangshan-nanhu" do echo $i echo " {'execute': 'qmp_capabilities'} {'execute': 'query-cpu-model-expansion', 'arguments':{'type': 'full', 'model': {'name': '$i'}}} {'execute': 'quit'} " | ./qemu-system-riscv64 -qmp stdio -display none -M none | jq .return.model > list-new/$i echo " {'execute': 'qmp_capabilities'} {'execute': 'query-cpu-model-expansion', 'arguments':{'type': 'full', 'model': {'name': '$i'}}} {'execute': 'quit'} " | ../../qemu-rust/+build/qemu-system-riscv64 -qmp stdio -display none -M none | jq .return.model > list-old/$i done Do you think this is a good approach? Paolo Paolo Bonzini (22): target/riscv: remove unused macro DEFINE_CPU target/riscv: introduce RISCVCPUDef target/riscv: store RISCVCPUDef struct directly in the class target/riscv: merge riscv_cpu_class_init with the class_base function target/riscv: move RISCVCPUConfig fields to a header file target/riscv: add more RISCVCPUDef fields target/riscv: convert abstract CPU classes to RISCVCPUDef target/riscv: convert profile CPU models to RISCVCPUDef target/riscv: convert bare CPU models to RISCVCPUDef target/riscv: move 128-bit check to TCG realize target/riscv: convert dynamic CPU models to RISCVCPUDef target/riscv: convert SiFive E CPU models to RISCVCPUDef target/riscv: convert ibex CPU models to RISCVCPUDef target/riscv: convert SiFive U models to RISCVCPUDef target/riscv: th: make CSR insertion test a bit more intuitive target/riscv: generalize custom CSR functionality target/riscv: convert TT C906 to RISCVCPUDef target/riscv: convert TT Ascalon to RISCVCPUDef target/riscv: convert Ventana V1 to RISCVCPUDef target/riscv: convert Xiangshan Nanhu to RISCVCPUDef target/riscv: remove .instance_post_init target/riscv: move SATP modes out of CPUConfig target/riscv/cpu-qom.h | 2 + target/riscv/cpu.h | 43 +- target/riscv/cpu_cfg.h | 175 +----- target/riscv/cpu_cfg_fields.h.inc | 163 +++++ hw/riscv/boot.c | 2 +- hw/riscv/virt-acpi-build.c | 6 +- hw/riscv/virt.c | 4 +- target/riscv/cpu.c | 967 ++++++++++++++---------------- target/riscv/csr.c | 2 +- target/riscv/gdbstub.c | 6 +- target/riscv/kvm/kvm-cpu.c | 21 +- target/riscv/machine.c | 2 +- target/riscv/tcg/tcg-cpu.c | 19 +- target/riscv/th_csr.c | 30 +- target/riscv/translate.c | 2 +- 15 files changed, 689 insertions(+), 755 deletions(-) create mode 100644 target/riscv/cpu_cfg_fields.h.inc