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 <tomitamo...@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.
