Re: [PATCH 6/8] vfio/igd: emulate GGC register in mmio bar0

[Tomita Moeko]

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.