From mboxrd@z Thu Jan 1 00:00:00 1970 From: Punit Agrawal Subject: Re: [PATCH v2 kvmtool 07/10] vfio-pci: add MSI-X support Date: Mon, 31 Jul 2017 18:49:44 +0100 Message-ID: <87a83kzebr.fsf@e105922-lin.cambridge.arm.com> References: <20170622170536.14319-1-jean-philippe.brucker@arm.com> <20170622170536.14319-8-jean-philippe.brucker@arm.com> Mime-Version: 1.0 Content-Type: text/plain Cc: kvm@vger.kernel.org, will.deacon@arm.com, robin.murphy@arm.com, lorenzo.pieralisi@arm.com, marc.zyngier@arm.com To: Jean-Philippe Brucker Return-path: Received: from foss.arm.com ([217.140.101.70]:58068 "EHLO foss.arm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751243AbdGaRtq (ORCPT ); Mon, 31 Jul 2017 13:49:46 -0400 In-Reply-To: <20170622170536.14319-8-jean-philippe.brucker@arm.com> (Jean-Philippe Brucker's message of "Thu, 22 Jun 2017 18:05:33 +0100") Sender: kvm-owner@vger.kernel.org List-ID: Hi Jean-Philippe, I ran into an issue while testing these series on a Seattle based system. More below... Jean-Philippe Brucker writes: > Add virtual MSI-X tables for PCI devices, and create IRQFD routes to let > the kernel inject MSIs from a physical device directly into the guest. > > It would be tempting to create the MSI routes at init time before starting > vCPUs, when we can afford to exit gracefully. But some of it must be > initialized when the guest requests it. > > * On the KVM side, MSIs must be enabled after devices allocate their IRQ > lines and irqchips are operational, which can happen until late_init. > > * On the VFIO side, hardware state of devices may be updated when setting > up MSIs. For example, when passing a virtio-pci-legacy device to the > guest: > > (1) The device-specific configuration layout (in BAR0) depends on > whether MSIs are enabled or not in the device. If they are enabled, > the device-specific configuration starts at offset 24, otherwise it > starts at offset 20. > (2) Linux guest assumes that MSIs are initially disabled (doesn't > actually check the capability). So it reads the device config at > offset 20. > (3) Had we enabled MSIs early, host would have enabled the MSI-X > capability and device would return the config at offset 24. > (4) The guest would read junk and explode. > > Therefore we have to create MSI-X routes when the guest requests MSIs, and > enable/disable them in VFIO when the guest pokes the MSI-X capability. We > have to follow both physical and virtual state of the capability, which > makes the state machine a bit complex, but I think it works. > > Signed-off-by: Jean-Philippe Brucker > --- > include/kvm/vfio.h | 54 +++++ > vfio/pci.c | 619 ++++++++++++++++++++++++++++++++++++++++++++++++++++- > 2 files changed, 664 insertions(+), 9 deletions(-) > [...] > +static ssize_t vfio_pci_cap_size(struct pci_cap_hdr *cap_hdr) > +{ > + switch (cap_hdr->type) { > + case PCI_CAP_ID_MSIX: > + return PCI_CAP_MSIX_SIZEOF; > + default: > + pr_err("unknown PCI capability 0x%x", cap_hdr->type); > + return 0; > + } > +} > + > +/* > + * Copy capability from physical header into virtual header, and add it to the > + * virtual capability list. > + * > + * @fd_offset: offset of pci header into vfio device fd > + * @pos: offset of capability from start of header > + */ > +static int vfio_pci_add_cap(struct vfio_device *vdev, struct pci_cap_hdr *cap_hdr, > + off_t fd_offset, off_t pos) > +{ > + int i; > + ssize_t size = vfio_pci_cap_size(cap_hdr); > + struct pci_device_header *hdr = &vdev->pci.hdr; > + struct pci_cap_hdr *out = (void *)hdr + pos; > + > + if (pread(vdev->fd, out, size, fd_offset + pos) != size) > + return -errno; > + > + out->next = 0; > + > + if (!hdr->capabilities) { > + hdr->capabilities = pos; > + hdr->status |= PCI_STATUS_CAP_LIST; > + } else { > + /* Add cap at end of list */ > + struct pci_cap_hdr *last; > + > + pci_for_each_cap(i, last, hdr) > + ; > + last->next = pos; Setting the next capability is somehow overwriting the vendor id in the virtual header. This leads to the device not being probed by the appropriate driver. I did a quick hack to prevent the vendor_id override and was able to proceed with probing the device. But there are still some issues with setting up the capabilities. Can you take a look to see what's going wrong? Thanks, Punit > + } > + > + return 0; > +} > + > static int vfio_pci_parse_caps(struct vfio_device *vdev) > { > + u8 pos; > + int ret; > + struct pci_cap_hdr cap; > + ssize_t sz = sizeof(cap); > + struct vfio_region_info *info; > struct vfio_pci_device *pdev = &vdev->pci; > > if (!(pdev->hdr.status & PCI_STATUS_CAP_LIST)) > return 0; > > + pos = pdev->hdr.capabilities & ~3; > + info = &vdev->regions[VFIO_PCI_CONFIG_REGION_INDEX].info; > + > pdev->hdr.status &= ~PCI_STATUS_CAP_LIST; > pdev->hdr.capabilities = 0; > > - /* TODO: install virtual capabilities */ > + for (; pos; pos = cap.next) { > + if (pos >= PCI_DEV_CFG_SIZE) { > + dev_warn(vdev, "ignoring cap outside of config space"); > + return -EINVAL; > + } > + > + if (pread(vdev->fd, &cap, sz, info->offset + pos) != sz) { > + dev_warn(vdev, "failed to read from capabilities pointer (0x%x)", > + pos); > + return -EINVAL; > + } > + > + switch (cap.type) { > + case PCI_CAP_ID_MSIX: > + ret = vfio_pci_add_cap(vdev, &cap, info->offset, pos); > + if (ret) { > + dev_warn(vdev, "failed to read capability structure %x", > + cap.type); > + return ret; > + } > + > + pdev->msi.pos = pos; > + pdev->irq_type = VFIO_PCI_IRQ_MSIX; > + break; > + > + /* Any other capability is hidden */ > + } > + } > > return 0; > } [...]