On 4/30/25 21:39, John Levon wrote:
Instead of requesting region information on demand with
VFIO_DEVICE_GET_REGION_INFO, maintain a cache: this will become
necessary for performance for vfio-user, where this call becomes a
message over the control socket, so is of higher overhead than the
traditional path.
We will also need it to generalize region accesses, as that means we
can't use ->config_offset for configuration space accesses, but must
look up the region offset (if relevant) each time.
This looks cleaner. One comment below,
Originally-by: John Johnson <john.g.john...@oracle.com>
Signed-off-by: Elena Ufimtseva <elena.ufimts...@oracle.com>
Signed-off-by: Jagannathan Raman <jag.ra...@oracle.com>
Signed-off-by: John Levon <john.le...@nutanix.com>
---
hw/vfio/ccw.c | 5 -----
hw/vfio/device.c | 27 +++++++++++++++++++++++----
hw/vfio/igd.c | 8 ++++----
hw/vfio/pci.c | 6 +++---
hw/vfio/region.c | 2 +-
include/hw/vfio/vfio-device.h | 1 +
6 files changed, 32 insertions(+), 17 deletions(-)
diff --git a/hw/vfio/ccw.c b/hw/vfio/ccw.c
index ab3fabf991..cea9d6e005 100644
--- a/hw/vfio/ccw.c
+++ b/hw/vfio/ccw.c
@@ -504,7 +504,6 @@ static bool vfio_ccw_get_region(VFIOCCWDevice *vcdev, Error
**errp)
vcdev->io_region_offset = info->offset;
vcdev->io_region = g_malloc0(info->size);
- g_free(info);
/* check for the optional async command region */
ret = vfio_device_get_region_info_type(vdev, VFIO_REGION_TYPE_CCW,
@@ -517,7 +516,6 @@ static bool vfio_ccw_get_region(VFIOCCWDevice *vcdev, Error
**errp)
}
vcdev->async_cmd_region_offset = info->offset;
vcdev->async_cmd_region = g_malloc0(info->size);
- g_free(info);
}
ret = vfio_device_get_region_info_type(vdev, VFIO_REGION_TYPE_CCW,
@@ -530,7 +528,6 @@ static bool vfio_ccw_get_region(VFIOCCWDevice *vcdev, Error
**errp)
}
vcdev->schib_region_offset = info->offset;
vcdev->schib_region = g_malloc(info->size);
- g_free(info);
}
ret = vfio_device_get_region_info_type(vdev, VFIO_REGION_TYPE_CCW,
@@ -544,7 +541,6 @@ static bool vfio_ccw_get_region(VFIOCCWDevice *vcdev, Error
**errp)
}
vcdev->crw_region_offset = info->offset;
vcdev->crw_region = g_malloc(info->size);
- g_free(info);
}
return true;
@@ -554,7 +550,6 @@ out_err:
g_free(vcdev->schib_region);
g_free(vcdev->async_cmd_region);
g_free(vcdev->io_region);
- g_free(info);
return false;
}
diff --git a/hw/vfio/device.c b/hw/vfio/device.c
index 468fb50eac..d08c0ab536 100644
--- a/hw/vfio/device.c
+++ b/hw/vfio/device.c
@@ -205,6 +205,12 @@ int vfio_device_get_region_info(VFIODevice *vbasedev, int
index,
size_t argsz = sizeof(struct vfio_region_info);
int ret;
+ /* check cache */
+ if (vbasedev->reginfo[index] != NULL) {
+ *info = vbasedev->reginfo[index];
+ return 0;
+ }
+
*info = g_malloc0(argsz);
(*info)->index = index;
@@ -225,6 +231,9 @@ retry:
goto retry;
}
+ /* fill cache */
+ vbasedev->reginfo[index] = *info;
+
return 0;
}
@@ -243,7 +252,6 @@ int vfio_device_get_region_info_type(VFIODevice *vbasedev, uint32_t type,
hdr = vfio_get_region_info_cap(*info, VFIO_REGION_INFO_CAP_TYPE);
if (!hdr) {
- g_free(*info);
continue;
}
@@ -255,8 +263,6 @@ int vfio_device_get_region_info_type(VFIODevice *vbasedev, uint32_t type,
if (cap_type->type == type && cap_type->subtype == subtype) {
return 0;
}
-
- g_free(*info);
}
*info = NULL;
@@ -265,7 +271,7 @@ int vfio_device_get_region_info_type(VFIODevice *vbasedev,
uint32_t type,
bool vfio_device_has_region_cap(VFIODevice *vbasedev, int region, uint16_t cap_type)
{
- g_autofree struct vfio_region_info *info = NULL;
+ struct vfio_region_info *info = NULL;
bool ret = false;
if (!vfio_device_get_region_info(vbasedev, region, &info)) {
@@ -438,10 +444,23 @@ void vfio_device_prepare(VFIODevice *vbasedev,
VFIOContainerBase *bcontainer,
QLIST_INSERT_HEAD(&bcontainer->device_list, vbasedev, container_next);
QLIST_INSERT_HEAD(&vfio_device_list, vbasedev, global_next);
+
+ if (vbasedev->reginfo == NULL) {
I don't think this test is necessary. Is it ?
Thanks,
C.
+ vbasedev->reginfo = g_new0(struct vfio_region_info *,
+ vbasedev->num_regions);
+ }
}
void vfio_device_unprepare(VFIODevice *vbasedev)
{
+ int i;
+
+ for (i = 0; i < vbasedev->num_regions; i++) {
+ g_free(vbasedev->reginfo[i]);
+ }
+ g_free(vbasedev->reginfo);
+ vbasedev->reginfo = NULL;
+
QLIST_REMOVE(vbasedev, container_next);
QLIST_REMOVE(vbasedev, global_next);
vbasedev->bcontainer = NULL;
diff --git a/hw/vfio/igd.c b/hw/vfio/igd.c
index d7e4728fdc..c7db74cde4 100644
--- a/hw/vfio/igd.c
+++ b/hw/vfio/igd.c
@@ -191,7 +191,7 @@ static bool vfio_pci_igd_opregion_init(VFIOPCIDevice *vdev,
static bool vfio_pci_igd_setup_opregion(VFIOPCIDevice *vdev, Error **errp)
{
- g_autofree struct vfio_region_info *opregion = NULL;
+ struct vfio_region_info *opregion = NULL;
int ret;
/* Hotplugging is not supported for opregion access */
@@ -355,8 +355,8 @@ static int vfio_pci_igd_lpc_init(VFIOPCIDevice *vdev,
static bool vfio_pci_igd_setup_lpc_bridge(VFIOPCIDevice *vdev, Error **errp)
{
- g_autofree struct vfio_region_info *host = NULL;
- g_autofree struct vfio_region_info *lpc = NULL;
+ struct vfio_region_info *host = NULL;
+ struct vfio_region_info *lpc = NULL;
PCIDevice *lpc_bridge;
int ret;
@@ -532,7 +532,7 @@ static bool vfio_pci_igd_config_quirk(VFIOPCIDevice *vdev, Error **errp)
* - OpRegion
* - Same LPC bridge and Host bridge VID/DID/SVID/SSID as host
*/
- g_autofree struct vfio_region_info *rom = NULL;
+ struct vfio_region_info *rom = NULL;
legacy_mode_enabled = true;
info_report("IGD legacy mode enabled, "
diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
index bbf95215cc..1aeb4d91d2 100644
--- a/hw/vfio/pci.c
+++ b/hw/vfio/pci.c
@@ -883,8 +883,8 @@ static void vfio_update_msi(VFIOPCIDevice *vdev)
static void vfio_pci_load_rom(VFIOPCIDevice *vdev)
{
- g_autofree struct vfio_region_info *reg_info = NULL;
VFIODevice *vbasedev = &vdev->vbasedev;
+ struct vfio_region_info *reg_info = NULL;
uint64_t size;
off_t off = 0;
ssize_t bytes;
@@ -2710,7 +2710,7 @@ static VFIODeviceOps vfio_pci_ops = {
bool vfio_populate_vga(VFIOPCIDevice *vdev, Error **errp)
{
VFIODevice *vbasedev = &vdev->vbasedev;
- g_autofree struct vfio_region_info *reg_info = NULL;
+ struct vfio_region_info *reg_info = NULL;
int ret;
ret = vfio_device_get_region_info(vbasedev, VFIO_PCI_VGA_REGION_INDEX, ®_info);
@@ -2775,7 +2775,7 @@ bool vfio_populate_vga(VFIOPCIDevice *vdev, Error **errp)
static bool vfio_populate_device(VFIOPCIDevice *vdev, Error **errp)
{
VFIODevice *vbasedev = &vdev->vbasedev;
- g_autofree struct vfio_region_info *reg_info = NULL;
+ struct vfio_region_info *reg_info = NULL;
struct vfio_irq_info irq_info;
int i, ret = -1;
diff --git a/hw/vfio/region.c b/hw/vfio/region.c
index 04bf9eb098..ef2630cac3 100644
--- a/hw/vfio/region.c
+++ b/hw/vfio/region.c
@@ -182,7 +182,7 @@ static int vfio_setup_region_sparse_mmaps(VFIORegion
*region,
int vfio_region_setup(Object *obj, VFIODevice *vbasedev, VFIORegion *region,
int index, const char *name)
{
- g_autofree struct vfio_region_info *info = NULL;
+ struct vfio_region_info *info = NULL;
int ret;
ret = vfio_device_get_region_info(vbasedev, index, &info);
diff --git a/include/hw/vfio/vfio-device.h b/include/hw/vfio/vfio-device.h
index e89ed02c0e..b4a28c2a54 100644
--- a/include/hw/vfio/vfio-device.h
+++ b/include/hw/vfio/vfio-device.h
@@ -83,6 +83,7 @@ typedef struct VFIODevice {
IOMMUFDBackend *iommufd;
VFIOIOASHwpt *hwpt;
QLIST_ENTRY(VFIODevice) hwpt_next;
+ struct vfio_region_info **reginfo;
} VFIODevice;
struct VFIODeviceOps {
