Re: [PATCH] qemu: domain: validate memory access during validate domain config

[lhuang <lhuang@xxxxxxxxxx>]

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@xxxxxxxxxx>
> > ---
> >   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);

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list