The ZPCI device validation is specific to qemu. So, let us move the
ZPCI uid validation out of domain xml parsing into qemu domain device
validation.
Signed-off-by: Shalini Chellathurai Saroja <shalini@linux.ibm.com>
Reviewed-by: Bjoern Walk <bwalk@linux.ibm.com>
Reviewed-by: Boris Fiuczynski <fiuczy@linux.ibm.com>
Reviewed-by: Andrea Bolognani <abologna@redhat.com>
Let us fix the issues with zPCI address validation and auto-generation
on s390.
Currently, there are two issues with handling the ZPCI address
extension. Firstly, when the uid is to be auto-generated with a
specified fid, .i.e.:
...
<address type='pci'>
<zpci fid='0x0000001f'/>
</address>
...
we expect uid='0x0001' (or the next available uid for the domain).
However, we get a parsing error:
$ virsh define zpci.xml
error: XML error: Invalid PCI address uid='0x0000', must be > 0x0000
and <= 0xffff
Secondly, when the uid is specified explicitly with the invalid
numerical value '0x0000', we actually expect the parsing error above.
However, the domain is being defined and the uid value is silently
changed to a valid value.
The first issue is a bug and the second one is undesired behaviour, and
both issues are related to how we (in-band) signal invalid values for
uid and fid. So let's fix the XML parsing to do validation based on what
is actually specified in the XML.
The first issue is also related to the current code behaviour, which
is, if either uid or fid is specified by the user, it is incorrectly
assumed that both uid and fid are specified. This bug is fixed by
identifying when the user specified ZPCI address is incomplete and
auto-generating the missing ZPCI address.
Signed-off-by: Bjoern Walk <bwalk@linux.ibm.com>
Signed-off-by: Boris Fiuczynski <fiuczy@linux.ibm.com>
Signed-off-by: Shalini Chellathurai Saroja <shalini@linux.ibm.com>
Reviewed-by: Andrea Bolognani <abologna@redhat.com>
This new naming matches the terminology used in the error
messages that the callers report.
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
All callers except for the test suite pass the same value
for the second arg, so it can be removed, simplifying the
code.
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
Since 9ea90206, @drvpath could be overwritten if we jumped to recheck
Found by Coverity.
Signed-off-by: John Ferlan <jferlan@redhat.com>
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
Historically, this file was a dump for most of our helper
functions and needed almost everywhere.
With the introduction of virfile.h and virstring.h,
and more importantly, virenum.h and the introduction
of GLib, that is no longer true.
Remove its include from C files that don't even use it.
Signed-off-by: Ján Tomko <jtomko@redhat.com>
Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>
The last_component() method is a GNULIB custom function
that returns a pointer to the base name in the path.
This is similar to g_path_get_basename() but without the
malloc. The extra malloc is no trouble for libvirt's
needs so we can use g_path_get_basename().
Reviewed-by: Fabiano Fidêncio <fidencio@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
This helper is cleaner than plain memcpy() because one doesn't
have to look into virPCIDeviceAddress struct to see if it
contains any strings / pointers.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
ACKed-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Cole Robinson <crobinso@redhat.com>
Sometimes, we have a PCI address and not fully allocated
virPCIDevice and yet we still want to know its /dev/vfio/N path.
Introduce virPCIDeviceAddressGetIOMMUGroupDev() function exactly
for that.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Cole Robinson <crobinso@redhat.com>
Glib implementation follows the ISO C99 standard so it's safe to replace
the gnulib implementation.
Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
Commit d19c21429f modified the condition so that it checks whether the
value is more than 0xFFFFFFFF. Since addr->domain is an unsigned int, it
will never be more than that.
Remove the whole check
src/util/virpci.c:1291:22: error: result of comparison 'unsigned int' > 4294967295 is always false [-Werror,-Wtautological-type-limit-compare]
if (addr->domain > 0xFFFFFFFF) {
~~~~~~~~~~~~ ^ ~~~~~~~~~~
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Replace all occurrences of
if (VIR_STRDUP(a, b) < 0)
/* effectively dead code */
with:
a = g_strdup(b);
Signed-off-by: Ján Tomko <jtomko@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Replace:
if (!s && VIR_STRDUP(s, str) < 0)
goto;
with:
if (!s)
s = g_strdup(str);
Signed-off-by: Ján Tomko <jtomko@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Since commit 44e7f02915
util: rewrite auto cleanup macros to use glib's equivalent
VIR_AUTOPTR aliases to g_autoptr. Replace all of its use by the GLib
macro version.
Signed-off-by: Ján Tomko <jtomko@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Since commit 44e7f02915
util: rewrite auto cleanup macros to use glib's equivalent
VIR_AUTOFREE is just an alias for g_autofree. Use the GLib macros
directly instead of our custom aliases.
Signed-off-by: Ján Tomko <jtomko@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Use G_GNUC_UNUSED from GLib instead of ATTRIBUTE_UNUSED.
Signed-off-by: Ján Tomko <jtomko@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
The usleep function was missing on older mingw versions, but we can rely
on it existing everywhere these days. It may only support times upto 1
second in duration though, so we'll prefer to use g_usleep instead.
The commandhelper program is not changed since that can't link to glib.
Fortunately it doesn't need to build on Windows platforms either.
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
After my previous patches we have virPCIDeviceBindToStub() and
virPCIDeviceUnbindFromStub() which really do nothing but call
virPCIDeviceBindToStubWithOverride() and
virPCIDeviceUnbindFromStubWithOverride() respectively.
Drop "WithOverride" from the names and drop the thin wrappers.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
As stated in 84f9358b18 all kernels that we are interested in
have 'drivers_override'. Drop the other, older style of
overriding PCI device driver - newid.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>
Tested-by: Daniel Henrique Barboza <danielhb413@gmail.com>
This function is no longer used after previous commit.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>
Tested-by: Daniel Henrique Barboza <danielhb413@gmail.com>
Now that no one uses KVM style of PCI assignment we can safely
remove 'pci-stub' backend.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>
Tested-by: Daniel Henrique Barboza <danielhb413@gmail.com>
The parent bridge configuration of the current device
should be read and reset, instead of reading the current
device configuration.
Signed-off-by: He Xin <hexin15@baidu.com>
Signed-off-by: Liu Qi <liuqi16@baidu.com>
Signed-off-by: Zhang Yu <zhangyu31@baidu.com>
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
All the callers left require virPCIDeviceConfigOpen to be fatal
and only use read-only access to the config file.
Signed-off-by: Ján Tomko <jtomko@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
For callers that only need read-only access and don't want
an error reported.
Signed-off-by: Ján Tomko <jtomko@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Only a handful of function need write access to the PCI config
space. Create a wrapper function for those so that we can
open it read only by default.
Signed-off-by: Ján Tomko <jtomko@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
This function does not change any of the passed addresses. It
just reads them.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
ACKed-by: Peter Krempa <pkrempa@redhat.com>
This function does not change any of the passed addresses. It
just reads them.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
ACKed-by: Peter Krempa <pkrempa@redhat.com>
virHostdevReattachPCIDevice() is a static that simply does
a wait loop with virPCIDeviceWaitForCleanup() before
calling virPCIDeviceReattach().
This loop traces back to commit d1e5676c0d, aiming to
solve a race condition between Libvirt returning the
device back to the host and QEMU trying to access it in
the meantime, which resulted in QEMU exiting on error
and killing the guest. This happens because device_del
is asynchronous, returning OK even if the guest didn't
release the device. Commit 01abc8a1b8 moved this code
to qemu_hostdev.c, 82e8dd4cf8 added the pci-stub conditional
for the loop, 899b261127 moved the code to virhostdev.c
where it stood until now.
The intent of this wait loop is still valid: device_del
is still not bullet proof into preventing the conditions
that commit d1e5676c0d aimed to fix, especially when considering
all the architectures we must support. However, this loop
is executed only in virHostdevReattachPCIDevice(), leaving
every other virPCIDeviceReattach() call prone to that error.
Let's move the wait loop code to virPCIDeviceReattach(). This
will:
- make every reattach call safe from this race condition
with the pci-stub;
- allow for a bit of code cleanup (virHostdevReattachPCIDevice()
can be erased, and virHostdevReAttachPCIDevices() can use
virPCIDeviceReattach() directly);
- make it easier to understand the overall reattach mechanisms in
Libvirt, without the risk of a newcomer wondering why reattach
is done slightly different in some instances.
Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
There is no restriction on maximum value of PCI domain. In fact,
Linux kernel uses plain atomic inc when assigning PCI domains:
drivers/pci/pci.c:static int pci_get_new_domain_nr(void)
drivers/pci/pci.c-{
drivers/pci/pci.c- return atomic_inc_return(&__domain_nr);
drivers/pci/pci.c-}
Of course, this function is called only if kernel was compiled
without PCI domain support or ACPI did not provide PCI domain.
However, QEMU still has the same restriction as us: in
set_pci_host_devaddr() QEMU checks if domain isn't greater than
0xffff. But one can argue that that's a QEMU limitation. We still
want to be able to cope with other hypervisors that don't have
this limitation (possibly).
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
The format string for a PCI address is copied over and over
again, often with slight adjustments. Introduce global
VIR_PCI_DEVICE_ADDRESS_FMT macro that holds the formatting string
and use it wherever possible.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
In near future, the length restriction of PCI domain is going to
be lifted. This means that our assumption that PCI address is 13
bytes long is no longer true. We can avoid this problem by making
@name dynamically allocated and thus not bother with actual
length of stringified PCI address.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
This function declares @ret variable and then uses
VIR_STEAL_PTR() to avoid freeing temporary variable @dev which is
constructed. Well, as of 267f1e6da5 we have VIR_RETURN_PTR()
macro so that we can avoid this pattern.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
We have two functions: virPCIDeviceAddressIsEqual() defined only
on Linux and virPCIDeviceAddressEqual() defined everywhere. And
both of them do the same. Drop the former in favour of the
latter.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Erik Skultety <eskultet@redhat.com>
Trivial change. Adding the name of the device that has an
unknown PCI header type in that function helps when debugging
PCI code.
Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
Reviewed-by: Andrea Bolognani <abologna@redhat.com>
Standardize on putting the _LAST enum value on the second line
of VIR_ENUM_IMPL invocations. Later patches that add string labels
to VIR_ENUM_IMPL will push most of these to the second line anyways,
so this saves some noise.
Signed-off-by: Cole Robinson <crobinso@redhat.com>
'viralloc.h' does not provide any type or macro which would be necessary
in headers. Prevent leakage of the inclusion.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Missing semicolon at the end of macros can confuse some analyzers
(like cppcheck <filename>). VIR_ONCE_GLOBAL_INIT is almost
exclusively called without an ending semicolon, but let's
standardize on using one like the other macros.
Add a dummy struct definition at the end of the macro, so
the compiler will require callers to add a semicolon.
Reviewed-by: John Ferlan <jferlan@redhat.com>
Signed-off-by: Cole Robinson <crobinso@redhat.com>
Missing semicolon at the end of macros can confuse some analyzers
(like cppcheck <filename>), and we have a mix of semicolon and
non-semicolon usage through the code. Let's standardize on using
a semicolon for VIR_ENUM_IMPL calls.
Move the verify() statement to the end of the macro and drop
the semicolon, so the compiler will require callers to add a
semicolon.
While we are touching these call sites, standardize on putting
the closing parenth on its own line, as discussed here:
https://www.redhat.com/archives/libvir-list/2019-January/msg00750.html
Reviewed-by: John Ferlan <jferlan@redhat.com>
Signed-off-by: Cole Robinson <crobinso@redhat.com>
This partially reverts 00dc991ca1.
2,030 (1,456 direct, 574 indirect) bytes in 14 blocks are definitely lost in loss record 77 of 80
at 0x4C30E96: calloc (vg_replace_malloc.c:711)
by 0x50F83AA: virAlloc (viralloc.c:143)
by 0x5178DFA: virPCIDeviceNew (virpci.c:1753)
by 0x51753E9: virPCIDeviceIterDevices (virpci.c:468)
by 0x5175EB5: virPCIDeviceGetParent (virpci.c:759)
by 0x517AB55: virPCIDeviceIsBehindSwitchLackingACS (virpci.c:2476)
by 0x517AC24: virPCIDeviceIsAssignable (virpci.c:2494)
by 0x10BF27: testVirPCIDeviceIsAssignable (virpcitest.c:229)
by 0x10D14C: virTestRun (testutils.c:174)
by 0x10C535: mymain (virpcitest.c:422)
by 0x10F1B6: virTestMain (testutils.c:1112)
by 0x10CF93: main (virpcitest.c:455)
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Erik Skultety <eskultet@redhat.com>
This is a return argument that is to be compared against NULL on
successful return. However, it is not initialized and therefore
relies on callers setting it to NULL prior calling the function.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Erik Skultety <eskultet@redhat.com>
