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@xxxxxxxx> > --- > 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 _______________________________________________ virt-tools-list mailing list virt-tools-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/virt-tools-list
