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
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
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
[...]
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
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
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
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
>
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
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
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
© 2016 - 2025 Red Hat, Inc.