igd devices have multipe registers mirroring mmio address and pci
config space, more than a single BDSM register. To support this,
the read/write functions are made common and a macro is defined to
simplify the declaration of MemoryRegionOps.
Signed-off-by: Tomita Moeko <tomitamoeko@gmail.com>
---
hw/vfio/igd.c | 60 ++++++++++++++++++++++++++++++---------------------
1 file changed, 35 insertions(+), 25 deletions(-)
diff --git a/hw/vfio/igd.c b/hw/vfio/igd.c
index fea9be0b2d..522845c509 100644
--- a/hw/vfio/igd.c
+++ b/hw/vfio/igd.c
@@ -418,16 +418,9 @@ static const MemoryRegionOps vfio_igd_index_quirk = {
.endianness = DEVICE_LITTLE_ENDIAN,
};
-#define IGD_BDSM_MMIO_OFFSET 0x1080C0
-
-static uint64_t vfio_igd_quirk_bdsm_read(void *opaque,
- hwaddr addr, unsigned size)
+static uint64_t vfio_igd_pci_config_read(VFIOPCIDevice *vdev, uint64_t offset,
+ unsigned size)
{
- VFIOPCIDevice *vdev = opaque;
- uint64_t offset;
-
- offset = IGD_BDSM_GEN11 + addr;
-
switch (size) {
case 1:
return pci_get_byte(vdev->pdev.config + offset);
@@ -438,21 +431,17 @@ static uint64_t vfio_igd_quirk_bdsm_read(void *opaque,
case 8:
return pci_get_quad(vdev->pdev.config + offset);
default:
- hw_error("igd: unsupported read size, %u bytes", size);
+ hw_error("igd: unsupported pci config read at %lx, size %u",
+ offset, size);
break;
}
return 0;
}
-static void vfio_igd_quirk_bdsm_write(void *opaque, hwaddr addr,
- uint64_t data, unsigned size)
+static void vfio_igd_pci_config_write(VFIOPCIDevice *vdev, uint64_t offset,
+ uint64_t data, unsigned size)
{
- VFIOPCIDevice *vdev = opaque;
- uint64_t offset;
-
- offset = IGD_BDSM_GEN11 + addr;
-
switch (size) {
case 1:
pci_set_byte(vdev->pdev.config + offset, data);
@@ -467,17 +456,37 @@ static void vfio_igd_quirk_bdsm_write(void *opaque, hwaddr addr,
pci_set_quad(vdev->pdev.config + offset, data);
break;
default:
- hw_error("igd: unsupported read size, %u bytes", size);
+ hw_error("igd: unsupported pci config write at %lx, size %u",
+ offset, size);
break;
}
}
-static const MemoryRegionOps vfio_igd_bdsm_quirk = {
- .read = vfio_igd_quirk_bdsm_read,
- .write = vfio_igd_quirk_bdsm_write,
- .endianness = DEVICE_LITTLE_ENDIAN,
+#define VFIO_IGD_QUIRK_MIRROR_REG(reg, name) \
+static uint64_t vfio_igd_quirk_read_##name(void *opaque, \
+ hwaddr addr, unsigned size) \
+{ \
+ VFIOPCIDevice *vdev = opaque; \
+ return vfio_igd_pci_config_read(vdev, reg + addr, size); \
+} \
+ \
+static void vfio_igd_quirk_write_##name(void *opaque, hwaddr addr, \
+ uint64_t data, unsigned size) \
+{ \
+ VFIOPCIDevice *vdev = opaque; \
+ vfio_igd_pci_config_write(vdev, reg + addr, data, size); \
+} \
+ \
+static const MemoryRegionOps vfio_igd_quirk_mirror_##name = { \
+ .read = vfio_igd_quirk_read_##name, \
+ .write = vfio_igd_quirk_write_##name, \
+ .endianness = DEVICE_LITTLE_ENDIAN, \
};
+VFIO_IGD_QUIRK_MIRROR_REG(IGD_BDSM_GEN11, bdsm)
+
+#define IGD_BDSM_MMIO_OFFSET 0x1080C0
+
void vfio_probe_igd_bar0_quirk(VFIOPCIDevice *vdev, int nr)
{
VFIOQuirk *quirk;
@@ -507,10 +516,11 @@ void vfio_probe_igd_bar0_quirk(VFIOPCIDevice *vdev, int nr)
quirk = vfio_quirk_alloc(1);
quirk->data = vdev;
- memory_region_init_io(&quirk->mem[0], OBJECT(vdev), &vfio_igd_bdsm_quirk,
- vdev, "vfio-igd-bdsm-quirk", 8);
+ memory_region_init_io(&quirk->mem[1], OBJECT(vdev),
+ &vfio_igd_quirk_mirror_bdsm, vdev,
+ "vfio-igd-bdsm-quirk", 8);
memory_region_add_subregion_overlap(vdev->bars[0].region.mem,
- IGD_BDSM_MMIO_OFFSET, &quirk->mem[0],
+ IGD_BDSM_MMIO_OFFSET, &quirk->mem[1],
1);
QLIST_INSERT_HEAD(&vdev->bars[nr].quirks, quirk, next);
--
2.45.2On Tue, 3 Dec 2024 21:35:45 +0800
Tomita Moeko <tomitamoeko@gmail.com> wrote:
> igd devices have multipe registers mirroring mmio address and pci
> config space, more than a single BDSM register. To support this,
> the read/write functions are made common and a macro is defined to
> simplify the declaration of MemoryRegionOps.
>
> Signed-off-by: Tomita Moeko <tomitamoeko@gmail.com>
> ---
> hw/vfio/igd.c | 60 ++++++++++++++++++++++++++++++---------------------
> 1 file changed, 35 insertions(+), 25 deletions(-)
>
> diff --git a/hw/vfio/igd.c b/hw/vfio/igd.c
> index fea9be0b2d..522845c509 100644
> --- a/hw/vfio/igd.c
> +++ b/hw/vfio/igd.c
> @@ -418,16 +418,9 @@ static const MemoryRegionOps vfio_igd_index_quirk = {
> .endianness = DEVICE_LITTLE_ENDIAN,
> };
>
> -#define IGD_BDSM_MMIO_OFFSET 0x1080C0
> -
> -static uint64_t vfio_igd_quirk_bdsm_read(void *opaque,
> - hwaddr addr, unsigned size)
> +static uint64_t vfio_igd_pci_config_read(VFIOPCIDevice *vdev, uint64_t offset,
> + unsigned size)
> {
> - VFIOPCIDevice *vdev = opaque;
> - uint64_t offset;
> -
> - offset = IGD_BDSM_GEN11 + addr;
> -
> switch (size) {
> case 1:
> return pci_get_byte(vdev->pdev.config + offset);
> @@ -438,21 +431,17 @@ static uint64_t vfio_igd_quirk_bdsm_read(void *opaque,
> case 8:
> return pci_get_quad(vdev->pdev.config + offset);
> default:
> - hw_error("igd: unsupported read size, %u bytes", size);
> + hw_error("igd: unsupported pci config read at %lx, size %u",
> + offset, size);
> break;
> }
>
> return 0;
> }
>
> -static void vfio_igd_quirk_bdsm_write(void *opaque, hwaddr addr,
> - uint64_t data, unsigned size)
> +static void vfio_igd_pci_config_write(VFIOPCIDevice *vdev, uint64_t offset,
> + uint64_t data, unsigned size)
> {
> - VFIOPCIDevice *vdev = opaque;
> - uint64_t offset;
> -
> - offset = IGD_BDSM_GEN11 + addr;
> -
> switch (size) {
> case 1:
> pci_set_byte(vdev->pdev.config + offset, data);
> @@ -467,17 +456,37 @@ static void vfio_igd_quirk_bdsm_write(void *opaque, hwaddr addr,
> pci_set_quad(vdev->pdev.config + offset, data);
> break;
> default:
> - hw_error("igd: unsupported read size, %u bytes", size);
> + hw_error("igd: unsupported pci config write at %lx, size %u",
> + offset, size);
> break;
> }
> }
>
> -static const MemoryRegionOps vfio_igd_bdsm_quirk = {
> - .read = vfio_igd_quirk_bdsm_read,
> - .write = vfio_igd_quirk_bdsm_write,
> - .endianness = DEVICE_LITTLE_ENDIAN,
> +#define VFIO_IGD_QUIRK_MIRROR_REG(reg, name) \
> +static uint64_t vfio_igd_quirk_read_##name(void *opaque, \
> + hwaddr addr, unsigned size) \
> +{ \
> + VFIOPCIDevice *vdev = opaque; \
I'm not sure if QEMU coding style requires this, but I'd still prefer
to see a blank line after variable declaration, even in a macro.
> + return vfio_igd_pci_config_read(vdev, reg + addr, size); \
> +} \
> + \
> +static void vfio_igd_quirk_write_##name(void *opaque, hwaddr addr, \
> + uint64_t data, unsigned size) \
> +{ \
> + VFIOPCIDevice *vdev = opaque; \
> + vfio_igd_pci_config_write(vdev, reg + addr, data, size); \
> +} \
> + \
> +static const MemoryRegionOps vfio_igd_quirk_mirror_##name = { \
> + .read = vfio_igd_quirk_read_##name, \
> + .write = vfio_igd_quirk_write_##name, \
> + .endianness = DEVICE_LITTLE_ENDIAN, \
> };
>
> +VFIO_IGD_QUIRK_MIRROR_REG(IGD_BDSM_GEN11, bdsm)
> +
> +#define IGD_BDSM_MMIO_OFFSET 0x1080C0
> +
> void vfio_probe_igd_bar0_quirk(VFIOPCIDevice *vdev, int nr)
> {
> VFIOQuirk *quirk;
> @@ -507,10 +516,11 @@ void vfio_probe_igd_bar0_quirk(VFIOPCIDevice *vdev, int nr)
> quirk = vfio_quirk_alloc(1);
> quirk->data = vdev;
>
> - memory_region_init_io(&quirk->mem[0], OBJECT(vdev), &vfio_igd_bdsm_quirk,
> - vdev, "vfio-igd-bdsm-quirk", 8);
> + memory_region_init_io(&quirk->mem[1], OBJECT(vdev),
> + &vfio_igd_quirk_mirror_bdsm, vdev,
> + "vfio-igd-bdsm-quirk", 8);
> memory_region_add_subregion_overlap(vdev->bars[0].region.mem,
> - IGD_BDSM_MMIO_OFFSET, &quirk->mem[0],
> + IGD_BDSM_MMIO_OFFSET, &quirk->mem[1],
As Corvin notes, changing the quirk memory region index here is a bug.
Thanks,
Alex
On Tue, 2024-12-03 at 21:35 +0800, Tomita Moeko wrote:
> CAUTION: External Email!!
> igd devices have multipe registers mirroring mmio address and pci
> config space, more than a single BDSM register. To support this,
> the read/write functions are made common and a macro is defined to
> simplify the declaration of MemoryRegionOps.
>
> Signed-off-by: Tomita Moeko <tomitamoeko@gmail.com>
> ---
> hw/vfio/igd.c | 60 ++++++++++++++++++++++++++++++---------------------
> 1 file changed, 35 insertions(+), 25 deletions(-)
>
> diff --git a/hw/vfio/igd.c b/hw/vfio/igd.c
> index fea9be0b2d..522845c509 100644
> --- a/hw/vfio/igd.c
> +++ b/hw/vfio/igd.c
> @@ -418,16 +418,9 @@ static const MemoryRegionOps vfio_igd_index_quirk = {
> .endianness = DEVICE_LITTLE_ENDIAN,
> };
>
> -#define IGD_BDSM_MMIO_OFFSET 0x1080C0
> -
> -static uint64_t vfio_igd_quirk_bdsm_read(void *opaque,
> - hwaddr addr, unsigned size)
> +static uint64_t vfio_igd_pci_config_read(VFIOPCIDevice *vdev, uint64_t
> offset,
> + unsigned size)
> {
> - VFIOPCIDevice *vdev = opaque;
> - uint64_t offset;
> -
> - offset = IGD_BDSM_GEN11 + addr;
> -
> switch (size) {
> case 1:
> return pci_get_byte(vdev->pdev.config + offset);
> @@ -438,21 +431,17 @@ static uint64_t vfio_igd_quirk_bdsm_read(void *opaque,
> case 8:
> return pci_get_quad(vdev->pdev.config + offset);
> default:
> - hw_error("igd: unsupported read size, %u bytes", size);
> + hw_error("igd: unsupported pci config read at %lx, size %u",
> + offset, size);
> break;
> }
>
> return 0;
> }
>
> -static void vfio_igd_quirk_bdsm_write(void *opaque, hwaddr addr,
> - uint64_t data, unsigned size)
> +static void vfio_igd_pci_config_write(VFIOPCIDevice *vdev, uint64_t offset,
> + uint64_t data, unsigned size)
> {
> - VFIOPCIDevice *vdev = opaque;
> - uint64_t offset;
> -
> - offset = IGD_BDSM_GEN11 + addr;
> -
> switch (size) {
> case 1:
> pci_set_byte(vdev->pdev.config + offset, data);
> @@ -467,17 +456,37 @@ static void vfio_igd_quirk_bdsm_write(void *opaque,
> hwaddr addr,
> pci_set_quad(vdev->pdev.config + offset, data);
> break;
> default:
> - hw_error("igd: unsupported read size, %u bytes", size);
> + hw_error("igd: unsupported pci config write at %lx, size %u",
> + offset, size);
> break;
> }
> }
>
> -static const MemoryRegionOps vfio_igd_bdsm_quirk = {
> - .read = vfio_igd_quirk_bdsm_read,
> - .write = vfio_igd_quirk_bdsm_write,
> - .endianness = DEVICE_LITTLE_ENDIAN,
> +#define VFIO_IGD_QUIRK_MIRROR_REG(reg, name) \
> +static uint64_t vfio_igd_quirk_read_##name(void *opaque, \
> + hwaddr addr, unsigned size) \
> +{ \
> + VFIOPCIDevice *vdev = opaque; \
> + return vfio_igd_pci_config_read(vdev, reg + addr, size); \
> +} \
> + \
> +static void vfio_igd_quirk_write_##name(void *opaque, hwaddr addr, \
> + uint64_t data, unsigned size) \
> +{ \
> + VFIOPCIDevice *vdev = opaque; \
> + vfio_igd_pci_config_write(vdev, reg + addr, data, size); \
> +} \
> + \
> +static const MemoryRegionOps vfio_igd_quirk_mirror_##name = { \
> + .read = vfio_igd_quirk_read_##name, \
> + .write = vfio_igd_quirk_write_##name, \
> + .endianness = DEVICE_LITTLE_ENDIAN, \
> };
>
> +VFIO_IGD_QUIRK_MIRROR_REG(IGD_BDSM_GEN11, bdsm)
> +
> +#define IGD_BDSM_MMIO_OFFSET 0x1080C0
> +
> void vfio_probe_igd_bar0_quirk(VFIOPCIDevice *vdev, int nr)
> {
> VFIOQuirk *quirk;
> @@ -507,10 +516,11 @@ void vfio_probe_igd_bar0_quirk(VFIOPCIDevice *vdev, int
> nr)
> quirk = vfio_quirk_alloc(1);
> quirk->data = vdev;
>
> - memory_region_init_io(&quirk->mem[0], OBJECT(vdev), &vfio_igd_bdsm_quirk,
> - vdev, "vfio-igd-bdsm-quirk", 8);
> + memory_region_init_io(&quirk->mem[1], OBJECT(vdev),
This should still be quirk->mem[0]. It slipped into the wrong commit.
> + &vfio_igd_quirk_mirror_bdsm, vdev,
> + "vfio-igd-bdsm-quirk", 8);
> memory_region_add_subregion_overlap(vdev->bars[0].region.mem,
> - IGD_BDSM_MMIO_OFFSET, &quirk->mem[0],
> + IGD_BDSM_MMIO_OFFSET, &quirk->mem[1],
Same here.
> 1);
>
> QLIST_INSERT_HEAD(&vdev->bars[nr].quirks, quirk, next);
--
Kind regards,
Corvin
© 2016 - 2026 Red Hat, Inc.