Re: [PATCHv2.5 08/10] qemu: conf: Add support for memory device cold(un)plug

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




On 03/04/2015 11:24 AM, Peter Krempa wrote:
> Add a few helpers that allow to operate with memory device definitions
> on the domain config and use them to implement memory device coldplug in
> the qemu driver.
> ---
>  src/conf/domain_conf.c   | 100 +++++++++++++++++++++++++++++++++++++++++++++++
>  src/conf/domain_conf.h   |  10 +++++
>  src/libvirt_private.syms |   4 ++
>  src/qemu/qemu_driver.c   |  15 ++++++-
>  4 files changed, 127 insertions(+), 2 deletions(-)
> 
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 4f20aa6..0f6058b 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -12687,6 +12687,106 @@ virDomainRNGRemove(virDomainDefPtr def,
>  }
> 
> 
> +static int
> +virDomainMemoryFindByDefInternal(virDomainDefPtr def,
> +                                 virDomainMemoryDefPtr mem,
> +                                 bool allowAddressFallback)
> +{
> +    size_t i;
> +
> +    for (i = 0; i < def->nmems; i++) {
> +        virDomainMemoryDefPtr tmp = def->mems[i];
> +
> +        /* address, if present */
> +        if (allowAddressFallback) {
> +            if (tmp->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE)
> +                continue;

So this path says - we want address fallback and this device has either
DIMM or LAST (oy!) as a type, so go to the next one and ignore this one

> +        } else {
> +            if (mem->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE &&
> +                !virDomainDeviceInfoAddressIsEqual(&tmp->info, &mem->info))
> +                continue;

This path says - we don't want address fallback and as long as we're
DIMM or LAST (oy!), then compare

What happens when we add a new address type? It would seem the more
straightforward way would be

    if (type == DIMM && !Equal)
        if (!allowAddressFallback)
            continue'

Otherwise we're falling through to alias, target, etc. checks

> +        }
> +
> +        /* alias, if present */
> +        if (mem->info.alias &&
> +            STRNEQ_NULLABLE(tmp->info.alias, mem->info.alias))
> +            continue;

I thought the NULLABLE checks both elems for NULL...

> +
> +        /* target info -> always present */
> +        if (tmp->model != mem->model ||
> +            tmp->targetNode != mem->targetNode ||
> +            tmp->size != mem->size)
> +            continue;
> +
> +        /* source stuff -> match with device */
> +        if (tmp->pagesize != mem->pagesize)
> +            continue;
> +
> +        if (!virBitmapEqual(tmp->sourceNodes, mem->sourceNodes))
> +            continue;
> +
> +        break;
> +    }
> +
> +    if (i == def->nmems)
> +        return -1;
> +
> +    return i;
> +}
> +
> +
> +int
> +virDomainMemoryFindByDef(virDomainDefPtr def,
> +                         virDomainMemoryDefPtr mem)
> +{
> +    return virDomainMemoryFindByDefInternal(def, mem, false);
> +}
> +
> +
> +int
> +virDomainMemoryFindInactiveByDef(virDomainDefPtr def,
> +                                 virDomainMemoryDefPtr mem)
> +{
> +    int ret;
> +
> +    if ((ret = virDomainMemoryFindByDefInternal(def, mem, false)) < 0)
> +        ret = virDomainMemoryFindByDefInternal(def, mem, true);

I would seem Inactive would probably not have the dimm address set, so
we're incurring a 2X penalty for perhaps no reason... Unless perhaps the
incoming mem def being checked has an address...

That is if my incoming has an address, then don't allow fallback;
otherwise, well best match.
> +
> +    return ret;
> +}
> +
> +
> +int
> +virDomainMemoryInsert(virDomainDefPtr def,
> +                      virDomainMemoryDefPtr mem)
> +{
> +    int id = def->nmems;
> +
> +    if (mem->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE &&
> +        virDomainDefHasDeviceAddress(def, &mem->info)) {

Hmm... so if our incoming mem has an address defined - it could be
anything - we're just failing declaring that the domain already contains
a device with the same address? Doesn't seem right.

And again - we have this problem of TYPE_NONE, TYPE_DIMM, TYPE_LAST and
who knows what in the future.


> +        virReportError(VIR_ERR_OPERATION_INVALID, "%s",
> +                       _("Domain already contains a device with the same "
> +                         "address"));
> +        return -1;
> +    }
> +
> +    if (VIR_APPEND_ELEMENT(def->mems, def->nmems, mem) < 0)
> +        return -1;
> +
> +    return id;
> +}
> +
> +
> +virDomainMemoryDefPtr
> +virDomainMemoryRemove(virDomainDefPtr def,
> +                      int idx)
> +{
> +    virDomainMemoryDefPtr ret = def->mems[idx];
> +    VIR_DELETE_ELEMENT(def->mems, idx, def->nmems);
> +    return ret;
> +}
> +
> +
>  char *
>  virDomainDefGetDefaultEmulator(virDomainDefPtr def,
>                                 virCapsPtr caps)
> diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
> index 706040d..c38614d 100644
> --- a/src/conf/domain_conf.h
> +++ b/src/conf/domain_conf.h
> @@ -2853,6 +2853,16 @@ virDomainChrDefGetSecurityLabelDef(virDomainChrDefPtr def, const char *model);
>  typedef const char* (*virEventActionToStringFunc)(int type);
>  typedef int (*virEventActionFromStringFunc)(const char *type);
> 
> +int virDomainMemoryInsert(virDomainDefPtr def, virDomainMemoryDefPtr mem)
> +    ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_RETURN_CHECK;
> +virDomainMemoryDefPtr virDomainMemoryRemove(virDomainDefPtr def, int idx)
> +    ATTRIBUTE_NONNULL(1);
> +int virDomainMemoryFindByDef(virDomainDefPtr def, virDomainMemoryDefPtr mem)
> +    ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_RETURN_CHECK;
> +int virDomainMemoryFindInactiveByDef(virDomainDefPtr def,
> +                                     virDomainMemoryDefPtr mem)
> +    ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_RETURN_CHECK;
> +
>  VIR_ENUM_DECL(virDomainTaint)
>  VIR_ENUM_DECL(virDomainVirt)
>  VIR_ENUM_DECL(virDomainBoot)
> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> index 2491d2d..1504c9a 100644
> --- a/src/libvirt_private.syms
> +++ b/src/libvirt_private.syms
> @@ -334,6 +334,10 @@ virDomainLockFailureTypeToString;
>  virDomainMemballoonModelTypeFromString;
>  virDomainMemballoonModelTypeToString;
>  virDomainMemoryDefFree;
> +virDomainMemoryFindByDef;
> +virDomainMemoryFindInactiveByDef;
> +virDomainMemoryInsert;
> +virDomainMemoryRemove;
>  virDomainNetAppendIpAddress;
>  virDomainNetDefFormat;
>  virDomainNetDefFree;
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index d6935d9..6b10e96 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -7438,7 +7438,10 @@ qemuDomainAttachDeviceConfig(virQEMUCapsPtr qemuCaps,
>          break;
> 
>      case VIR_DOMAIN_DEVICE_MEMORY:
> -        /* TODO: implement later */
> +        if (virDomainMemoryInsert(vmdef, dev->data.memory) < 0)
> +            return -1;
> +        dev->data.memory = NULL;
> +        break;
> 
>      case VIR_DOMAIN_DEVICE_INPUT:
>      case VIR_DOMAIN_DEVICE_SOUND:
> @@ -7566,7 +7569,15 @@ qemuDomainDetachDeviceConfig(virDomainDefPtr vmdef,
>          break;
> 
>      case VIR_DOMAIN_DEVICE_MEMORY:
> -        /* TODO: implement later */
> +        if ((idx = virDomainMemoryFindInactiveByDef(vmdef,
> +                                                    dev->data.memory)) < 0) {
> +            virReportError(VIR_ERR_OPERATION_FAILED, "%s",
> +                           _("matching memory device was not found"));
> +            return -1;
> +        }
> +
> +        virDomainMemoryDefFree(virDomainMemoryRemove(vmdef, idx));

Ug - tricked my eyes on calling the Remove inside the DefFree


ACK with some cleanups which I think are more or less simple

John

I have to run - I will finish 9 & 10 a bit later.


> +        break;
> 
>      case VIR_DOMAIN_DEVICE_INPUT:
>      case VIR_DOMAIN_DEVICE_SOUND:
> 

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




[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]