On Fri, 19 Jan 2018 19:55:49 +1100
Alexey Kardashevskiy <a...@ozlabs.ru> wrote:

> On 19/01/18 08:59, Alex Williamson wrote:
> > On Tue, 16 Jan 2018 16:17:58 +1100
> > Alexey Kardashevskiy <a...@ozlabs.ru> wrote:
> >   
> >> On 06/01/18 02:29, Alex Williamson wrote:  
> >>> On Fri, 5 Jan 2018 10:48:07 +0100
> >>> Auger Eric <eric.au...@redhat.com> wrote:
> >>>     
> >>>> Hi Alexey,
> >>>>
> >>>> On 15/12/17 07:29, 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);
> >>>>>  }
> >>>>>  
> >>>>>  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;
> >>>>> +    }      
> >>>>
> >>>> I am testing this patch on ARM with a Mellanox CX-4 card. Single bar0 +
> >>>> msix table at 0x2000, using 64kB pages.
> >>>>
> >>>> 0000000000000000-0000000001ffffff (prio 1, i/o): 0000:01:00.0 BAR 0
> >>>>  0000000000000000-0000000001ffffff (prio 0, ramd): 0000:01:00.0 BAR 0
> >>>> mmaps[0]
> >>>>   0000000000002000-00000000000023ff (prio 0, i/o): msix-table
> >>>>   0000000000003000-0000000000003007 (prio 0, i/o): msix-pba [disabled]
> >>>>
> >>>>
> >>>> My kernel includes your "vfio-pci: Allow mapping MSIX BAR" so we bypass
> >>>> vfio_pci_fixup_msix_region. But as the mmaps[0] is not fixed up the
> >>>> vfio_listener_region_add_ram gets called on the first 8kB which cannot
> >>>> be mapped with 64kB page, leading to a qemu abort.
> >>>>
> >>>> 29520@1515145231.232161:vfio_listener_region_add_ram region_add [ram]
> >>>> 0x8000000000 - 0x8000001fff [0xffffa8820000]
> >>>> qemu-system-aarch64: VFIO_MAP_DMA: -22
> >>>> qemu-system-aarch64: vfio_dma_map(0x1e265f60, 0x8000000000, 0x2000,
> >>>> 0xffffa8820000) = -22 (Invalid argument)
> >>>> qemu: hardware error: vfio: DMA mapping failed, unable to continue
> >>>>
> >>>> In my case don't we still need the fixup logic?    
> >>>
> >>> Ah, I didn't realize you had Alexey's QEMU patch when you asked about
> >>> this.  I'd say no, the fixup shouldn't be needed, but apparently we
> >>> can't remove it until we figure out this bug and who should be filtering
> >>> out sub-page size listener events.  Thanks,    
> >>
> >>
> >> This should do it:
> >>
> >> @@ -303,11 +303,19 @@ static bool
> >> vfio_listener_skipped_section(MemoryRegionSection *section)
> >>              * Sizing an enabled 64-bit BAR can cause spurious mappings to
> >>              * addresses in the upper part of the 64-bit address space.  
> >> These
> >>              * are never accessed by the CPU and beyond the address width 
> >> of
> >>              * some IOMMU hardware.  TODO: VFIO should tell us the IOMMU 
> >> width.
> >>              */
> >> -           section->offset_within_address_space & (1ULL << 63);
> >> +           section->offset_within_address_space & (1ULL << 63) ||
> >> +           /*
> >> +            * Non-system-page aligned RAM regions have no chance to be
> >> +            * DMA mapped so skip them too.
> >> +            */
> >> +           (memory_region_is_ram(section->mr) &&
> >> +            ((section->offset_within_region & qemu_real_host_page_mask) ||
> >> +             (int128_getlo(section->size) & qemu_real_host_page_mask))
> >> +           );
> >>  }
> >>
> >>
> >> Do we VFIO DMA map on every RAM MR just because it is not necessary to have
> >> more complicated logic to detect whether it is MMIO? Or there is something
> >> bigger?  
> > 
> > Both, a) it's not clear how we determine a MR is MMIO vs RAM, thus the
> > silly address hack above to try to exclude PCI BARs as they're being
> > resized, and b) there is some value to enabling peer-to-peer mappings
> > between devices.  Is there a better, more generic solution to a) now?
> > If we know a MR is MMIO, then I think we can handle any associated
> > mapping failure as non-fatal.  It's nice to enable p2p, but it's barely
> > used and not clear that it works reliably (sometimes even on bare
> > metal).   
> 
> When P2P works in a guest, what does it look like exactly? Do the devices
> talk via the IOMMU or directly? I'd assume the latter but then the guest
> device driver needs to know physical MMIO addresses and it does not or
> guest must program BARs exactly as the host did... Has anyone actually
> reported this working?

I've seen it work, NVIDIA has a peer to peer test in their cuda tests,
see previous patches enabling GPUDirect.  The route is generally
through the IOMMU, guests don't know host physical addresses.  I say
generally though because ACS does have a Directed Translated P2P
feature that would allow a more efficient path, but I've never seen
anything that can take advantage of this.
 
> I just tried exposing MMIO on a PCI AS ("[PATCH qemu] spapr_pci: Alias MMIO
> windows to PHB address space") and ended up having
> vfio_listener_region_add() called with RAM regions (which are mapped MMIO)
> and sPAPR cannot handle it otherwise than creating a window per a memory
> section and since we can have just two, this fails. Adding
> memory_region_is_ram_device() to vfio_listener_skipped_section() solves my
> particular problem but I wonder if I should rather enable this P2P on sPAPR?

Yeah, as mentioned it'd be nice to at least make ram_device (ie. MMIO)
mappings non-fatal.  I think there's probably enough skepticism in
drivers as to whether p2p works that this isn't as critical.  Of course
making it work is even better.  Thanks,

Alex

Reply via email to