On Sat, Jun 11, 2016 at 12:13:49PM -0400, Cole Robinson wrote: > On 06/11/2016 12:09 PM, Cole Robinson wrote: > > On 06/11/2016 11:36 AM, Pavel Hrdina wrote: > >> On Sat, Jun 11, 2016 at 10:58:17AM -0400, Cole Robinson wrote: > >>> On 06/10/2016 01:30 PM, Pavel Hrdina wrote: > >>>> Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1334857 > >>>> > >>>> Signed-off-by: Pavel Hrdina <phrdina@xxxxxxxxxx> > >>>> --- > >>>> man/virt-install.pod | 5 +++ > >>>> .../compare/virt-install-aarch64-kvm-gic.xml | 33 ++++++++++++++++++ > >>>> tests/clitest.py | 1 + > >>>> virtinst/cli.py | 39 ++++++++++++---------- > >>>> virtinst/guest.py | 16 +++++++++ > >>>> 5 files changed, 77 insertions(+), 17 deletions(-) > >>>> create mode 100644 tests/cli-test-xml/compare/virt-install-aarch64-kvm-gic.xml > >>>> > >>>> diff --git a/man/virt-install.pod b/man/virt-install.pod > >>>> index 0537693..8054f68 100644 > >>>> --- a/man/virt-install.pod > >>>> +++ b/man/virt-install.pod > >>>> @@ -254,6 +254,11 @@ Allow the KVM hypervisor signature to be hidden from the guest > >>>> > >>>> Notify the guest that the host supports paravirtual spinlocks for example by exposing the pvticketlocks mechanism. > >>>> > >>>> +=item B<--features gic=2> > >>>> + > >>>> +This is relevant only for aarch64 architecture. Possible values are "host" or > >>>> +version number. > >>>> + > >>>> =back > >>>> > >>>> Use --features=? to see a list of all available sub options. Complete details at L<http://libvirt.org/formatdomain.html#elementsFeatures> > >>>> diff --git a/tests/cli-test-xml/compare/virt-install-aarch64-kvm-gic.xml b/tests/cli-test-xml/compare/virt-install-aarch64-kvm-gic.xml > >>>> new file mode 100644 > >>>> index 0000000..189373d > >>>> --- /dev/null > >>>> +++ b/tests/cli-test-xml/compare/virt-install-aarch64-kvm-gic.xml > >>>> @@ -0,0 +1,33 @@ > >>>> +<domain type="kvm"> > >>>> + <name>foobar</name> > >>>> + <uuid>00000000-1111-2222-3333-444444444444</uuid> > >>>> + <memory>65536</memory> > >>>> + <currentMemory>65536</currentMemory> > >>>> + <vcpu>1</vcpu> > >>>> + <os> > >>>> + <type arch="aarch64" machine="virt">hvm</type> > >>>> + <loader readonly="yes" type="pflash">/usr/share/AAVMF/AAVMF_CODE.fd</loader> > >>>> + <boot dev="hd"/> > >>>> + </os> > >>>> + <features> > >>>> + <gic version="host"/> > >>>> + </features> > >>>> + <cpu mode="host-passthrough"/> > >>>> + <clock offset="utc"/> > >>>> + <on_poweroff>destroy</on_poweroff> > >>>> + <on_reboot>restart</on_reboot> > >>>> + <on_crash>restart</on_crash> > >>>> + <devices> > >>>> + <emulator>/usr/bin/qemu-system-aarch64</emulator> > >>>> + <interface type="bridge"> > >>>> + <source bridge="eth0"/> > >>>> + <mac address="00:11:22:33:44:55"/> > >>>> + <model type="virtio"/> > >>>> + </interface> > >>>> + <console type="pty"/> > >>>> + <channel type="unix"> > >>>> + <source mode="bind"/> > >>>> + <target type="virtio" name="org.qemu.guest_agent.0"/> > >>>> + </channel> > >>>> + </devices> > >>>> +</domain> > >>>> diff --git a/tests/clitest.py b/tests/clitest.py > >>>> index 5d26078..376a17e 100644 > >>>> --- a/tests/clitest.py > >>>> +++ b/tests/clitest.py > >>>> @@ -711,6 +711,7 @@ c.add_compare("--arch aarch64 --machine virt --boot kernel=/f19-arm.kernel,initr > >>>> c.add_compare("--arch aarch64 --boot kernel=/f19-arm.kernel,initrd=/f19-arm.initrd,kernel_args=\"console=ttyAMA0,1234 rw root=/dev/vda3\",extra_args=foo --disk %(EXISTIMG1)s", "aarch64-machdefault") > >>>> c.add_compare("--arch aarch64 --cdrom %(EXISTIMG2)s --boot loader=CODE.fd,nvram_template=VARS.fd --disk %(EXISTIMG1)s --cpu none --events on_crash=preserve,on_reboot=destroy,on_poweroff=restart", "aarch64-cdrom") > >>>> c.add_compare("--connect %(URI-KVM-AARCH64)s --disk %(EXISTIMG1)s --import --os-variant fedora21", "aarch64-kvm-import") > >>>> +c.add_compare("--connect %(URI-KVM-AARCH64)s --disk none --os-variant fedora23 --features gic=host", "aarch64-kvm-gic") > >>>> > >>>> # ppc64 tests > >>>> c.add_compare("--arch ppc64 --machine pseries --boot network --disk %(EXISTIMG1)s --disk device=cdrom --os-variant fedora20 --network none", "ppc64-pseries-f20") > >>>> diff --git a/virtinst/cli.py b/virtinst/cli.py > >>>> index 7924b14..1f486b2 100644 > >>>> --- a/virtinst/cli.py > >>>> +++ b/virtinst/cli.py > >>>> @@ -54,7 +54,6 @@ from .devicetpm import VirtualTPMDevice > >>>> from .devicevideo import VirtualVideoDevice > >>>> from .devicewatchdog import VirtualWatchdog > >>>> from .domainblkiotune import DomainBlkiotune > >>>> -from .domainfeatures import DomainFeatures > >>>> from .domainmemorybacking import DomainMemorybacking > >>>> from .domainmemorytune import DomainMemorytune > >>>> from .domainnumatune import DomainNumatune > >>>> @@ -1518,32 +1517,38 @@ class ParserSecurity(VirtCLIParser): > >>>> > >>>> class ParserFeatures(VirtCLIParser): > >>>> def _init_params(self): > >>>> - self.objclass = DomainFeatures > >>>> > >>> > >>> This has some subtle side effects, basically disabling virt-xml --features > >>> clearxml=off, but that could be worked around. > >> > >> So that's why ParserBoot has 'self.clear_attr = "os"'. I'll add the same for > >> features: > >> self.clear_attr = "features" > >> > >>>> - self.set_param("acpi", "acpi", is_onoff=True) > >>>> - self.set_param("apic", "apic", is_onoff=True) > >>>> - self.set_param("pae", "pae", is_onoff=True) > >>>> - self.set_param("privnet", "privnet", > >>>> + self.set_param("features.acpi", "acpi", is_onoff=True) > >>>> + self.set_param("features.apic", "apic", is_onoff=True) > >>>> + self.set_param("features.pae", "pae", is_onoff=True) > >>>> + self.set_param("features.privnet", "privnet", > >>>> is_onoff=True) > >>>> - self.set_param("hap", "hap", > >>>> + self.set_param("features.hap", "hap", > >>>> is_onoff=True) > >>>> - self.set_param("viridian", "viridian", > >>>> + self.set_param("features.viridian", "viridian", > >>>> is_onoff=True) > >>>> - self.set_param("eoi", "eoi", is_onoff=True) > >>>> - self.set_param("pmu", "pmu", is_onoff=True) > >>>> + self.set_param("features.eoi", "eoi", is_onoff=True) > >>>> + self.set_param("features.pmu", "pmu", is_onoff=True) > >>>> > >>>> - self.set_param("hyperv_vapic", "hyperv_vapic", > >>>> + self.set_param("features.hyperv_vapic", "hyperv_vapic", > >>>> is_onoff=True) > >>>> - self.set_param("hyperv_relaxed", "hyperv_relaxed", > >>>> + self.set_param("features.hyperv_relaxed", "hyperv_relaxed", > >>>> is_onoff=True) > >>>> - self.set_param("hyperv_spinlocks", "hyperv_spinlocks", > >>>> + self.set_param("features.hyperv_spinlocks", "hyperv_spinlocks", > >>>> is_onoff=True) > >>>> - self.set_param("hyperv_spinlocks_retries", > >>>> + self.set_param("features.hyperv_spinlocks_retries", > >>>> "hyperv_spinlocks_retries") > >>>> > >>>> - self.set_param("vmport", "vmport", is_onoff=True) > >>>> - self.set_param("kvm_hidden", "kvm_hidden", is_onoff=True) > >>>> - self.set_param("pvspinlock", "pvspinlock", is_onoff=True) > >>>> + self.set_param("features.vmport", "vmport", is_onoff=True) > >>>> + self.set_param("features.kvm_hidden", "kvm_hidden", is_onoff=True) > >>>> + self.set_param("features.pvspinlock", "pvspinlock", is_onoff=True) > >>>> + > >>>> + def set_gic(opts, inst, cliname, val): > >>>> + ignore = opts > >>>> + ignore = cliname > >>>> + inst.set_gic_version(val) > >>>> + > >>>> + self.set_param(None, "gic", setter_cb=set_gic) > >>>> > >>>> > >>>> ################### > >>>> diff --git a/virtinst/guest.py b/virtinst/guest.py > >>>> index 490b90b..544a65a 100644 > >>>> --- a/virtinst/guest.py > >>>> +++ b/virtinst/guest.py > >>>> @@ -587,6 +587,22 @@ class Guest(XMLBuilder): > >>>> self.os.loader = path > >>>> > >>>> > >>>> + def set_gic_version(self, val): > >>>> + """ > >>>> + Checks and sets GIC version for ARM guest. > >>>> + """ > >>>> + > >>>> + domcaps = DomainCapabilities.build_from_guest(self) > >>>> + > >>>> + if not domcaps.supports_gic(self): > >>>> + raise RuntimeError(_("GIC is not supported.")) > >>>> + > >>>> + if val != "host" and val not in domcaps.get_gic_versions(): > >>>> + raise RuntimeError(_("GIC version '%s' is not supported") % val) > >>>> + > >>>> + self.features.gic_version = val > >>>> + > >>>> + > >>> > >>> Do we actually need this validation here? Isn't libvirt going to throw us > >>> these same errors once the XML is defined? > >> > >> Unfortunately not, libvirt allows to create a domain with invalid gic version > >> but fails to start such domain. The only validation what libvirt does is > >> against XML schema where are all known gic versions, but that doesn't validate > >> whether the host supports that gic version or not. > >> > > > > Hmm okay. Still I think I'd rather not add this here in the cli path since > > it's basically just duplicating libvirt logic for not a ton of gain. For the > > common case of actually starting a VM with virt-install it won't matter, so it > > will only give extra validation in less common cases like virt-install > > --print-xml, or virt-xml. That's a good point, I'll drop the validation. > > > > For virt-manager if we choose to expose it in the UI, we could make the UI a > > combo box based on the domcaps output, and avoid the need to do validation > > entirely. > > To clarify, I'm only objecting to the validation. Exposing the --features > gic_version=X option in general is fine, feel free to push it if you agree, > but without dropping the self.objclass = DomainFeatures bit I'll push patches 1-4, thanks. Pavel _______________________________________________ virt-tools-list mailing list virt-tools-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/virt-tools-list
