On Tue, 19 Dec 2017 15:52:02 +1100 David Gibson <da...@gibson.dropbear.id.au> wrote:
> On Fri, Dec 15, 2017 at 05:29:14PM +1100, Alexey Kardashevskiy wrote: > > This makes use of a new VFIO_REGION_INFO_CAP_MSIX_MAPPABLE capability > > which tells that a region with MSIX data can be mapped entirely, i.e. > > the VFIO PCI driver won't prevent MSIX vectors area from being mapped. > > > > With this change, all BARs are mapped in a single chunk and MSIX vectors > > are emulated on top unless the machine requests not to by defining and > > enabling a new "vfio-no-msix-emulation" property. At the moment only > > sPAPR machine does so - it prohibits MSIX emulation and does not allow > > enabling it as it does not define the "set" callback for the new property; > > the new property also does not appear in "-machine pseries,help". > > > > If the new capability is present, this puts MSIX IO memory region under > > mapped memory region. If the capability is not there, it falls back to > > the old behaviour with the sparse capability. > > > > In MSIX vectors section is not aligned to the page size, the KVM memory > > listener does not register it with the KVM as a memory slot and MSIX is > > emulated by QEMU as before. > > > > This requires the kernel change - "vfio-pci: Allow mapping MSIX BAR" - > > for the new capability: https://www.spinics.net/lists/kvm/msg160282.html > > > > Signed-off-by: Alexey Kardashevskiy <a...@ozlabs.ru> > > --- > > > > This is mtree and flatview BEFORE this patch: > > > > "info mtree": > > memory-region: p...@800000020000000.mmio > > 0000000000000000-ffffffffffffffff (prio 0, i/o): p...@800000020000000.mmio > > 0000210000000000-000021000000ffff (prio 1, i/o): 0001:03:00.0 BAR 1 > > 000021000000e000-000021000000e5ff (prio 0, i/o): msix-table > > 000021000000f000-000021000000f00f (prio 0, i/o): msix-pba [disabled] > > 0000210000040000-000021000007ffff (prio 1, i/o): 0001:03:00.0 BAR 3 > > 0000210000040000-000021000007ffff (prio 0, ramd): 0001:03:00.0 BAR 3 > > mmaps[0] > > > > "info mtree -f": > > FlatView #0 > > AS "memory", root: system > > AS "cpu-memory", root: system > > Root memory region: system > > 0000000000000000-000000007fffffff (prio 0, ram): ppc_spapr.ram > > 0000210000000000-000021000000dfff (prio 1, i/o): 0001:03:00.0 BAR 1 > > 000021000000e000-000021000000e5ff (prio 0, i/o): msix-table > > 000021000000e600-000021000000ffff (prio 1, i/o): 0001:03:00.0 BAR 1 > > @000000000000e600 > > 0000210000040000-000021000007ffff (prio 0, ramd): 0001:03:00.0 BAR 3 > > mmaps[0] > > > > > > > > This is AFTER this patch applied: > > > > "info mtree": > > memory-region: p...@800000020000000.mmio > > 0000000000000000-ffffffffffffffff (prio 0, i/o): p...@800000020000000.mmio > > 0000210000000000-000021000000ffff (prio 1, i/o): 0001:03:00.0 BAR 1 > > 0000210000000000-000021000000ffff (prio 0, ramd): 0001:03:00.0 BAR 1 > > mmaps[0] > > 000021000000e000-000021000000e5ff (prio 0, i/o): msix-table > > [disabled] > > 000021000000f000-000021000000f00f (prio 0, i/o): msix-pba [disabled] > > 0000210000040000-000021000007ffff (prio 1, i/o): 0001:03:00.0 BAR 3 > > 0000210000040000-000021000007ffff (prio 0, ramd): 0001:03:00.0 BAR 3 > > mmaps[0] > > > > > > "info mtree -f": > > FlatView #2 > > AS "memory", root: system > > AS "cpu-memory", root: system > > Root memory region: system > > 0000000000000000-000000007fffffff (prio 0, ram): ppc_spapr.ram > > 0000210000000000-000021000000ffff (prio 0, ramd): 0001:03:00.0 BAR 1 > > mmaps[0] > > 0000210000040000-000021000007ffff (prio 0, ramd): 0001:03:00.0 BAR 3 > > mmaps[0] > > > > > > > > This is AFTER this patch applied AND spapr_get_msix_emulation() patched > > to enable emulation: > > > > "info mtree": > > memory-region: p...@800000020000000.mmio > > 0000000000000000-ffffffffffffffff (prio 0, i/o): p...@800000020000000.mmio > > 0000210000000000-000021000000ffff (prio 1, i/o): 0001:03:00.0 BAR 1 > > 0000210000000000-000021000000ffff (prio 0, ramd): 0001:03:00.0 BAR 1 > > mmaps[0] > > 000021000000e000-000021000000e5ff (prio 0, i/o): msix-table > > 000021000000f000-000021000000f00f (prio 0, i/o): msix-pba [disabled] > > 0000210000040000-000021000007ffff (prio 1, i/o): 0001:03:00.0 BAR 3 > > 0000210000040000-000021000007ffff (prio 0, ramd): 0001:03:00.0 BAR 3 > > mmaps[0] > > > > "info mtree -f": > > FlatView #1 > > AS "memory", root: system > > AS "cpu-memory", root: system > > Root memory region: system > > 0000000000000000-000000007fffffff (prio 0, ram): ppc_spapr.ram > > 0000210000000000-000021000000dfff (prio 0, ramd): 0001:03:00.0 BAR 1 > > mmaps[0] > > 000021000000e000-000021000000e5ff (prio 0, i/o): msix-table > > 000021000000e600-000021000000ffff (prio 0, ramd): 0001:03:00.0 BAR 1 > > mmaps[0] @000000000000e600 > > 0000210000040000-000021000007ffff (prio 0, ramd): 0001:03:00.0 BAR 3 > > mmaps[0] > > --- > > include/hw/vfio/vfio-common.h | 1 + > > linux-headers/linux/vfio.h | 5 +++++ > > hw/ppc/spapr.c | 8 ++++++++ > > hw/vfio/common.c | 15 +++++++++++++++ > > hw/vfio/pci.c | 23 +++++++++++++++++++++-- > > 5 files changed, 50 insertions(+), 2 deletions(-) > > > > diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-common.h > > index f3a2ac9..927d600 100644 > > --- a/include/hw/vfio/vfio-common.h > > +++ b/include/hw/vfio/vfio-common.h > > @@ -171,6 +171,7 @@ int vfio_get_region_info(VFIODevice *vbasedev, int > > index, > > struct vfio_region_info **info); > > int vfio_get_dev_region_info(VFIODevice *vbasedev, uint32_t type, > > uint32_t subtype, struct vfio_region_info > > **info); > > +bool vfio_is_cap_present(VFIODevice *vbasedev, uint16_t cap_type, int > > region); > > #endif > > extern const MemoryListener vfio_prereg_listener; > > > > diff --git a/linux-headers/linux/vfio.h b/linux-headers/linux/vfio.h > > index 4312e96..b45182e 100644 > > --- a/linux-headers/linux/vfio.h > > +++ b/linux-headers/linux/vfio.h > > @@ -301,6 +301,11 @@ struct vfio_region_info_cap_type { > > #define VFIO_REGION_SUBTYPE_INTEL_IGD_HOST_CFG (2) > > #define VFIO_REGION_SUBTYPE_INTEL_IGD_LPC_CFG (3) > > > > +/* > > + * The MSIX mappable capability informs that MSIX data of a BAR can be > > mmapped. > > + */ > > +#define VFIO_REGION_INFO_CAP_MSIX_MAPPABLE 3 > > + > > /** > > * VFIO_DEVICE_GET_IRQ_INFO - _IOWR(VFIO_TYPE, VFIO_BASE + 9, > > * struct vfio_irq_info) > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c > > index 9de63f0..693394a 100644 > > --- a/hw/ppc/spapr.c > > +++ b/hw/ppc/spapr.c > > @@ -2772,6 +2772,11 @@ static void spapr_set_modern_hotplug_events(Object > > *obj, bool value, > > spapr->use_hotplug_event_source = value; > > } > > > > +static bool spapr_get_msix_emulation(Object *obj, Error **errp) > > +{ > > + return true; > > +} > > + > > static char *spapr_get_resize_hpt(Object *obj, Error **errp) > > { > > sPAPRMachineState *spapr = SPAPR_MACHINE(obj); > > @@ -2853,6 +2858,8 @@ static void spapr_machine_initfn(Object *obj) > > object_property_set_description(obj, "vsmt", > > "Virtual SMT: KVM behaves as if this > > were" > > " the host's SMT mode", &error_abort); > > + object_property_add_bool(obj, "vfio-no-msix-emulation", > > + spapr_get_msix_emulation, NULL, NULL); > > I prefer the approach where the property is in the PCI device, set by > the machine defaults mechanism, rather than the property being in the > machine and the device having to look it up. > > If Alex prefers the latter though, I'm ok with that way around. I prefer anything that doesn't result in a user visible option. > > } > > > > static void spapr_machine_finalizefn(Object *obj) > > @@ -3742,6 +3749,7 @@ static const TypeInfo spapr_machine_info = { > > /* > > * pseries-2.11 > > */ > > + > > static void spapr_machine_2_11_instance_options(MachineState *machine) > > { > > } > > diff --git a/hw/vfio/common.c b/hw/vfio/common.c > > index 1fb8a8e..da5f182 100644 > > --- a/hw/vfio/common.c > > +++ b/hw/vfio/common.c > > @@ -1411,6 +1411,21 @@ int vfio_get_dev_region_info(VFIODevice *vbasedev, > > uint32_t type, > > return -ENODEV; > > } > > > > +bool vfio_is_cap_present(VFIODevice *vbasedev, uint16_t cap_type, int > > region) > > +{ > > + struct vfio_region_info *info = NULL; > > + bool ret = false; > > + > > + if (!vfio_get_region_info(vbasedev, region, &info)) { > > + if (vfio_get_region_info_cap(info, cap_type)) { > > + ret = true; > > + } > > + g_free(info); > > + } > > + > > + return ret; > > +} > > + > > /* > > * Interfaces for IBM EEH (Enhanced Error Handling) > > */ > > diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c > > index c977ee3..27a3706 100644 > > --- a/hw/vfio/pci.c > > +++ b/hw/vfio/pci.c > > @@ -1289,6 +1289,11 @@ static void vfio_pci_fixup_msix_region(VFIOPCIDevice > > *vdev) > > off_t start, end; > > VFIORegion *region = &vdev->bars[vdev->msix->table_bar].region; > > > > + if (vfio_is_cap_present(&vdev->vbasedev, > > VFIO_REGION_INFO_CAP_MSIX_MAPPABLE, > > + vdev->msix->table_bar)) { > > + return; > > + } > > + > > /* > > * We expect to find a single mmap covering the whole BAR, anything > > else > > * means it's either unsupported or already setup. > > @@ -1432,6 +1437,15 @@ static void vfio_msix_early_setup(VFIOPCIDevice > > *vdev, Error **errp) > > vfio_pci_fixup_msix_region(vdev); > > } > > > > +static MemoryRegion *vfio_msix_parent(VFIORegion *region) > > +{ > > + if (region->nr_mmaps == 1 && region->mmaps[0].size == region->size) { > > + return ®ion->mmaps[0].mem; > > + } > > + > > + return region->mem; > > +} > > + > > static int vfio_msix_setup(VFIOPCIDevice *vdev, int pos, Error **errp) > > { > > int ret; > > @@ -1440,9 +1454,9 @@ static int vfio_msix_setup(VFIOPCIDevice *vdev, int > > pos, Error **errp) > > vdev->msix->pending = g_malloc0(BITS_TO_LONGS(vdev->msix->entries) * > > sizeof(unsigned long)); > > ret = msix_init(&vdev->pdev, vdev->msix->entries, > > - vdev->bars[vdev->msix->table_bar].region.mem, > > + > > vfio_msix_parent(&vdev->bars[vdev->msix->table_bar].region), > > vdev->msix->table_bar, vdev->msix->table_offset, > > - vdev->bars[vdev->msix->pba_bar].region.mem, > > + > > vfio_msix_parent(&vdev->bars[vdev->msix->pba_bar].region), > > vdev->msix->pba_bar, vdev->msix->pba_offset, pos, > > &err); > > if (ret < 0) { > > @@ -1473,6 +1487,11 @@ static int vfio_msix_setup(VFIOPCIDevice *vdev, int > > pos, Error **errp) > > */ > > memory_region_set_enabled(&vdev->pdev.msix_pba_mmio, false); > > > > + if (object_property_get_bool(OBJECT(qdev_get_machine()), > > + "vfio-no-msix-emulation", NULL)) { > > + memory_region_set_enabled(&vdev->pdev.msix_table_mmio, false); > > + } > > + > > As Alex pointed out elsewhere, the need for the MSI-X emulation isn't > really VFIO specific. It would make more sense to have the test > directly in msix_init(), and have it skip adding the msix region to > the BAR if the machine has said it's not necessary. How much of the MSI-X capability do SPAPR guests make use of? Asking msix_init() to manage an MSI-X capability that references MMIO regions on a BAR that aren't implemented is a bit incongruous. Is there some way that the MSI-X capability can make it clear that MSI-X MMIO isn't implemented? Offsets past the end of the BAR? Reserved BIR values? You're basically asking msix_init() to create either an incomplete or invalid configuration. Thanks, Alex
