Add these helpers that access config space and return an -errno style
return.
Signed-off-by: John Levon <john.levon@nutanix.com>
---
hw/vfio/pci.c | 134 ++++++++++++++++++++++++++++++++++----------------
1 file changed, 91 insertions(+), 43 deletions(-)
diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
index ddeee33aa9..c3842d2f8d 100644
--- a/hw/vfio/pci.c
+++ b/hw/vfio/pci.c
@@ -964,6 +964,28 @@ static void vfio_pci_load_rom(VFIOPCIDevice *vdev)
}
}
+/* "Raw" read of underlying config space. */
+static int vfio_pci_config_space_read(VFIOPCIDevice *vdev, off_t offset,
+ uint32_t size, void *data)
+{
+ ssize_t ret;
+
+ ret = pread(vdev->vbasedev.fd, data, size, vdev->config_offset + offset);
+
+ return ret < 0 ? -errno : (int)ret;
+}
+
+/* "Raw" write of underlying config space. */
+static int vfio_pci_config_space_write(VFIOPCIDevice *vdev, off_t offset,
+ uint32_t size, void *data)
+{
+ ssize_t ret;
+
+ ret = pwrite(vdev->vbasedev.fd, data, size, vdev->config_offset + offset);
+
+ return ret < 0 ? -errno : (int)ret;
+}
+
static uint64_t vfio_rom_read(void *opaque, hwaddr addr, unsigned size)
{
VFIOPCIDevice *vdev = opaque;
@@ -1016,10 +1038,9 @@ static const MemoryRegionOps vfio_rom_ops = {
static void vfio_pci_size_rom(VFIOPCIDevice *vdev)
{
+ VFIODevice *vbasedev = &vdev->vbasedev;
uint32_t orig, size = cpu_to_le32((uint32_t)PCI_ROM_ADDRESS_MASK);
- off_t offset = vdev->config_offset + PCI_ROM_ADDRESS;
char *name;
- int fd = vdev->vbasedev.fd;
if (vdev->pdev.romfile || !vdev->pdev.rom_bar) {
/* Since pci handles romfile, just print a message and return */
@@ -1036,11 +1057,12 @@ static void vfio_pci_size_rom(VFIOPCIDevice *vdev)
* Use the same size ROM BAR as the physical device. The contents
* will get filled in later when the guest tries to read it.
*/
- if (pread(fd, &orig, 4, offset) != 4 ||
- pwrite(fd, &size, 4, offset) != 4 ||
- pread(fd, &size, 4, offset) != 4 ||
- pwrite(fd, &orig, 4, offset) != 4) {
- error_report("%s(%s) failed: %m", __func__, vdev->vbasedev.name);
+ if (vfio_pci_config_space_read(vdev, PCI_ROM_ADDRESS, 4, &orig) != 4 ||
+ vfio_pci_config_space_write(vdev, PCI_ROM_ADDRESS, 4, &size) != 4 ||
+ vfio_pci_config_space_read(vdev, PCI_ROM_ADDRESS, 4, &size) != 4 ||
+ vfio_pci_config_space_write(vdev, PCI_ROM_ADDRESS, 4, &orig) != 4) {
+
+ error_report("%s(%s) ROM access failed", __func__, vbasedev->name);
return;
}
@@ -1220,6 +1242,7 @@ static void vfio_sub_page_bar_update_mapping(PCIDevice *pdev, int bar)
uint32_t vfio_pci_read_config(PCIDevice *pdev, uint32_t addr, int len)
{
VFIOPCIDevice *vdev = VFIO_PCI_BASE(pdev);
+ VFIODevice *vbasedev = &vdev->vbasedev;
uint32_t emu_bits = 0, emu_val = 0, phys_val = 0, val;
memcpy(&emu_bits, vdev->emulated_config_bits + addr, len);
@@ -1232,12 +1255,13 @@ uint32_t vfio_pci_read_config(PCIDevice *pdev, uint32_t addr, int len)
if (~emu_bits & (0xffffffffU >> (32 - len * 8))) {
ssize_t ret;
- ret = pread(vdev->vbasedev.fd, &phys_val, len,
- vdev->config_offset + addr);
+ ret = vfio_pci_config_space_read(vdev, addr, len, &phys_val);
if (ret != len) {
- error_report("%s(%s, 0x%x, 0x%x) failed: %m",
- __func__, vdev->vbasedev.name, addr, len);
- return -errno;
+ const char *err = ret < 0 ? strerror(-ret) : "short read";
+
+ error_report("%s(%s, 0x%x, 0x%x) failed: %s",
+ __func__, vbasedev->name, addr, len, err);
+ return -1;
}
phys_val = le32_to_cpu(phys_val);
}
@@ -1253,15 +1277,19 @@ void vfio_pci_write_config(PCIDevice *pdev,
uint32_t addr, uint32_t val, int len)
{
VFIOPCIDevice *vdev = VFIO_PCI_BASE(pdev);
+ VFIODevice *vbasedev = &vdev->vbasedev;
uint32_t val_le = cpu_to_le32(val);
+ int ret;
trace_vfio_pci_write_config(vdev->vbasedev.name, addr, val, len);
/* Write everything to VFIO, let it filter out what we can't write */
- if (pwrite(vdev->vbasedev.fd, &val_le, len, vdev->config_offset + addr)
- != len) {
- error_report("%s(%s, 0x%x, 0x%x, 0x%x) failed: %m",
- __func__, vdev->vbasedev.name, addr, val, len);
+ ret = vfio_pci_config_space_write(vdev, addr, len, &val_le);
+ if (ret != len) {
+ const char *err = ret < 0 ? strerror(-ret) : "short write";
+
+ error_report("%s(%s, 0x%x, 0x%x, 0x%x) failed: %s",
+ __func__, vbasedev->name, addr, val, len, err);
}
/* MSI/MSI-X Enabling/Disabling */
@@ -1349,9 +1377,12 @@ static bool vfio_msi_setup(VFIOPCIDevice *vdev, int pos, Error **errp)
int ret, entries;
Error *err = NULL;
- if (pread(vdev->vbasedev.fd, &ctrl, sizeof(ctrl),
- vdev->config_offset + pos + PCI_CAP_FLAGS) != sizeof(ctrl)) {
- error_setg_errno(errp, errno, "failed reading MSI PCI_CAP_FLAGS");
+ ret = vfio_pci_config_space_read(vdev, pos + PCI_CAP_FLAGS,
+ sizeof(ctrl), &ctrl);
+ if (ret != sizeof(ctrl)) {
+ const char *errmsg = ret < 0 ? strerror(-ret) : "short read";
+
+ error_setg(errp, "failed reading MSI PCI_CAP_FLAGS: %s", errmsg);
return false;
}
ctrl = le16_to_cpu(ctrl);
@@ -1558,30 +1589,39 @@ static bool vfio_msix_early_setup(VFIOPCIDevice *vdev, Error **errp)
uint8_t pos;
uint16_t ctrl;
uint32_t table, pba;
- int ret, fd = vdev->vbasedev.fd;
struct vfio_irq_info irq_info;
VFIOMSIXInfo *msix;
+ int ret;
pos = pci_find_capability(&vdev->pdev, PCI_CAP_ID_MSIX);
if (!pos) {
return true;
}
- if (pread(fd, &ctrl, sizeof(ctrl),
- vdev->config_offset + pos + PCI_MSIX_FLAGS) != sizeof(ctrl)) {
- error_setg_errno(errp, errno, "failed to read PCI MSIX FLAGS");
+ ret = vfio_pci_config_space_read(vdev, pos + PCI_MSIX_FLAGS,
+ sizeof(ctrl), &ctrl);
+ if (ret != sizeof(ctrl)) {
+ const char *err = ret < 0 ? strerror(-ret) : "short read";
+
+ error_setg(errp, "failed to read PCI MSIX FLAGS: %s", err);
return false;
}
- if (pread(fd, &table, sizeof(table),
- vdev->config_offset + pos + PCI_MSIX_TABLE) != sizeof(table)) {
- error_setg_errno(errp, errno, "failed to read PCI MSIX TABLE");
+ ret = vfio_pci_config_space_read(vdev, pos + PCI_MSIX_TABLE,
+ sizeof(table), &table);
+ if (ret != sizeof(table)) {
+ const char *err = ret < 0 ? strerror(-ret) : "short read";
+
+ error_setg(errp, "failed to read PCI MSIX TABLE: %s", err);
return false;
}
- if (pread(fd, &pba, sizeof(pba),
- vdev->config_offset + pos + PCI_MSIX_PBA) != sizeof(pba)) {
- error_setg_errno(errp, errno, "failed to read PCI MSIX PBA");
+ ret = vfio_pci_config_space_read(vdev, pos + PCI_MSIX_PBA,
+ sizeof(pba), &pba);
+ if (ret != sizeof(pba)) {
+ const char *err = ret < 0 ? strerror(-ret) : "short read";
+
+ error_setg(errp, "failed to read PCI MSIX PBA: %s", err);
return false;
}
@@ -1741,10 +1781,12 @@ static void vfio_bar_prepare(VFIOPCIDevice *vdev, int nr)
}
/* Determine what type of BAR this is for registration */
- ret = pread(vdev->vbasedev.fd, &pci_bar, sizeof(pci_bar),
- vdev->config_offset + PCI_BASE_ADDRESS_0 + (4 * nr));
+ ret = vfio_pci_config_space_read(vdev, PCI_BASE_ADDRESS_0 + (4 * nr),
+ sizeof(pci_bar), &pci_bar);
if (ret != sizeof(pci_bar)) {
- error_report("vfio: Failed to read BAR %d (%m)", nr);
+ const char *err = ret < 0 ? strerror(-ret) : "short read";
+
+ error_report("vfio: Failed to read BAR %d: %s", nr, err);
return;
}
@@ -2448,21 +2490,25 @@ void vfio_pci_pre_reset(VFIOPCIDevice *vdev)
void vfio_pci_post_reset(VFIOPCIDevice *vdev)
{
+ VFIODevice *vbasedev = &vdev->vbasedev;
Error *err = NULL;
- int nr;
+ int ret, nr;
if (!vfio_intx_enable(vdev, &err)) {
error_reportf_err(err, VFIO_MSG_PREFIX, vdev->vbasedev.name);
}
for (nr = 0; nr < PCI_NUM_REGIONS - 1; ++nr) {
- off_t addr = vdev->config_offset + PCI_BASE_ADDRESS_0 + (4 * nr);
+ off_t addr = PCI_BASE_ADDRESS_0 + (4 * nr);
uint32_t val = 0;
uint32_t len = sizeof(val);
- if (pwrite(vdev->vbasedev.fd, &val, len, addr) != len) {
- error_report("%s(%s) reset bar %d failed: %m", __func__,
- vdev->vbasedev.name, nr);
+ ret = vfio_pci_config_space_write(vdev, addr, len, &val);
+ if (ret != len) {
+ const char *errmsg = ret < 0 ? strerror(-ret) : "short write";
+
+ error_report("%s(%s) reset bar %d failed: %s", __func__,
+ vbasedev->name, nr, errmsg);
}
}
@@ -3099,6 +3145,7 @@ static void vfio_realize(PCIDevice *pdev, Error **errp)
int i, ret;
char uuid[UUID_STR_LEN];
g_autofree char *name = NULL;
+ size_t config_space_size;
if (vbasedev->fd < 0 && !vbasedev->sysfsdev) {
if (!(~vdev->host.domain || ~vdev->host.bus ||
@@ -3153,13 +3200,14 @@ static void vfio_realize(PCIDevice *pdev, Error **errp)
goto error;
}
+ config_space_size = MIN(pci_config_size(&vdev->pdev), vdev->config_size);
+
/* Get a copy of config space */
- ret = pread(vbasedev->fd, vdev->pdev.config,
- MIN(pci_config_size(&vdev->pdev), vdev->config_size),
- vdev->config_offset);
- if (ret < (int)MIN(pci_config_size(&vdev->pdev), vdev->config_size)) {
- ret = ret < 0 ? -errno : -EFAULT;
- error_setg_errno(errp, -ret, "failed to read device config space");
+ ret = vfio_pci_config_space_read(vdev, 0, config_space_size,
+ vdev->pdev.config);
+ if (ret < (int)config_space_size) {
+ ret = ret < 0 ? -ret : EFAULT;
+ error_setg_errno(errp, ret, "failed to read device config space");
goto error;
}
--
2.34.1On 4/9/25 15:48, John Levon wrote:
> Add these helpers that access config space and return an -errno style
> return.
>
> Signed-off-by: John Levon <john.levon@nutanix.com>
> ---
> hw/vfio/pci.c | 134 ++++++++++++++++++++++++++++++++++----------------
> 1 file changed, 91 insertions(+), 43 deletions(-)
>
> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
> index ddeee33aa9..c3842d2f8d 100644
> --- a/hw/vfio/pci.c
> +++ b/hw/vfio/pci.c
> @@ -964,6 +964,28 @@ static void vfio_pci_load_rom(VFIOPCIDevice *vdev)
> }
> }
>
> +/* "Raw" read of underlying config space. */
> +static int vfio_pci_config_space_read(VFIOPCIDevice *vdev, off_t offset,
> + uint32_t size, void *data)
> +{
> + ssize_t ret;
> +
> + ret = pread(vdev->vbasedev.fd, data, size, vdev->config_offset + offset);
> +
> + return ret < 0 ? -errno : (int)ret;
> +}
> +
> +/* "Raw" write of underlying config space. */
> +static int vfio_pci_config_space_write(VFIOPCIDevice *vdev, off_t offset,
> + uint32_t size, void *data)
> +{
> + ssize_t ret;
> +
> + ret = pwrite(vdev->vbasedev.fd, data, size, vdev->config_offset + offset);
> +
> + return ret < 0 ? -errno : (int)ret;
> +}
> +
> static uint64_t vfio_rom_read(void *opaque, hwaddr addr, unsigned size)
> {
> VFIOPCIDevice *vdev = opaque;
> @@ -1016,10 +1038,9 @@ static const MemoryRegionOps vfio_rom_ops = {
>
> static void vfio_pci_size_rom(VFIOPCIDevice *vdev)
> {
> + VFIODevice *vbasedev = &vdev->vbasedev;
> uint32_t orig, size = cpu_to_le32((uint32_t)PCI_ROM_ADDRESS_MASK);
> - off_t offset = vdev->config_offset + PCI_ROM_ADDRESS;
> char *name;
> - int fd = vdev->vbasedev.fd;
>
> if (vdev->pdev.romfile || !vdev->pdev.rom_bar) {
> /* Since pci handles romfile, just print a message and return */
> @@ -1036,11 +1057,12 @@ static void vfio_pci_size_rom(VFIOPCIDevice *vdev)
> * Use the same size ROM BAR as the physical device. The contents
> * will get filled in later when the guest tries to read it.
> */
> - if (pread(fd, &orig, 4, offset) != 4 ||
> - pwrite(fd, &size, 4, offset) != 4 ||
> - pread(fd, &size, 4, offset) != 4 ||
> - pwrite(fd, &orig, 4, offset) != 4) {
> - error_report("%s(%s) failed: %m", __func__, vdev->vbasedev.name);
> + if (vfio_pci_config_space_read(vdev, PCI_ROM_ADDRESS, 4, &orig) != 4 ||
> + vfio_pci_config_space_write(vdev, PCI_ROM_ADDRESS, 4, &size) != 4 ||
> + vfio_pci_config_space_read(vdev, PCI_ROM_ADDRESS, 4, &size) != 4 ||
> + vfio_pci_config_space_write(vdev, PCI_ROM_ADDRESS, 4, &orig) != 4) {
> +
> + error_report("%s(%s) ROM access failed", __func__, vbasedev->name);
> return;
> }
>
> @@ -1220,6 +1242,7 @@ static void vfio_sub_page_bar_update_mapping(PCIDevice *pdev, int bar)
> uint32_t vfio_pci_read_config(PCIDevice *pdev, uint32_t addr, int len)
> {
> VFIOPCIDevice *vdev = VFIO_PCI_BASE(pdev);
> + VFIODevice *vbasedev = &vdev->vbasedev;
> uint32_t emu_bits = 0, emu_val = 0, phys_val = 0, val;
>
> memcpy(&emu_bits, vdev->emulated_config_bits + addr, len);
> @@ -1232,12 +1255,13 @@ uint32_t vfio_pci_read_config(PCIDevice *pdev, uint32_t addr, int len)
> if (~emu_bits & (0xffffffffU >> (32 - len * 8))) {
> ssize_t ret;
>
> - ret = pread(vdev->vbasedev.fd, &phys_val, len,
> - vdev->config_offset + addr);
> + ret = vfio_pci_config_space_read(vdev, addr, len, &phys_val);
> if (ret != len) {
> - error_report("%s(%s, 0x%x, 0x%x) failed: %m",
> - __func__, vdev->vbasedev.name, addr, len);
> - return -errno;
> + const char *err = ret < 0 ? strerror(-ret) : "short read";
These "short read/write" messages, here and below, are a bit invasive
in the code but they are interesting to keep. I wonder if we could
improve readability with some helper.
> +
> + error_report("%s(%s, 0x%x, 0x%x) failed: %s",
> + __func__, vbasedev->name, addr, len, err);
> + return -1;
-1 (all ones) seems more correct than returning -errno as before.
> }
> phys_val = le32_to_cpu(phys_val);
> }
> @@ -1253,15 +1277,19 @@ void vfio_pci_write_config(PCIDevice *pdev,
> uint32_t addr, uint32_t val, int len)
> {
> VFIOPCIDevice *vdev = VFIO_PCI_BASE(pdev);
> + VFIODevice *vbasedev = &vdev->vbasedev;
> uint32_t val_le = cpu_to_le32(val);
> + int ret;
>
> trace_vfio_pci_write_config(vdev->vbasedev.name, addr, val, len);
>
> /* Write everything to VFIO, let it filter out what we can't write */
> - if (pwrite(vdev->vbasedev.fd, &val_le, len, vdev->config_offset + addr)
> - != len) {
> - error_report("%s(%s, 0x%x, 0x%x, 0x%x) failed: %m",
> - __func__, vdev->vbasedev.name, addr, val, len);
> + ret = vfio_pci_config_space_write(vdev, addr, len, &val_le);
> + if (ret != len) {
> + const char *err = ret < 0 ? strerror(-ret) : "short write";
> +
> + error_report("%s(%s, 0x%x, 0x%x, 0x%x) failed: %s",
> + __func__, vbasedev->name, addr, val, len, err);
> }
>
> /* MSI/MSI-X Enabling/Disabling */
> @@ -1349,9 +1377,12 @@ static bool vfio_msi_setup(VFIOPCIDevice *vdev, int pos, Error **errp)
> int ret, entries;
> Error *err = NULL;
>
> - if (pread(vdev->vbasedev.fd, &ctrl, sizeof(ctrl),
> - vdev->config_offset + pos + PCI_CAP_FLAGS) != sizeof(ctrl)) {
> - error_setg_errno(errp, errno, "failed reading MSI PCI_CAP_FLAGS");
> + ret = vfio_pci_config_space_read(vdev, pos + PCI_CAP_FLAGS,
> + sizeof(ctrl), &ctrl);
> + if (ret != sizeof(ctrl)) {
> + const char *errmsg = ret < 0 ? strerror(-ret) : "short read";
> +
> + error_setg(errp, "failed reading MSI PCI_CAP_FLAGS: %s", errmsg);
> return false;
> }
> ctrl = le16_to_cpu(ctrl);
> @@ -1558,30 +1589,39 @@ static bool vfio_msix_early_setup(VFIOPCIDevice *vdev, Error **errp)
> uint8_t pos;
> uint16_t ctrl;
> uint32_t table, pba;
> - int ret, fd = vdev->vbasedev.fd;
> struct vfio_irq_info irq_info;
> VFIOMSIXInfo *msix;
> + int ret;
>
> pos = pci_find_capability(&vdev->pdev, PCI_CAP_ID_MSIX);
> if (!pos) {
> return true;
> }
>
> - if (pread(fd, &ctrl, sizeof(ctrl),
> - vdev->config_offset + pos + PCI_MSIX_FLAGS) != sizeof(ctrl)) {
> - error_setg_errno(errp, errno, "failed to read PCI MSIX FLAGS");
> + ret = vfio_pci_config_space_read(vdev, pos + PCI_MSIX_FLAGS,
> + sizeof(ctrl), &ctrl);
> + if (ret != sizeof(ctrl)) {
> + const char *err = ret < 0 ? strerror(-ret) : "short read";
> +
> + error_setg(errp, "failed to read PCI MSIX FLAGS: %s", err);
> return false;
> }
>
> - if (pread(fd, &table, sizeof(table),
> - vdev->config_offset + pos + PCI_MSIX_TABLE) != sizeof(table)) {
> - error_setg_errno(errp, errno, "failed to read PCI MSIX TABLE");
> + ret = vfio_pci_config_space_read(vdev, pos + PCI_MSIX_TABLE,
> + sizeof(table), &table);
> + if (ret != sizeof(table)) {
> + const char *err = ret < 0 ? strerror(-ret) : "short read";
> +
> + error_setg(errp, "failed to read PCI MSIX TABLE: %s", err);
> return false;
> }
>
> - if (pread(fd, &pba, sizeof(pba),
> - vdev->config_offset + pos + PCI_MSIX_PBA) != sizeof(pba)) {
> - error_setg_errno(errp, errno, "failed to read PCI MSIX PBA");
> + ret = vfio_pci_config_space_read(vdev, pos + PCI_MSIX_PBA,
> + sizeof(pba), &pba);
> + if (ret != sizeof(pba)) {
> + const char *err = ret < 0 ? strerror(-ret) : "short read";
> +
> + error_setg(errp, "failed to read PCI MSIX PBA: %s", err);
> return false;
> }
>
> @@ -1741,10 +1781,12 @@ static void vfio_bar_prepare(VFIOPCIDevice *vdev, int nr)
> }
>
> /* Determine what type of BAR this is for registration */
> - ret = pread(vdev->vbasedev.fd, &pci_bar, sizeof(pci_bar),
> - vdev->config_offset + PCI_BASE_ADDRESS_0 + (4 * nr));
> + ret = vfio_pci_config_space_read(vdev, PCI_BASE_ADDRESS_0 + (4 * nr),
> + sizeof(pci_bar), &pci_bar);
> if (ret != sizeof(pci_bar)) {
> - error_report("vfio: Failed to read BAR %d (%m)", nr);
> + const char *err = ret < 0 ? strerror(-ret) : "short read";
> +
> + error_report("vfio: Failed to read BAR %d: %s", nr, err);
> return;
> }
>
> @@ -2448,21 +2490,25 @@ void vfio_pci_pre_reset(VFIOPCIDevice *vdev)
>
> void vfio_pci_post_reset(VFIOPCIDevice *vdev)
> {
> + VFIODevice *vbasedev = &vdev->vbasedev;
> Error *err = NULL;
> - int nr;
> + int ret, nr;
>
> if (!vfio_intx_enable(vdev, &err)) {
> error_reportf_err(err, VFIO_MSG_PREFIX, vdev->vbasedev.name);
> }
>
> for (nr = 0; nr < PCI_NUM_REGIONS - 1; ++nr) {
> - off_t addr = vdev->config_offset + PCI_BASE_ADDRESS_0 + (4 * nr);
> + off_t addr = PCI_BASE_ADDRESS_0 + (4 * nr);
> uint32_t val = 0;
> uint32_t len = sizeof(val);
>
> - if (pwrite(vdev->vbasedev.fd, &val, len, addr) != len) {
> - error_report("%s(%s) reset bar %d failed: %m", __func__,
> - vdev->vbasedev.name, nr);
> + ret = vfio_pci_config_space_write(vdev, addr, len, &val);
> + if (ret != len) {
> + const char *errmsg = ret < 0 ? strerror(-ret) : "short write";
> +
> + error_report("%s(%s) reset bar %d failed: %s", __func__,
> + vbasedev->name, nr, errmsg);
> }
> }
>
> @@ -3099,6 +3145,7 @@ static void vfio_realize(PCIDevice *pdev, Error **errp)
> int i, ret;
> char uuid[UUID_STR_LEN];
> g_autofree char *name = NULL;
> + size_t config_space_size;
why not use uint32_t ?
Thanks,
C.
>
> if (vbasedev->fd < 0 && !vbasedev->sysfsdev) {
> if (!(~vdev->host.domain || ~vdev->host.bus ||
> @@ -3153,13 +3200,14 @@ static void vfio_realize(PCIDevice *pdev, Error **errp)
> goto error;
> }
>
> + config_space_size = MIN(pci_config_size(&vdev->pdev), vdev->config_size);
> +
> /* Get a copy of config space */
> - ret = pread(vbasedev->fd, vdev->pdev.config,
> - MIN(pci_config_size(&vdev->pdev), vdev->config_size),
> - vdev->config_offset);
> - if (ret < (int)MIN(pci_config_size(&vdev->pdev), vdev->config_size)) {
> - ret = ret < 0 ? -errno : -EFAULT;
> - error_setg_errno(errp, -ret, "failed to read device config space");
> + ret = vfio_pci_config_space_read(vdev, 0, config_space_size,
> + vdev->pdev.config);
> + if (ret < (int)config_space_size) {
> + ret = ret < 0 ? -ret : EFAULT;
> + error_setg_errno(errp, ret, "failed to read device config space");
> goto error;
> }
>
On 4/9/25 21:48, John Levon wrote:
> Add these helpers that access config space and return an -errno style
> return.
>
> Signed-off-by: John Levon <john.levon@nutanix.com>
> ---
> hw/vfio/pci.c | 134 ++++++++++++++++++++++++++++++++++----------------
> 1 file changed, 91 insertions(+), 43 deletions(-)
>
> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
> index ddeee33aa9..c3842d2f8d 100644
> --- a/hw/vfio/pci.c
> +++ b/hw/vfio/pci.c
> @@ -964,6 +964,28 @@ static void vfio_pci_load_rom(VFIOPCIDevice *vdev)
> }
> }
>
> +/* "Raw" read of underlying config space. */
> +static int vfio_pci_config_space_read(VFIOPCIDevice *vdev, off_t offset,
> + uint32_t size, void *data)
Returning ssize_t here might be better here to avoid casting issues,
though we would never read/write something exceeds INT32_MAX.
Thanks,
Moeko
> +{
> + ssize_t ret;
> +
> + ret = pread(vdev->vbasedev.fd, data, size, vdev->config_offset + offset);
> +
> + return ret < 0 ? -errno : (int)ret;
> +}
> +
> +/* "Raw" write of underlying config space. */
> +static int vfio_pci_config_space_write(VFIOPCIDevice *vdev, off_t offset,
> + uint32_t size, void *data)
> +{
> + ssize_t ret;
> +
> + ret = pwrite(vdev->vbasedev.fd, data, size, vdev->config_offset + offset);
> +
> + return ret < 0 ? -errno : (int)ret;
> +}
> +
> static uint64_t vfio_rom_read(void *opaque, hwaddr addr, unsigned size)
> {
> VFIOPCIDevice *vdev = opaque;
> @@ -1016,10 +1038,9 @@ static const MemoryRegionOps vfio_rom_ops = {
>
> static void vfio_pci_size_rom(VFIOPCIDevice *vdev)
> {
> + VFIODevice *vbasedev = &vdev->vbasedev;
> uint32_t orig, size = cpu_to_le32((uint32_t)PCI_ROM_ADDRESS_MASK);
> - off_t offset = vdev->config_offset + PCI_ROM_ADDRESS;
> char *name;
> - int fd = vdev->vbasedev.fd;
>
> if (vdev->pdev.romfile || !vdev->pdev.rom_bar) {
> /* Since pci handles romfile, just print a message and return */
> @@ -1036,11 +1057,12 @@ static void vfio_pci_size_rom(VFIOPCIDevice *vdev)
> * Use the same size ROM BAR as the physical device. The contents
> * will get filled in later when the guest tries to read it.
> */
> - if (pread(fd, &orig, 4, offset) != 4 ||
> - pwrite(fd, &size, 4, offset) != 4 ||
> - pread(fd, &size, 4, offset) != 4 ||
> - pwrite(fd, &orig, 4, offset) != 4) {
> - error_report("%s(%s) failed: %m", __func__, vdev->vbasedev.name);
> + if (vfio_pci_config_space_read(vdev, PCI_ROM_ADDRESS, 4, &orig) != 4 ||
> + vfio_pci_config_space_write(vdev, PCI_ROM_ADDRESS, 4, &size) != 4 ||
> + vfio_pci_config_space_read(vdev, PCI_ROM_ADDRESS, 4, &size) != 4 ||
> + vfio_pci_config_space_write(vdev, PCI_ROM_ADDRESS, 4, &orig) != 4) {
> +
> + error_report("%s(%s) ROM access failed", __func__, vbasedev->name);
> return;
> }
>
> @@ -1220,6 +1242,7 @@ static void vfio_sub_page_bar_update_mapping(PCIDevice *pdev, int bar)
> uint32_t vfio_pci_read_config(PCIDevice *pdev, uint32_t addr, int len)
> {
> VFIOPCIDevice *vdev = VFIO_PCI_BASE(pdev);
> + VFIODevice *vbasedev = &vdev->vbasedev;
> uint32_t emu_bits = 0, emu_val = 0, phys_val = 0, val;
>
> memcpy(&emu_bits, vdev->emulated_config_bits + addr, len);
> @@ -1232,12 +1255,13 @@ uint32_t vfio_pci_read_config(PCIDevice *pdev, uint32_t addr, int len)
> if (~emu_bits & (0xffffffffU >> (32 - len * 8))) {
> ssize_t ret;
>
> - ret = pread(vdev->vbasedev.fd, &phys_val, len,
> - vdev->config_offset + addr);
> + ret = vfio_pci_config_space_read(vdev, addr, len, &phys_val);
> if (ret != len) {
> - error_report("%s(%s, 0x%x, 0x%x) failed: %m",
> - __func__, vdev->vbasedev.name, addr, len);
> - return -errno;
> + const char *err = ret < 0 ? strerror(-ret) : "short read";
> +
> + error_report("%s(%s, 0x%x, 0x%x) failed: %s",
> + __func__, vbasedev->name, addr, len, err);
> + return -1;
> }
> phys_val = le32_to_cpu(phys_val);
> }
> @@ -1253,15 +1277,19 @@ void vfio_pci_write_config(PCIDevice *pdev,
> uint32_t addr, uint32_t val, int len)
> {
> VFIOPCIDevice *vdev = VFIO_PCI_BASE(pdev);
> + VFIODevice *vbasedev = &vdev->vbasedev;
> uint32_t val_le = cpu_to_le32(val);
> + int ret;
>
> trace_vfio_pci_write_config(vdev->vbasedev.name, addr, val, len);
>
> /* Write everything to VFIO, let it filter out what we can't write */
> - if (pwrite(vdev->vbasedev.fd, &val_le, len, vdev->config_offset + addr)
> - != len) {
> - error_report("%s(%s, 0x%x, 0x%x, 0x%x) failed: %m",
> - __func__, vdev->vbasedev.name, addr, val, len);
> + ret = vfio_pci_config_space_write(vdev, addr, len, &val_le);
> + if (ret != len) {
> + const char *err = ret < 0 ? strerror(-ret) : "short write";
> +
> + error_report("%s(%s, 0x%x, 0x%x, 0x%x) failed: %s",
> + __func__, vbasedev->name, addr, val, len, err);
> }
>
> /* MSI/MSI-X Enabling/Disabling */
> @@ -1349,9 +1377,12 @@ static bool vfio_msi_setup(VFIOPCIDevice *vdev, int pos, Error **errp)
> int ret, entries;
> Error *err = NULL;
>
> - if (pread(vdev->vbasedev.fd, &ctrl, sizeof(ctrl),
> - vdev->config_offset + pos + PCI_CAP_FLAGS) != sizeof(ctrl)) {
> - error_setg_errno(errp, errno, "failed reading MSI PCI_CAP_FLAGS");
> + ret = vfio_pci_config_space_read(vdev, pos + PCI_CAP_FLAGS,
> + sizeof(ctrl), &ctrl);
> + if (ret != sizeof(ctrl)) {
> + const char *errmsg = ret < 0 ? strerror(-ret) : "short read";
> +
> + error_setg(errp, "failed reading MSI PCI_CAP_FLAGS: %s", errmsg);
> return false;
> }
> ctrl = le16_to_cpu(ctrl);
> @@ -1558,30 +1589,39 @@ static bool vfio_msix_early_setup(VFIOPCIDevice *vdev, Error **errp)
> uint8_t pos;
> uint16_t ctrl;
> uint32_t table, pba;
> - int ret, fd = vdev->vbasedev.fd;
> struct vfio_irq_info irq_info;
> VFIOMSIXInfo *msix;
> + int ret;
>
> pos = pci_find_capability(&vdev->pdev, PCI_CAP_ID_MSIX);
> if (!pos) {
> return true;
> }
>
> - if (pread(fd, &ctrl, sizeof(ctrl),
> - vdev->config_offset + pos + PCI_MSIX_FLAGS) != sizeof(ctrl)) {
> - error_setg_errno(errp, errno, "failed to read PCI MSIX FLAGS");
> + ret = vfio_pci_config_space_read(vdev, pos + PCI_MSIX_FLAGS,
> + sizeof(ctrl), &ctrl);
> + if (ret != sizeof(ctrl)) {
> + const char *err = ret < 0 ? strerror(-ret) : "short read";
> +
> + error_setg(errp, "failed to read PCI MSIX FLAGS: %s", err);
> return false;
> }
>
> - if (pread(fd, &table, sizeof(table),
> - vdev->config_offset + pos + PCI_MSIX_TABLE) != sizeof(table)) {
> - error_setg_errno(errp, errno, "failed to read PCI MSIX TABLE");
> + ret = vfio_pci_config_space_read(vdev, pos + PCI_MSIX_TABLE,
> + sizeof(table), &table);
> + if (ret != sizeof(table)) {
> + const char *err = ret < 0 ? strerror(-ret) : "short read";
> +
> + error_setg(errp, "failed to read PCI MSIX TABLE: %s", err);
> return false;
> }
>
> - if (pread(fd, &pba, sizeof(pba),
> - vdev->config_offset + pos + PCI_MSIX_PBA) != sizeof(pba)) {
> - error_setg_errno(errp, errno, "failed to read PCI MSIX PBA");
> + ret = vfio_pci_config_space_read(vdev, pos + PCI_MSIX_PBA,
> + sizeof(pba), &pba);
> + if (ret != sizeof(pba)) {
> + const char *err = ret < 0 ? strerror(-ret) : "short read";
> +
> + error_setg(errp, "failed to read PCI MSIX PBA: %s", err);
> return false;
> }
>
> @@ -1741,10 +1781,12 @@ static void vfio_bar_prepare(VFIOPCIDevice *vdev, int nr)
> }
>
> /* Determine what type of BAR this is for registration */
> - ret = pread(vdev->vbasedev.fd, &pci_bar, sizeof(pci_bar),
> - vdev->config_offset + PCI_BASE_ADDRESS_0 + (4 * nr));
> + ret = vfio_pci_config_space_read(vdev, PCI_BASE_ADDRESS_0 + (4 * nr),
> + sizeof(pci_bar), &pci_bar);
> if (ret != sizeof(pci_bar)) {
> - error_report("vfio: Failed to read BAR %d (%m)", nr);
> + const char *err = ret < 0 ? strerror(-ret) : "short read";
> +
> + error_report("vfio: Failed to read BAR %d: %s", nr, err);
> return;
> }
>
> @@ -2448,21 +2490,25 @@ void vfio_pci_pre_reset(VFIOPCIDevice *vdev)
>
> void vfio_pci_post_reset(VFIOPCIDevice *vdev)
> {
> + VFIODevice *vbasedev = &vdev->vbasedev;
> Error *err = NULL;
> - int nr;
> + int ret, nr;
>
> if (!vfio_intx_enable(vdev, &err)) {
> error_reportf_err(err, VFIO_MSG_PREFIX, vdev->vbasedev.name);
> }
>
> for (nr = 0; nr < PCI_NUM_REGIONS - 1; ++nr) {
> - off_t addr = vdev->config_offset + PCI_BASE_ADDRESS_0 + (4 * nr);
> + off_t addr = PCI_BASE_ADDRESS_0 + (4 * nr);
> uint32_t val = 0;
> uint32_t len = sizeof(val);
>
> - if (pwrite(vdev->vbasedev.fd, &val, len, addr) != len) {
> - error_report("%s(%s) reset bar %d failed: %m", __func__,
> - vdev->vbasedev.name, nr);
> + ret = vfio_pci_config_space_write(vdev, addr, len, &val);
> + if (ret != len) {
> + const char *errmsg = ret < 0 ? strerror(-ret) : "short write";
> +
> + error_report("%s(%s) reset bar %d failed: %s", __func__,
> + vbasedev->name, nr, errmsg);
> }
> }
>
> @@ -3099,6 +3145,7 @@ static void vfio_realize(PCIDevice *pdev, Error **errp)
> int i, ret;
> char uuid[UUID_STR_LEN];
> g_autofree char *name = NULL;
> + size_t config_space_size;
>
> if (vbasedev->fd < 0 && !vbasedev->sysfsdev) {
> if (!(~vdev->host.domain || ~vdev->host.bus ||
> @@ -3153,13 +3200,14 @@ static void vfio_realize(PCIDevice *pdev, Error **errp)
> goto error;
> }
>
> + config_space_size = MIN(pci_config_size(&vdev->pdev), vdev->config_size);
> +
> /* Get a copy of config space */
> - ret = pread(vbasedev->fd, vdev->pdev.config,
> - MIN(pci_config_size(&vdev->pdev), vdev->config_size),
> - vdev->config_offset);
> - if (ret < (int)MIN(pci_config_size(&vdev->pdev), vdev->config_size)) {
> - ret = ret < 0 ? -errno : -EFAULT;
> - error_setg_errno(errp, -ret, "failed to read device config space");
> + ret = vfio_pci_config_space_read(vdev, 0, config_space_size,
> + vdev->pdev.config);
> + if (ret < (int)config_space_size) {
> + ret = ret < 0 ? -ret : EFAULT;
> + error_setg_errno(errp, ret, "failed to read device config space");
> goto error;
> }
>
On Wed, Apr 09, 2025 at 11:51:09PM +0800, Tomita Moeko wrote: > On 4/9/25 21:48, John Levon wrote: > > Add these helpers that access config space and return an -errno style > > return. > > > > Signed-off-by: John Levon <john.levon@nutanix.com> > > --- > > hw/vfio/pci.c | 134 ++++++++++++++++++++++++++++++++++---------------- > > 1 file changed, 91 insertions(+), 43 deletions(-) > > > > diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c > > index ddeee33aa9..c3842d2f8d 100644 > > --- a/hw/vfio/pci.c > > +++ b/hw/vfio/pci.c > > @@ -964,6 +964,28 @@ static void vfio_pci_load_rom(VFIOPCIDevice *vdev) > > } > > } > > > > +/* "Raw" read of underlying config space. */ > > +static int vfio_pci_config_space_read(VFIOPCIDevice *vdev, off_t offset, > > + uint32_t size, void *data) > > Returning ssize_t here might be better here to avoid casting issues, > though we would never read/write something exceeds INT32_MAX. I considered this (and the later helpers in the patch), but most of the existing code already uses int. Happy to look at fixing the callers too (e.g. vfio_msi_setup()) if that's everyone's preference. regards john
On 4/9/25 23:54, John Levon wrote: > On Wed, Apr 09, 2025 at 11:51:09PM +0800, Tomita Moeko wrote: > >> On 4/9/25 21:48, John Levon wrote: >>> Add these helpers that access config space and return an -errno style >>> return. >>> >>> Signed-off-by: John Levon <john.levon@nutanix.com> >>> --- >>> hw/vfio/pci.c | 134 ++++++++++++++++++++++++++++++++++---------------- >>> 1 file changed, 91 insertions(+), 43 deletions(-) >>> >>> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c >>> index ddeee33aa9..c3842d2f8d 100644 >>> --- a/hw/vfio/pci.c >>> +++ b/hw/vfio/pci.c >>> @@ -964,6 +964,28 @@ static void vfio_pci_load_rom(VFIOPCIDevice *vdev) >>> } >>> } >>> >>> +/* "Raw" read of underlying config space. */ >>> +static int vfio_pci_config_space_read(VFIOPCIDevice *vdev, off_t offset, >>> + uint32_t size, void *data) >> >> Returning ssize_t here might be better here to avoid casting issues, >> though we would never read/write something exceeds INT32_MAX. > > I considered this (and the later helpers in the patch), but most of the existing > code already uses int. Happy to look at fixing the callers too (e.g. > vfio_msi_setup()) if that's everyone's preference. > > regards > john I checked the code, caller casts return of pread/pwrite to int because the `count` argument, bytes read/write at most, they passed does not exceed int. Given that uint32_t can exceed int, returning ssize_t here and let callers to determine cast or not is better I believe. Thanks, Moeko
On 4/9/25 18:30, Tomita Moeko wrote: > On 4/9/25 23:54, John Levon wrote: >> On Wed, Apr 09, 2025 at 11:51:09PM +0800, Tomita Moeko wrote: >> >>> On 4/9/25 21:48, John Levon wrote: >>>> Add these helpers that access config space and return an -errno style >>>> return. >>>> >>>> Signed-off-by: John Levon <john.levon@nutanix.com> >>>> --- >>>> hw/vfio/pci.c | 134 ++++++++++++++++++++++++++++++++++---------------- >>>> 1 file changed, 91 insertions(+), 43 deletions(-) >>>> >>>> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c >>>> index ddeee33aa9..c3842d2f8d 100644 >>>> --- a/hw/vfio/pci.c >>>> +++ b/hw/vfio/pci.c >>>> @@ -964,6 +964,28 @@ static void vfio_pci_load_rom(VFIOPCIDevice *vdev) >>>> } >>>> } >>>> >>>> +/* "Raw" read of underlying config space. */ >>>> +static int vfio_pci_config_space_read(VFIOPCIDevice *vdev, off_t offset, >>>> + uint32_t size, void *data) >>> >>> Returning ssize_t here might be better here to avoid casting issues, >>> though we would never read/write something exceeds INT32_MAX. >> >> I considered this (and the later helpers in the patch), but most of the existing >> code already uses int. Happy to look at fixing the callers too (e.g. >> vfio_msi_setup()) if that's everyone's preference. >> >> regards >> john > > I checked the code, caller casts return of pread/pwrite to int because > the `count` argument, bytes read/write at most, they passed does not > exceed int. Given that uint32_t can exceed int, returning ssize_t here > and let callers to determine cast or not is better I believe. I have no strong preference. The less change the better I would say. Thanks, C.
© 2016 - 2025 Red Hat, Inc.