[libvirt] [PATCH] qemu: domain: validate memory access during validate domain config
commit 6534b3c4 try to raise an error when there is no numa nodes but set access='shared' in domain config. In that commit, we add a memory access validate function for memory device, but this check is not related to memory device and won't work if there is no memory device in guest xml. Move the memory access related check in the domain config validate function, and simplify the unit test xml to avoid other error. https://bugzilla.redhat.com/show_bug.cgi?id=1448149#c14 Signed-off-by: Luyao Huang <lhuang@redhat.com> --- src/qemu/qemu_domain.c | 55 +++++++++++----------- tests/qemuxml2argvdata/hugepages-memaccess3.xml | 62 +------------------------ tests/qemuxml2argvtest.c | 6 +-- 3 files changed, 32 insertions(+), 91 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index f570081..f0c461b 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -3888,6 +3888,29 @@ qemuDomainDefValidateFeatures(const virDomainDef *def, static int +qemuDomainDefValidateMemory(const virDomainDef *def) +{ + const long system_page_size = virGetSystemPageSizeKB(); + + /* We can't guarantee any other mem.access + * if no guest NUMA nodes are defined. */ + if (def->mem.nhugepages != 0 && + def->mem.hugepages[0].size != system_page_size && + virDomainNumaGetNodeCount(def->numa) == 0 && + 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; + } + + return 0; +} + + +static int qemuDomainDefValidate(const virDomainDef *def, virCapsPtr caps ATTRIBUTE_UNUSED, void *opaque) @@ -4009,6 +4032,9 @@ qemuDomainDefValidate(const virDomainDef *def, if (qemuDomainDefValidateFeatures(def, qemuCaps) < 0) goto cleanup; + if (qemuDomainDefValidateMemory(def) < 0) + goto cleanup; + ret = 0; cleanup: @@ -5531,30 +5557,6 @@ qemuDomainDeviceDefValidateController(const virDomainControllerDef *controller, static int -qemuDomainDeviceDefValidateMemory(const virDomainMemoryDef *memory ATTRIBUTE_UNUSED, - const virDomainDef *def) -{ - const long system_page_size = virGetSystemPageSizeKB(); - - /* We can't guarantee any other mem.access - * if no guest NUMA nodes are defined. */ - if (def->mem.nhugepages != 0 && - def->mem.hugepages[0].size != system_page_size && - virDomainNumaGetNodeCount(def->numa) == 0 && - 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; - } - - return 0; -} - - -static int qemuDomainDeviceDefValidateVsock(const virDomainVsockDef *vsock, const virDomainDef *def, virQEMUCapsPtr qemuCaps) @@ -5712,10 +5714,6 @@ qemuDomainDeviceDefValidate(const virDomainDeviceDef *dev, qemuCaps); break; - case VIR_DOMAIN_DEVICE_MEMORY: - ret = qemuDomainDeviceDefValidateMemory(dev->data.memory, def); - break; - case VIR_DOMAIN_DEVICE_VSOCK: ret = qemuDomainDeviceDefValidateVsock(dev->data.vsock, def, qemuCaps); break; @@ -5737,6 +5735,7 @@ qemuDomainDeviceDefValidate(const virDomainDeviceDef *dev, case VIR_DOMAIN_DEVICE_MEMBALLOON: case VIR_DOMAIN_DEVICE_NVRAM: case VIR_DOMAIN_DEVICE_SHMEM: + case VIR_DOMAIN_DEVICE_MEMORY: case VIR_DOMAIN_DEVICE_PANIC: case VIR_DOMAIN_DEVICE_IOMMU: case VIR_DOMAIN_DEVICE_NONE: diff --git a/tests/qemuxml2argvdata/hugepages-memaccess3.xml b/tests/qemuxml2argvdata/hugepages-memaccess3.xml index 8ec38d8..43c3ec6 100644 --- a/tests/qemuxml2argvdata/hugepages-memaccess3.xml +++ b/tests/qemuxml2argvdata/hugepages-memaccess3.xml @@ -12,76 +12,18 @@ <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='usb' index='0'/> <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> + <memballoon model='none'/> </devices> </domain> diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index 0e9eef6..6b55316 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -1023,9 +1023,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_OBJECT_MEMORY_RAM, QEMU_CAPS_OBJECT_MEMORY_FILE, - QEMU_CAPS_VIRTIO_SCSI); + DO_TEST_PARSE_ERROR("hugepages-memaccess3", + QEMU_CAPS_OBJECT_MEMORY_RAM, + QEMU_CAPS_OBJECT_MEMORY_FILE); DO_TEST_CAPS_LATEST("hugepages-nvdimm"); DO_TEST("nosharepages", QEMU_CAPS_MEM_MERGE); DO_TEST("disk-cdrom", NONE); -- 1.8.3.1
On 08/20/2018 05:48 AM, Luyao Huang wrote:
commit 6534b3c4 try to raise an error when there is no numa nodes but set access='shared' in domain config. In that commit, we add a memory access validate function for memory device, but this check is not related to memory device and won't work if there is no memory device in guest xml.
Move the memory access related check in the domain config validate function, and simplify the unit test xml to avoid other error.
https://bugzilla.redhat.com/show_bug.cgi?id=1448149#c14
Signed-off-by: Luyao Huang <lhuang@redhat.com> --- src/qemu/qemu_domain.c | 55 +++++++++++----------- tests/qemuxml2argvdata/hugepages-memaccess3.xml | 62 +------------------------ tests/qemuxml2argvtest.c | 6 +-- 3 files changed, 32 insertions(+), 91 deletions(-)
Hmm... I have a recollection of reviewing this... Let's see, yes: https://www.redhat.com/archives/libvir-list/2017-December/msg00387.html and follow-ups... I think I agree with you the validation should be done in qemuDomainDefValidate instead of qemuDomainDeviceDefValidate; however, you're making multiple changes at one time - so I went to dissect... This response got lengthy and I'm kind of at a hmm? wtf moment that hopefully Michal can look at since he was the original author. Maybe he can recall 8 months ago and whether he got the expected error message or not on output. My concern/point was changing hugepages-memaccess3.xml and qemuxml2argvtest.c to remove some things while also changing from DO_TEST_FAILURE to DO_TEST_PARSE_ERROR is doing two different things in one patch, which we generally try to avoid, but in this case it may just be necessary. When I tried to separate the changes in such a way that the unnecessary lines were removed from hugepages-memaccess3.xml first - the test ended up succeeding! So that was strange, but I guess not totally unexpected since the device mems don't exist and the wrong check was being done. So I went back to whence we started and note that qemuxml2argvtest fails the hugepages-memaccess3 without any of these changes with (as seen with VIR_TEST_DEBUG=1 VIR_TEST_RANGE=169 tests/qemuxml2argvtest): 169) QEMU XML-2-ARGV hugepages-memaccess3 ... Got expected error: 2018-08-27 21:24:04.934+0000: 5816: info : libvirt version: 4.7.0 2018-08-27 21:24:04.934+0000: 5816: info : hostname: localhost.localdomain 2018-08-27 21:24:04.934+0000: 5816: error : qemuProcessStartValidateVideo:5033 : unsupported configuration: this QEMU does not support 'virtio' video device So, going a bit slower... and removing things to see if I can get that "memory access mode '%s' not supported..." error message... 1. Remove <video> and <graphics> entries, but get: 169) QEMU XML-2-ARGV hugepages-memaccess3 ... Got expected error: 2018-08-27 21:26:28.754+0000: 5907: info : libvirt version: 4.7.0 2018-08-27 21:26:28.754+0000: 5907: info : hostname: localhost.localdomain 2018-08-27 21:26:28.754+0000: 5907: error : qemuBuildPMCommandLine:6523 : unsupported configuration: setting ACPI S3 not supported 2. Remove <pm> and get: 169) QEMU XML-2-ARGV hugepages-memaccess3 ... Got expected error: 2018-08-27 21:27:48.000+0000: 5972: info : libvirt version: 4.7.0 2018-08-27 21:27:48.000+0000: 5972: info : hostname: localhost.localdomain 2018-08-27 21:27:48.000+0000: 5972: error : qemuBuildUSBControllerDevStr:2681 : unsupported configuration: piix3-usb-uhci not supported in this QEMU binary 3. Remove "model='piix3-uhci'" from the <controller type='usb' and get: 169) QEMU XML-2-ARGV hugepages-memaccess3 ... Got expected error: 2018-08-27 21:29:01.554+0000: 6179: info : libvirt version: 4.7.0 2018-08-27 21:29:01.554+0000: 6179: info : hostname: localhost.localdomain 2018-08-27 21:29:01.554+0000: 6179: error : qemuCheckDiskConfig:1297 : unsupported configuration: discard is not supported by this QEMU binary 4. Remove "discard='unmap' from the <disk> and get: 169) QEMU XML-2-ARGV hugepages-memaccess3 ... Got expected error: 2018-08-27 21:30:21.989+0000: 6360: info : libvirt version: 4.7.0 2018-08-27 21:30:21.989+0000: 6360: info : hostname: localhost.localdomain 2018-08-27 21:30:21.989+0000: 6360: error : qemuBuildSerialChrDeviceStr:10576 : unsupported configuration: 'isa-serial' is not supported in this QEMU binary 5. Remove <serial>, <console>, and <channel> and get passed instead of failure. So at first I was a bit stumped as to what happened... Could there be "conflict" or no error message now due to changes Pavel Hrdina made dealing with hugepages and parsing. So, in any case, I'd like to wait for Michal's input before proceeding further on this one. BTW: Your changes look correct and fine to me as well as eliciting the proper error message, which is good/proper, but before pushing let's see if there's feedback from Michal. John
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index f570081..f0c461b 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -3888,6 +3888,29 @@ qemuDomainDefValidateFeatures(const virDomainDef *def,
static int +qemuDomainDefValidateMemory(const virDomainDef *def) +{ + const long system_page_size = virGetSystemPageSizeKB(); + + /* We can't guarantee any other mem.access + * if no guest NUMA nodes are defined. */ + if (def->mem.nhugepages != 0 && + def->mem.hugepages[0].size != system_page_size && + virDomainNumaGetNodeCount(def->numa) == 0 && + 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; + } + + return 0; +} + + +static int qemuDomainDefValidate(const virDomainDef *def, virCapsPtr caps ATTRIBUTE_UNUSED, void *opaque) @@ -4009,6 +4032,9 @@ qemuDomainDefValidate(const virDomainDef *def, if (qemuDomainDefValidateFeatures(def, qemuCaps) < 0) goto cleanup;
+ if (qemuDomainDefValidateMemory(def) < 0) + goto cleanup; + ret = 0;
cleanup: @@ -5531,30 +5557,6 @@ qemuDomainDeviceDefValidateController(const virDomainControllerDef *controller,
static int -qemuDomainDeviceDefValidateMemory(const virDomainMemoryDef *memory ATTRIBUTE_UNUSED, - const virDomainDef *def) -{ - const long system_page_size = virGetSystemPageSizeKB(); - - /* We can't guarantee any other mem.access - * if no guest NUMA nodes are defined. */ - if (def->mem.nhugepages != 0 && - def->mem.hugepages[0].size != system_page_size && - virDomainNumaGetNodeCount(def->numa) == 0 && - 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; - } - - return 0; -} - - -static int qemuDomainDeviceDefValidateVsock(const virDomainVsockDef *vsock, const virDomainDef *def, virQEMUCapsPtr qemuCaps) @@ -5712,10 +5714,6 @@ qemuDomainDeviceDefValidate(const virDomainDeviceDef *dev, qemuCaps); break;
- case VIR_DOMAIN_DEVICE_MEMORY: - ret = qemuDomainDeviceDefValidateMemory(dev->data.memory, def); - break; - case VIR_DOMAIN_DEVICE_VSOCK: ret = qemuDomainDeviceDefValidateVsock(dev->data.vsock, def, qemuCaps); break; @@ -5737,6 +5735,7 @@ qemuDomainDeviceDefValidate(const virDomainDeviceDef *dev, case VIR_DOMAIN_DEVICE_MEMBALLOON: case VIR_DOMAIN_DEVICE_NVRAM: case VIR_DOMAIN_DEVICE_SHMEM: + case VIR_DOMAIN_DEVICE_MEMORY: case VIR_DOMAIN_DEVICE_PANIC: case VIR_DOMAIN_DEVICE_IOMMU: case VIR_DOMAIN_DEVICE_NONE: diff --git a/tests/qemuxml2argvdata/hugepages-memaccess3.xml b/tests/qemuxml2argvdata/hugepages-memaccess3.xml index 8ec38d8..43c3ec6 100644 --- a/tests/qemuxml2argvdata/hugepages-memaccess3.xml +++ b/tests/qemuxml2argvdata/hugepages-memaccess3.xml @@ -12,76 +12,18 @@ <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='usb' index='0'/> <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> + <memballoon model='none'/> </devices> </domain> diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index 0e9eef6..6b55316 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -1023,9 +1023,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_OBJECT_MEMORY_RAM, QEMU_CAPS_OBJECT_MEMORY_FILE, - QEMU_CAPS_VIRTIO_SCSI); + DO_TEST_PARSE_ERROR("hugepages-memaccess3", + QEMU_CAPS_OBJECT_MEMORY_RAM, + QEMU_CAPS_OBJECT_MEMORY_FILE); DO_TEST_CAPS_LATEST("hugepages-nvdimm"); DO_TEST("nosharepages", QEMU_CAPS_MEM_MERGE); DO_TEST("disk-cdrom", NONE);
On 08/28/2018 06:01 AM, John Ferlan wrote:
On 08/20/2018 05:48 AM, Luyao Huang wrote:
commit 6534b3c4 try to raise an error when there is no numa nodes but set access='shared' in domain config. In that commit, we add a memory access validate function for memory device, but this check is not related to memory device and won't work if there is no memory device in guest xml.
Move the memory access related check in the domain config validate function, and simplify the unit test xml to avoid other error.
https://bugzilla.redhat.com/show_bug.cgi?id=1448149#c14
Signed-off-by: Luyao Huang <lhuang@redhat.com> --- src/qemu/qemu_domain.c | 55 +++++++++++----------- tests/qemuxml2argvdata/hugepages-memaccess3.xml | 62 +------------------------ tests/qemuxml2argvtest.c | 6 +-- 3 files changed, 32 insertions(+), 91 deletions(-)
Hmm... I have a recollection of reviewing this... Let's see, yes:
https://www.redhat.com/archives/libvir-list/2017-December/msg00387.html
and follow-ups... I think I agree with you the validation should be done in qemuDomainDefValidate instead of qemuDomainDeviceDefValidate; however, you're making multiple changes at one time - so I went to dissect... This response got lengthy and I'm kind of at a hmm? wtf moment that hopefully Michal can look at since he was the original author. Maybe he can recall 8 months ago and whether he got the expected error message or not on output.
My concern/point was changing hugepages-memaccess3.xml and qemuxml2argvtest.c to remove some things while also changing from DO_TEST_FAILURE to DO_TEST_PARSE_ERROR is doing two different things in one patch, which we generally try to avoid, but in this case it may just be necessary.
yes, i was trying to fix the hugepages-memaccess3.xml since my changes will break the unit test. Actually the code changes for the hugepages-memaccess3.xml and qemuxml2argvtest.c included 2 different fix, one for fixing the exist unit test and another for fixing the failure introduced in this patch. And i merged them in one patch, maybe i should keep them split in next time ?
When I tried to separate the changes in such a way that the unnecessary lines were removed from hugepages-memaccess3.xml first - the test ended up succeeding! So that was strange, but I guess not totally unexpected since the device mems don't exist and the wrong check was being done.
So I went back to whence we started and note that qemuxml2argvtest fails the hugepages-memaccess3 without any of these changes with (as seen with VIR_TEST_DEBUG=1 VIR_TEST_RANGE=169 tests/qemuxml2argvtest):
169) QEMU XML-2-ARGV hugepages-memaccess3 ... Got expected error: 2018-08-27 21:24:04.934+0000: 5816: info : libvirt version: 4.7.0 2018-08-27 21:24:04.934+0000: 5816: info : hostname: localhost.localdomain 2018-08-27 21:24:04.934+0000: 5816: error : qemuProcessStartValidateVideo:5033 : unsupported configuration: this QEMU does not support 'virtio' video device
So, going a bit slower... and removing things to see if I can get that "memory access mode '%s' not supported..." error message...
1. Remove <video> and <graphics> entries, but get:
169) QEMU XML-2-ARGV hugepages-memaccess3 ... Got expected error: 2018-08-27 21:26:28.754+0000: 5907: info : libvirt version: 4.7.0 2018-08-27 21:26:28.754+0000: 5907: info : hostname: localhost.localdomain 2018-08-27 21:26:28.754+0000: 5907: error : qemuBuildPMCommandLine:6523 : unsupported configuration: setting ACPI S3 not supported
2. Remove <pm> and get:
169) QEMU XML-2-ARGV hugepages-memaccess3 ... Got expected error: 2018-08-27 21:27:48.000+0000: 5972: info : libvirt version: 4.7.0 2018-08-27 21:27:48.000+0000: 5972: info : hostname: localhost.localdomain 2018-08-27 21:27:48.000+0000: 5972: error : qemuBuildUSBControllerDevStr:2681 : unsupported configuration: piix3-usb-uhci not supported in this QEMU binary
3. Remove "model='piix3-uhci'" from the <controller type='usb' and get:
169) QEMU XML-2-ARGV hugepages-memaccess3 ... Got expected error: 2018-08-27 21:29:01.554+0000: 6179: info : libvirt version: 4.7.0 2018-08-27 21:29:01.554+0000: 6179: info : hostname: localhost.localdomain 2018-08-27 21:29:01.554+0000: 6179: error : qemuCheckDiskConfig:1297 : unsupported configuration: discard is not supported by this QEMU binary
4. Remove "discard='unmap' from the <disk> and get:
169) QEMU XML-2-ARGV hugepages-memaccess3 ... Got expected error: 2018-08-27 21:30:21.989+0000: 6360: info : libvirt version: 4.7.0 2018-08-27 21:30:21.989+0000: 6360: info : hostname: localhost.localdomain 2018-08-27 21:30:21.989+0000: 6360: error : qemuBuildSerialChrDeviceStr:10576 : unsupported configuration: 'isa-serial' is not supported in this QEMU binary
5. Remove <serial>, <console>, and <channel> and get passed instead of failure.
You really have a good patience ;) i deleted all the unrelated elements when i saw the error come from qemuProcessStartValidateVideo() function. BTW: I think maybe we can add some function like DO_TEST_FAILURE_CHECK_ERROR which use regex to check if the error is expected.
So at first I was a bit stumped as to what happened... Could there be "conflict" or no error message now due to changes Pavel Hrdina made dealing with hugepages and parsing. So, in any case, I'd like to wait for Michal's input before proceeding further on this one.
Sure, and i have checked that there is no conflict with the Pavel Hrdina's "Fix and improve hugepage code" patch set.
BTW: Your changes look correct and fine to me as well as eliciting the proper error message, which is good/proper, but before pushing let's see if there's feedback from Michal.
John
Thanks a lot for your review ! Have a nice day ! Luyao
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index f570081..f0c461b 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -3888,6 +3888,29 @@ qemuDomainDefValidateFeatures(const virDomainDef *def,
static int +qemuDomainDefValidateMemory(const virDomainDef *def) +{ + const long system_page_size = virGetSystemPageSizeKB(); + + /* We can't guarantee any other mem.access + * if no guest NUMA nodes are defined. */ + if (def->mem.nhugepages != 0 && + def->mem.hugepages[0].size != system_page_size && + virDomainNumaGetNodeCount(def->numa) == 0 && + 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; + } + + return 0; +} + + +static int qemuDomainDefValidate(const virDomainDef *def, virCapsPtr caps ATTRIBUTE_UNUSED, void *opaque) @@ -4009,6 +4032,9 @@ qemuDomainDefValidate(const virDomainDef *def, if (qemuDomainDefValidateFeatures(def, qemuCaps) < 0) goto cleanup;
+ if (qemuDomainDefValidateMemory(def) < 0) + goto cleanup; + ret = 0;
cleanup: @@ -5531,30 +5557,6 @@ qemuDomainDeviceDefValidateController(const virDomainControllerDef *controller,
static int -qemuDomainDeviceDefValidateMemory(const virDomainMemoryDef *memory ATTRIBUTE_UNUSED, - const virDomainDef *def) -{ - const long system_page_size = virGetSystemPageSizeKB(); - - /* We can't guarantee any other mem.access - * if no guest NUMA nodes are defined. */ - if (def->mem.nhugepages != 0 && - def->mem.hugepages[0].size != system_page_size && - virDomainNumaGetNodeCount(def->numa) == 0 && - 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; - } - - return 0; -} - - -static int qemuDomainDeviceDefValidateVsock(const virDomainVsockDef *vsock, const virDomainDef *def, virQEMUCapsPtr qemuCaps) @@ -5712,10 +5714,6 @@ qemuDomainDeviceDefValidate(const virDomainDeviceDef *dev, qemuCaps); break;
- case VIR_DOMAIN_DEVICE_MEMORY: - ret = qemuDomainDeviceDefValidateMemory(dev->data.memory, def); - break; - case VIR_DOMAIN_DEVICE_VSOCK: ret = qemuDomainDeviceDefValidateVsock(dev->data.vsock, def, qemuCaps); break; @@ -5737,6 +5735,7 @@ qemuDomainDeviceDefValidate(const virDomainDeviceDef *dev, case VIR_DOMAIN_DEVICE_MEMBALLOON: case VIR_DOMAIN_DEVICE_NVRAM: case VIR_DOMAIN_DEVICE_SHMEM: + case VIR_DOMAIN_DEVICE_MEMORY: case VIR_DOMAIN_DEVICE_PANIC: case VIR_DOMAIN_DEVICE_IOMMU: case VIR_DOMAIN_DEVICE_NONE: diff --git a/tests/qemuxml2argvdata/hugepages-memaccess3.xml b/tests/qemuxml2argvdata/hugepages-memaccess3.xml index 8ec38d8..43c3ec6 100644 --- a/tests/qemuxml2argvdata/hugepages-memaccess3.xml +++ b/tests/qemuxml2argvdata/hugepages-memaccess3.xml @@ -12,76 +12,18 @@ <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='usb' index='0'/> <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> + <memballoon model='none'/> </devices> </domain> diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index 0e9eef6..6b55316 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -1023,9 +1023,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_OBJECT_MEMORY_RAM, QEMU_CAPS_OBJECT_MEMORY_FILE, - QEMU_CAPS_VIRTIO_SCSI); + DO_TEST_PARSE_ERROR("hugepages-memaccess3", + QEMU_CAPS_OBJECT_MEMORY_RAM, + QEMU_CAPS_OBJECT_MEMORY_FILE); DO_TEST_CAPS_LATEST("hugepages-nvdimm"); DO_TEST("nosharepages", QEMU_CAPS_MEM_MERGE); DO_TEST("disk-cdrom", NONE);
On 08/27/2018 11:51 PM, lhuang wrote:
On 08/28/2018 06:01 AM, John Ferlan wrote:
On 08/20/2018 05:48 AM, Luyao Huang wrote:
commit 6534b3c4 try to raise an error when there is no numa nodes but set access='shared' in domain config. In that commit, we add a memory access validate function for memory device, but this check is not related to memory device and won't work if there is no memory device in guest xml.
Move the memory access related check in the domain config validate function, and simplify the unit test xml to avoid other error.
https://bugzilla.redhat.com/show_bug.cgi?id=1448149#c14
Signed-off-by: Luyao Huang <lhuang@redhat.com> --- src/qemu/qemu_domain.c | 55 +++++++++++----------- tests/qemuxml2argvdata/hugepages-memaccess3.xml | 62 +------------------------ tests/qemuxml2argvtest.c | 6 +-- 3 files changed, 32 insertions(+), 91 deletions(-)
Hmm... I have a recollection of reviewing this... Let's see, yes:
https://www.redhat.com/archives/libvir-list/2017-December/msg00387.html
and follow-ups... I think I agree with you the validation should be done in qemuDomainDefValidate instead of qemuDomainDeviceDefValidate; however, you're making multiple changes at one time - so I went to dissect... This response got lengthy and I'm kind of at a hmm? wtf moment that hopefully Michal can look at since he was the original author. Maybe he can recall 8 months ago and whether he got the expected error message or not on output.
My concern/point was changing hugepages-memaccess3.xml and qemuxml2argvtest.c to remove some things while also changing from DO_TEST_FAILURE to DO_TEST_PARSE_ERROR is doing two different things in one patch, which we generally try to avoid, but in this case it may just be necessary.
yes, i was trying to fix the hugepages-memaccess3.xml since my changes will break the unit test. Actually the code changes for the hugepages-memaccess3.xml and qemuxml2argvtest.c included 2 different fix, one for fixing the exist unit test and another for fixing the failure introduced in this patch. And i merged them in one patch, maybe i should keep them split in next time ?
When I tried to separate the changes in such a way that the unnecessary lines were removed from hugepages-memaccess3.xml first - the test ended up succeeding! So that was strange, but I guess not totally unexpected since the device mems don't exist and the wrong check was being done.
So I went back to whence we started and note that qemuxml2argvtest fails the hugepages-memaccess3 without any of these changes with (as seen with VIR_TEST_DEBUG=1 VIR_TEST_RANGE=169 tests/qemuxml2argvtest):
169) QEMU XML-2-ARGV hugepages-memaccess3 ... Got expected error: 2018-08-27 21:24:04.934+0000: 5816: info : libvirt version: 4.7.0 2018-08-27 21:24:04.934+0000: 5816: info : hostname: localhost.localdomain 2018-08-27 21:24:04.934+0000: 5816: error : qemuProcessStartValidateVideo:5033 : unsupported configuration: this QEMU does not support 'virtio' video device
So, going a bit slower... and removing things to see if I can get that "memory access mode '%s' not supported..." error message...
1. Remove <video> and <graphics> entries, but get:
169) QEMU XML-2-ARGV hugepages-memaccess3 ... Got expected error: 2018-08-27 21:26:28.754+0000: 5907: info : libvirt version: 4.7.0 2018-08-27 21:26:28.754+0000: 5907: info : hostname: localhost.localdomain 2018-08-27 21:26:28.754+0000: 5907: error : qemuBuildPMCommandLine:6523 : unsupported configuration: setting ACPI S3 not supported
2. Remove <pm> and get:
169) QEMU XML-2-ARGV hugepages-memaccess3 ... Got expected error: 2018-08-27 21:27:48.000+0000: 5972: info : libvirt version: 4.7.0 2018-08-27 21:27:48.000+0000: 5972: info : hostname: localhost.localdomain 2018-08-27 21:27:48.000+0000: 5972: error : qemuBuildUSBControllerDevStr:2681 : unsupported configuration: piix3-usb-uhci not supported in this QEMU binary
3. Remove "model='piix3-uhci'" from the <controller type='usb' and get:
169) QEMU XML-2-ARGV hugepages-memaccess3 ... Got expected error: 2018-08-27 21:29:01.554+0000: 6179: info : libvirt version: 4.7.0 2018-08-27 21:29:01.554+0000: 6179: info : hostname: localhost.localdomain 2018-08-27 21:29:01.554+0000: 6179: error : qemuCheckDiskConfig:1297 : unsupported configuration: discard is not supported by this QEMU binary
4. Remove "discard='unmap' from the <disk> and get:
169) QEMU XML-2-ARGV hugepages-memaccess3 ... Got expected error: 2018-08-27 21:30:21.989+0000: 6360: info : libvirt version: 4.7.0 2018-08-27 21:30:21.989+0000: 6360: info : hostname: localhost.localdomain 2018-08-27 21:30:21.989+0000: 6360: error : qemuBuildSerialChrDeviceStr:10576 : unsupported configuration: 'isa-serial' is not supported in this QEMU binary
5. Remove <serial>, <console>, and <channel> and get passed instead of failure.
You really have a good patience ;) i deleted all the unrelated elements when i saw the error come from qemuProcessStartValidateVideo() function.
BTW: I think maybe we can add some function like DO_TEST_FAILURE_CHECK_ERROR which use regex to check if the error is expected.
So at first I was a bit stumped as to what happened... Could there be "conflict" or no error message now due to changes Pavel Hrdina made dealing with hugepages and parsing. So, in any case, I'd like to wait for Michal's input before proceeding further on this one.
Sure, and i have checked that there is no conflict with the Pavel Hrdina's "Fix and improve hugepage code" patch set.
BTW: Your changes look correct and fine to me as well as eliciting the proper error message, which is good/proper, but before pushing let's see if there's feedback from Michal.
John
Thanks a lot for your review !
Have a nice day ! Luyao
I got word from Michal in private chat that he agrees with the patch... So, I adjusted the commit message a bit and pushed (using bugfix during freeze) with my R-By tag, John
participants (3)
-
John Ferlan -
lhuang -
Luyao Huang