From: Jagannathan Raman <jag.raman@oracle.com>
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.
Originally-by: John Johnson <john.g.johnson@oracle.com>
Signed-off-by: Elena Ufimtseva <elena.ufimtseva@oracle.com>
Signed-off-by: Jagannathan Raman <jag.raman@oracle.com>
Signed-off-by: John Levon <john.levon@nutanix.com>
---
hw/vfio/ccw.c | 5 -----
hw/vfio/common.c | 12 ++++++++++++
hw/vfio/container.c | 10 ++++++++++
hw/vfio/helpers.c | 21 ++++++++++++++++-----
hw/vfio/igd.c | 8 ++++----
hw/vfio/pci.c | 8 ++++----
include/hw/vfio/vfio-common.h | 1 +
7 files changed, 47 insertions(+), 18 deletions(-)
diff --git a/hw/vfio/ccw.c b/hw/vfio/ccw.c
index 67bc137f9b..22378d50bc 100644
--- a/hw/vfio/ccw.c
+++ b/hw/vfio/ccw.c
@@ -510,7 +510,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_get_dev_region_info(vdev, VFIO_REGION_TYPE_CCW,
@@ -523,7 +522,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_get_dev_region_info(vdev, VFIO_REGION_TYPE_CCW,
@@ -536,7 +534,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_get_dev_region_info(vdev, VFIO_REGION_TYPE_CCW,
@@ -550,7 +547,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;
@@ -560,7 +556,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/common.c b/hw/vfio/common.c
index 4434e0a0a2..1866b3d3c5 100644
--- a/hw/vfio/common.c
+++ b/hw/vfio/common.c
@@ -1569,6 +1569,16 @@ retry:
return info;
}
+static void vfio_get_all_regions(VFIODevice *vbasedev)
+{
+ struct vfio_region_info *info;
+ int i;
+
+ for (i = 0; i < vbasedev->num_regions; i++) {
+ vfio_get_region_info(vbasedev, i, &info);
+ }
+}
+
void vfio_prepare_device(VFIODevice *vbasedev, VFIOContainerBase *bcontainer,
VFIOGroup *group, struct vfio_device_info *info)
{
@@ -1586,6 +1596,8 @@ void vfio_prepare_device(VFIODevice *vbasedev, VFIOContainerBase *bcontainer,
}
QLIST_INSERT_HEAD(&vfio_device_list, vbasedev, global_next);
+
+ vfio_get_all_regions(vbasedev);
}
bool vfio_attach_device_by_iommu_type(const char *iommu_type, char *name,
diff --git a/hw/vfio/container.c b/hw/vfio/container.c
index 37a3befbc5..36cd245c92 100644
--- a/hw/vfio/container.c
+++ b/hw/vfio/container.c
@@ -886,6 +886,16 @@ static bool vfio_get_device(VFIOGroup *group, const char *name,
static void vfio_put_base_device(VFIODevice *vbasedev)
{
+ if (vbasedev->regions != NULL) {
+ int i;
+
+ for (i = 0; i < vbasedev->num_regions; i++) {
+ g_free(vbasedev->regions[i]);
+ }
+ g_free(vbasedev->regions);
+ vbasedev->regions = NULL;
+ }
+
if (!vbasedev->group) {
return;
}
diff --git a/hw/vfio/helpers.c b/hw/vfio/helpers.c
index 4b255d4f3a..3c923d23b9 100644
--- a/hw/vfio/helpers.c
+++ b/hw/vfio/helpers.c
@@ -345,7 +345,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_get_region_info(vbasedev, index, &info);
@@ -562,6 +562,17 @@ int vfio_get_region_info(VFIODevice *vbasedev, int index,
{
size_t argsz = sizeof(struct vfio_region_info);
+ /* create region cache */
+ if (vbasedev->regions == NULL) {
+ vbasedev->regions = g_new0(struct vfio_region_info *,
+ vbasedev->num_regions);
+ }
+ /* check cache */
+ if (vbasedev->regions[index] != NULL) {
+ *info = vbasedev->regions[index];
+ return 0;
+ }
+
*info = g_malloc0(argsz);
(*info)->index = index;
@@ -581,6 +592,9 @@ retry:
goto retry;
}
+ /* fill cache */
+ vbasedev->regions[index] = *info;
+
return 0;
}
@@ -599,7 +613,6 @@ int vfio_get_dev_region_info(VFIODevice *vbasedev, uint32_t type,
hdr = vfio_get_region_info_cap(*info, VFIO_REGION_INFO_CAP_TYPE);
if (!hdr) {
- g_free(*info);
continue;
}
@@ -611,8 +624,6 @@ int vfio_get_dev_region_info(VFIODevice *vbasedev, uint32_t type,
if (cap_type->type == type && cap_type->subtype == subtype) {
return 0;
}
-
- g_free(*info);
}
*info = NULL;
@@ -621,7 +632,7 @@ int vfio_get_dev_region_info(VFIODevice *vbasedev, uint32_t type,
bool vfio_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_get_region_info(vbasedev, region, &info)) {
diff --git a/hw/vfio/igd.c b/hw/vfio/igd.c
index b1a237edd6..b5425ba9c0 100644
--- a/hw/vfio/igd.c
+++ b/hw/vfio/igd.c
@@ -490,10 +490,10 @@ void vfio_probe_igd_bar0_quirk(VFIOPCIDevice *vdev, int nr)
void vfio_probe_igd_bar4_quirk(VFIOPCIDevice *vdev, int nr)
{
- g_autofree struct vfio_region_info *rom = NULL;
- g_autofree struct vfio_region_info *opregion = NULL;
- g_autofree struct vfio_region_info *host = NULL;
- g_autofree struct vfio_region_info *lpc = NULL;
+ struct vfio_region_info *rom = NULL;
+ struct vfio_region_info *opregion = NULL;
+ struct vfio_region_info *host = NULL;
+ struct vfio_region_info *lpc = NULL;
VFIOQuirk *quirk;
VFIOIGDQuirk *igd;
PCIDevice *lpc_bridge;
diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
index 83fe329474..a4f99fc5e0 100644
--- a/hw/vfio/pci.c
+++ b/hw/vfio/pci.c
@@ -879,7 +879,7 @@ static void vfio_update_msi(VFIOPCIDevice *vdev)
static void vfio_pci_load_rom(VFIOPCIDevice *vdev)
{
- g_autofree struct vfio_region_info *reg_info = NULL;
+ struct vfio_region_info *reg_info = NULL;
uint64_t size;
off_t off = 0;
ssize_t bytes;
@@ -2665,7 +2665,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_get_region_info(vbasedev, VFIO_PCI_VGA_REGION_INDEX, ®_info);
@@ -2730,7 +2730,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 = { .argsz = sizeof(irq_info) };
int i, ret = -1;
@@ -3177,7 +3177,7 @@ static void vfio_realize(PCIDevice *pdev, Error **errp)
if (!vdev->igd_opregion &&
vdev->features & VFIO_FEATURE_ENABLE_IGD_OPREGION) {
- g_autofree struct vfio_region_info *opregion = NULL;
+ struct vfio_region_info *opregion = NULL;
if (vdev->pdev.qdev.hotplugged) {
error_setg(errp,
diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-common.h
index ae3ecbd9f6..304030e71d 100644
--- a/include/hw/vfio/vfio-common.h
+++ b/include/hw/vfio/vfio-common.h
@@ -151,6 +151,7 @@ typedef struct VFIODevice {
IOMMUFDBackend *iommufd;
VFIOIOASHwpt *hwpt;
QLIST_ENTRY(VFIODevice) hwpt_next;
+ struct vfio_region_info **regions;
} VFIODevice;
struct VFIODeviceOps {
--
2.34.1On 2/19/25 15:48, John Levon wrote:
> From: Jagannathan Raman <jag.raman@oracle.com>
>
> 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.
>
> Originally-by: John Johnson <john.g.johnson@oracle.com>
> Signed-off-by: Elena Ufimtseva <elena.ufimtseva@oracle.com>
> Signed-off-by: Jagannathan Raman <jag.raman@oracle.com>
> Signed-off-by: John Levon <john.levon@nutanix.com>
> ---
> hw/vfio/ccw.c | 5 -----
> hw/vfio/common.c | 12 ++++++++++++
> hw/vfio/container.c | 10 ++++++++++
> hw/vfio/helpers.c | 21 ++++++++++++++++-----
> hw/vfio/igd.c | 8 ++++----
> hw/vfio/pci.c | 8 ++++----
> include/hw/vfio/vfio-common.h | 1 +
> 7 files changed, 47 insertions(+), 18 deletions(-)
>
> diff --git a/hw/vfio/ccw.c b/hw/vfio/ccw.c
> index 67bc137f9b..22378d50bc 100644
> --- a/hw/vfio/ccw.c
> +++ b/hw/vfio/ccw.c
> @@ -510,7 +510,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_get_dev_region_info(vdev, VFIO_REGION_TYPE_CCW,
> @@ -523,7 +522,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_get_dev_region_info(vdev, VFIO_REGION_TYPE_CCW,
> @@ -536,7 +534,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_get_dev_region_info(vdev, VFIO_REGION_TYPE_CCW,
> @@ -550,7 +547,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;
> @@ -560,7 +556,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/common.c b/hw/vfio/common.c
> index 4434e0a0a2..1866b3d3c5 100644
> --- a/hw/vfio/common.c
> +++ b/hw/vfio/common.c
> @@ -1569,6 +1569,16 @@ retry:
> return info;
> }
>
> +static void vfio_get_all_regions(VFIODevice *vbasedev)
> +{
> + struct vfio_region_info *info;
> + int i;
> +
> + for (i = 0; i < vbasedev->num_regions; i++) {
> + vfio_get_region_info(vbasedev, i, &info);
> + }
> +}
> +
> void vfio_prepare_device(VFIODevice *vbasedev, VFIOContainerBase *bcontainer,
> VFIOGroup *group, struct vfio_device_info *info)
> {
> @@ -1586,6 +1596,8 @@ void vfio_prepare_device(VFIODevice *vbasedev, VFIOContainerBase *bcontainer,
> }
>
> QLIST_INSERT_HEAD(&vfio_device_list, vbasedev, global_next);
> +
> + vfio_get_all_regions(vbasedev);
> }
>
> bool vfio_attach_device_by_iommu_type(const char *iommu_type, char *name,
> diff --git a/hw/vfio/container.c b/hw/vfio/container.c
> index 37a3befbc5..36cd245c92 100644
> --- a/hw/vfio/container.c
> +++ b/hw/vfio/container.c
> @@ -886,6 +886,16 @@ static bool vfio_get_device(VFIOGroup *group, const char *name,
>
> static void vfio_put_base_device(VFIODevice *vbasedev)
> {
> + if (vbasedev->regions != NULL) {
> + int i;
> +
> + for (i = 0; i < vbasedev->num_regions; i++) {
> + g_free(vbasedev->regions[i]);
> + }
> + g_free(vbasedev->regions);
> + vbasedev->regions = NULL;
> + }
> +
> if (!vbasedev->group) {
> return;
> }
> diff --git a/hw/vfio/helpers.c b/hw/vfio/helpers.c
> index 4b255d4f3a..3c923d23b9 100644
> --- a/hw/vfio/helpers.c
> +++ b/hw/vfio/helpers.c
> @@ -345,7 +345,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_get_region_info(vbasedev, index, &info);
> @@ -562,6 +562,17 @@ int vfio_get_region_info(VFIODevice *vbasedev, int index,
> {
> size_t argsz = sizeof(struct vfio_region_info);
>
> + /* create region cache */
> + if (vbasedev->regions == NULL) {
> + vbasedev->regions = g_new0(struct vfio_region_info *,
> + vbasedev->num_regions);
> + }
> + /* check cache */
> + if (vbasedev->regions[index] != NULL) {
> + *info = vbasedev->regions[index];
> + return 0;
> + }
> +
why not populate vbasedev->regions[index] in vfio_get_all_regions() ?
Thanks,
C.
> *info = g_malloc0(argsz);
>
> (*info)->index = index;
> @@ -581,6 +592,9 @@ retry:
> goto retry;
> }
>
> + /* fill cache */
> + vbasedev->regions[index] = *info;
> +
> return 0;
> }
>
> @@ -599,7 +613,6 @@ int vfio_get_dev_region_info(VFIODevice *vbasedev, uint32_t type,
>
> hdr = vfio_get_region_info_cap(*info, VFIO_REGION_INFO_CAP_TYPE);
> if (!hdr) {
> - g_free(*info);
> continue;
> }
>
> @@ -611,8 +624,6 @@ int vfio_get_dev_region_info(VFIODevice *vbasedev, uint32_t type,
> if (cap_type->type == type && cap_type->subtype == subtype) {
> return 0;
> }
> -
> - g_free(*info);
> }
>
> *info = NULL;
> @@ -621,7 +632,7 @@ int vfio_get_dev_region_info(VFIODevice *vbasedev, uint32_t type,
>
> bool vfio_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_get_region_info(vbasedev, region, &info)) {
> diff --git a/hw/vfio/igd.c b/hw/vfio/igd.c
> index b1a237edd6..b5425ba9c0 100644
> --- a/hw/vfio/igd.c
> +++ b/hw/vfio/igd.c
> @@ -490,10 +490,10 @@ void vfio_probe_igd_bar0_quirk(VFIOPCIDevice *vdev, int nr)
>
> void vfio_probe_igd_bar4_quirk(VFIOPCIDevice *vdev, int nr)
> {
> - g_autofree struct vfio_region_info *rom = NULL;
> - g_autofree struct vfio_region_info *opregion = NULL;
> - g_autofree struct vfio_region_info *host = NULL;
> - g_autofree struct vfio_region_info *lpc = NULL;
> + struct vfio_region_info *rom = NULL;
> + struct vfio_region_info *opregion = NULL;
> + struct vfio_region_info *host = NULL;
> + struct vfio_region_info *lpc = NULL;
> VFIOQuirk *quirk;
> VFIOIGDQuirk *igd;
> PCIDevice *lpc_bridge;
> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
> index 83fe329474..a4f99fc5e0 100644
> --- a/hw/vfio/pci.c
> +++ b/hw/vfio/pci.c
> @@ -879,7 +879,7 @@ static void vfio_update_msi(VFIOPCIDevice *vdev)
>
> static void vfio_pci_load_rom(VFIOPCIDevice *vdev)
> {
> - g_autofree struct vfio_region_info *reg_info = NULL;
> + struct vfio_region_info *reg_info = NULL;
> uint64_t size;
> off_t off = 0;
> ssize_t bytes;
> @@ -2665,7 +2665,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_get_region_info(vbasedev, VFIO_PCI_VGA_REGION_INDEX, ®_info);
> @@ -2730,7 +2730,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 = { .argsz = sizeof(irq_info) };
> int i, ret = -1;
>
> @@ -3177,7 +3177,7 @@ static void vfio_realize(PCIDevice *pdev, Error **errp)
>
> if (!vdev->igd_opregion &&
> vdev->features & VFIO_FEATURE_ENABLE_IGD_OPREGION) {
> - g_autofree struct vfio_region_info *opregion = NULL;
> + struct vfio_region_info *opregion = NULL;
>
> if (vdev->pdev.qdev.hotplugged) {
> error_setg(errp,
> diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-common.h
> index ae3ecbd9f6..304030e71d 100644
> --- a/include/hw/vfio/vfio-common.h
> +++ b/include/hw/vfio/vfio-common.h
> @@ -151,6 +151,7 @@ typedef struct VFIODevice {
> IOMMUFDBackend *iommufd;
> VFIOIOASHwpt *hwpt;
> QLIST_ENTRY(VFIODevice) hwpt_next;
> + struct vfio_region_info **regions;
> } VFIODevice;
>
> struct VFIODeviceOps {
On Thu, Apr 03, 2025 at 05:46:03PM +0200, Cédric Le Goater wrote:
> > +static void vfio_get_all_regions(VFIODevice *vbasedev)
> > +{
> > + struct vfio_region_info *info;
> > + int i;
> > +
> > + for (i = 0; i < vbasedev->num_regions; i++) {
> > + vfio_get_region_info(vbasedev, i, &info);
> > + }
> > +}
> > +
> > void vfio_prepare_device(VFIODevice *vbasedev, VFIOContainerBase *bcontainer,
> > VFIOGroup *group, struct vfio_device_info *info)
> > {
> > @@ -1586,6 +1596,8 @@ void vfio_prepare_device(VFIODevice *vbasedev, VFIOContainerBase *bcontainer,
> > }
> > QLIST_INSERT_HEAD(&vfio_device_list, vbasedev, global_next);
> > +
> > + vfio_get_all_regions(vbasedev);
> > }
> > + /* check cache */
> > + if (vbasedev->regions[index] != NULL) {
> > + *info = vbasedev->regions[index];
> > + return 0;
> > + }
> > +
>
> why not populate vbasedev->regions[index] in vfio_get_all_regions() ?
Good question. I presume it's not possible for us to ever look up a region
that has somehow appeared *after* vfio_prepare_device() ?
We'd end up off the end of the array in that case anyway.
regards
john
On 4/3/25 18:00, John Levon wrote:
> On Thu, Apr 03, 2025 at 05:46:03PM +0200, Cédric Le Goater wrote:
>
>>> +static void vfio_get_all_regions(VFIODevice *vbasedev)
>>> +{
>>> + struct vfio_region_info *info;
>>> + int i;
>>> +
>>> + for (i = 0; i < vbasedev->num_regions; i++) {
>>> + vfio_get_region_info(vbasedev, i, &info);
>>> + }
>>> +}
>>> +
>>> void vfio_prepare_device(VFIODevice *vbasedev, VFIOContainerBase *bcontainer,
>>> VFIOGroup *group, struct vfio_device_info *info)
>>> {
>>> @@ -1586,6 +1596,8 @@ void vfio_prepare_device(VFIODevice *vbasedev, VFIOContainerBase *bcontainer,
>>> }
>>> QLIST_INSERT_HEAD(&vfio_device_list, vbasedev, global_next);
>>> +
>>> + vfio_get_all_regions(vbasedev);
>>> }
>
>>> + /* check cache */
>>> + if (vbasedev->regions[index] != NULL) {
>>> + *info = vbasedev->regions[index];
>>> + return 0;
>>> + }
>>> +
>>
>> why not populate vbasedev->regions[index] in vfio_get_all_regions() ?
>
> Good question. I presume it's not possible for us to ever look up a region
> that has somehow appeared *after* vfio_prepare_device() ?
>
> We'd end up off the end of the array in that case anyway.
I was confused. I thought we were caching VFIORegions ...
Anyway, this is an optimisation and I fail to understand where
the VFIO_DEVICE_GET_REGION_INFO ioctl is called on a hot path.
Is it for interrupts ? Please explain.
Do you have figures ?
Thanks,
C.
On Fri, Apr 04, 2025 at 06:57:46PM +0200, Cédric Le Goater wrote: > > > why not populate vbasedev->regions[index] in vfio_get_all_regions() ? > > > > Good question. I presume it's not possible for us to ever look up a region > > that has somehow appeared *after* vfio_prepare_device() ? > > > > We'd end up off the end of the array in that case anyway. > > I was confused. I thought we were caching VFIORegions ... > > Anyway, this is an optimisation and I fail to understand where > the VFIO_DEVICE_GET_REGION_INFO ioctl is called on a hot path. > > Is it for interrupts ? Please explain. > > Do you have figures ? That's a great question that I don't know the answer to (like much of this code I just inherited it). Let me try to investigate. regards john
On Fri, Apr 04, 2025 at 06:18:20PM +0100, John Levon wrote:
> On Fri, Apr 04, 2025 at 06:57:46PM +0200, Cédric Le Goater wrote:
>
> > > > why not populate vbasedev->regions[index] in vfio_get_all_regions() ?
> > >
> > > Good question. I presume it's not possible for us to ever look up a region
> > > that has somehow appeared *after* vfio_prepare_device() ?
> > >
> > > We'd end up off the end of the array in that case anyway.
> >
> > I was confused. I thought we were caching VFIORegions ...
> >
> > Anyway, this is an optimisation and I fail to understand where
> > the VFIO_DEVICE_GET_REGION_INFO ioctl is called on a hot path.
> >
> > Is it for interrupts ? Please explain.
> >
> > Do you have figures ?
>
> That's a great question that I don't know the answer to (like much of this code
> I just inherited it). Let me try to investigate.
I found one reason. hw/vfio/pci.c stores VFIOPCIDevice::config_offset so it
doesn't need to do a get region info on every config space access.
But after the refactoring, vfio_io_region_read() gets passed a region index (the
idea of a "region offset" isn't meaningful to vfio-user).
Without the cache, the kernel vfio implementation:
```
867 static int vfio_io_region_write(VFIODevice *vbasedev, uint8_t index, off_t off,
868 uint32_t size, void *data, bool post)
869 {
870 struct vfio_region_info *info = vbasedev->regions[index];
871 int ret;
872
873 ret = pwrite(vbasedev->fd, data, size, info->offset + off);
```
would have to look up the region offset every time.
regards
john
© 2016 - 2025 Red Hat, Inc.