7:37 p.m.
I think there's an adequate description in patch 6 (and maybe in the
bugzilla record: https://bugzilla.redhat.com/1901685). I already typed
a long description once, and git send-email failed due to network
problems and threw away everything I typed. I don't feel like typing
it again.
Laine Stump (7):
qemu: use g_autoptr for a virPCIDevice
util: simplify calling of virPCIDeviceDetectFunctionLevelReset()
util: simplify call to virPCIDeviceDetectPowerManagementReset()
util: make read error of PCI config file more detailed
util: change call sequence for virPCIDeviceFindCapabilityOffset()
util: make virPCIDeviceIsPCIExpress() more intelligent
qemu: remove now-redundant check for file length when determining PCIe
vs. PCI
src/qemu/qemu_domain_address.c | 43 ++-----------
src/util/virpci.c | 107 ++++++++++++++++++++++++---------
2 files changed, 82 insertions(+), 68 deletions(-)
--
2.28.0
7:37 p.m.
New subject: [PATCH 1/7] qemu: use g_autoptr for a virPCIDevice
The one instance of a virPCIDevice in
qemuDomainDeviceCalculatePCIConnectFlags() needs to be converted to
use g_autoptr as a prerequisite for a bugfix. It's in this patch by
itself (rather than in a patch converting all virPCIDevice usages to
g_autoptr) to simplify any backport of said bugfix.
Signed-off-by: Laine Stump <laine(a)redhat.com>
---
src/qemu/qemu_domain_address.c | 5 +----
1 file changed, 1 insertion(+), 4 deletions(-)
diff --git a/src/qemu/qemu_domain_address.c b/src/qemu/qemu_domain_address.c
index d872f75b38..b07672e2f4 100644
--- a/src/qemu/qemu_domain_address.c
+++ b/src/qemu/qemu_domain_address.c
@@ -803,7 +803,7 @@ qemuDomainDeviceCalculatePCIConnectFlags(virDomainDeviceDefPtr dev,
case VIR_DOMAIN_DEVICE_HOSTDEV: {
virDomainHostdevDefPtr hostdev = dev->data.hostdev;
bool isExpress = false;
- virPCIDevicePtr pciDev;
+ g_autoptr(virPCIDevice) pciDev = NULL;
virPCIDeviceAddressPtr hostAddr = &hostdev->source.subsys.u.pci.addr;
if (!virHostdevIsMdevDevice(hostdev) &&
@@ -891,8 +891,6 @@ qemuDomainDeviceCalculatePCIConnectFlags(virDomainDeviceDefPtr dev,
off_t configLen
= virFileLength(virPCIDeviceGetConfigPath(pciDev), -1);
- virPCIDeviceFree(pciDev);
-
if (configLen == 256)
return pciFlags;
@@ -904,7 +902,6 @@ qemuDomainDeviceCalculatePCIConnectFlags(virDomainDeviceDefPtr dev,
* a definitive answer.
*/
isExpress = virPCIDeviceIsPCIExpress(pciDev);
- virPCIDeviceFree(pciDev);
if (isExpress)
return pcieFlags;
--
2.28.0
7:37 p.m.
New subject: [PATCH 2/7] util: simplify calling of virPCIDeviceDetectFunctionLevelReset()
This function returned an int, and that int was being checked for < 0
in its solitary caller, but within the function it would only ever
return 0 or 1. Change the function itself to return a bool, and the
caller to just directly set the flag in the virPCIDevice.
Signed-off-by: Laine Stump <laine(a)redhat.com>
---
src/util/virpci.c | 19 +++++++------------
1 file changed, 7 insertions(+), 12 deletions(-)
diff --git a/src/util/virpci.c b/src/util/virpci.c
index b63abac8cc..403ef1ec8e 100644
--- a/src/util/virpci.c
+++ b/src/util/virpci.c
@@ -549,7 +549,7 @@ virPCIDeviceFindExtendedCapabilityOffset(virPCIDevicePtr dev,
/* detects whether this device has FLR. Returns 0 if the device does
* not have FLR, 1 if it does, and -1 on error
*/
-static int
+static bool
virPCIDeviceDetectFunctionLevelReset(virPCIDevicePtr dev, int cfgfd)
{
uint32_t caps;
@@ -567,7 +567,7 @@ virPCIDeviceDetectFunctionLevelReset(virPCIDevicePtr dev, int cfgfd)
caps = virPCIDeviceRead32(dev, cfgfd, dev->pcie_cap_pos + PCI_EXP_DEVCAP);
if (caps & PCI_EXP_DEVCAP_FLR) {
VIR_DEBUG("%s %s: detected PCIe FLR capability", dev->id,
dev->name);
- return 1;
+ return true;
}
}
@@ -580,7 +580,7 @@ virPCIDeviceDetectFunctionLevelReset(virPCIDevicePtr dev, int cfgfd)
caps = virPCIDeviceRead16(dev, cfgfd, pos + PCI_AF_CAP);
if (caps & PCI_AF_CAP_FLR) {
VIR_DEBUG("%s %s: detected PCI FLR capability", dev->id,
dev->name);
- return 1;
+ return true;
}
}
@@ -596,12 +596,12 @@ virPCIDeviceDetectFunctionLevelReset(virPCIDevicePtr dev, int
cfgfd)
if (found) {
VIR_DEBUG("%s %s: buggy device didn't advertise FLR, but is a VF;
forcing flr on",
dev->id, dev->name);
- return 1;
+ return true;
}
VIR_DEBUG("%s %s: no FLR capability found", dev->id, dev->name);
- return 0;
+ return false;
}
/* Require the device has the PCI Power Management capability
@@ -885,15 +885,10 @@ virPCIDeviceTryPowerManagementReset(virPCIDevicePtr dev, int cfgfd)
static int
virPCIDeviceInit(virPCIDevicePtr dev, int cfgfd)
{
- int flr;
-
dev->pcie_cap_pos = virPCIDeviceFindCapabilityOffset(dev, cfgfd,
PCI_CAP_ID_EXP);
dev->pci_pm_cap_pos = virPCIDeviceFindCapabilityOffset(dev, cfgfd,
PCI_CAP_ID_PM);
- flr = virPCIDeviceDetectFunctionLevelReset(dev, cfgfd);
- if (flr < 0)
- return flr;
- dev->has_flr = !!flr;
- dev->has_pm_reset = !!virPCIDeviceDetectPowerManagementReset(dev, cfgfd);
+ dev->has_flr = virPCIDeviceDetectFunctionLevelReset(dev, cfgfd);
+ dev->has_pm_reset = !!virPCIDeviceDetectPowerManagementReset(dev, cfgfd);
return 0;
}
--
2.28.0
7:37 p.m.
New subject: [PATCH 3/7] util: simplify call to virPCIDeviceDetectPowerManagementReset()
This function returned an int, but would only return 0 or 1, and the
one place it was called would just use !! to convert that value to a
bool. Change the function to directly return bool instead.
Signed-off-by: Laine Stump <laine(a)redhat.com>
---
src/util/virpci.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/src/util/virpci.c b/src/util/virpci.c
index 403ef1ec8e..31622cddfa 100644
--- a/src/util/virpci.c
+++ b/src/util/virpci.c
@@ -608,7 +608,7 @@ virPCIDeviceDetectFunctionLevelReset(virPCIDevicePtr dev, int cfgfd)
* and that a D3hot->D0 transition will results in a full
* internal reset, not just a soft reset.
*/
-static unsigned int
+static bool
virPCIDeviceDetectPowerManagementReset(virPCIDevicePtr dev, int cfgfd)
{
if (dev->pci_pm_cap_pos) {
@@ -618,13 +618,13 @@ virPCIDeviceDetectPowerManagementReset(virPCIDevicePtr dev, int
cfgfd)
ctl = virPCIDeviceRead32(dev, cfgfd, dev->pci_pm_cap_pos + PCI_PM_CTRL);
if (!(ctl & PCI_PM_CTRL_NO_SOFT_RESET)) {
VIR_DEBUG("%s %s: detected PM reset capability", dev->id,
dev->name);
- return 1;
+ return true;
}
}
VIR_DEBUG("%s %s: no PM reset capability found", dev->id,
dev->name);
- return 0;
+ return false;
}
/* Any active devices on the same domain/bus ? */
@@ -888,7 +888,7 @@ virPCIDeviceInit(virPCIDevicePtr dev, int cfgfd)
dev->pcie_cap_pos = virPCIDeviceFindCapabilityOffset(dev, cfgfd,
PCI_CAP_ID_EXP);
dev->pci_pm_cap_pos = virPCIDeviceFindCapabilityOffset(dev, cfgfd,
PCI_CAP_ID_PM);
dev->has_flr = virPCIDeviceDetectFunctionLevelReset(dev, cfgfd);
- dev->has_pm_reset = !!virPCIDeviceDetectPowerManagementReset(dev, cfgfd);
+ dev->has_pm_reset = virPCIDeviceDetectPowerManagementReset(dev, cfgfd);
return 0;
}
--
2.28.0
7:37 p.m.
New subject: [PATCH 4/7] util: make read error of PCI config file more detailed
The new message is more verbose/useful, but only logged at debug level
instead of as a warning (since it could easily happen in a non-error
situation).
Signed-off-by: Laine Stump <laine(a)redhat.com>
---
src/util/virpci.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/src/util/virpci.c b/src/util/virpci.c
index 31622cddfa..9109fb4f3a 100644
--- a/src/util/virpci.c
+++ b/src/util/virpci.c
@@ -332,8 +332,8 @@ virPCIDeviceRead(virPCIDevicePtr dev,
if (lseek(cfgfd, pos, SEEK_SET) != pos ||
saferead(cfgfd, buf, buflen) != buflen) {
- VIR_WARN("Failed to read from '%s' : %s", dev->path,
- g_strerror(errno));
+ VIR_DEBUG("Failed to read %u bytes at %u from '%s' : %s",
+ buflen, pos, dev->path, g_strerror(errno));
return -1;
}
return 0;
--
2.28.0
7:37 p.m.
New subject: [PATCH 5/7] util: change call sequence for virPCIDeviceFindCapabilityOffset()
Previously there was no way to differentiate between this function 1)
encountering an error while reading the pci config, and 2) determining
that the device in question is a conventional PCI device, and so has
no Express Capabilities.
The difference between these two conditions is important, because an
unprivileged libvirtd will be unable to read all of the pci config (it
can only read the first 64 bytes, and will get ENOENT when it tries to
seek past that limit) even though the device is in fact a PCIe device.
This patch just changes virPCIDeviceFindCapabilityOffset() to put the
determined offset into an argument of the function (rather than
sending it back as the return value), and to return the standard "0 on
success, -1 on failure". Failure is determined by checking the value
of errno after each attemptd read of the config file (which can only
work reliably if errno is reset to 0 before each read, and after
virPCIDeviceFindCapabilityOffset() has finished examining it).
(NB: if the config file is read successfully, but no Express
Capabilities are found, then the function returns success, but the
returned offset will be 0 (which is an impossible offset for Express
Capabilities, and so easily recognizeable).
An upcoming patch will take advantage of the change made here.
Signed-off-by: Laine Stump <laine(a)redhat.com>
---
src/util/virpci.c | 42 +++++++++++++++++++++++++++++++-----------
1 file changed, 31 insertions(+), 11 deletions(-)
diff --git a/src/util/virpci.c b/src/util/virpci.c
index 9109fb4f3a..2f99bb93bd 100644
--- a/src/util/virpci.c
+++ b/src/util/virpci.c
@@ -329,6 +329,7 @@ virPCIDeviceRead(virPCIDevicePtr dev,
unsigned int buflen)
{
memset(buf, 0, buflen);
+ errno = 0;
if (lseek(cfgfd, pos, SEEK_SET) != pos ||
saferead(cfgfd, buf, buflen) != buflen) {
@@ -483,19 +484,24 @@ virPCIDeviceIterDevices(virPCIDeviceIterPredicate predicate,
return ret;
}
-static uint8_t
+static int
virPCIDeviceFindCapabilityOffset(virPCIDevicePtr dev,
int cfgfd,
- unsigned int capability)
+ unsigned int capability,
+ unsigned int *offset)
{
uint16_t status;
uint8_t pos;
+ *offset = 0; /* assume failure (*nothing* can be at offset 0) */
+
status = virPCIDeviceRead16(dev, cfgfd, PCI_STATUS);
- if (!(status & PCI_STATUS_CAP_LIST))
- return 0;
+ if (errno != 0 || !(status & PCI_STATUS_CAP_LIST))
+ goto error;
pos = virPCIDeviceRead8(dev, cfgfd, PCI_CAPABILITY_LIST);
+ if (errno != 0)
+ goto error;
/* Zero indicates last capability, capabilities can't
* be in the config space header and 0xff is returned
@@ -506,18 +512,31 @@ virPCIDeviceFindCapabilityOffset(virPCIDevicePtr dev,
*/
while (pos >= PCI_CONF_HEADER_LEN && pos != 0xff) {
uint8_t capid = virPCIDeviceRead8(dev, cfgfd, pos);
+ if (errno != 0)
+ goto error;
+
if (capid == capability) {
VIR_DEBUG("%s %s: found cap 0x%.2x at 0x%.2x",
dev->id, dev->name, capability, pos);
- return pos;
+ *offset = pos;
+ return 0;
}
pos = virPCIDeviceRead8(dev, cfgfd, pos + 1);
+ if (errno != 0)
+ goto error;
}
- VIR_DEBUG("%s %s: failed to find cap 0x%.2x", dev->id, dev->name,
capability);
+ error:
+ VIR_DEBUG("%s %s: failed to find cap 0x%.2x (%s)",
+ dev->id, dev->name, capability, g_strerror(errno));
- return 0;
+ /* reset errno in case the failure was due to insufficient
+ * privileges to read the entire PCI config file
+ */
+ errno = 0;
+
+ return -1;
}
static unsigned int
@@ -553,7 +572,7 @@ static bool
virPCIDeviceDetectFunctionLevelReset(virPCIDevicePtr dev, int cfgfd)
{
uint32_t caps;
- uint8_t pos;
+ unsigned int pos;
g_autofree char *path = NULL;
int found;
@@ -575,7 +594,8 @@ virPCIDeviceDetectFunctionLevelReset(virPCIDevicePtr dev, int cfgfd)
* the same thing, except for conventional PCI
* devices. This is not common yet.
*/
- pos = virPCIDeviceFindCapabilityOffset(dev, cfgfd, PCI_CAP_ID_AF);
+ virPCIDeviceFindCapabilityOffset(dev, cfgfd, PCI_CAP_ID_AF, &pos);
+
if (pos) {
caps = virPCIDeviceRead16(dev, cfgfd, pos + PCI_AF_CAP);
if (caps & PCI_AF_CAP_FLR) {
@@ -885,8 +905,8 @@ virPCIDeviceTryPowerManagementReset(virPCIDevicePtr dev, int cfgfd)
static int
virPCIDeviceInit(virPCIDevicePtr dev, int cfgfd)
{
- dev->pcie_cap_pos = virPCIDeviceFindCapabilityOffset(dev, cfgfd,
PCI_CAP_ID_EXP);
- dev->pci_pm_cap_pos = virPCIDeviceFindCapabilityOffset(dev, cfgfd,
PCI_CAP_ID_PM);
+ virPCIDeviceFindCapabilityOffset(dev, cfgfd, PCI_CAP_ID_EXP,
&dev->pcie_cap_pos);
+ virPCIDeviceFindCapabilityOffset(dev, cfgfd, PCI_CAP_ID_PM,
&dev->pci_pm_cap_pos);
dev->has_flr = virPCIDeviceDetectFunctionLevelReset(dev, cfgfd);
dev->has_pm_reset = virPCIDeviceDetectPowerManagementReset(dev, cfgfd);
--
2.28.0
3:18 a.m.
New subject: [PATCH 5/7] util: change call sequence for virPCIDeviceFindCapabilityOffset()
On Tue, Dec 08, 2020 at 08:37:43PM -0500, Laine Stump wrote:
Previously there was no way to differentiate between this function
1)
encountering an error while reading the pci config, and 2) determining
that the device in question is a conventional PCI device, and so has
no Express Capabilities.
The difference between these two conditions is important, because an
unprivileged libvirtd will be unable to read all of the pci config (it
can only read the first 64 bytes, and will get ENOENT when it tries to
seek past that limit) even though the device is in fact a PCIe device.
This patch just changes virPCIDeviceFindCapabilityOffset() to put the
determined offset into an argument of the function (rather than
sending it back as the return value), and to return the standard "0 on
success, -1 on failure". Failure is determined by checking the value
of errno after each attemptd read of the config file (which can only
work reliably if errno is reset to 0 before each read, and after
virPCIDeviceFindCapabilityOffset() has finished examining it).
(NB: if the config file is read successfully, but no Express
Capabilities are found, then the function returns success, but the
returned offset will be 0 (which is an impossible offset for Express
Capabilities, and so easily recognizeable).
An upcoming patch will take advantage of the change made here.
Signed-off-by: Laine Stump <laine(a)redhat.com>
---
src/util/virpci.c | 42 +++++++++++++++++++++++++++++++-----------
1 file changed, 31 insertions(+), 11 deletions(-)
diff --git a/src/util/virpci.c b/src/util/virpci.c
index 9109fb4f3a..2f99bb93bd 100644
--- a/src/util/virpci.c
+++ b/src/util/virpci.c
@@ -329,6 +329,7 @@ virPCIDeviceRead(virPCIDevicePtr dev,
unsigned int buflen)
{
memset(buf, 0, buflen);
+ errno = 0;
if (lseek(cfgfd, pos, SEEK_SET) != pos ||
saferead(cfgfd, buf, buflen) != buflen) {
@@ -483,19 +484,24 @@ virPCIDeviceIterDevices(virPCIDeviceIterPredicate predicate,
return ret;
}
I think there should be some API docs added here to describe the
semantics this method is trying to achieve wrt unprivileged
usage and pci vs pcie.
-static uint8_t
+static int
virPCIDeviceFindCapabilityOffset(virPCIDevicePtr dev,
int cfgfd,
- unsigned int capability)
+ unsigned int capability,
+ unsigned int *offset)
{
uint16_t status;
uint8_t pos;
+ *offset = 0; /* assume failure (*nothing* can be at offset 0) */
+
status = virPCIDeviceRead16(dev, cfgfd, PCI_STATUS);
- if (!(status & PCI_STATUS_CAP_LIST))
- return 0;
+ if (errno != 0 || !(status & PCI_STATUS_CAP_LIST))
+ goto error;
pos = virPCIDeviceRead8(dev, cfgfd, PCI_CAPABILITY_LIST);
+ if (errno != 0)
+ goto error;
/* Zero indicates last capability, capabilities can't
* be in the config space header and 0xff is returned
@@ -506,18 +512,31 @@ virPCIDeviceFindCapabilityOffset(virPCIDevicePtr dev,
*/
while (pos >= PCI_CONF_HEADER_LEN && pos != 0xff) {
uint8_t capid = virPCIDeviceRead8(dev, cfgfd, pos);
+ if (errno != 0)
+ goto error;
+
if (capid == capability) {
VIR_DEBUG("%s %s: found cap 0x%.2x at 0x%.2x",
dev->id, dev->name, capability, pos);
- return pos;
+ *offset = pos;
+ return 0;
}
pos = virPCIDeviceRead8(dev, cfgfd, pos + 1);
+ if (errno != 0)
+ goto error;
}
- VIR_DEBUG("%s %s: failed to find cap 0x%.2x", dev->id, dev->name,
capability);
+ error:
+ VIR_DEBUG("%s %s: failed to find cap 0x%.2x (%s)",
+ dev->id, dev->name, capability, g_strerror(errno));
- return 0;
+ /* reset errno in case the failure was due to insufficient
+ * privileges to read the entire PCI config file
+ */
+ errno = 0;
+
+ return -1;
}
static unsigned int
@@ -553,7 +572,7 @@ static bool
virPCIDeviceDetectFunctionLevelReset(virPCIDevicePtr dev, int cfgfd)
{
uint32_t caps;
- uint8_t pos;
+ unsigned int pos;
g_autofree char *path = NULL;
int found;
@@ -575,7 +594,8 @@ virPCIDeviceDetectFunctionLevelReset(virPCIDevicePtr dev, int cfgfd)
* the same thing, except for conventional PCI
* devices. This is not common yet.
*/
- pos = virPCIDeviceFindCapabilityOffset(dev, cfgfd, PCI_CAP_ID_AF);
+ virPCIDeviceFindCapabilityOffset(dev, cfgfd, PCI_CAP_ID_AF, &pos);
+
if (pos) {
caps = virPCIDeviceRead16(dev, cfgfd, pos + PCI_AF_CAP);
if (caps & PCI_AF_CAP_FLR) {
@@ -885,8 +905,8 @@ virPCIDeviceTryPowerManagementReset(virPCIDevicePtr dev, int cfgfd)
static int
virPCIDeviceInit(virPCIDevicePtr dev, int cfgfd)
{
- dev->pcie_cap_pos = virPCIDeviceFindCapabilityOffset(dev, cfgfd,
PCI_CAP_ID_EXP);
- dev->pci_pm_cap_pos = virPCIDeviceFindCapabilityOffset(dev, cfgfd,
PCI_CAP_ID_PM);
+ virPCIDeviceFindCapabilityOffset(dev, cfgfd, PCI_CAP_ID_EXP,
&dev->pcie_cap_pos);
+ virPCIDeviceFindCapabilityOffset(dev, cfgfd, PCI_CAP_ID_PM,
&dev->pci_pm_cap_pos);
dev->has_flr = virPCIDeviceDetectFunctionLevelReset(dev, cfgfd);
dev->has_pm_reset = virPCIDeviceDetectPowerManagementReset(dev, cfgfd);
--
2.28.0
Regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
3:15 p.m.
New subject: [PATCH 5/7] util: change call sequence for virPCIDeviceFindCapabilityOffset()
On 12/9/20 4:18 AM, Daniel P. Berrangé wrote:
On Tue, Dec 08, 2020 at 08:37:43PM -0500, Laine Stump wrote:
> Previously there was no way to differentiate between this function 1)
> encountering an error while reading the pci config, and 2) determining
> that the device in question is a conventional PCI device, and so has
> no Express Capabilities.
>
> The difference between these two conditions is important, because an
> unprivileged libvirtd will be unable to read all of the pci config (it
> can only read the first 64 bytes, and will get ENOENT when it tries to
> seek past that limit) even though the device is in fact a PCIe device.
>
> This patch just changes virPCIDeviceFindCapabilityOffset() to put the
> determined offset into an argument of the function (rather than
> sending it back as the return value), and to return the standard "0 on
> success, -1 on failure". Failure is determined by checking the value
> of errno after each attemptd read of the config file (which can only
> work reliably if errno is reset to 0 before each read, and after
> virPCIDeviceFindCapabilityOffset() has finished examining it).
>
> (NB: if the config file is read successfully, but no Express
> Capabilities are found, then the function returns success, but the
> returned offset will be 0 (which is an impossible offset for Express
> Capabilities, and so easily recognizeable).
>
> An upcoming patch will take advantage of the change made here.
>
> Signed-off-by: Laine Stump <laine(a)redhat.com>
> ---
> src/util/virpci.c | 42 +++++++++++++++++++++++++++++++-----------
> 1 file changed, 31 insertions(+), 11 deletions(-)
>
> diff --git a/src/util/virpci.c b/src/util/virpci.c
> index 9109fb4f3a..2f99bb93bd 100644
> --- a/src/util/virpci.c
> +++ b/src/util/virpci.c
> @@ -329,6 +329,7 @@ virPCIDeviceRead(virPCIDevicePtr dev,
> unsigned int buflen)
> {
> memset(buf, 0, buflen);
> + errno = 0;
>
> if (lseek(cfgfd, pos, SEEK_SET) != pos ||
> saferead(cfgfd, buf, buflen) != buflen) {
> @@ -483,19 +484,24 @@ virPCIDeviceIterDevices(virPCIDeviceIterPredicate predicate,
> return ret;
> }
>
I think there should be some API docs added here to describe the
semantics this method is trying to achieve wrt unprivileged
usage and pci vs pcie.
There is something about that in patch 6/7, but I think all of these
functions could benefit from documenting their API (e.g. the fact that
errno may be set on return from virPCIDeviceReadNN(), since the
functions themselves directly return the value that is read, and so are
unable indicate an error in the return value (I considered doing that,
but wanted a less invasive change, and there are 20-30 callers of those
functions, most of which wouldn't otherwise need to be changed).
I'll add in some API docublurbs and resend the entire series just for
simplicity...
> -static uint8_t
> +static int
> virPCIDeviceFindCapabilityOffset(virPCIDevicePtr dev,
> int cfgfd,
> - unsigned int capability)
> + unsigned int capability,
> + unsigned int *offset)
> {
> uint16_t status;
> uint8_t pos;
>
> + *offset = 0; /* assume failure (*nothing* can be at offset 0) */
> +
> status = virPCIDeviceRead16(dev, cfgfd, PCI_STATUS);
> - if (!(status & PCI_STATUS_CAP_LIST))
> - return 0;
> + if (errno != 0 || !(status & PCI_STATUS_CAP_LIST))
> + goto error;
>
> pos = virPCIDeviceRead8(dev, cfgfd, PCI_CAPABILITY_LIST);
> + if (errno != 0)
> + goto error;
>
> /* Zero indicates last capability, capabilities can't
> * be in the config space header and 0xff is returned
> @@ -506,18 +512,31 @@ virPCIDeviceFindCapabilityOffset(virPCIDevicePtr dev,
> */
> while (pos >= PCI_CONF_HEADER_LEN && pos != 0xff) {
> uint8_t capid = virPCIDeviceRead8(dev, cfgfd, pos);
> + if (errno != 0)
> + goto error;
> +
> if (capid == capability) {
> VIR_DEBUG("%s %s: found cap 0x%.2x at 0x%.2x",
> dev->id, dev->name, capability, pos);
> - return pos;
> + *offset = pos;
> + return 0;
> }
>
> pos = virPCIDeviceRead8(dev, cfgfd, pos + 1);
> + if (errno != 0)
> + goto error;
> }
>
> - VIR_DEBUG("%s %s: failed to find cap 0x%.2x", dev->id,
dev->name, capability);
> + error:
> + VIR_DEBUG("%s %s: failed to find cap 0x%.2x (%s)",
> + dev->id, dev->name, capability, g_strerror(errno));
>
> - return 0;
> + /* reset errno in case the failure was due to insufficient
> + * privileges to read the entire PCI config file
> + */
> + errno = 0;
> +
> + return -1;
> }
>
> static unsigned int
> @@ -553,7 +572,7 @@ static bool
> virPCIDeviceDetectFunctionLevelReset(virPCIDevicePtr dev, int cfgfd)
> {
> uint32_t caps;
> - uint8_t pos;
> + unsigned int pos;
> g_autofree char *path = NULL;
> int found;
>
> @@ -575,7 +594,8 @@ virPCIDeviceDetectFunctionLevelReset(virPCIDevicePtr dev, int
cfgfd)
> * the same thing, except for conventional PCI
> * devices. This is not common yet.
> */
> - pos = virPCIDeviceFindCapabilityOffset(dev, cfgfd, PCI_CAP_ID_AF);
> + virPCIDeviceFindCapabilityOffset(dev, cfgfd, PCI_CAP_ID_AF, &pos);
> +
> if (pos) {
> caps = virPCIDeviceRead16(dev, cfgfd, pos + PCI_AF_CAP);
> if (caps & PCI_AF_CAP_FLR) {
> @@ -885,8 +905,8 @@ virPCIDeviceTryPowerManagementReset(virPCIDevicePtr dev, int
cfgfd)
> static int
> virPCIDeviceInit(virPCIDevicePtr dev, int cfgfd)
> {
> - dev->pcie_cap_pos = virPCIDeviceFindCapabilityOffset(dev, cfgfd,
PCI_CAP_ID_EXP);
> - dev->pci_pm_cap_pos = virPCIDeviceFindCapabilityOffset(dev, cfgfd,
PCI_CAP_ID_PM);
> + virPCIDeviceFindCapabilityOffset(dev, cfgfd, PCI_CAP_ID_EXP,
&dev->pcie_cap_pos);
> + virPCIDeviceFindCapabilityOffset(dev, cfgfd, PCI_CAP_ID_PM,
&dev->pci_pm_cap_pos);
> dev->has_flr = virPCIDeviceDetectFunctionLevelReset(dev, cfgfd);
> dev->has_pm_reset = virPCIDeviceDetectPowerManagementReset(dev, cfgfd);
>
> --
> 2.28.0
>
Regards,
Daniel
7:37 p.m.
New subject: [PATCH 6/7] util: make virPCIDeviceIsPCIExpress() more intelligent
Until now there has been an extra bit of code in
qemuDomainDeviceCalculatePCIConnectFlag() (one of the two callers of
virPCIDeviceIsPCIExpress()) that tries to determine if a device is
PCIe by looking at the *length* of its sysfs config file; it only does
this when libvirt is running as a non-root process.
This patch takes advantage of our newfound ability to tell the
difference between "I read a 0 from the device PCI config file" and "I
couldn't read the PCI Express Capabilities because I don't have
sufficient permission" to move the file length check down into
virPCIDeviceIsPCIExpress(), and do that check any time we fail while
reading the config file (not only when the process is non-root).
Fixes: https://bugzilla.redhat.com/1901685
Signed-off-by: Laine Stump <laine(a)redhat.com>
---
src/util/virpci.c | 42 ++++++++++++++++++++++++++++++++++++++----
1 file changed, 38 insertions(+), 4 deletions(-)
diff --git a/src/util/virpci.c b/src/util/virpci.c
index 2f99bb93bd..01a156dfcf 100644
--- a/src/util/virpci.c
+++ b/src/util/virpci.c
@@ -73,10 +73,18 @@ struct _virPCIDevice {
char *used_by_drvname;
char *used_by_domname;
+ /* The following 5 items are only valid after virPCIDeviceInit()
+ * has been called for the virPCIDevice object. This is *not* done
+ * in most cases (because it creates extra overhead, and parts of
+ * it can fail if libvirtd is running unprivileged)
+ */
unsigned int pcie_cap_pos;
unsigned int pci_pm_cap_pos;
bool has_flr;
bool has_pm_reset;
+ bool is_pcie;
+ /**/
+
bool managed;
virPCIStubDriver stubDriver;
@@ -594,7 +602,8 @@ virPCIDeviceDetectFunctionLevelReset(virPCIDevicePtr dev, int cfgfd)
* the same thing, except for conventional PCI
* devices. This is not common yet.
*/
- virPCIDeviceFindCapabilityOffset(dev, cfgfd, PCI_CAP_ID_AF, &pos);
+ if (virPCIDeviceFindCapabilityOffset(dev, cfgfd, PCI_CAP_ID_AF, &pos) < 0)
+ goto error;
if (pos) {
caps = virPCIDeviceRead16(dev, cfgfd, pos + PCI_AF_CAP);
@@ -619,8 +628,8 @@ virPCIDeviceDetectFunctionLevelReset(virPCIDevicePtr dev, int cfgfd)
return true;
}
+ error:
VIR_DEBUG("%s %s: no FLR capability found", dev->id, dev->name);
-
return false;
}
@@ -905,7 +914,32 @@ virPCIDeviceTryPowerManagementReset(virPCIDevicePtr dev, int cfgfd)
static int
virPCIDeviceInit(virPCIDevicePtr dev, int cfgfd)
{
- virPCIDeviceFindCapabilityOffset(dev, cfgfd, PCI_CAP_ID_EXP,
&dev->pcie_cap_pos);
+ dev->is_pcie = false;
+ if (virPCIDeviceFindCapabilityOffset(dev, cfgfd, PCI_CAP_ID_EXP,
&dev->pcie_cap_pos) < 0) {
+ /* an unprivileged process is unable to read *all* of a
+ * device's PCI config (it can only read the first 64
+ * bytes, which isn't enough for see the Express
+ * Capabilities data). If virPCIDeviceFindCapabilityOffset
+ * returns failure (and not just a pcie_cap_pos == 0,
+ * which is *success* at determining the device is *not*
+ * PCIe) we make an educated guess based on the length of
+ * the device's config file - if it is 256 bytes, then it
+ * is definitely a legacy PCI device. If it's larger than
+ * that, then it is *probably PCIe (although it could be
+ * PCI-x, but those are extremely rare). If the config
+ * file can't be found (in which case the "length" will be
+ * -1), then we blindly assume the most likely outcome -
+ * PCIe.
+ */
+ off_t configLen = virFileLength(virPCIDeviceGetConfigPath(dev), -1);
+
+ if (configLen != 256)
+ dev->is_pcie = true;
+
+ } else {
+ dev->is_pcie = (dev->pcie_cap_pos != 0);
+ }
+
virPCIDeviceFindCapabilityOffset(dev, cfgfd, PCI_CAP_ID_PM,
&dev->pci_pm_cap_pos);
dev->has_flr = virPCIDeviceDetectFunctionLevelReset(dev, cfgfd);
dev->has_pm_reset = virPCIDeviceDetectPowerManagementReset(dev, cfgfd);
@@ -2609,7 +2643,7 @@ virPCIDeviceIsPCIExpress(virPCIDevicePtr dev)
if (virPCIDeviceInit(dev, fd) < 0)
goto cleanup;
- ret = dev->pcie_cap_pos != 0;
+ ret = dev->is_pcie;
cleanup:
virPCIDeviceConfigClose(dev, fd);
--
2.28.0
7:37 p.m.
New subject: [PATCH 7/7] qemu: remove now-redundant check for file length when determining PCIe vs. PCI
Now that virPCIDeviceIsPCIExpress() checks the length of the file when
the process lacks sufficient privilege to read the entire PCI config
file in sysfs, we can remove the open-coding for that case from its
consumer.
Signed-off-by: Laine Stump <laine(a)redhat.com>
---
src/qemu/qemu_domain_address.c | 38 +++-------------------------------
1 file changed, 3 insertions(+), 35 deletions(-)
diff --git a/src/qemu/qemu_domain_address.c b/src/qemu/qemu_domain_address.c
index b07672e2f4..f0ba318cc8 100644
--- a/src/qemu/qemu_domain_address.c
+++ b/src/qemu/qemu_domain_address.c
@@ -579,7 +579,6 @@ qemuDomainDeviceCalculatePCIAddressExtensionFlags(virQEMUCapsPtr
qemuCaps,
*/
static virDomainPCIConnectFlags
qemuDomainDeviceCalculatePCIConnectFlags(virDomainDeviceDefPtr dev,
- virQEMUDriverPtr driver,
virDomainPCIConnectFlags pcieFlags,
virDomainPCIConnectFlags virtioFlags)
{
@@ -802,7 +801,6 @@ qemuDomainDeviceCalculatePCIConnectFlags(virDomainDeviceDefPtr dev,
case VIR_DOMAIN_DEVICE_HOSTDEV: {
virDomainHostdevDefPtr hostdev = dev->data.hostdev;
- bool isExpress = false;
g_autoptr(virPCIDevice) pciDev = NULL;
virPCIDeviceAddressPtr hostAddr = &hostdev->source.subsys.u.pci.addr;
@@ -873,37 +871,7 @@ qemuDomainDeviceCalculatePCIConnectFlags(virDomainDeviceDefPtr dev,
return pcieFlags;
}
- if (!driver->privileged) {
- /* unprivileged libvirtd is unable to read *all* of a
- * device's PCI config (it can only read the first 64
- * bytes, which isn't enough for the check that's done
- * in virPCIDeviceIsPCIExpress()), so instead of
- * trying and failing, we make an educated guess based
- * on the length of the device's config file - if it
- * is 256 bytes, then it is definitely a legacy PCI
- * device. If it's larger than that, then it is
- * *probably PCIe (although it could be PCI-x, but
- * those are extremely rare). If the config file can't
- * be found (in which case the "length" will be -1),
- * then we blindly assume the most likely outcome -
- * PCIe.
- */
- off_t configLen
- = virFileLength(virPCIDeviceGetConfigPath(pciDev), -1);
-
- if (configLen == 256)
- return pciFlags;
-
- return pcieFlags;
- }
-
- /* If we are running with privileges, we can examine the
- * PCI config contents with virPCIDeviceIsPCIExpress() for
- * a definitive answer.
- */
- isExpress = virPCIDeviceIsPCIExpress(pciDev);
-
- if (isExpress)
+ if (virPCIDeviceIsPCIExpress(pciDev))
return pcieFlags;
return pciFlags;
@@ -1124,7 +1092,7 @@ qemuDomainFillDevicePCIConnectFlagsIter(virDomainDefPtr def
G_GNUC_UNUSED,
qemuDomainFillDevicePCIConnectFlagsIterData *data = opaque;
info->pciConnectFlags
- = qemuDomainDeviceCalculatePCIConnectFlags(dev, data->driver,
+ = qemuDomainDeviceCalculatePCIConnectFlags(dev,
data->pcieFlags,
data->virtioFlags);
return 0;
@@ -1468,7 +1436,7 @@ qemuDomainFillDevicePCIConnectFlags(virDomainDefPtr def,
qemuDomainFillDevicePCIConnectFlagsIterInit(def, qemuCaps, driver, &data);
info->pciConnectFlags
- = qemuDomainDeviceCalculatePCIConnectFlags(dev, data.driver,
+ = qemuDomainDeviceCalculatePCIConnectFlags(dev,
data.pcieFlags,
data.virtioFlags);
}
--
2.28.0
2:28 a.m.
On 12/9/20 2:37 AM, Laine Stump wrote:
I think there's an adequate description in patch 6 (and maybe in
the
bugzilla record: https://bugzilla.redhat.com/1901685). I already typed
a long description once, and git send-email failed due to network
problems and threw away everything I typed. I don't feel like typing
it again.
Laine Stump (7):
qemu: use g_autoptr for a virPCIDevice
util: simplify calling of virPCIDeviceDetectFunctionLevelReset()
util: simplify call to virPCIDeviceDetectPowerManagementReset()
util: make read error of PCI config file more detailed
util: change call sequence for virPCIDeviceFindCapabilityOffset()
util: make virPCIDeviceIsPCIExpress() more intelligent
qemu: remove now-redundant check for file length when determining PCIe
vs. PCI
src/qemu/qemu_domain_address.c | 43 ++-----------
src/util/virpci.c | 107 ++++++++++++++++++++++++---------
2 files changed, 82 insertions(+), 68 deletions(-)
Reviewed-by: Michal Privoznik <mprivozn(a)redhat.com>
Michal
1428
days inactive
1428
days old
10 comments
3 participants
participants (3)
-
Daniel P. Berrangé -
Laine Stump -
Michal Privoznik