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

["Corvin Köhne" <C.Koehne@beckhoff.com>]

[-- Attachment #1: Type: text/plain, Size: 6754 bytes --]

On Tue, 2024-12-03 at 21:35 +0800, Tomita Moeko wrote:
> CAUTION: External Email!!
> 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;                                       \
> +    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),

This should still be quirk->mem[0]. It slipped into the wrong commit.

> +                          &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],

Same here.

>                                          1);
>  
>      QLIST_INSERT_HEAD(&vdev->bars[nr].quirks, quirk, next);

-- 
Kind regards,
Corvin

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]