Convert to virGetLastErrorMessage() in the rest of the code

SRIOV VFs used in macvtap passthrough mode can take advantage of the
SRIOV card's transparent vlan tagging. All the code was there to set
the vlan tag, and it has been used for SRIOV VFs used for hostdev
interfaces for several years, but for some reason, the vlan tag for
macvtap passthrough devices was stubbed out with a -1.

This patch moves a bit of common validation down to a lower level
(virNetDevReplaceNetConfig()) so it is shared by hostdev and macvtap
modes, and updates the macvtap caller to actually send the vlan config
instead of -1.

This reverts commit ee4cfb56.

Since we're still not persisting our bookkeeping lists across
daemon restarts, we might have lost some information
virPCIDeviceReattach() relies on, for example whether the
device needs to be unbound from the stub driver.

As a result, if the daemon has been restarted in the meantime,
the device might end up remaining bound to the stub driver even
after 'virsh nodedev-reattach' or similar has been called, with
no way of giving it back to the host short of messing with
sysfs behind libvirt's back.

Revert back to the previous behavior of always trying to bind
the device to the host driver, regardless of its status when it
was detached, until persistent bookkeeping lists have been
implemented.

Instead of forcing the values for the unbind_from_stub, remove_slot
and reprobe properties, look up the actual device and use that when
calling virPCIDeviceReattach().

This ensures the device is restored to its original state after
reattach: for example, if it was not bound to any driver before
detach, it will not be bound forcefully during reattach.

We would be just fine looking up the information in pcidevs most
of the time; however, some corner cases would not be handled
properly, so look up the actual device instead.

After this patch, ownership of virPCIDevice instances is very easy
to keep track of: for each host PCI device, the only instance that
actually matters is the one inside one of the bookkeeping list.

Whenever some operation needs to be performed on a PCI device, the
actual device is looked up first; when this is not the case, a
comment explains the reason.

Unmanaged devices, as the name suggests, are not detached
automatically from the host by libvirt before being attached to a
guest: it's the user's responsability to detach them manually
beforehand. If that preliminary step has not been performed, the
attach operation can't complete successfully.

Instead of relying on the lower layers to error out with cryptic
messages such as

  error: Failed to attach device from /tmp/hostdev.xml
  error: Path '/dev/vfio/12' is not accessible: No such file or directory

prevent the situation altogether and provide the user with a more
useful error message.

Unmanaged devices are attached to guests in two steps: first,
the device is detached from the host and marked as inactive;
subsequently, it is marked as active and attached to the guest.

If the daemon is restarted between these two operations, we lose
track of the inactive device.

Steps 5 and 6 of virHostdevPreparePCIDevices() already subtly
take care of this situation, but some planned changes will make
it so that's no longer the case. Plus, explicit is always better
than implicit.

These comments explain the difference between a virPCIDevice
instance used for lookups and an actual device instance; some
information is also provided for specific uses.

This is not just a cosmetic change: the name of the variable now
gives a hint about what it is supposed to be used for.

virHostdevGetPCIHostDeviceList() is similar but does not filter out
devices that are not in the active list; that said, we are looking
up the device in the active list just a few lines after anyway, so
we might as well just keep a single function around.

This also helps stress the fact the objects contained in pcidevs are
only for looking up the actual devices, which is something later
commits will make even more explicit.

Acronyms should be written in all caps.

We're in the hostdev module, so mgr is not an ambiguous name, and
in fact it's already used in some cases. Switch all the code over.

Take the chance to shorten declaration of
virHostdevIsPCINodeDeviceUsedData structures.

When we want to look up a device in a device list and we already
have the IDs from another source, we can simply use
virPCIDeviceListFindByIDs() instead of creating a temporary device
object.

The new order aligns better with the virHostdev prefix.

The only possible return values are true and false, so the return
type should be bool instead of int.

If 'last_processed_hostdev_vf != -1' is false then, since the
loop counter 'i' starts at 0, 'i <= last_processed_hostdev_vf'
can't possibly be true and the loop body will never be executed.

However, since 'i' is unsigned and 'last_processed_hostdev_vf'
is signed, we can't just get rid of the check completely; what
we can do is move it outside of the loop to avoid checking its
value on every iteration and cluttering the actual loop
condition.

NULL checks are performed implicitly in the rest of the module,
including other allocations in the very same function.

The comment claimed that virPCIDeviceReattach() does not reattach
a device to the host driver; except it actually does, so the
comment is just confusing and we're better off removing it.

Replace the term "loop" with the more generic "step". This allows us
to be more flexible and eg. have a step that consists in a single
function call.

Don't include the number of steps in the first comment of the
function, so that we can add or remove steps without having to worry
about keeping that comment in sync.

For the same reason, remove the summary contained in that comment.

Clean up some weird vertical spacing while we're at it.

