Message ID | 20240206134739.15345-1-twiederh@redhat.com |
---|---|
Headers | show |
Series | Generate x86 cpu features | expand |
ping On Tue, 2024-02-06 at 14:47 +0100, Tim Wiederhake wrote: > Synchronizing the list of cpu features and models with qemu is a > recurring > task in libvirt. For x86, this is done by reading qom-list-properties > for > max-x86_64-cpu and manually filtering out everthing that does not > look like > a feature name, as well as parsing target/i386/cpu.c for cpu models. > > This is a flawed, tedious and error-prone procedure. Ideally, qemu > and libvirt would query a common source for cpu feature and model > related information. Meanwhile, converting this information into an > easier > to parse format would help libvirt a lot. > > This patch series converts the cpu feature information present in > target/i386/cpu.c (`feature_word_info`) into a yaml file and adds a > script to generate the c code from this data. > > v1: > https://lists.nongnu.org/archive/html/qemu-devel/2023-08/msg02005.html > v2: > https://lists.nongnu.org/archive/html/qemu-devel/2023-09/msg01773.html > > Changes since v2: > * Rebased on top of current master > * Removed all "drive-by" changes to feature names ("vmx-ept-uc", > "vmx-ept-wb", > "kvmclock", "vmx-invept-single-context", and > "vmx-invept-single-context-noglobals") as these needed further > discussion. > * Changes to the generator script reduce the changes in formatting to > the > current feature_word_info even further to address the concern about > code > legibility. See Patch 5, "target/i386: Generate > feature_word_info.c.inc" for > all non-whitespace changes. > > Tim Wiederhake (5): > target/i386: Split out feature_word_info > target/i386: Translate feature_word_info to yaml > target/i386: Remove comments from feature_word_info.c.inc > target/i386: Fix feature_word_info.c.inc formatting > target/i386: Generate feature_word_info.c.inc > > target/i386/cpu.c | 679 +------------------------- > target/i386/feature_word_info.c.inc | 710 > ++++++++++++++++++++++++++++ > target/i386/feature_word_info.py | 71 +++ > target/i386/feature_word_info.yaml | 701 > +++++++++++++++++++++++++++ > 4 files changed, 1483 insertions(+), 678 deletions(-) > create mode 100644 target/i386/feature_word_info.c.inc > create mode 100755 target/i386/feature_word_info.py > create mode 100644 target/i386/feature_word_info.yaml >
On Tue, Feb 06, 2024 at 02:47:34PM +0100, Tim Wiederhake wrote: > Synchronizing the list of cpu features and models with qemu is a recurring > task in libvirt. For x86, this is done by reading qom-list-properties for > max-x86_64-cpu and manually filtering out everthing that does not look like > a feature name, as well as parsing target/i386/cpu.c for cpu models. > > This is a flawed, tedious and error-prone procedure. Ideally, qemu > and libvirt would query a common source for cpu feature and model > related information. Meanwhile, converting this information into an easier > to parse format would help libvirt a lot. > > This patch series converts the cpu feature information present in > target/i386/cpu.c (`feature_word_info`) into a yaml file and adds a > script to generate the c code from this data. Looking at this fresh, I'm left wondering why I didn't suggested using 'QMP' to expose this information when reviewing the earlier versions. I see Igor did indeed suggest this: https://lists.nongnu.org/archive/html/qemu-devel/2023-09/msg03905.html Your commentry that "qom-list-properties" doesn't distinguish between CPU features and other random QOM properties is bang on the money. I think what this highlights, is that 'qom-list-properties' is a very poor design/fit for the problem that management apps need to solve in this regard. Libvirt should not need to manually exclude non-feature properties like 'check' 'enforce' 'migratable' etc. QEMU already has this knowledge, as IIUC, 'query-cpu-model-expansion' can distinguish this: query-cpu-model-expansion type=static model={'name':'Nehalem'} { "return": { "model": { "name": "base", "props": { "3dnow": false, ...snip... "xtpr": false } } } } We still have the problem that we're not exposing the CPUID/MSR leafs/register bits. So query-cpu-model-expansion isn't a fit for the problem. Rather than try to design something super general purpose, I'd suggest we take a short cut and design something entirley x86 specific, and simply mark the QMP command as "unstable" eg a 'x-query-x86-cpu-model-features', and then basically report all the information libvirt needs there. This is functionally equivalent to what you expose in the YAML file, while still using QEMU's formal 'QMP' API mechanism, so we avoid inventing a new API concept via YAML. I think this would avoid need to have a code generator refactor the CPU definitions too. We just need to expose the values of the existing CPUID_xxx constants against each register. With regards, Daniel
On Tue, 2024-03-05 at 14:17 +0000, Daniel P. Berrangé wrote: > > On Tue, Feb 06, 2024 at 02:47:34PM +0100, Tim Wiederhake wrote: > > > > Synchronizing the list of cpu features and models with qemu is > > > > a > > > > recurring > > > > task in libvirt. For x86, this is done by reading > > > > qom-list-properties for > > > > max-x86_64-cpu and manually filtering out everthing that does > > > > not > > > > look like > > > > a feature name, as well as parsing target/i386/cpu.c for cpu > > > > models. > > > > > > > > This is a flawed, tedious and error-prone procedure. Ideally, > > > > qemu > > > > and libvirt would query a common source for cpu feature and > > > > model > > > > related information. Meanwhile, converting this information > > > > into an > > > > easier > > > > to parse format would help libvirt a lot. > > > > > > > > This patch series converts the cpu feature information present > > > > in > > > > target/i386/cpu.c (`feature_word_info`) into a yaml file and > > > > adds a > > > > script to generate the c code from this data. > > > > Looking at this fresh, I'm left wondering why I didn't suggested > > using 'QMP' to expose this information when reviewing the earlier > > versions. I see Igor did indeed suggest this: > > > > > > https://lists.nongnu.org/archive/html/qemu-devel/2023-09/msg03905.html > > > > Your commentry that "qom-list-properties" doesn't distinguish > > between CPU features and other random QOM properties is bang > > on the money. > > > > I think what this highlights, is that 'qom-list-properties' > > is a very poor design/fit for the problem that management apps > > need to solve in this regard. > > > > Libvirt should not need to manually exclude non-feature properties > > like 'check' 'enforce' 'migratable' etc. > > > > QEMU already has this knowledge, as IIUC, 'query-cpu-model- > > expansion' > > can distinguish this: > > > > query-cpu-model-expansion type=static model={'name':'Nehalem'} > > { > > "return": { > > "model": { > > "name": "base", > > "props": { > > "3dnow": false, > > ...snip... > > "xtpr": false > > } > > } > > } > > } > > > > We still have the problem that we're not exposing the CPUID/MSR > > leafs/register bits. So query-cpu-model-expansion isn't a fit > > for the problem. > > > > Rather than try to design something super general purpose, I'd > > suggest we take a short cut and design something entirley x86 > > specific, and simply mark the QMP command as "unstable" > > eg a 'x-query-x86-cpu-model-features', and then basically > > report all the information libvirt needs there. > > > > This is functionally equivalent to what you expose in the YAML > > file, while still using QEMU's formal 'QMP' API mechanism, so > > we avoid inventing a new API concept via YAML. > > > > I think this would avoid need to have a code generator refactor > > the CPU definitions too. We just need to expose the values of > > the existing CPUID_xxx constants against each register. > > > > > > > > With regards, > > Daniel Thank you for your feedback. I do not see the patches and your proposed x-query-x86-cpu-model- features QMP command being mutually exclusive. In fact, I'd advocate for merging this patches still, as they provide a solution (albeit not through QMP) already whereas the QMP command would still need to be written. Additionally, there are more benefits to the generate-code approach, as the code generator can be extended to also generate the feature bits "#define CPUID_* (1U << ...)" in cpu.h, removing one more source of errors. And with the generated `feature_word_info` structure being virtually identical to the current version, I see no downsides: If the generator does become obsolete in the future, simply remove the python script and the yaml file, and all that is left is the original feature_word_info code, but better formatted. Regards, Tim
On Mon, Mar 11, 2024 at 12:32:03PM +0100, Tim Wiederhake wrote: > On Tue, 2024-03-05 at 14:17 +0000, Daniel P. Berrangé wrote: > > > Looking at this fresh, I'm left wondering why I didn't suggested > > > using 'QMP' to expose this information when reviewing the earlier > > > versions. I see Igor did indeed suggest this: > > > > > > > > > https://lists.nongnu.org/archive/html/qemu-devel/2023-09/msg03905.html > > > > > > Your commentry that "qom-list-properties" doesn't distinguish > > > between CPU features and other random QOM properties is bang > > > on the money. > > > > > > I think what this highlights, is that 'qom-list-properties' > > > is a very poor design/fit for the problem that management apps > > > need to solve in this regard. > > > > > > Libvirt should not need to manually exclude non-feature properties > > > like 'check' 'enforce' 'migratable' etc. > > > > > > QEMU already has this knowledge, as IIUC, 'query-cpu-model- > > > expansion' > > > can distinguish this: > > > > > > query-cpu-model-expansion type=static model={'name':'Nehalem'} > > > { > > > "return": { > > > "model": { > > > "name": "base", > > > "props": { > > > "3dnow": false, > > > ...snip... > > > "xtpr": false > > > } > > > } > > > } > > > } > > > > > > We still have the problem that we're not exposing the CPUID/MSR > > > leafs/register bits. So query-cpu-model-expansion isn't a fit > > > for the problem. > > > > > > Rather than try to design something super general purpose, I'd > > > suggest we take a short cut and design something entirley x86 > > > specific, and simply mark the QMP command as "unstable" > > > eg a 'x-query-x86-cpu-model-features', and then basically > > > report all the information libvirt needs there. > > > > > > This is functionally equivalent to what you expose in the YAML > > > file, while still using QEMU's formal 'QMP' API mechanism, so > > > we avoid inventing a new API concept via YAML. > > > > > > I think this would avoid need to have a code generator refactor > > > the CPU definitions too. We just need to expose the values of > > > the existing CPUID_xxx constants against each register. > > I do not see the patches and your proposed x-query-x86-cpu-model- > features QMP command being mutually exclusive. > > In fact, I'd advocate for merging this patches still, as they provide a > solution (albeit not through QMP) already whereas the QMP command would > still need to be written. I would not class this as an advantage for QEMU in merging this series. It is defining what amounts to a new externally consumable interface for QEMU, which is intended to be used by libvirt. Even if it not consumed at runtime, it is still implying API guarantees. QEMU really does not want to be adding new interfaces, as there is a strong desire to reduce all external interaction with QEMU to exclusively be QMP based. > Additionally, there are more benefits to the > generate-code approach, as the code generator can be extended to also > generate the feature bits "#define CPUID_* (1U << ...)" in cpu.h, > removing one more source of errors. And with the generated > `feature_word_info` structure being virtually identical to the current > version, I see no downsides: If the generator does become obsolete in > the future, simply remove the python script and the yaml file, and all > that is left is the original feature_word_info code, but better > formatted. If we external usage, then we're left with a pretty weak justification for introducing a new custom code generator here IMHO. In terms of the CPUID_ constants, I think what would make more sense from QEMU's POV is to define them all as a QAPI enum. eg { 'enum': 'CPUID', 'data': [ 'fpu', 'vme', 'de', 'pse', 'tsc', ..... ] } which will result in QAPI generating constants CPUID_VME CPUID_FPU CPUID_DE ...etc along with a constant <-> string conversion table. The feature words could use the constants directly instead of their string form FeatureWordInfo feature_word_info[FEATURE_WORDS] = { [FEAT_1_EDX] = { .type = CPUID_FEATURE_WORD, .features = { [0] = CPUID_FPU, [1] = CPUID_VME, [2] = CPUID_DE, ...etc... }, .cpuid = {.eax = 1, .reg = R_EDX, }, .tcg_features = TCG_FEATURES, .no_autoenable_flags = CPUID_HT, }, which I think encodes the information just as effectively as the yaml file does. With regards, Daniel