[PATCH 11/32] sync_qemu_models_i386: Copy signatures from base model

Jiri Denemark posted 32 patches 2 weeks ago
There is a newer version of this series
[PATCH 11/32] sync_qemu_models_i386: Copy signatures from base model
Posted by Jiri Denemark 2 weeks ago
The signatures in the CPU map are used for matching physical CPUs and
thus we need to cover all possible real world variants we know about.
When adding a new version of an existing CPU model, we should copy the
signature(s) of the existing model rather than replacing it with the
signature that QEMU uses.

Signed-off-by: Jiri Denemark <jdenemar@redhat.com>
---
 src/cpu_map/sync_qemu_models_i386.py | 46 ++++++++++++++++++++++++----
 1 file changed, 40 insertions(+), 6 deletions(-)

diff --git a/src/cpu_map/sync_qemu_models_i386.py b/src/cpu_map/sync_qemu_models_i386.py
index 40d71e66be..9b7cb8a047 100755
--- a/src/cpu_map/sync_qemu_models_i386.py
+++ b/src/cpu_map/sync_qemu_models_i386.py
@@ -429,7 +429,30 @@ def transform(item):
     raise RuntimeError("unexpected item type")
 
 
-def expand_model(model):
+def get_signature(outdir, model):
+    file = os.path.join(outdir, f"x86_{model}.xml")
+
+    if not os.path.isfile(file):
+        return None
+
+    xml = lxml.etree.parse(file)
+
+    signature = []
+    for sig in xml.xpath("//signature"):
+        attr = sig.attrib
+        family = attr["family"]
+        model = attr["model"]
+        if "stepping" in attr:
+            stepping = attr["stepping"]
+        else:
+            stepping = None
+
+        signature.append((family, model, stepping))
+
+    return signature
+
+
+def expand_model(outdir, model):
     """Expand a qemu cpu model description that has its feature split up into
     different fields and may have differing versions into several libvirt-
     friendly cpu models."""
@@ -438,11 +461,14 @@ def expand_model(model):
         "name": model.pop(".name"),
         "vendor": translate_vendor(model.pop(".vendor")),
         "features": set(),
-        "extra": dict()}
+        "extra": dict(),
+        "signature": list(),
+    }
 
     if ".family" in model and ".model" in model:
-        result["family"] = model.pop(".family")
-        result["model"] = model.pop(".model")
+        result["signature"].append((model.pop(".family"),
+                                    model.pop(".model"),
+                                    None))
 
     for k in [k for k in model if k.startswith(".features")]:
         v = model.pop(k)
@@ -469,6 +495,10 @@ def expand_model(model):
         if not alias and ver == 1:
             alias = name
 
+            sig = get_signature(outdir, name)
+            if sig:
+                result["signature"] = sig
+
         props = version.pop(".props", dict())
         for k, v in props:
             if k not in ("model-id", "stepping", "model"):
@@ -525,7 +555,11 @@ def output_model(f, extra, model):
     if alias:
         f.write(f"    <model name='{model['alias']}'/>\n")
     else:
-        f.write(f"    <signature family='{model['family']}' model='{model['model']}'/>\n")
+        for sig_family, sig_model, sig_stepping in model['signature']:
+            f.write(f"    <signature family='{sig_family}' model='{sig_model}'")
+            if sig_stepping:
+                f.write(f" stepping='{sig_stepping}'")
+            f.write("/>\n")
         f.write(f"    <vendor name='{model['vendor']}'/>\n")
 
     for feature in sorted(model["features"]):
@@ -601,7 +635,7 @@ def main():
 
     models = list()
     for model in models_json:
-        models.extend(expand_model(model))
+        models.extend(expand_model(args.outdir, model))
 
     files = dict()
 
-- 
2.47.0
Re: [PATCH 11/32] sync_qemu_models_i386: Copy signatures from base model
Posted by Daniel P. Berrangé 1 week, 6 days ago
On Tue, Nov 19, 2024 at 07:49:47PM +0100, Jiri Denemark wrote:
> The signatures in the CPU map are used for matching physical CPUs and
> thus we need to cover all possible real world variants we know about.
> When adding a new version of an existing CPU model, we should copy the
> signature(s) of the existing model rather than replacing it with the
> signature that QEMU uses.

