Re: [PATCH v2 6/9] vfio/igd: add macro for declaring mirrored registers

[Alex Williamson <alex.williamson@redhat.com>]

On 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