[libvirt] [PATCH] qemu: domain: validate memory access during validate domain config
lhuang
lhuang at redhat.com
Tue Aug 28 03:51:26 UTC 2018
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 at 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);
>>
More information about the libvir-list
mailing list