[libvirt] [PATCH v3 08/10] qemu: Check whether hub device is busy

[John Ferlan]

On 11/11/18 10:59 PM, Han Han wrote:
> qemuDomainHubIsBusy is to check whether a usb device are attached to the hub
> device. It will be used for hotunplugging and live device update of hub
> device.
> 
> Signed-off-by: Han Han <hhan at redhat.com>
> ---
>  src/qemu/qemu_hotplug.c | 64 +++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 64 insertions(+)
> 

This would need to be merged with the subsequent patch because the new
method is static, but it sure makes it easier to review when it's pulled
out.

Beyond that I find I see there is a virDomainUSBDeviceDefForeach which
perhaps could have be used. This code did miss the nserials

The whole assign/remove address space isn't something I know really
well, but in reading the code I note:

1. When a new hub is created, virDomainUSBAddressHubNew will create a
"hub->portmap" to manage the active ports for the hub. This will be
inserted into the priv->usbaddrs address set (something important to
understand later).

2. When a device is assigned to a hub, virDomainUSBAddressSetAddHub is
called which calls virDomainUSBAddressFindPort to find the targetHub for
which a virBitmapSetBit on the portmap is done for the port being used.

3. When a device using a hub is removed, virDomainUSBAddressRelease is
called by either qemuDomainReleaseDeviceAddress or various disk hot
unplug helpers. This in turn calls virDomainUSBAddressFindPort to return
the targetHub which is being unplugged. Then a virBitmapClearBit on done
for the port being used.

4. Nothing seems to care that an address set entry has an empty bitmap.
That is, if the last port is cleared on the hub, there's no automatic
removal, although there is automatic add when space is full. I think
that's an important distinction.

5. The only time virDomainUSBAddressSetFree is called to free
priv->usbaddrs is when a domain is stopped or the domain object is
disposed of (call to privateDataFreeFunc).

So what does this all mean, well if this device were removed it would
likewise call qemuDomainReleaseDeviceAddress in order to "clear" the
targetPort bit on the targetHub that this device was using if by chance
it was plugged into another hub (daisy chaining).

Still, shouldn't checking whether the device about to be deleted hub has
anything attached to be as simple as checking that it's own portmap is
clear? If it is clear, then removing it wouldn't remove any other
devices in collateral damage.

However, there's another gotcha. Recall that when a hub is added the
priv->usbaddrs is referenced, VIR_EXPAND_N called, and the hub placed
into the address set.

If you free something in the address set, then the address set is
potentially pointing at memory that it no longer owns. Once a add a
device to a hub is called, it's going to attempt to reference that
address and "may" or "may not" succeed. It may look like it succeeds,
but corruption is sure to follow.  And from experience, I can tell you
that type of corruption is the absolute most difficult thing to find.

So you will need to add code to handle shrinking the priv->usbaddrs
since you're essentially about to free something in it.  You should be
able to use logic from patch7 to become a callback/iter function for
virDomainUSBDeviceDefForeach (and it does matter if a hub is plugged
into the hub you'd be attempting to remove). Study the existing
consumers of virDomainUSBDeviceDefForeach - especially how they add
hubdef's and hubaddr's. Then consider how would you safely remove. The
logic seems to be intertwined with controllers too.

John

> diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
> index 1b6cc36bc8..ca73456260 100644
> --- a/src/qemu/qemu_hotplug.c
> +++ b/src/qemu/qemu_hotplug.c
> @@ -5332,6 +5332,70 @@ static bool qemuDomainControllerIsBusy(virDomainObjPtr vm,
>      }
>  }
>  
> +static bool qemuDomainHubIsBusy(virDomainObjPtr vm,
> +                                virDomainHubDefPtr detach)
> +{
> +    size_t i;
> +    virDomainDiskDefPtr disk;
> +    virDomainHubDefPtr hub;
> +    virDomainInputDefPtr input;
> +    virDomainSoundDefPtr sound;
> +    virDomainHostdevDefPtr hostdev;
> +    virDomainRedirdevDefPtr redirdev;
> +    virDomainControllerDefPtr controller;
> +
> +    for (i = 0; i < vm->def->ndisks; i++) {
> +        disk = vm->def->disks[i];
> +        if (disk->bus == VIR_DOMAIN_DISK_BUS_USB &&
> +            virDomainUSBAddressIsAttachedToHub(&(disk->info), detach))
> +            return true;
> +    }
> +
> +    for (i = 0; i < vm->def->nhubs; i++) {
> +        hub = vm->def->hubs[i];
> +        if (virDomainUSBAddressIsAttachedToHub(&(hub->info), detach))
> +            return true;
> +    }
> +
> +    for (i = 0; i < vm->def->ninputs; i++) {
> +        input = vm->def->inputs[i];
> +        if (input->bus == VIR_DOMAIN_INPUT_BUS_USB &&
> +            virDomainUSBAddressIsAttachedToHub(&(input->info), detach))
> +            return true;
> +    }
> +
> +    for (i = 0; i < vm->def->nsounds; i++) {
> +        sound = vm->def->sounds[i];
> +        if (sound->model == VIR_DOMAIN_SOUND_MODEL_USB &&
> +            virDomainUSBAddressIsAttachedToHub(&(sound->info), detach))
> +            return true;
> +    }
> +
> +    for (i = 0; i < vm->def->nhostdevs; i++) {
> +        hostdev = vm->def->hostdevs[i];
> +        if (hostdev->mode == VIR_DOMAIN_HOSTDEV_MODE_SUBSYS &&
> +            hostdev->source.subsys.type == VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_USB &&
> +            virDomainUSBAddressIsAttachedToHub(hostdev->info, detach))
> +            return true;
> +    }
> +
> +    for (i = 0; i < vm->def->nredirdevs; i++) {
> +        redirdev = vm->def->redirdevs[i];
> +        if (redirdev->bus == VIR_DOMAIN_REDIRDEV_BUS_USB &&
> +            virDomainUSBAddressIsAttachedToHub(&(redirdev->info), detach))
> +            return true;
> +    }
> +
> +    for (i = 0; i < vm->def->ncontrollers; i++) {
> +        controller = vm->def->controllers[i];
> +        if (controller->type == VIR_DOMAIN_CONTROLLER_TYPE_CCID &&
> +            virDomainUSBAddressIsAttachedToHub(&(controller->info), detach))
> +            return false;
> +    }
> +
> +    return false;
> +}
> +
>  int qemuDomainDetachControllerDevice(virQEMUDriverPtr driver,
>                                       virDomainObjPtr vm,
>                                       virDomainDeviceDefPtr dev,
>