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

["Corvin Köhne" <corvin.koehne@gmail.com>]

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

On Mon, 2024-08-26 at 10:35 -0600, Alex Williamson wrote:
> CAUTION: External Email!!
> 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.
> 

Will drop leXX_to_cpu in the read path.

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

Thanks for catching.

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

Sry for the noise. I can't drop it, so I'm going to use another mail
address to post my patches.

> > +                                        &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;
> 
> 

-- 
Thanks,
Corvin

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