10:35 a.m.
On Fri, Mar 13, 2015 at 10:39:05 -0400, John Ferlan wrote:
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
or any other type as PCI, USB ... basically the check is here as if the
device had any address assigned it should have been rejected already by
previous call to this function with @allowAddressFallback equal to
false.
> + } 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
... or any other supported address type which translates to "the address
is set"
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'
No, it would actually make it less straightforward. This code is shared
so it has to be device address type agnostic.
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...
It does indeed, but in this case the logic of the lookup requires the
extra check as if the user doesn't specify the alias of the memory
device to find, we still do might want do find a device definition that
already has the alias filled but the user didn't specify it explicitly.
> +
> + /* 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...
The address will be set if and only if the user provided it when adding
the device. Ohterwise the address will be empty. In that case we want to
find the correct device first and ignore addresses in case the user
provided the XML from the running device but is expecting the inactive
configuration (which does not have an address set) to be removed too.
That is if my incoming has an address, then don't allow fallback;
otherwise, well best match.
That exactly wouldn't work in my previous example, where you take the
live XML (with the addres auto-assigned) the code would not match it's
original definition that did not have the address assigned.
> +
> + 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.
This actually is code in the common path, so that it only looks if a
device with the same address is not present and adds the device.
The device address type may contain any other device address type we
currently support although it may not make sense (PCI memory modules for
example).
It's the duty of the individual driver to check that the configuration
is correct, this is merely to deduplicate some work that would be shared
if more drivers supported memory hotplug.
> + virReportError(VIR_ERR_OPERATION_INVALID, "%s",
> + _("Domain already contains a device with the same
"
> + "address"));
> + return -1;
> + }
Peter