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

[Alex Williamson <alex.williamson@redhat.com>]

On Tue, 12 Dec 2017 18:01:40 +1100
Alexey Kardashevskiy <aik@ozlabs.ru> wrote:

> On 12/12/17 17:06, Alexey Kardashevskiy wrote:
> > On 12/12/17 16:54, Alex Williamson wrote:  
> >> On Tue, 12 Dec 2017 16:21:31 +1100
> >> Alexey Kardashevskiy <aik@ozlabs.ru> 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.
> >>>
> >>> This adds a "msix-no-mmap" property to the vfio-pci device, it is "true"
> >>> by default and "false" for pseries-2.12+ machines.
> >>>
> >>> This requites kernel's "vfio-pci: Allow mapping MSIX BAR"
> >>> https://www.spinics.net/lists/kvm/msg160282.html
> >>>
> >>> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> >>> ---
> >>>
> >>> This is an RFC as it requires kernel headers update which is not there yet.
> >>>
> >>> I'd like to make it "msix-mmap" (without "no") but could not find a way
> >>> of enabling a device property for machine versions newer than some value.
> >>>
> >>> I changed 2.11 machine just for the demonstration purpose.
> >>>
> >>>
> >>> ---
> >>>  hw/vfio/pci.h                 |  1 +
> >>>  include/hw/vfio/vfio-common.h |  1 +
> >>>  linux-headers/linux/vfio.h    |  5 +++++
> >>>  hw/ppc/spapr.c                | 10 +++++++++-
> >>>  hw/vfio/common.c              | 15 +++++++++++++++
> >>>  hw/vfio/pci.c                 | 11 +++++++++++
> >>>  6 files changed, 42 insertions(+), 1 deletion(-)
> >>>
> >>> diff --git a/hw/vfio/pci.h b/hw/vfio/pci.h
> >>> index a8fb3b3..53912ef 100644
> >>> --- a/hw/vfio/pci.h
> >>> +++ b/hw/vfio/pci.h
> >>> @@ -142,6 +142,7 @@ typedef struct VFIOPCIDevice {
> >>>      bool no_kvm_intx;
> >>>      bool no_kvm_msi;
> >>>      bool no_kvm_msix;
> >>> +    bool msix_no_mmap;
> >>>  } VFIOPCIDevice;
> >>>  
> >>>  uint32_t vfio_pci_read_config(PCIDevice *pdev, uint32_t addr, int len);
> >>> 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 4e7ab4c..bce9baf 100644
> >>> --- a/linux-headers/linux/vfio.h
> >>> +++ b/linux-headers/linux/vfio.h
> >>> @@ -300,6 +300,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..1dfc386 100644
> >>> --- a/hw/ppc/spapr.c
> >>> +++ b/hw/ppc/spapr.c
> >>> @@ -3742,13 +3742,21 @@ static const TypeInfo spapr_machine_info = {
> >>>  /*
> >>>   * pseries-2.11
> >>>   */
> >>> +#define SPAPR_COMPAT_2_11                                             \
> >>> +    HW_COMPAT_2_10                                                    \
> >>> +    {                                                                 \
> >>> +        .driver = "vfio-pci",                                         \
> >>> +        .property = "msix-no-mmap",                                   \
> >>> +        .value    = "on",                                             \
> >>> +    },                                                                \
> >>> +
> >>>  static void spapr_machine_2_11_instance_options(MachineState *machine)
> >>>  {
> >>>  }
> >>>  
> >>>  static void spapr_machine_2_11_class_options(MachineClass *mc)
> >>>  {
> >>> -    /* Defaults for the latest behaviour inherited from the base class */
> >>> +    SET_MACHINE_COMPAT(mc, SPAPR_COMPAT_2_11);
> >>>  }
> >>>  
> >>>  DEFINE_SPAPR_MACHINE(2_11, "2.11", true);
> >>> diff --git a/hw/vfio/common.c b/hw/vfio/common.c
> >>> index ed7717d..593514c 100644
> >>> --- a/hw/vfio/common.c
> >>> +++ b/hw/vfio/common.c
> >>> @@ -1408,6 +1408,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..d9aeae8 100644
> >>> --- a/hw/vfio/pci.c
> >>> +++ b/hw/vfio/pci.c
> >>> @@ -1289,6 +1289,12 @@ static void vfio_pci_fixup_msix_region(VFIOPCIDevice *vdev)
> >>>      off_t start, end;
> >>>      VFIORegion *region = &vdev->bars[vdev->msix->table_bar].region;
> >>>  
> >>> +    if (!vdev->msix_no_mmap &&
> >>> +        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.
> >>> @@ -1473,6 +1479,10 @@ static int vfio_msix_setup(VFIOPCIDevice *vdev, int pos, Error **errp)
> >>>       */
> >>>      memory_region_set_enabled(&vdev->pdev.msix_pba_mmio, false);
> >>>  
> >>> +    if (!vdev->msix_no_mmap) {
> >>> +        memory_region_set_enabled(&vdev->pdev.msix_table_mmio, false);
> >>> +    }  
> >>
> >> No, you're conflating issues.  There's (a) can we mmap over the MSI-X
> >> vector table and (b) do we require QEMU emulation of the MSI-X vector
> >> table.  (a) does NOT imply (b).  AFAICT, (a) should be enabled any time
> >> the kernel supports it,  
> > 
> > This is the default setting, or you do not want it to be a property so the
> > user cannot shoot himself in a foot?

If the kernel allows mmap, other than debugging, why would it ever need
to be disabled?

   
> >> (b) should never be enabled on ppc, regardless
> >> of (a).  Thanks,  
> > 
> > 
> > The intention is to have a property - msix_no_mmap=true, except a single
> > case - ppc-pseries, I just do not know how to enforce it for a specific
> > machine type.  

The intention is wrong.  (a) should be done any time the kernel allows
it.  (b) is an independent concept of disabling QEMU MSI-X emulation
for platforms, ie. machine types, that do not require it.  (b) has
nothing to do with the mmap'ability of the msix table area.  So far (b)
includes only the ppc spapr machine and I don't see a reason to allow
the user to control this.  Thanks,

Alex