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

[Corvin Köhne]

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.

> +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

signature.asc
Description: This is a digitally signed message part