[libvirt] [PATCH 0/2] Forbid memoryBacking/access for non-numa case
*** BLURB HERE *** Michal Privoznik (2): src: Export virDomainMemoryAccessType*String qemuBuildMemPathStr: Forbid memoryBacking/access for non-numa case src/libvirt_private.syms | 2 + src/qemu/qemu_command.c | 9 +++ tests/qemuxml2argvdata/hugepages-memaccess3.xml | 87 +++++++++++++++++++++++++ tests/qemuxml2argvtest.c | 3 + 4 files changed, 101 insertions(+) create mode 100644 tests/qemuxml2argvdata/hugepages-memaccess3.xml -- 2.13.6
These are already exported at header file level because of VIR_ENUM_DECL being in numa_conf.h. However, they are not being exported at object level because of missing libvirt_private.syms record. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/libvirt_private.syms | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index de4ec4d44..7018ba0cd 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -718,6 +718,8 @@ virNodeDeviceEventUpdateNew; # conf/numa_conf.h +virDomainMemoryAccessTypeFromString; +virDomainMemoryAccessTypeToString; virDomainNumaCheckABIStability; virDomainNumaEquals; virDomainNumaFree; -- 2.13.6
On 12/12/2017 08:36 AM, Michal Privoznik wrote:
These are already exported at header file level because of VIR_ENUM_DECL being in numa_conf.h. However, they are not being exported at object level because of missing libvirt_private.syms record.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/libvirt_private.syms | 2 ++ 1 file changed, 2 insertions(+)
Reviewed-by: John Ferlan <jferlan@redhat.com> John
https://bugzilla.redhat.com/show_bug.cgi?id=1448149 If a domain has no numa nodes, that means we don't put any memory-backend-file onto the qemu command line. That in turn means we can't set access='shared'. Therefore, we should produce an error instead of ignoring the setting silently. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_command.c | 9 +++ tests/qemuxml2argvdata/hugepages-memaccess3.xml | 87 +++++++++++++++++++++++++ tests/qemuxml2argvtest.c | 3 + 3 files changed, 99 insertions(+) create mode 100644 tests/qemuxml2argvdata/hugepages-memaccess3.xml diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 2dd50a214..dfc17ce34 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -7682,6 +7682,15 @@ qemuBuildMemPathStr(virQEMUDriverConfigPtr cfg, return -1; } + if (def->mem.access != VIR_DOMAIN_MEMORY_ACCESS_DEFAULT && + def->mem.access != VIR_DOMAIN_MEMORY_ACCESS_PRIVATE) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("memory access mode '%s' not supported " + "without guest numa node"), + virDomainMemoryAccessTypeToString(def->mem.access)); + return -1; + } + if (qemuGetDomainHupageMemPath(def, cfg, def->mem.hugepages[0].size, &mem_path) < 0) return -1; diff --git a/tests/qemuxml2argvdata/hugepages-memaccess3.xml b/tests/qemuxml2argvdata/hugepages-memaccess3.xml new file mode 100644 index 000000000..8ec38d802 --- /dev/null +++ b/tests/qemuxml2argvdata/hugepages-memaccess3.xml @@ -0,0 +1,87 @@ +<domain type='kvm'> + <name>fedora</name> + <uuid>63840878-0deb-4095-97e6-fc444d9bc9fa</uuid> + <memory unit='KiB'>4194304</memory> + <currentMemory unit='KiB'>4194304</currentMemory> + <memoryBacking> + <hugepages/> + <access mode='shared'/> + </memoryBacking> + <vcpu placement='static'>4</vcpu> + <os> + <type arch='x86_64' machine='pc-i440fx-2.9'>hvm</type> + <bootmenu enable='yes'/> + </os> + <features> + <acpi/> + <apic/> + <pae/> + </features> + <cpu mode='host-model' check='partial'> + <model fallback='allow'/> + </cpu> + <clock offset='variable' adjustment='500' basis='utc'> + <timer name='rtc' tickpolicy='catchup'/> + <timer name='pit' tickpolicy='delay'/> + <timer name='hpet' present='no'/> + </clock> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <pm> + <suspend-to-mem enabled='yes'/> + <suspend-to-disk enabled='yes'/> + </pm> + <devices> + <emulator>/usr/bin/qemu-system-x86_64</emulator> + <disk type='file' device='disk'> + <driver name='qemu' type='qcow2' discard='unmap'/> + <source file='/var/lib/libvirt/images/fedora.qcow2'/> + <target dev='sda' bus='scsi'/> + <boot order='1'/> + <address type='drive' controller='0' bus='0' target='0' unit='0'/> + </disk> + <controller type='usb' index='0' model='piix3-uhci'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x2'/> + </controller> + <controller type='pci' index='0' model='pci-root'/> + <controller type='scsi' index='0'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x05' function='0x0'/> + </controller> + <controller type='virtio-serial' index='0'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x06' function='0x0'/> + </controller> + <controller type='ide' index='0'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x1'/> + </controller> + <serial type='pty'> + <target type='isa-serial' port='1'> + <model name='isa-serial'/> + </target> + </serial> + <console type='pty'> + <target type='serial' port='1'/> + </console> + <channel type='unix'> + <target type='virtio' name='org.qemu.guest_agent.0'/> + <address type='virtio-serial' controller='0' bus='0' port='1'/> + </channel> + <input type='mouse' bus='ps2'/> + <input type='keyboard' bus='ps2'/> + <graphics type='vnc' port='-1' autoport='yes' listen='0.0.0.0'> + <listen type='address' address='0.0.0.0'/> + </graphics> + <graphics type='spice'> + <listen type='socket' socket='/tmp/spice.sock'/> + </graphics> + <video> + <model type='virtio' heads='1' primary='yes'> + <acceleration accel3d='yes'/> + </model> + <address type='pci' domain='0x0000' bus='0x00' slot='0x02' function='0x0'/> + </video> + <memballoon model='virtio'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x07' function='0x0'/> + </memballoon> + </devices> +</domain> diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index ca24e0bbb..c09ce75f4 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -861,6 +861,9 @@ mymain(void) DO_TEST("hugepages-memaccess2", QEMU_CAPS_OBJECT_MEMORY_FILE, QEMU_CAPS_OBJECT_MEMORY_RAM, QEMU_CAPS_DEVICE_PC_DIMM, QEMU_CAPS_NUMA); + DO_TEST_FAILURE("hugepages-memaccess3", QEMU_CAPS_MEM_PATH, + QEMU_CAPS_OBJECT_MEMORY_RAM, QEMU_CAPS_OBJECT_MEMORY_FILE, + QEMU_CAPS_VIRTIO_SCSI); DO_TEST("nosharepages", QEMU_CAPS_MACHINE_OPT, QEMU_CAPS_MEM_MERGE); DO_TEST("disk-cdrom", NONE); DO_TEST("disk-iscsi", NONE); -- 2.13.6
On 12/12/2017 08:36 AM, Michal Privoznik wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=1448149
If a domain has no numa nodes, that means we don't put any memory-backend-file onto the qemu command line. That in turn means we can't set access='shared'. Therefore, we should produce an error instead of ignoring the setting silently.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_command.c | 9 +++ tests/qemuxml2argvdata/hugepages-memaccess3.xml | 87 +++++++++++++++++++++++++ tests/qemuxml2argvtest.c | 3 + 3 files changed, 99 insertions(+) create mode 100644 tests/qemuxml2argvdata/hugepages-memaccess3.xml
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 2dd50a214..dfc17ce34 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -7682,6 +7682,15 @@ qemuBuildMemPathStr(virQEMUDriverConfigPtr cfg, return -1; }
+ if (def->mem.access != VIR_DOMAIN_MEMORY_ACCESS_DEFAULT && + def->mem.access != VIR_DOMAIN_MEMORY_ACCESS_PRIVATE) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("memory access mode '%s' not supported " + "without guest numa node"), + virDomainMemoryAccessTypeToString(def->mem.access)); + return -1; + } +
This works; however, why not move this and the virQEMUCapsGet check into a qemu_domain qemuDomainDef*Memory*Validate type function? Thus removing failure checks from qemu_command... I suppose it's OK here, but I think better there. Still for the concept, Reviewed-by: John Ferlan <jferlan@redhat.com> John [...]
On 12/15/2017 08:48 PM, John Ferlan wrote:
On 12/12/2017 08:36 AM, Michal Privoznik wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=1448149
If a domain has no numa nodes, that means we don't put any memory-backend-file onto the qemu command line. That in turn means we can't set access='shared'. Therefore, we should produce an error instead of ignoring the setting silently.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_command.c | 9 +++ tests/qemuxml2argvdata/hugepages-memaccess3.xml | 87 +++++++++++++++++++++++++ tests/qemuxml2argvtest.c | 3 + 3 files changed, 99 insertions(+) create mode 100644 tests/qemuxml2argvdata/hugepages-memaccess3.xml
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 2dd50a214..dfc17ce34 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -7682,6 +7682,15 @@ qemuBuildMemPathStr(virQEMUDriverConfigPtr cfg, return -1; }
+ if (def->mem.access != VIR_DOMAIN_MEMORY_ACCESS_DEFAULT && + def->mem.access != VIR_DOMAIN_MEMORY_ACCESS_PRIVATE) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("memory access mode '%s' not supported " + "without guest numa node"), + virDomainMemoryAccessTypeToString(def->mem.access)); + return -1; + } +
This works; however, why not move this and the virQEMUCapsGet check into a qemu_domain qemuDomainDef*Memory*Validate type function? Thus removing failure checks from qemu_command...
I'm not quite sure which function you have in mind.
I suppose it's OK here, but I think better there. Still for the concept,
Reviewed-by: John Ferlan <jferlan@redhat.com>
Thanks, I'll hold pushing these until we have clear consensus here. Michal
On 12/18/2017 03:28 AM, Michal Privoznik wrote:
On 12/15/2017 08:48 PM, John Ferlan wrote:
On 12/12/2017 08:36 AM, Michal Privoznik wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=1448149
If a domain has no numa nodes, that means we don't put any memory-backend-file onto the qemu command line. That in turn means we can't set access='shared'. Therefore, we should produce an error instead of ignoring the setting silently.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_command.c | 9 +++ tests/qemuxml2argvdata/hugepages-memaccess3.xml | 87 +++++++++++++++++++++++++ tests/qemuxml2argvtest.c | 3 + 3 files changed, 99 insertions(+) create mode 100644 tests/qemuxml2argvdata/hugepages-memaccess3.xml
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 2dd50a214..dfc17ce34 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -7682,6 +7682,15 @@ qemuBuildMemPathStr(virQEMUDriverConfigPtr cfg, return -1; }
+ if (def->mem.access != VIR_DOMAIN_MEMORY_ACCESS_DEFAULT && + def->mem.access != VIR_DOMAIN_MEMORY_ACCESS_PRIVATE) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("memory access mode '%s' not supported " + "without guest numa node"), + virDomainMemoryAccessTypeToString(def->mem.access)); + return -1; + } +
This works; however, why not move this and the virQEMUCapsGet check into a qemu_domain qemuDomainDef*Memory*Validate type function? Thus removing failure checks from qemu_command...
I'm not quite sure which function you have in mind.
Create one called from qemuDomainDefValidate, but it's not that important. Just figured that since there seems to be a more accepted way to catch these types of domain validation things before we get all the way to building the command line that we should try to do so when we can. John
I suppose it's OK here, but I think better there. Still for the concept,
Reviewed-by: John Ferlan <jferlan@redhat.com>
Thanks, I'll hold pushing these until we have clear consensus here.
Michal
On Mon, Dec 18, 2017 at 09:28:24AM +0100, Michal Privoznik wrote:
On 12/15/2017 08:48 PM, John Ferlan wrote:
On 12/12/2017 08:36 AM, Michal Privoznik wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=1448149
If a domain has no numa nodes, that means we don't put any memory-backend-file onto the qemu command line. That in turn means we can't set access='shared'. Therefore, we should produce an error instead of ignoring the setting silently.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_command.c | 9 +++ tests/qemuxml2argvdata/hugepages-memaccess3.xml | 87 +++++++++++++++++++++++++ tests/qemuxml2argvtest.c | 3 + 3 files changed, 99 insertions(+) create mode 100644 tests/qemuxml2argvdata/hugepages-memaccess3.xml
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 2dd50a214..dfc17ce34 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -7682,6 +7682,15 @@ qemuBuildMemPathStr(virQEMUDriverConfigPtr cfg, return -1; }
+ if (def->mem.access != VIR_DOMAIN_MEMORY_ACCESS_DEFAULT && + def->mem.access != VIR_DOMAIN_MEMORY_ACCESS_PRIVATE) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("memory access mode '%s' not supported " + "without guest numa node"), + virDomainMemoryAccessTypeToString(def->mem.access)); + return -1; + } +
This works; however, why not move this and the virQEMUCapsGet check into a qemu_domain qemuDomainDef*Memory*Validate type function? Thus removing failure checks from qemu_command...
I'm not quite sure which function you have in mind.
A newly created qemuDomainDeviceDefValidateMemory function called from qemuDomainDeviceDefValidate if this error is qemu-specific or a newly created virDomainMemoryDefValidate called from virDomainDeviceDefValidateInternal if this combination is nonsensical for all drivers. Jan
I suppose it's OK here, but I think better there. Still for the concept,
Reviewed-by: John Ferlan <jferlan@redhat.com>
Thanks, I'll hold pushing these until we have clear consensus here.
Michal
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
participants (3)
-
John Ferlan -
Ján Tomko -
Michal Privoznik