Re: [Qemu-devel] [PATCH qemu v3] RFC: vfio-pci: Allow mmap of MSIX BAR

[Alex Williamson]

On Wed, 24 Jan 2018 16:30:04 +0100
Auger Eric <address@hidden> wrote:

> Hi Alex,
> On 24/01/18 16:02, Alex Williamson wrote:
> > On Wed, 24 Jan 2018 15:13:06 +0100
> > Auger Eric <address@hidden> wrote:
> >   
> >> Hi Alex,
> >> On 23/01/18 20:55, Alex Williamson wrote:  
> >>> On Tue, 23 Jan 2018 16:34:34 +0100
> >>> Auger Eric <address@hidden> 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 <address@hidden>
> >>>>> ---
> >>>>> 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?  
> dma map sorry
>   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.  
> Ah OK this is the piece I was missing.
> 
> OK so above description makes sense to me. However at the moment the
> user doesn't get a clear indication about what's wrong and this is what
> I pointed out first.

So should QEMU detect when this is related to MSI-X alignment and
suggest relocation as a non-fatal warning?  As noted, we currently
avoid the DMA map on this page entirely without any log of it, so
noting it is an improvement and recommending a solution would be an
improvement on top of that.  Thanks,

Alex