The GGC register at 0x50 of pci config space is a mirror of the same
register at 0x108040 of mmio bar0 [1]. i915 driver also reads that
register from mmio bar0 instead of config space. As GGC is programmed
and emulated by qemu, the mmio address should also be emulated, in the
same way of BDSM register.
A macro is defined to simplify the declaration of MemoryRegionOps for
a register mirrored to pcie config space.
[1] 4.1.28, 12th Generation Intel Core Processors Datasheet Volume 2
https://www.intel.com/content/www/us/en/content-details/655259
Signed-off-by: Tomita Moeko <tomitamoeko@gmail.com>
---
hw/vfio/igd.c | 67 ++++++++++++++++++++++++++++++++-------------------
1 file changed, 42 insertions(+), 25 deletions(-)
diff --git a/hw/vfio/igd.c b/hw/vfio/igd.c
index a86faf2fa9..07700dce30 100644
--- a/hw/vfio/igd.c
+++ b/hw/vfio/igd.c
@@ -415,16 +415,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);
@@ -442,14 +435,12 @@ static uint64_t vfio_igd_quirk_bdsm_read(void *opaque,
return 0;
}
-static void vfio_igd_quirk_bdsm_write(void *opaque, hwaddr addr,
- uint64_t data, unsigned size)
-{
- VFIOPCIDevice *vdev = opaque;
- uint64_t offset;
-
- offset = IGD_BDSM_GEN11 + addr;
+#define IGD_GGC_MMIO_OFFSET 0x108040
+#define IGD_BDSM_MMIO_OFFSET 0x1080C0
+static void vfio_igd_pci_config_write(VFIOPCIDevice *vdev, uint64_t offset,
+ uint64_t data, unsigned size)
+{
switch (size) {
case 1:
pci_set_byte(vdev->pdev.config + offset, data);
@@ -464,17 +455,35 @@ 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 write size, %u bytes", 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_GMCH, ggc)
+VFIO_IGD_QUIRK_MIRROR_REG(IGD_BDSM_GEN11, bdsm)
+
void vfio_probe_igd_bar0_quirk(VFIOPCIDevice *vdev, int nr)
{
VFIOQuirk *quirk;
@@ -501,13 +510,21 @@ void vfio_probe_igd_bar0_quirk(VFIOPCIDevice *vdev, int nr)
return;
}
- quirk = vfio_quirk_alloc(1);
+ quirk = vfio_quirk_alloc(2);
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[0], OBJECT(vdev),
+ &vfio_igd_quirk_mirror_ggc, vdev,
+ "vfio-igd-ggc-quirk", 2);
+ memory_region_add_subregion_overlap(vdev->bars[0].region.mem,
+ IGD_GGC_MMIO_OFFSET, &quirk->mem[0],
+ 1);
+
+ 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 Mon, 2024-12-02 at 00:09 +0800, Tomita Moeko wrote:
> CAUTION: External Email!!
> The GGC register at 0x50 of pci config space is a mirror of the same
> register at 0x108040 of mmio bar0 [1]. i915 driver also reads that
> register from mmio bar0 instead of config space. As GGC is programmed
> and emulated by qemu, the mmio address should also be emulated, in the
> same way of BDSM register.
>
> A macro is defined to simplify the declaration of MemoryRegionOps for
> a register mirrored to pcie config space.
>
> [1] 4.1.28, 12th Generation Intel Core Processors Datasheet Volume 2
> https://www.intel.com/content/www/us/en/content-details/655259
>
> Signed-off-by: Tomita Moeko <tomitamoeko@gmail.com>
> ---
> hw/vfio/igd.c | 67 ++++++++++++++++++++++++++++++++-------------------
> 1 file changed, 42 insertions(+), 25 deletions(-)
>
> diff --git a/hw/vfio/igd.c b/hw/vfio/igd.c
> index a86faf2fa9..07700dce30 100644
> --- a/hw/vfio/igd.c
> +++ b/hw/vfio/igd.c
> @@ -415,16 +415,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);
> @@ -442,14 +435,12 @@ static uint64_t vfio_igd_quirk_bdsm_read(void *opaque,
> return 0;
> }
>
> -static void vfio_igd_quirk_bdsm_write(void *opaque, hwaddr addr,
> - uint64_t data, unsigned size)
> -{
> - VFIOPCIDevice *vdev = opaque;
> - uint64_t offset;
> -
> - offset = IGD_BDSM_GEN11 + addr;
> +#define IGD_GGC_MMIO_OFFSET 0x108040
> +#define IGD_BDSM_MMIO_OFFSET 0x1080C0
>
While already moving this down, I would move it even more down in front of vfio_probe_igd_bar0_quirk
to the place where it's actually used.
> +static void vfio_igd_pci_config_write(VFIOPCIDevice *vdev, uint64_t offset,
> + uint64_t data, unsigned size)
> +{
> switch (size) {
> case 1:
> pci_set_byte(vdev->pdev.config + offset, data);
> @@ -464,17 +455,35 @@ 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 write size, %u bytes", size);
It would make sense to log the faulting address too.
> 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_GMCH, ggc)
> +VFIO_IGD_QUIRK_MIRROR_REG(IGD_BDSM_GEN11, bdsm)
> +
> void vfio_probe_igd_bar0_quirk(VFIOPCIDevice *vdev, int nr)
> {
> VFIOQuirk *quirk;
> @@ -501,13 +510,21 @@ void vfio_probe_igd_bar0_quirk(VFIOPCIDevice *vdev, int nr)
> return;
> }
>
> - quirk = vfio_quirk_alloc(1);
> + quirk = vfio_quirk_alloc(2);
> 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[0], OBJECT(vdev),
> + &vfio_igd_quirk_mirror_ggc, vdev,
> + "vfio-igd-ggc-quirk", 2);
> + memory_region_add_subregion_overlap(vdev->bars[0].region.mem,
> + IGD_GGC_MMIO_OFFSET, &quirk->mem[0],
> + 1);
> +
> + 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);
I slightly prefer splitting this patch into two. The first one adding a new macro for mirroring
register and the second one adding the GGC mirror. However, that's just my personal preference.
--
Kind regards,
Corvin
disable-disclaimer-BADE
On 12/2/24 17:39, Corvin Köhne wrote:
> On Mon, 2024-12-02 at 00:09 +0800, Tomita Moeko wrote:
>> CAUTION: External Email!!
>> The GGC register at 0x50 of pci config space is a mirror of the same
>> register at 0x108040 of mmio bar0 [1]. i915 driver also reads that
>> register from mmio bar0 instead of config space. As GGC is programmed
>> and emulated by qemu, the mmio address should also be emulated, in the
>> same way of BDSM register.
>>
>> A macro is defined to simplify the declaration of MemoryRegionOps for
>> a register mirrored to pcie config space.
>>
>> [1] 4.1.28, 12th Generation Intel Core Processors Datasheet Volume 2
>> https://www.intel.com/content/www/us/en/content-details/655259
>>
>> Signed-off-by: Tomita Moeko <tomitamoeko@gmail.com>
>> ---
>> hw/vfio/igd.c | 67 ++++++++++++++++++++++++++++++++-------------------
>> 1 file changed, 42 insertions(+), 25 deletions(-)
>>
>> diff --git a/hw/vfio/igd.c b/hw/vfio/igd.c
>> index a86faf2fa9..07700dce30 100644
>> --- a/hw/vfio/igd.c
>> +++ b/hw/vfio/igd.c
>> @@ -415,16 +415,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);
>> @@ -442,14 +435,12 @@ static uint64_t vfio_igd_quirk_bdsm_read(void *opaque,
>> return 0;
>> }
>>
>> -static void vfio_igd_quirk_bdsm_write(void *opaque, hwaddr addr,
>> - uint64_t data, unsigned size)
>> -{
>> - VFIOPCIDevice *vdev = opaque;
>> - uint64_t offset;
>> -
>> - offset = IGD_BDSM_GEN11 + addr;
>> +#define IGD_GGC_MMIO_OFFSET 0x108040
>> +#define IGD_BDSM_MMIO_OFFSET 0x1080C0
>>
>
> While already moving this down, I would move it even more down in front of vfio_probe_igd_bar0_quirk
> to the place where it's actually used.
Will move it right above vfio_probe_igd_bar0_quirk()
>> +static void vfio_igd_pci_config_write(VFIOPCIDevice *vdev, uint64_t offset,
>> + uint64_t data, unsigned size)
>> +{
>> switch (size) {
>> case 1:
>> pci_set_byte(vdev->pdev.config + offset, data);
>> @@ -464,17 +455,35 @@ 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 write size, %u bytes", size);
>
> It would make sense to log the faulting address too.
Good catch
>> 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_GMCH, ggc)
>> +VFIO_IGD_QUIRK_MIRROR_REG(IGD_BDSM_GEN11, bdsm)
>> +
>> void vfio_probe_igd_bar0_quirk(VFIOPCIDevice *vdev, int nr)
>> {
>> VFIOQuirk *quirk;
>> @@ -501,13 +510,21 @@ void vfio_probe_igd_bar0_quirk(VFIOPCIDevice *vdev, int nr)
>> return;
>> }
>>
>> - quirk = vfio_quirk_alloc(1);
>> + quirk = vfio_quirk_alloc(2);
>> 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[0], OBJECT(vdev),
>> + &vfio_igd_quirk_mirror_ggc, vdev,
>> + "vfio-igd-ggc-quirk", 2);
>> + memory_region_add_subregion_overlap(vdev->bars[0].region.mem,
>> + IGD_GGC_MMIO_OFFSET, &quirk->mem[0],
>> + 1);
>> +
>> + 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);
>
> I slightly prefer splitting this patch into two. The first one adding a new macro for mirroring
> register and the second one adding the GGC mirror. However, that's just my personal preference.
>
Will split them in v2.
© 2016 - 2026 Red Hat, Inc.