The virHostdevIsVirtualFunction() was called exactly twice, and in
both cases the return value was saved to a temporary variable before
being checked. This would be okay if it improved readability, but in
this case is pretty pointless.

Get rid of the temporary variable and check the return value
directly; while at it, change the check from '<= 0' to '!= 1' to
align it with the way other similar *IsVirtualFunction() functions
are used thorough the code.

Mostly labels names and whitespace.

No functional changes.

The name is confusing, and there are just two uses: one is a test case,
and the other will be removed as part of an upcoming refactoring of
the hostdev code.

Both detach and reattach are complex operations involving several steps,
and it can be useful to be able to follow along by reading the log.

Since we don't detach unmanaged devices before attaching them to a
domain, we shouldn't reattach them to rollback an error either.

We want to eventually factor out the code dealing with device detaching
and reattaching, so that we can share it and make sure it's called eg.
when 'virsh nodedev-detach' is used.

For that to happen, it's important that the lists of active and inactive
PCI devices are updated every time a device changes its state.

Instead of passing NULL as the last argument of virPCIDeviceDetach() and
virPCIDeviceReattach(), pass the proper list so that it can be updated.

This replaces the virPCIKnownStubs string array that was used
internally for stub driver validation.

Advantages:

  * possible values are well-defined
  * typos in driver names will be detected at compile time
  * avoids having several copies of the same string around
  * no error checking required when setting / getting value

The names used mirror those in the
virDomainHostdevSubsysPCIBackendType enumeration.

Instead of replicating the information (domain, bus, slot, function)
inside the virPCIDevice structure, use the already-existing
virPCIDeviceAddress structure.

For users of the module, this means that the object returned by
virPCIDeviceGetAddress() can no longer be NULL and must no longer
be freed by the caller.

The new name, virHostdevUpdateActiveDomainDevices(), follows the
same naming conventions used by the rest of the module.

No functional changes.

The ACS checks are meaningless when using the more modern VFIO driver
for device assignment since VFIO has its own more complete and exact
checks, but I didn't realize that when I added support for VFIO. This
patch eliminates the ACS check when preparing PCI devices for
assignment if VFIO is being used.

This resolves:

  https://bugzilla.redhat.com/show_bug.cgi?id=1256486

The commit 7e72de49 didn't consider the hotplug scenarios. The patch addresses
the hotplug case whereby if atleast one of the pci function is owned by a
guest, the hotplug of other functions/devices in the same iommu group to the
same guest goes through successfully.
Signed-off-by: NShivaprasad G Bhat <sbhat@linux.vnet.ibm.com>

The SCSI Architecture Model defines a logical unit address
as 64-bits in length, so change the field accordingly so
that the entire value could be stored.
Signed-off-by: NEric Farman <farman@linux.vnet.ibm.com>

The address elements are all unsigned integers, so we should
use the appropriate print directive when printing it.
Signed-off-by: NEric Farman <farman@linux.vnet.ibm.com>

Pushed previous patch (commit id 'd45dadae') too quickly...

The patch just updates the patch from Laine's suggestions.

Fix for such a case:
1. Domain A and B xml contain the same SRIOV net hostdev(<interface
type='hostdev' /> with same pci address).
2. virsh start A (Successfully, and configure the SRIOV net with
custom mac)
3. virsh start B (Fail because of the hostdev used by domain A or other
reason.)
In step 3, 'virHostdevNetConfigRestore' is called for the hostdev
which is still used by domain A. It makes the mac/vlan of the SRIOV net
change.

Code Change in this fix:
1. As the pci used by other domain have been removed from
'pcidevs' in previous loop, we only restore the nic config for
the hostdev still in 'pcidevs'(used by this domain)
2. update the comments to make it more clear
Signed-off-by: NHuanle Han <hanxueluo@gmail.com>

Refactor some code to create a static function virHostdevIsPCINetDevice
which will detect whether the hostdev is a pci net device or not.
Signed-off-by: NHuanle Han <hanxueluo@gmail.com>

The variable 'last_processed_hostdev_vf' indicates index of the last
successfully configed vf. When resetvfnetconfig because of failure,
hostdevs[last_processed_hostdev_vf] should also be reset.
Signed-off-by: NHuanle Han <hanxueluo@gmail.com>

The nodedev-detach can report the name of the domain using the device
just the way nodedev-reattach does it.
Signed-off-by: NShivaprasad G Bhat <sbhat@linux.vnet.ibm.com>
Signed-off-by: NJán Tomko <jtomko@redhat.com>

'virPCIDeviceList' is actually an array. Removing one element makes the
rest of the element move.

Use while loop, increase index only when not virPCIDeviceListDel(pcidevs, dev)
Signed-off-by: NHuanle Han <hanxueluo@gmail.com>