On Wed, 24 Jan 2018 15:13:06 +0100 Auger Eric <eric.au...@redhat.com> wrote:
> Hi Alex, > On 23/01/18 20:55, Alex Williamson wrote: > > On Tue, 23 Jan 2018 16:34:34 +0100 > > Auger Eric <eric.au...@redhat.com> wrote: > > > >> Hi Alexey, > >> On 23/01/18 02:22, 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 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 may create MMIO RAM memory sections with > >>> an address or/and a size not aligned which will make vfio_dma_map() fail; > >>> to address this, this makes treats such failures as non-fatal. > >>> > >>> 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> > >>> --- > >>> Changes: > >>> v3: > >>> * vfio_listener_region_add() won't make qemu exit if failed on MMIO MR > >>> > >>> --- > >>> include/hw/vfio/vfio-common.h | 1 + > >>> linux-headers/linux/vfio.h | 5 +++++ > >>> hw/ppc/spapr.c | 7 +++++++ > >>> hw/vfio/common.c | 19 +++++++++++++++++++ > >>> hw/vfio/pci.c | 10 ++++++++++ > >>> 5 files changed, 42 insertions(+) > >>> > >>> 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 d1acfe8..5ff43ce 100644 > >>> --- a/hw/ppc/spapr.c > >>> +++ b/hw/ppc/spapr.c > >>> @@ -2789,6 +2789,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); > >>> @@ -2870,6 +2875,8 @@ static void spapr_instance_init(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); > >>> } > >>> > >>> static void spapr_machine_finalizefn(Object *obj) > >>> diff --git a/hw/vfio/common.c b/hw/vfio/common.c > >>> index b77be3a..842c5b2 100644 > >>> --- a/hw/vfio/common.c > >>> +++ b/hw/vfio/common.c > >>> @@ -530,6 +530,10 @@ static void vfio_listener_region_add(MemoryListener > >>> *listener, > >>> return; > >>> > >>> fail: > >>> + if (memory_region_is_ram_device(section->mr)) { > >>> + error_report("failed to vfio_dma_map. pci p2p may not work"); > >>> + return; > >>> + } > >> > >> I don't think this is an acceptable solution as it produces plenty of > >> errors such as > >> > >> qemu-system-aarch64: VFIO_MAP_DMA: -22 > >> qemu-system-aarch64: vfio_dma_map(0x30a7ab90, 0x8000000000, 0x2000, > >> 0xfffd79a00000) = -22 (Invalid argument) > > > > This much of the error above could be avoided by looking at the type1 > > page size bitmap before trying to perform the mapping. > > > >> qemu-system-aarch64: failed to vfio_dma_map. pci p2p may not work > > > > This of course is still valid though we can continue to discuss if it's > > worthwhile user information. > > > >> testing context is: aarch64 + Mellanox CX-4 PF where we still try to map > >> the BAR0 area [0 - 0x2000] before the MSI-X table which is not aligned > >> with the host 64kB page. > >> > >> Region 0: Memory at 80400000000 (64-bit, prefetchable) [size=32M] > >> Capabilities: [9c] MSI-X: Enable+ Count=64 Masked- > >> Vector table: BAR=0 offset=00002000 > >> PBA: BAR=0 offset=00003000 > >> > >> > >> To me a kind of mmaps region fixup looks required? > > > > I don't think so. vfio is now telling us that we can mmap the entire > > BAR, this is true. The type1 info is telling us that the minimum DMA > > mapping granularity, PAGE_SIZE on the host (64K in your case). The > > MSI-X offset on this device introduces an 8K range that type1 cannot > > map and QEMU already has all the information it needs to know this. The > > solutions are either to move MSI-X elsewhere or to extend type1 to > > allow DMA mappings down to the IOMMU minimum granularity. > > > > The MSI-X fixup that is now bypassed avoided this not generating an > > mmap over the vector table, which meant this 8K region wasn't covered > > by a memory slot and therefore never got passed through region_add. So > > p2p dma was silently disabled. Now we do mmap the entire BAR, but we > > add the MSI-X region on top of it so that when it gets flattened we end > > up with these sub-page slots. Skipping them at region_add has the same > > overall effect. > > > > Before: > > > > |-| <-- MSI-X vector emulation MR > > + > > |---PAGE_SIZE aligned mmap MR----| > > + > > |------------slow mapped BAR M------------| > > > > = > > > > |----Resulting region_add--------| (PAGE_SIZE aligned) > > > > After: > > > > |-| <-- MSI-X vector emulation MR > > + > > |------------PAGE_SIZE aligned mmap MR----| > > + > > |------------slow mapped BAR M------------| > > > > = > > > > |-| |----Resulting region_add-------------| (Gap no longer PAGE_SIZE > > aligned) > > > > So doesn't it make sense that region_add would now filter these out? > > Should we continue to silently skip unmapped MMIO? Thanks, > > I am confused because I don't see the vfio_region_read/write for > segments whose mmap failed. Nevertheless the assignment looks to work > properly. Segments whose mmap failed or whose dma map failed? When we have the "after" case above, the ram_device mmap covers the entire direct access range to the device, therefore the flattened view will hit those MemoryRegion.ops first, which will be ram_device_mem_ops. We'll only get to vfio_region_ops in the "before" case where the flattened map exposes the slow MR. Thanks, Alex