Also sanity check that the signature QEMU uses is part of
libvirt's existing recorded signature list ?

> 
> Signed-off-by: Jiri Denemark <jdenemar@redhat.com>
> ---
>  src/cpu_map/sync_qemu_models_i386.py | 46 ++++++++++++++++++++++++----
>  1 file changed, 40 insertions(+), 6 deletions(-)
> 
> diff --git a/src/cpu_map/sync_qemu_models_i386.py b/src/cpu_map/sync_qemu_models_i386.py
> index 40d71e66be..9b7cb8a047 100755
> --- a/src/cpu_map/sync_qemu_models_i386.py
> +++ b/src/cpu_map/sync_qemu_models_i386.py
> @@ -429,7 +429,30 @@ def transform(item):
>      raise RuntimeError("unexpected item type")
>  
>  
> -def expand_model(model):
> +def get_signature(outdir, model):
> +    file = os.path.join(outdir, f"x86_{model}.xml")
> +
> +    if not os.path.isfile(file):
> +        return None
> +
> +    xml = lxml.etree.parse(file)
> +
> +    signature = []
> +    for sig in xml.xpath("//signature"):
> +        attr = sig.attrib
> +        family = attr["family"]
> +        model = attr["model"]
> +        if "stepping" in attr:
> +            stepping = attr["stepping"]
> +        else:
> +            stepping = None
> +
> +        signature.append((family, model, stepping))
> +
> +    return signature
> +
> +
> +def expand_model(outdir, model):
>      """Expand a qemu cpu model description that has its feature split up into
>      different fields and may have differing versions into several libvirt-
>      friendly cpu models."""
> @@ -438,11 +461,14 @@ def expand_model(model):
>          "name": model.pop(".name"),
>          "vendor": translate_vendor(model.pop(".vendor")),
>          "features": set(),
> -        "extra": dict()}
> +        "extra": dict(),
> +        "signature": list(),
> +    }
>  
>      if ".family" in model and ".model" in model:
> -        result["family"] = model.pop(".family")
> -        result["model"] = model.pop(".model")
> +        result["signature"].append((model.pop(".family"),
> +                                    model.pop(".model"),
> +                                    None))
>  
>      for k in [k for k in model if k.startswith(".features")]:
>          v = model.pop(k)
> @@ -469,6 +495,10 @@ def expand_model(model):
>          if not alias and ver == 1:
>              alias = name
>  
> +            sig = get_signature(outdir, name)
> +            if sig:

Assert here that 'result["signature"' is part of 'sig',
before replacing it ?

> +                result["signature"] = sig



With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|
Re: [PATCH 11/32] sync_qemu_models_i386: Copy signatures from base model
Posted by Jiri Denemark 1 week, 5 days ago
On Wed, Nov 20, 2024 at 12:16:26 +0000, Daniel P. Berrangé wrote:
> On Tue, Nov 19, 2024 at 07:49:47PM +0100, Jiri Denemark wrote:
> > The signatures in the CPU map are used for matching physical CPUs and
> > thus we need to cover all possible real world variants we know about.
> > When adding a new version of an existing CPU model, we should copy the
> > signature(s) of the existing model rather than replacing it with the
> > signature that QEMU uses.
> 
> Also sanity check that the signature QEMU uses is part of
> libvirt's existing recorded signature list ?

I don't think this is needed. CPU model versions in QEMU do not have the
ability to change the signature so they inherit it from the base model.
And the base model is either new to libvirt in which case we will use
the signature from QEMU or it is an existing model with signatures
configured according to real world CPUs and it doesn't really matter
whether QEMU agrees with it.

Remember the CPU signature is only used for selecting the best CPU model
for describing a host CPU and the relation to the numbers used by QEMU
is irrelevant. Well, unless you are nested and want to detect a host CPU
model for the "host" CPU created by QEMU. But anyway, I'm pretty sure
libvirt signatures include the one used by QEMU for all models in our
CPU map.

Jirka