On Wed, Nov 20, 2024 at 12:11:19 +0000, Daniel P. Berrangé wrote: > On Tue, Nov 19, 2024 at 07:49:45PM +0100, Jiri Denemark wrote: > > We already visually group the included models according to vendor using > > comments. This patch introduces a new <group> element for doing it > > properly in a machine friendly way. > > AFAICT the <group> has no functional effect I added it so the following commit can automatically add new CPU models in the right place. So yes, no functional effect for the CPU map itself. > > > > > Signed-off-by: Jiri Denemark <jdenemar@xxxxxxxxxx> > > --- > > src/cpu/cpu_map.c | 2 +- > > src/cpu_map/index.xml | 226 ++++++++++++++++++++++-------------------- > > 2 files changed, 121 insertions(+), 107 deletions(-) > > > diff --git a/src/cpu_map/index.xml b/src/cpu_map/index.xml > > index 15cb63afe5..2cb97a83ba 100644 > > --- a/src/cpu_map/index.xml > > +++ b/src/cpu_map/index.xml > > @@ -3,122 +3,136 @@ > > <include filename='x86_vendors.xml'/> > > <include filename='x86_features.xml'/> > > > + > > + <group name='Intel-based QEMU generic CPU models'> > > + <include filename='x86_pentium.xml'/> > > + <include filename='x86_pentium2.xml'/> > > + <include filename='x86_pentium3.xml'/> > > + <include filename='x86_pentiumpro.xml'/> > > + <include filename='x86_coreduo.xml'/> > > + <include filename='x86_n270.xml'/> > > + <include filename='x86_core2duo.xml'/> > > + </group> > > + > > + <group name='Generic QEMU CPU models'> > > + <include filename='x86_qemu32.xml'/> > > + <include filename='x86_kvm32.xml'/> > > + <include filename='x86_cpu64-rhel5.xml'/> > > + <include filename='x86_cpu64-rhel6.xml'/> > > + <include filename='x86_qemu64.xml'/> > > + <include filename='x86_kvm64.xml'/> > > + </group> > > + > > + <group vendor='Intel'> > > In some cases youve used 'name' and some 'vendor', which seems > fairly arbitrary, but I guess we're ignoring this attrbiute > entirely. > > Still all the CPUs above have either 'Intel' or 'AMD' set > as their vendor in QEMU, so separating them off feels a > bit odd. Yes, it's completely arbitrary. I basically just changed the comments into <group name='the text of the comment'/>, with the exception of a few groups to which the script is going to add new models. I agree it's odd, but after playing with XPath and trying to come up with a way to use the existing comments to detect where new models should be added this solution looked very clean and nice :-) I guess I could also go with just <group name='the text of the comment'/> in all cases for consistency. BTW, the XPath way almost works as you can really reference a comment in the document and even match its content. I just didn't find a way to select a node that is after a specific comment, but before another comment. Which I guess is a good thing as it was pretty disgusting :-) > If we're just going to group everything based on vendor, > why not just call the tag <vendor name=...> ? We can't use just vendor because some models (the old ones) don't have a vendor. We could perhaps use something like vendor='generic', vendor='QEMU' or something similar, although using the complete text from the comment makes reading a bit easier for people. Jirka
