On Tue, 31 Dec 2024 23:19:53 +0800
Tomita Moeko <tomitamo...@gmail.com> wrote:
> With the introduction of config_offset field, VFIOConfigMirrorQuirk can
> now be used for those mirrored register in igd bar0. This eliminates
> the need for the macro intoduced in 1a2623b5c9e7 ("vfio/igd: add macro
> for declaring mirrored registers").
>
> Signed-off-by: Tomita Moeko <tomitamo...@gmail.com>
> ---
> hw/vfio/igd.c | 128 ++++++++++++++------------------------------------
> 1 file changed, 36 insertions(+), 92 deletions(-)
>
> diff --git a/hw/vfio/igd.c b/hw/vfio/igd.c
> index f5414b0f8d..28a1d17f01 100644
> --- a/hw/vfio/igd.c
> +++ b/hw/vfio/igd.c
> @@ -18,6 +18,7 @@
> #include "hw/hw.h"
> #include "hw/nvram/fw_cfg.h"
> #include "pci.h"
> +#include "pci-quirks.h"
> #include "trace.h"
>
> /*
> @@ -422,84 +423,21 @@ static const MemoryRegionOps vfio_igd_index_quirk = {
> .endianness = DEVICE_LITTLE_ENDIAN,
> };
>
> -static uint64_t vfio_igd_pci_config_read(VFIOPCIDevice *vdev, uint64_t
> offset,
> - unsigned size)
> -{
> - switch (size) {
> - case 1:
> - return pci_get_byte(vdev->pdev.config + offset);
> - case 2:
> - return pci_get_word(vdev->pdev.config + offset);
> - case 4:
> - return pci_get_long(vdev->pdev.config + offset);
> - case 8:
> - return pci_get_quad(vdev->pdev.config + offset);
> - default:
> - hw_error("igd: unsupported pci config read at %"PRIx64", size %u",
> - offset, size);
> - break;
> - }
> -
> - return 0;
> -}
> -
> -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);
> - break;
> - case 2:
> - pci_set_word(vdev->pdev.config + offset, data);
> - break;
> - case 4:
> - pci_set_long(vdev->pdev.config + offset, data);
> - break;
> - case 8:
> - pci_set_quad(vdev->pdev.config + offset, data);
> - break;
> - default:
> - hw_error("igd: unsupported pci config write at %"PRIx64", size %u",
> - offset, size);
> - break;
> - }
> -}
> -
> -#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, bdsm)
> -VFIO_IGD_QUIRK_MIRROR_REG(IGD_BDSM_GEN11, bdsm64)
> -
> #define IGD_GGC_MMIO_OFFSET 0x108040
> #define IGD_BDSM_MMIO_OFFSET 0x1080C0
>
> +typedef struct VFIOIGDBAR0Quirk {
> + VFIOConfigMirrorQuirk ggc_mirror;
> + VFIOConfigMirrorQuirk bdsm_mirror;
> +} VFIOIGDBAR0Quirk;
> +
I don't understand why this needs to exist. There's really no reason
that the ggc and bdsm mirrors need to be tied to the same quirk, just
do something like vfio_probe_nvidia_bar0_quirk() where we allocate a
quirk with a single memory region, setup the mirror, init the region,
add the overlap, insert into the quirk list, then repeat for the other.
> void vfio_probe_igd_bar0_quirk(VFIOPCIDevice *vdev, int nr)
> {
> VFIOQuirk *quirk;
> + VFIOIGDBAR0Quirk *bar0;
> + VFIOConfigMirrorQuirk *ggc_mirror, *bdsm_mirror;
> int gen;
> + uint32_t bdsm_reg_size;
>
> /*
> * This must be an Intel VGA device at address 00:02.0 for us to even
> @@ -523,30 +461,36 @@ void vfio_probe_igd_bar0_quirk(VFIOPCIDevice *vdev, int
> nr)
> }
>
> quirk = vfio_quirk_alloc(2);
> - quirk->data = vdev;
> -
> - memory_region_init_io(&quirk->mem[0], OBJECT(vdev),
> - &vfio_igd_quirk_mirror_ggc, vdev,
> + bar0 = quirk->data = g_malloc0(sizeof(*bar0));
> +
> + ggc_mirror = &bar0->ggc_mirror;
> + ggc_mirror->vdev = vdev;
> + ggc_mirror->mem = &quirk->mem[0];
> + ggc_mirror->bar = nr;
> + ggc_mirror->offset = IGD_GGC_MMIO_OFFSET;
> + ggc_mirror->config_offset = IGD_GMCH;
> +
> + bdsm_mirror = &bar0->bdsm_mirror;
> + bdsm_mirror->mem = &quirk->mem[1];
> + bdsm_mirror->vdev = vdev;
> + bdsm_mirror->offset = IGD_BDSM_MMIO_OFFSET;
> + bdsm_mirror->config_offset = (gen < 11) ? IGD_BDSM : IGD_BDSM_GEN11;
> + bdsm_mirror->bar = nr;
> + bdsm_reg_size = (gen < 11) ? 4 : 8;
This looks like it could just be calculated inline when the region is
initialized. Thanks,
Alex
> +
> + memory_region_init_io(ggc_mirror->mem, OBJECT(vdev),
> + &vfio_generic_mirror_quirk, ggc_mirror,
> "vfio-igd-ggc-quirk", 2);
> - memory_region_add_subregion_overlap(vdev->bars[0].region.mem,
> - IGD_GGC_MMIO_OFFSET, &quirk->mem[0],
> + memory_region_add_subregion_overlap(vdev->bars[nr].region.mem,
> + ggc_mirror->offset, ggc_mirror->mem,
> 1);
>
> - if (gen < 11) {
> - memory_region_init_io(&quirk->mem[1], OBJECT(vdev),
> - &vfio_igd_quirk_mirror_bdsm, vdev,
> - "vfio-igd-bdsm-quirk", 4);
> - memory_region_add_subregion_overlap(vdev->bars[0].region.mem,
> - IGD_BDSM_MMIO_OFFSET,
> - &quirk->mem[1], 1);
> - } else {
> - memory_region_init_io(&quirk->mem[1], OBJECT(vdev),
> - &vfio_igd_quirk_mirror_bdsm64, vdev,
> - "vfio-igd-bdsm-quirk", 8);
> - memory_region_add_subregion_overlap(vdev->bars[0].region.mem,
> - IGD_BDSM_MMIO_OFFSET,
> - &quirk->mem[1], 1);
> - }
> + memory_region_init_io(bdsm_mirror->mem, OBJECT(vdev),
> + &vfio_generic_mirror_quirk, bdsm_mirror,
> + "vfio-igd-bdsm-quirk", bdsm_reg_size);
> + memory_region_add_subregion_overlap(vdev->bars[nr].region.mem,
> + bdsm_mirror->offset,
> bdsm_mirror->mem,
> + 1);
>
> QLIST_INSERT_HEAD(&vdev->bars[nr].quirks, quirk, next);
> }
