On Thu, 22 Aug 2024 13:08:29 +0200 Corvin Köhne <c.koe...@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.koe...@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;
