On Fri, Feb 7, 2025 at 4:28 AM Paolo Bonzini <pbonz...@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
