[virt-tools-list] [virt-manager PATCH 1/2] virt-install: add param 'cachemode' for option '--cpu'

[Cole Robinson]

Firstly I suggest for this instance where the test case is small to just
squash patch #2 into this one.

On 08/21/2017 06:17 AM, Lin Ma wrote:
> libvirt supports guest CPU cache by commit df13c0b, So add this feature
> to virt-install to configure cpu L3 cache mode.
> GUI support will be added later.
> 

This 'GUI support' comment doesn't really need to go into the commit message.
Better in the cover letter or after the ---

That said, I need to know more about the feature before I can give my opinion
on whether it goes in the GUI. We keep the CPU config pretty lax at the
moment. So maybe you can explain a bit what are the benefits/tradeoffs of the
different cache configuration values? A pointer to docs is fine too

> Currently, The valid value are 'passthrough', 'emulate' or 'disable'.
> say:
> --cpu host-passthrough,cachemode=passthrough
> --cpu $CPU,cachemode=emulate
> --cpu $CPU,cachemode=disable
> 

Since it seems like the libvirt code could be adapted to handle multiple
<cache> lines at some point, better to make the option name 'cache.mode'. And
also add 'cache.level' while you are here too

> Signed-off-by: Lin Ma <lma at suse.com>
> ---
>  man/virt-install.pod |  4 ++++
>  virtinst/cli.py      | 12 ++++++++++--
>  virtinst/cpu.py      | 29 +++++++++++++++++++++++++++++
>  3 files changed, 43 insertions(+), 2 deletions(-)
> 
> diff --git a/man/virt-install.pod b/man/virt-install.pod
> index 3482e53..49ee9b8 100644
> --- a/man/virt-install.pod
> +++ b/man/virt-install.pod
> @@ -247,6 +247,10 @@ Example of specifying two NUMA cells. This will generate XML like:
>      </numa>
>    </cpu>
>  
> +=item B<--cpu host-passthrough,cachemode=passthrough>
> +
> +Example of passing through the host cpu's cache information.
> +
>  =back
>  
>  Use --cpu=? to see a list of all available sub options. Complete details at L<http://libvirt.org/formatdomain.html#elementsCPU>
> diff --git a/virtinst/cli.py b/virtinst/cli.py
> index ece9b86..786395f 100644
> --- a/virtinst/cli.py
> +++ b/virtinst/cli.py
> @@ -617,8 +617,9 @@ def vcpu_cli_options(grp, backcompat=True, editexample=False):
>      if editexample:
>          extramsg = "--cpu host-model,clearxml=yes"
>      grp.add_argument("--cpu",
> -        help=_("CPU model and features. Ex:\n"
> -               "--cpu coreduo,+x2apic\n") + extramsg)
> +        help=_("CPU model, L3 cache mode and features. Ex:\n"
> +               "--cpu coreduo,+x2apic\n"
> +               "--cpu host-passthrough,cachemode=passthrough\n") + extramsg)
>  

I don't think it's worth it to add an example of cachemode= to the --help
output. I try to keep that output slim and generally only targeting common
usecases or demonstrating weird option syntax

Though I think it's a good idea to add a --cpu host-passthrough example to the
--help output but that can be its own patch

>      if backcompat:
>          grp.add_argument("--check-cpu", action="store_true",
> @@ -1467,6 +1468,10 @@ class ParserCPU(VirtCLIParser):
>              else:
>                  inst.add_feature(feature_name, policy)
>  
> +    def set_l3_cache_cb(self, inst, val, virtarg):
> +        cpu = inst
> +        cpu.set_l3_cache_mode(cpu, val)
> +
>      def _parse(self, inst):
>          # Convert +feature, -feature into expected format
>          for key, value in self.optdict.items():
> @@ -1508,6 +1513,9 @@ ParserCPU.add_arg("cpus", "cell[0-9]*.cpus", can_comma=True,
>  ParserCPU.add_arg("memory", "cell[0-9]*.memory",
>                    find_inst_cb=ParserCPU.cell_find_inst_cb)
>  
> +# Options for CPU.cache
> +ParserCPU.add_arg(None, "cachemode", cb=ParserCPU.set_l3_cache_cb)
> +
>  
>  ###################
>  # --vcpus parsing #
> diff --git a/virtinst/cpu.py b/virtinst/cpu.py
> index 468853f..ec46452 100644
> --- a/virtinst/cpu.py
> +++ b/virtinst/cpu.py
> @@ -32,6 +32,20 @@ class _CPUCell(XMLBuilder):
>      memory = XMLProperty("./@memory", is_int=True)
>  
>  
> +class CPUCache(XMLBuilder):
> +    """
> +    Class for generating <cpu> child <cache> XML
> +    """
> +
> +    MODES = ["passthrough", "emulate", "disable"]
> +
> +    _XML_ROOT_NAME = "cache"
> +    _XML_PROP_ORDER = ["level", "mode"]
> +
> +    level = XMLProperty("./@level")
> +    mode = XMLProperty("./@mode")
> +
> +
>  class CPUFeature(XMLBuilder):
>      """
>      Class for generating <cpu> child <feature> XML
> @@ -107,6 +121,21 @@ class CPU(XMLBuilder):
>          self.add_child(obj)
>          return obj
>  
> +    cache = XMLChildProperty(CPUCache)
> +    def set_l3_cache_mode(self, cpu, cache_mode):
> +        cache = CPUCache(self.conn)
> +        if cache_mode not in ["emulate", "passthrough", "disable"]:
> +            raise RuntimeError("valid cache mode: 'passthrough' or "
> +                "'emulate' or 'disable'")

Libvirt will already validate that for us, let's not duplicate it here.

> +        if cache_mode == "emulate":
> +            cache.level = "3"

Is cache_mode == "emulate" always going to map to level=3? Or is that the only
valid combination at the moment? If it is potentially going to grow to support
different level= values in the future, I'd rather require the user to specify
cache.level=3 explicitly on the command line than trying to get magic about
it, especially if it's going to be an uncommon operation

> +        elif (cache_mode == "passthrough" and
> +              cpu.mode != self.SPECIAL_MODE_HOST_PASSTHROUGH):
> +            raise RuntimeError("cache mode 'passthrough' requires "
> +                "CPU model 'host-passthrough'")

If libvirt validates this for us, let's not duplicate it. If libvirt doesn't
validate this for us and it's indeed and invalid combination, we should fix it
there

Thanks,
Cole