Re: [PATCH 4/7] vfio/igd: add new bar0 quirk to emulate BDSM mirror

[Alex Williamson]

On Thu, 22 Aug 2024 13:08:29 +0200
Corvin Köhne <c.koehne@beckhoff.com> wrote:

> The BDSM register is mirrored into MMIO space at least for gen 11 and
> later devices. Unfortunately, the Windows driver reads the register
> value from MMIO space instead of PCI config space for those devices [1].
> Therefore, we either have to keep a 1:1 mapping for the host and guest
> address or we have to emulate the MMIO register too. Using the igd in
> legacy mode is already hard due to it's many constraints. Keeping a 1:1
> mapping may not work in all cases and makes it even harder to use. An
> MMIO emulation has to trap the whole MMIO page. This makes accesses to
> this page slower compared to using second level address translation.
> Nevertheless, it doesn't have any constraints and I haven't noticed any
> performance degradation yet making it a better solution.
> 
> [1] 
> https://github.com/projectacrn/acrn-hypervisor/blob/5c351bee0f6ae46250eefc07f44b4a31e770f3cf/devicemodel/hw/pci/passthrough.c#L650-L653
> 
> Signed-off-by: Corvin Köhne <c.koehne@beckhoff.com>
> ---
>  hw/vfio/igd.c        | 97 ++++++++++++++++++++++++++++++++++++++++++++
>  hw/vfio/pci-quirks.c |  1 +
>  hw/vfio/pci.h        |  1 +
>  3 files changed, 99 insertions(+)
> 
> diff --git a/hw/vfio/igd.c b/hw/vfio/igd.c
> index 0b6533bbf7..863b58565e 100644
> --- a/hw/vfio/igd.c
> +++ b/hw/vfio/igd.c
> @@ -374,6 +374,103 @@ 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)
> +{
> +    VFIOPCIDevice *vdev = opaque;
> +    uint64_t offset;
> +
> +    offset = IGD_BDSM_GEN11 + addr;
> +
> +    switch (size) {
> +    case 1:
> +        return pci_get_byte(vdev->pdev.config + offset);
> +    case 2:
> +        return le16_to_cpu(pci_get_word(vdev->pdev.config + offset));
> +    case 4:
> +        return le32_to_cpu(pci_get_long(vdev->pdev.config + offset));
> +    case 8:
> +        return le64_to_cpu(pci_get_quad(vdev->pdev.config + offset));
> +    default:
> +        hw_error("igd: unsupported read size, %u bytes", size);
> +        break;
> +    }
> +
> +    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;
> +
> +    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 read size, %u bytes", size);
> +        break;
> +    }
> +}

If we have the leXX_to_cpu() in the read path, don't we need
cpu_to_leXX() in the write path?  Maybe we should in fact just get rid
of all of them since we're quirking a device that's specific to a
little endian architecture and we're defining the memory region as
little endian, but minimally we should be consistent.

> +
> +static const MemoryRegionOps vfio_igd_bdsm_quirk = {
> +    .read = vfio_igd_quirk_bdsm_read,
> +    .write = vfio_igd_quirk_bdsm_write,
> +    .endianness = DEVICE_LITTLE_ENDIAN,
> +};
> +
> +void vfio_probe_igd_bar0_quirk(VFIOPCIDevice *vdev, int nr)
> +{
> +    VFIOQuirk *quirk;
> +    int gen;
> +
> +    /*
> +     * This must be an Intel VGA device at address 00:02.0 for us to even
> +     * consider enabling legacy mode. Some driver have dependencies on the 
> PCI
> +     * bus address.
> +     */
> +    if (!vfio_pci_is(vdev, PCI_VENDOR_ID_INTEL, PCI_ANY_ID) ||
> +        !vfio_is_vga(vdev) || nr != 0 ||
> +        &vdev->pdev != pci_find_device(pci_device_root_bus(&vdev->pdev),
> +                                       0, PCI_DEVFN(0x2, 0))) {
> +        return;
> +    }
> +
> +    /*
> +     * Only on IGD devices of gen 11 and above, the BDSM register is mirrored
> +     * into MMIO space and read from MMIO space by the Windows driver.
> +     */
> +    gen = igd_gen(vdev);
> +    if (gen < 11) {
> +        return;
> +    }
> +
> +    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_add_subregion_overlap(vdev->bars[0].region.mem, 0x1080C0,

Use your macro here, IGD_BDSM_MMIO_OFFSET.  Thanks,

Alex

PS - please drop the confidential email warning signature when posting
to public lists.

> +                                        &quirk->mem[0], 1);
> +
> +    QLIST_INSERT_HEAD(&vdev->bars[nr].quirks, quirk, next);
> +}
> +
>  void vfio_probe_igd_bar4_quirk(VFIOPCIDevice *vdev, int nr)
>  {
>      g_autofree struct vfio_region_info *rom = NULL;
> diff --git a/hw/vfio/pci-quirks.c b/hw/vfio/pci-quirks.c
> index 39dae72497..d37f722cce 100644
> --- a/hw/vfio/pci-quirks.c
> +++ b/hw/vfio/pci-quirks.c
> @@ -1259,6 +1259,7 @@ void vfio_bar_quirk_setup(VFIOPCIDevice *vdev, int nr)
>      vfio_probe_nvidia_bar0_quirk(vdev, nr);
>      vfio_probe_rtl8168_bar2_quirk(vdev, nr);
>  #ifdef CONFIG_VFIO_IGD
> +    vfio_probe_igd_bar0_quirk(vdev, nr);
>      vfio_probe_igd_bar4_quirk(vdev, nr);
>  #endif
>  }
> diff --git a/hw/vfio/pci.h b/hw/vfio/pci.h
> index bf67df2fbc..5ad090a229 100644
> --- a/hw/vfio/pci.h
> +++ b/hw/vfio/pci.h
> @@ -215,6 +215,7 @@ void vfio_setup_resetfn_quirk(VFIOPCIDevice *vdev);
>  bool vfio_add_virt_caps(VFIOPCIDevice *vdev, Error **errp);
>  void vfio_quirk_reset(VFIOPCIDevice *vdev);
>  VFIOQuirk *vfio_quirk_alloc(int nr_mem);
> +void vfio_probe_igd_bar0_quirk(VFIOPCIDevice *vdev, int nr);
>  void vfio_probe_igd_bar4_quirk(VFIOPCIDevice *vdev, int nr);
>  
>  extern const PropertyInfo qdev_prop_nv_gpudirect_clique;